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.bylok...@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.bylok...@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. >
