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

[Sergey Bylokhov]

On 24.07.2013 0:12, Anthony Petrov wrote:
> 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:
I just check on SwingSet2, when I drag toolbar out of the main window, 
the menubar is not disappear.
>
>    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.
>>>>>>
>>>>
>>>>
>>>
>>
>>


-- 
Best regards, Sergey.