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

[Leonid Romanov]

So, there are no memory leaks? I'm OK with the fix, then. 

On Jul 24, 2013, at 4:28 PM, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> wrote:

> Hi, Leonid.
> Yes. When the child window is created, it create its parent also. And the owner cannot be deallocated until child window is live, but when the owner is disposed, it dispose all its child windows.
> 
> On 24.07.2013 15:46, Leonid Romanov wrote:
>> Looks good for me as well. The only thing that is not clear to me is the memory management in AWTWindow: since every child references its parent now, wouldn't it prevent the parent from  being deallocated after it has been disposed?
>> 
>> On Jul 24, 2013, at 3:17 PM, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> 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.
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 
>>> -- 
>>> Best regards, Sergey.
>>> 
> 
> 
> -- 
> Best regards, Sergey.
>