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

[Anthony Petrov]

Looks fine.

--
best regards,
Anthony

On 07/24/13 15:17, Sergey Bylokhov wrote:
> Hi, Anthony.
> Please review the updated version
> http://cr.openjdk.java.net/~serb/8017189/webrev.02/
> In the fix a dependency from the dialog was removed. now we use menubar
> from the toplvl parent window.
>
> 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:
>>
>> 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.
>>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>
>