Project

General

Profile

Bug #2970

Bug #2677: fix drawing and functional differences between P2J GUI and 4GL GUI

rework menus to use OverlayWindow

Added by Greg Shah about 8 years ago. Updated over 7 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:

Related issues

Related to User Interface - Bug #2837: GUI combo-box drop-down needs to be able to draw outside of it's containing top-level window Closed

History

#1 Updated by Greg Shah about 8 years ago

Sub-menus and popup menus must be contained in an OverlayWindow so that their contents can draw outside of the bounds of the "real" containing window (when needed).

Please see #2837 for details of how this was resolved for combo-box drop-downs. The same approach should be used here.

#2 Updated by Greg Shah about 8 years ago

  • Assignee set to Eugenie Lyzenko

#3 Updated by Greg Shah about 8 years ago

  • Target version changed from Milestone 12 to Milestone 16

#4 Updated by Hynek Cihlar almost 8 years ago

  • Status changed from New to WIP
  • Assignee changed from Eugenie Lyzenko to Hynek Cihlar

#5 Updated by Hynek Cihlar almost 8 years ago

2970a rebased against trunk 11039, now at revision 11045.

#6 Updated by Hynek Cihlar almost 8 years ago

2970a rebased against trunk 11041, now at revision 11049.

#7 Updated by Hynek Cihlar almost 8 years ago

Task branch 2970a 11058 is the reworked menus ready for review. The revision has passed ChUI and Swing GUI regression testing. I am still having some minor issues with Web GUI, but as Swing GUI already works OK I only expect small changes to come.

#8 Updated by Hynek Cihlar almost 8 years ago

Task branch 2970a 11059 resolves the issues mentioned in previous note and passes Web GUI regression testing. Please review.

#9 Updated by Greg Shah almost 8 years ago

Code Review Task Branch 2970a Revision 11059

This is a very nice improvement. I think the menu implementation is cleaner and easier to understand with this approach. I also like the addition of the GuiDriver.withSelectedWindow() and the AbstractContainer.cycleFocus() code.

1. Why was the KA_CURSOR_RIGHT event processing removed from SubMenu.processKeyEvent()? Is it only needed for GUI and the SubMenuGuiImpl body inner class event processing handles it?

2. The constructor for WindowTitlePopupGuiImpl should be at the top of the class, even though it is private (it is a documented exception in our coding standards).

3. Please move the private void initEditorItems(boolean textEmpty) to the bottom of EditorPopupGuiImpl.

4. Please make the import java.awt.event.MouseEvent; explicit again in OverlayWindow. Also please add a comment that the AWT dependency is being imported explicitly for use of event processing and AWT support in general is not in use here.

#10 Updated by Constantin Asofiei almost 8 years ago

Hynek, beside what Greg noted, the changes in 2970a/11059 are OK, I like that they are not invasive so they do not affect the ChUI menu widgets.

You have a TODO: remove for @SubMenuGuiImpl.isDescendantFocused.

#11 Updated by Hynek Cihlar almost 8 years ago

Greg Shah wrote:

Code Review Task Branch 2970a Revision 11059

1. Why was the KA_CURSOR_RIGHT event processing removed from SubMenu.processKeyEvent()? Is it only needed for GUI and the SubMenuGuiImpl body inner class event processing handles it?

Good question. This was removed because the condition was never executed. The same condition a couple of lines higher makes the method return early. Also there didn't seem to be a valid use case for it, it was probably copy&pasted from MenuItem.

2. The constructor for WindowTitlePopupGuiImpl should be at the top of the class, even though it is private (it is a documented exception in our coding standards).

3. Please move the private void initEditorItems(boolean textEmpty) to the bottom of EditorPopupGuiImpl.

4. Please make the import java.awt.event.MouseEvent; explicit again in OverlayWindow. Also please add a comment that the AWT dependency is being imported explicitly for use of event processing and AWT support in general is not in use here.

2-4 fixed.

The changes committed to 2970a revision 11060.

#12 Updated by Hynek Cihlar almost 8 years ago

Constantin Asofiei wrote:

You have a TODO: remove for @SubMenuGuiImpl.isDescendantFocused.

The unused method removed in 2970a revision 11060. Thanks.

#13 Updated by Eugenie Lyzenko almost 8 years ago

Code review for 11060 revision.

1. gui/MenuItemGuiImpl.java. The boolean flag mouseEntered is removed from onFocusGained() method. Does it mean the pop-up menu can not have the focus after displaying sometimes?

2. gui/SubMenuGuiImpl.java. The boolean flag mouseEntered usage is reworked. Please explain the idea behind the change and current approach of this flag usage.

3. widget/AbstractContainer.java removed code to find out the menu behind container. How this functionality will now work?

4. widgetbrowser/WidgetBrowserAspect.java needs to remove commented out code or put additional TODO comments.

5. client/SubMenu.java does not remove menu on right key. This is OK when current menu item has submenu item to show on right key reaction. What if not?

#14 Updated by Vadim Gindin almost 8 years ago

I've just tried to run menu/simple_sm.p and I got infinite loop. Did you run that test?

#15 Updated by Vadim Gindin almost 8 years ago

Some additional questions:
  1. OverlayWindow.draw() contains the following code.
          
    if (!isVisible())
    {
        return;
    }
    

    As I understand draw() method must not be called if isVisible() == false.
  2. SubMenuGuiImpl.setVisible() makes all child items visible. But it is not necessary when sub-menu body isn't shown. At some moment we'd discussed that case and came to solution, when child items become visible at the body show moment. Just a remark. I'm not sure if it good for new approach.

#16 Updated by Hynek Cihlar almost 8 years ago

Eugenie Lyzenko wrote:

Code review for 11060 revision.

1. gui/MenuItemGuiImpl.java. The boolean flag mouseEntered is removed from onFocusGained() method. Does it mean the pop-up menu can not have the focus after displaying sometimes?

I moved the flag mouseEntered to mouseEntered() method. It didn't make much sense to have the flag set in onFocusGained(), this way it was redundant with AbstractWidget.hasFocus().

2. gui/SubMenuGuiImpl.java. The boolean flag mouseEntered usage is reworked. Please explain the idea behind the change and current approach of this flag usage.

I am not sure what usage do you refer to, could you maybe point to a specific piece of code?

3. widget/AbstractContainer.java removed code to find out the menu behind container. How this functionality will now work?

I am glad you point this out. I am a bit worried that I can brake something with this change. I wasn't able to find any valid use case for this and with the menus now being in separate window, this seemed to be redundant. Can you describe the functionality you are referring to?

4. widgetbrowser/WidgetBrowserAspect.java needs to remove commented out code or put additional TODO comments.

Thanks for pointing this out, I will yet remove this change as it was only for debugging purposes.

5. client/SubMenu.java does not remove menu on right key. This is OK when current menu item has submenu item to show on right key reaction. What if not?

The removed code didn't have any effect and there doesn't seem to be a use case for it anyway.

#17 Updated by Hynek Cihlar almost 8 years ago

Vadim Gindin wrote:

I've just tried to run menu/simple_sm.p and I got infinite loop. Did you run that test?

Thanks for catching this. There was indeed an unhandled exception when there was a popup menu not attached anywhere.

I have committed a fix to 2670a revisions 11061 and 11062.

#18 Updated by Eugenie Lyzenko almost 8 years ago

2. gui/SubMenuGuiImpl.java. The boolean flag mouseEntered usage is reworked. Please explain the idea behind the change and current approach of this flag usage.

I am not sure what usage do you refer to, could you maybe point to a specific piece of code?

Previously mouseEntered become true when submenu gained focus or be shown and become false when submenu lost focus. So this is after respective event happening. Now this flag is changing in mouse event mouse(Entered/Exited) - it is a bit different approach. The flag is heavily used within class file. So I would like to know why the flag setting approach was changed, what was wrong with old way and what is the idea behind now.

3. widget/AbstractContainer.java removed code to find out the menu behind container. How this functionality will now work?

I am glad you point this out. I am a bit worried that I can brake something with this change. I wasn't able to find any valid use case for this and with the menus now being in separate window, this seemed to be redundant. Can you describe the functionality you are referring to?

If the container that holds menu(in OverlayWindow I guess) will run currentFocus() and focus() methods how it will find and return the menu widget reference if there is one?

#19 Updated by Hynek Cihlar almost 8 years ago

Eugenie Lyzenko wrote:

2. gui/SubMenuGuiImpl.java. The boolean flag mouseEntered usage is reworked. Please explain the idea behind the change and current approach of this flag usage.

I am not sure what usage do you refer to, could you maybe point to a specific piece of code?

Previously mouseEntered become true when submenu gained focus or be shown and become false when submenu lost focus. So this is after respective event happening. Now this flag is changing in mouse event mouse(Entered/Exited) - it is a bit different approach. The flag is heavily used within class file. So I would like to know why the flag setting approach was changed, what was wrong with old way and what is the idea behind now.

There is no functional difference, the change makes the code more readable.

Consider the following condition from the menu logic.

if (SubMenuGuiImpl.this.mouseEntered)
{
...
}

Would you expect the condition above to hold true when mouse is entered the widget? I suppose so. Previously mouseEntered set to true didn't really mean "mouse entered the widget" but instead "widget has focus". This was confusing and making the code harder to read and maintain.

3. widget/AbstractContainer.java removed code to find out the menu behind container. How this functionality will now work?

I am glad you point this out. I am a bit worried that I can brake something with this change. I wasn't able to find any valid use case for this and with the menus now being in separate window, this seemed to be redundant. Can you describe the functionality you are referring to?

If the container that holds menu(in OverlayWindow I guess) will run currentFocus() and focus() methods how it will find and return the menu widget reference if there is one?

Yes, but does the container need to return the menu widget from currentFocus()? In what case?

#20 Updated by Vadim Gindin almost 8 years ago

Did you also test popup_ext.p and system popups (window title popup, scroll popup and editor popup)?

#21 Updated by Hynek Cihlar almost 8 years ago

Vadim Gindin wrote:

Did you also test popup_ext.p and system popups (window title popup, scroll popup and editor popup)?

Vadim, thanks for your testing efforts.

I tried to run as many tests as I could in uast/menu but apparently I missed some due to high number of failing tests there (I think we should look at these failures).

I have committed more fixes to revision 11065 and I don't see any new failures from running uast/menu tests.

I am also rerunning ChUI regression tests. The changes also touch ChUI code, though they should be safe.

#22 Updated by Hynek Cihlar almost 8 years ago

2970a 11065 has passed ChUI regression testing, there are just the usual failed tests not related to this issue.

If there is no more input, the branch is ready to be merged to trunk.

#23 Updated by Eugenie Lyzenko almost 8 years ago

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:
There is no functional difference, the change makes the code more readable.

Consider the following condition from the menu logic.

[...]

Would you expect the condition above to hold true when mouse is entered the widget? I suppose so. Previously mouseEntered set to true didn't really mean "mouse entered the widget" but instead "widget has focus". This was confusing and making the code harder to read and maintain.

...

Yes, but does the container need to return the menu widget from currentFocus()? In what case?

Both can trigger the regression when the menu is first drawing. The mouse pointer will not be over menu, correct? Will the menu consider mouseEntered as true? Will it be focused and having highlight item?

#24 Updated by Hynek Cihlar almost 8 years ago

Eugenie Lyzenko wrote:

The mouse pointer will not be over menu, correct? Will the menu consider mouseEntered as true?

When mouse pointer is not positioned over a menu item, mouseEntered of the respective item will be false.

Will it be focused and having highlight item?

When an item is focused, it is highlighted.

#25 Updated by Eugenie Lyzenko almost 8 years ago

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:

The mouse pointer will not be over menu, correct? Will the menu consider mouseEntered as true?

When mouse pointer is not positioned over a menu item, mouseEntered of the respective item will be false.

Will it be focused and having highlight item?

When an item is focused, it is highlighted.

So you mean it is possible to have focused menu with mouseEntered == false, right? And the current implementation does properly handle this case? If the answer is yes I have no other concerns.

#26 Updated by Hynek Cihlar almost 8 years ago

Eugenie Lyzenko wrote:

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:

The mouse pointer will not be over menu, correct? Will the menu consider mouseEntered as true?

When mouse pointer is not positioned over a menu item, mouseEntered of the respective item will be false.

Will it be focused and having highlight item?

When an item is focused, it is highlighted.

So you mean it is possible to have focused menu with mouseEntered == false, right?

Yes.

And the current implementation does properly handle this case?

Yes, the tests show it does.

#27 Updated by Greg Shah almost 8 years ago

Please merge to trunk.

#28 Updated by Hynek Cihlar almost 8 years ago

2970a merged to trunk as revision 11042 and archived.

#29 Updated by Hynek Cihlar almost 8 years ago

  • % Done changed from 0 to 100

#30 Updated by Greg Shah almost 8 years ago

  • Status changed from WIP to Closed

#31 Updated by Greg Shah over 7 years ago

  • Target version changed from Milestone 16 to Cleanup and Stabilization for GUI

Also available in: Atom PDF