FWD » Core Development » User Interface

This problem is a regression after the change 13461.

Alexandru, take a look, please.

I've tried your example movable-frame.p, it seems to works correctly even before the 13461 change. What exactly did not work in this test example before your change?

Your example differs from mine in this issue in that in my example I do not set the movable frame as selectable.

Vladimir/Eugenie/Hynek: Please review.

Code review for 13549.

Seems OK. But I have a questions.

If the frame is not selectable how it can become selected?
What if we press inside selected frame that has selected widgets inside? Should not we deselect everything in this case(firing SE_EMPTY_SELECTION)?

Greg Shah wrote:

Vladimir/Eugenie/Hynek: Please review.

The change looks good, and it fixes the original issue.

I would extract the ((BaseConfig) widgetEff.config() expression into a local variable in order not to call it twice in one expression.

Vladimir Tsichevski wrote:

Got NPE while testing movable-frame.p here:

This happens with WEB only.

The change I am working on in #6013 seems to fix #6074-12 and #6074-14.

Eugenie Lyzenko wrote:

If the frame is not selectable how it can become selected?

This was the same reasoning I had when implementing MouseEmptySelector. It seems that, for the moving process, FWD requires the widget to be selected and movable: MouseDirectManipulation.getMovableWidgetsBounds and MouseDirectManipulation.getWidgetsToMove. In this particular case, when dragging, the frame gets selected without being selectable.

What if we press inside selected frame that has selected widgets inside? Should not we deselect everything in this case(firing SE_EMPTY_SELECTION)?

Just tested; in this 4GL scenario no empty selection is fired and the inner widgets are not deselected.

Vladimir Tsichevski wrote:

I would extract the ((BaseConfig) widgetEff.config() expression into a local variable in order not to call it twice in one expression.

Fixed in 3821c/rev. 13555.

Alexandru Lungu wrote:

Vladimir Tsichevski wrote:

I would extract the ((BaseConfig) widgetEff.config() expression into a local variable in order not to call it twice in one expression.

Fixed in 3821c/rev. 13555.

I meant extracting to a variable of the BaseConfig type at the beginning of the code: this eliminates one more config reference, two Java class casts and one explicit instanceof usage:

   @Override
   public void mousePressed(MouseEvent e)
   {
      final BaseConfig config = widgetEff.config();
      if (isFrame && !config.selectable && !config.selected)
      {
         EventManager.postEvent(new DirectManipulationEvent(widgetEff,
                                                            Keyboard.SE_EMPTY_SELECTION));

         final TitledWindow<?> topWindow = widgetEff.ancestor();
         if (topWindow != null)
         {
            Collection<? extends Widget> selectedWidgets = topWindow.getAllSelectedWidgets();
            for (Widget w : selectedWidgets)
            {
               if (config.selected)
               {
                  EventManager.postEvent(new DirectManipulationEvent(w, UiUtils.locateFrame(w),
                                                                     true, Keyboard.SE_DESELECTION));
               }
            }
         }
      }
   }

Vladimir Tsichevski wrote:

I meant extracting to a variable of the BaseConfig type at the beginning of the code: this eliminates one more config reference, two Java class casts and one explicit instanceof usage:

[...]

Done in 3821c/rev. 13557. Note that the second widget configuration is for the selected widget, not the parent container: if (((BaseConfig) w.config()).selected) instead of if (config.selected).

Alexandru Lungu wrote:

Vladimir Tsichevski wrote:

I meant extracting to a variable of the BaseConfig type at the beginning of the code: this eliminates one more config reference, two Java class casts and one explicit instanceof usage:

[...]

Done in 3821c/rev. 13557. Note that the second widget configuration is for the selected widget, not the parent container: if (((BaseConfig) w.config()).selected) instead of if (config.selected).

Yes, you are correct here, thanks!