<AWT Dev> [8] Request for review: 8017189 [macosx] AWT program menu disabled on Mac

[Anthony Petrov]

Hi Sergey,

I'll let Leonid test this patch since he has a number of good test 
cases. As for the code changes, they look good to me overall. The only 
scenario that concerns me is if we have a hierarchy of a frame and an 
owned undecorated window (e.g., a toolbar). With your current fix the 
menu will disappear as soon as the window gets activated because it is 
not a dialog and its menubar is obviously null:

>  546     // Finds appropriate menubar in our hierarchy,
>  547      if (self.javaMenuBar != nil || !IS(self.styleBits, IS_DIALOG)) {
>  548         // shortpath
>  549         [CMenuBar activate:self.javaMenuBar modallyDisabled:NO];
>  550     }

IMO, this is undesirable. Can we remove this if/else altogether and 
instead code this logic as follows:

    CMenuBar *menu = <traverse-owners-till-first-non-null-menu>;
    [CMenuBar activate:menu modallyDisabled:!<menu-owner>.isEnabled];

? It seems to me that this should cover all possible use cases.

--
best regards,
Anthony

On 07/23/2013 09:37 PM, Sergey Bylokhov wrote:
> Hello.
> Please review updated version of the fix:
> http://cr.openjdk.java.net/~serb/8017189/webrev.01
> After the fix, for dialogs we activates a menubar from the first visible
> and enabled owner. I use awtwindow owner instead of
> nswindow.parentWindow, because when the windowDidBecomeKey is called for
> the first time our nswindow still have no parentWindow(it is added later).
> Any testing are welcome.
> Thanks.
>
> On 23.07.2013 14:37, Leonid Romanov wrote:
>> On 7/23/2013 14:06, Sergey Bylokhov wrote:
>>> On 22.07.2013 23:32, Leonid Romanov wrote:
>>>> Well, I'd like us to stay consistent with JDK 6. However, if we
>>>> decide to fix this issue in some other way, we need to be consistent
>>>> with other possible cases, like setting frame's menu to null before
>>>> showing a dialog, making frame invisible, and so on.
>>>> But as you've said, this issue is not related to 8017189, so let's
>>>> go back to the fix for 8017189. I've got another question about it.
>>>> When native window loses focus, we call -(void) deactivate method of
>>>> CMenuBar class. At first, I thought that it basically removes all
>>>> the menu items from the menu bar, but then I realized that it is not
>>>> the case, because your fix depends on the fact that the window
>>>> gaining focus inherits the menu bar from the window that just lost
>>>> it. Now, consider step 4 of your scenario. Here, the dialog2 is the
>>>> window that is loosing focus, and dialog1 is the window that is
>>>> gaining it. As a result of dialog2 loosing focus, the current menu
>>>> bar gets marked as not active (sActiveMenuBar in CMenuBar is set to
>>>> false). When dialog1 gains focus, we do nothing with the current
>>>> menu, because the opposite window (dialog2) doesn't formally have a
>>>> menu (opposite->javaMenuBar is NULL). This means that dialog1 now
>>>> has a menu that is formally inactive.
>>>> Since I don't really understand the purpose of  -(void) deactivate
>>>> method, I can't say whether the situation I've described above is
>>>> problematic or not.  What do you think?
>>> Actually this is not a problem of my fix, this is a problem of
>>> 8010906,  which was implemented on top of "opposite" property instead
>>> of "dialog parent".  Probably you know why?
>>> I'll try to change it, but not sure is it dangerous to change it now
>>> or not.
>> I agree, after looking more closely at the original code, it seems
>> that we will get the same deactivation issue in case of showing non
>> modal dialog. I've no idea why 8010906 was implemented on top of the
>> opposite, perhaps it looked like the simplest approach back then. Do
>> you think that traversing windows hierarchy tree from the dialog being
>> shown to an ancestor frame with a menu would have been a better idea?
>>
>>>>
>>>>> On 22.07.2013 16:57, Leonid Romanov wrote:
>>>>>> Hi.
>>>>>> Here is a test case that, with your patch applied, works
>>>>>> differently than JDK 6:
>>>>>> 1. Show JFrame with a menu
>>>>>> 2.  Create a modal dialog with the frame as a parent
>>>>>> 3. Dispose the frame
>>>>>> 4. Make dialog visible
>>>>>>
>>>>>> With JDK 6, the dialog's menu will be disabled. With JDK 8, it
>>>>>> will be enabled.  So, formally, we've got a regression. I'm not
>>>>>> sure whether it is worth fixing, because it looks like a corner
>>>>>> case, but still.
>>>>>>
>>>>>> On Jul 19, 2013, at 10:15 PM, Sergey Bylokhov
>>>>>> <Sergey.Bylokhov at oracle.com> wrote:
>>>>>>
>>>>>>> Hello,
>>>>>>> Please review the fix for jdk 8 and 7u40.
>>>>>>> The fix for JDK-8010906 don't take into account situation then
>>>>>>> first parent has no menu bar, but the second has.
>>>>>>> So it introduce the next scenario:
>>>>>>> #1. Open the window with File menu.
>>>>>>> #2. Open modal dialog1 =>File menu is disabled
>>>>>>> #3. Open modal dialog2 =>File menu disappears
>>>>>>> #4. Close dialog two
>>>>>>> #5. Close dialog one. File menu reappears, but File still disabled
>>>>>>>
>>>>>>> The steps #3. occurred, because CMenuBar.activate resets the
>>>>>>> current menubar if a passed javaMenuBar is null.
>>>>>>> The steps #5. occurred, because at step #3 we do not remove our
>>>>>>> nsmenu from the deleted NSMenuItem, when the appropriate
>>>>>>> NSMenuItem removed from mainMenu. So at step #5 we got a
>>>>>>> situation, when our nsmenu was added to the two different
>>>>>>> nsmenuitems: old(disabled) and new(enabled).
>>>>>>>
>>>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8017189
>>>>>>> Webrev can be found at:
>>>>>>> http://cr.openjdk.java.net/~serb/8017189/webrev.00
>>>>>>>
>>>>>>> --
>>>>>>> Best regards, Sergey.
>>>>>>>
>>>>>
>>>>> --
>>>>> Best regards, Sergey.
>>>>>
>>>
>>>
>>
>
>