Review request for 7124554: [macosx] JWindow does ignore setAlwaysOnTop property

Anthony Petrov anthony.petrov at oracle.com
Tue Jan 24 09:29:54 PST 2012 

Hi Mike,

Thanks for your suggestions. I've just published an updated webrev at:

http://cr.openjdk.java.net/~anthony/x-6-alwaysOnTop.3/

Note that I've switched to using the dispatch_once() now since it seems 
to be thread safe, however, I still put this initialization code in a 
separate function in order to reuse it later when we implement 
CWrapper.NSWindow.level() (which isn't needed right now).

--
best regards,
Anthony

On 1/24/2012 8:44 PM, Mike Swingler wrote:
> This comment does not seem accurate:
> 
> // Used for NSWindow -setLevel: and -level (not yet implemented)
> 
> 
> Since you are making the window levels static final ints, you should use 
> those constants in CWrapper.m, and add a MAX_WINDOW_LEVELS in Java:
> 
> static NSInteger LEVELS[sun_lwawt_macosx_CWrapper_MAX_WINDOW_LEVELS];
> 
> LEVELS[sun_lwawt_macosx_CWrapper_NSNormalWindowLevel] = NSNormalWindowLevel;
> LEVELS[sun_lwawt_macosx_CWrapper_NSFloatingWindowLevel] = 
> NSFloatingWindowLevel;
> 
> With this level of verbosity and links back to the real logic in Java, 
> the level check (which you can easily raise a JNFException of type 
> kIllegalArgumentException, if that's what you really want to do), 
> shouldn't even be necessary.
> 
> Also, instead of creating a whole separate function to init the window 
> levels array, just use dispatch_once() with a block: 
> <http://developer.apple.com/library/mac/#documentation/Darwin/Reference/Manpages/man3/dispatch_once.3.html>
> It's more efficient, fewer lines of code, and not racy from multiple 
> threads.
> 
> This is looking a lot better,
> Mike Swingler
> Apple Inc.
> 
> On Jan 24, 2012, at 6:39 AM, Anthony Petrov wrote:
> 
>> Hi Mike,
>>
>> Thanks for proposing an alternative solution for this issue. I agree I 
>> find it more correct. Please take a look at the new fix at:
>>
>> http://cr.openjdk.java.net/~anthony/x-6-alwaysOnTop.2/
>>
>> Note that I decided to eliminate the changes from the 
>> CWrapper.NSWindow.addChildWindow() since the CWrapper is supposed to 
>> be a transparent proxy for Cocoa API w/o adding some logic to the 
>> wrapped methods. So instead I'm introducing 
>> CWrapper.NSWindow.setLevel() and using it where appropriate in the 
>> CPlatformWindow.
>>
>> --
>> best regards,
>> Anthony
>>
>> On 1/18/2012 3:36 AM, Mike Swingler wrote:
>>> On Jan 17, 2012, at 11:07 AM, Anthony Petrov wrote:
>>>> Hi Mike,
>>>>
>>>> On 1/17/2012 8:52 PM, Mike Swingler wrote:
>>>>> On Jan 17, 2012, at 3:54 AM, Anthony Petrov wrote:
>>>>>> On 1/16/2012 8:24 PM, Mike Swingler wrote:
>>>>>>> I still don't understand why the list of hardcoded window levels 
>>>>>>> needs to be included. Can't you just compare the window levels by 
>>>>>>> their NSInteger numeric values?
>>>>>> There are two reasons for not using the numeric values directly, 
>>>>>> or comparing them directly:
>>>>>>
>>>>>> 1. Cocoa specification doesn't define the numeric values for the 
>>>>>> NS*WindowLevel constants. They may (theoretically) change in the 
>>>>>> future. As long as they aren't specified, we don't want to rely on 
>>>>>> their current values.
>>>>> Why do you have to know the values at all? Couldn't you just reset 
>>>>> the child window level if it's different after the parent/child 
>>>>> operation? After I re-read what you are trying to accomplish, even 
>>>>> numeric comparison seems inappropriate.
>>>> I've already answered this question to Leonid on Jan 12, but I'll 
>>>> copy my reply here for your convenience:
>>>>
>>>> If the parent window is an always-on-top window, its level is 
>>>> NSFloatingWindowLevel. Suppose a child window being added to it 
>>>> hasn't been assigned any level explicitly, so its default level is 
>>>> NSNormalWindowLevel.
>>>>
>>>> Now, when we call -addChildWindow:, we really want to update the level
>>>> of the child window so that both the parent and the child share the same
>>>> window level. The if check ensures that we don't reset the level of the
>>>> child window back to normal in this case.
>>> I think what you really want to do here is check if the parent and 
>>> the child are Always-On-Top, and then reset the window level of the 
>>> child only when the child is AOT and the parent is not. -[NSWindow 
>>> addChildWindow:order:] forces the child to the same level as the 
>>> parent already.
>>> I'd avoid trying to infer if there is any "higher" vs. "lower" 
>>> relationship between the parent based on the window level as seen 
>>> from AppKit using that table.
>>>>>> 2. There's a reference to the Quartz spec that defines the 
>>>>>> kCG*WindowLevel constants as a enum. The NS*WindowLevel macros are 
>>>>>> defined using these constants. However, if you take a look at the 
>>>>>> order of the constants in that enum [1] (and therefore their 
>>>>>> relative numerical order), you may notice that they don't exactly 
>>>>>> reflect the relative visual z-order between the levels as defined 
>>>>>> in the Cocoa spec [2]. E.g. kCGModalPanelWindowLevelKey is greater 
>>>>>> than kCGDockWindowLevelKey, while NSDockWindowLevel is defined 
>>>>>> being visually above the NSModalPanelWindowLevel. As such, a 
>>>>>> direct numeric comparison of window levels produces incorrect results.
>>>>> Have you tried setting something at the "dock" level? That level 
>>>>> may have been true for the NextStep dock, but not the Mac. And BTW: 
>>>>> the NSDockWindowLevel is deprecated. Don't use it.
>>>> Yes, this is a good tip. I've removed the NSDockWindowLevel from the 
>>>> array. An updated fix is available at:
>>>>
>>>> http://cr.openjdk.java.net/~anthony/x-6-alwaysOnTop.1/
>>> Also, NSSubmenuWindowLevel and NSTornOffMenuWindowLevel are synonyms 
>>> for each other, and end up being the same value. The documentation 
>>> for NSWindow [2] says "The constant NSTornOffMenuWindowLevel is 
>>> preferable to its synonym, NSSubmenuWindowLevel.", but again, I'd 
>>> recommend just removing the table and higher/lower comparison.
>>>>>> The array of hardcoded window levels allows us to convert between 
>>>>>> window levels ordered arbitrarily and integer values ordered 
>>>>>> according to their visual z-order. This allows us to compare 
>>>>>> windows level with respect to their relative z-order as specified 
>>>>>> by Cocoa documentation.
>>>>> These constants are for assignment based on intention, not simple 
>>>>> comparison of stacking level. They have behavioral meaning beyond 
>>>>> just simple visual level ordering. Windows above the normal window 
>>>>> level will disappear in Exposé/Mission Control, and certain levels 
>>>>> will float in all Spaces. Other levels exclude the window from 
>>>>> Cmd-~ keyboard window cycling, or showing up in the Dock window 
>>>>> menu. Please, please, please test the side effects of these changes 
>>>>> before just popping windows into different levels.
>>>> Could you please clarify what exact "different levels" you're 
>>>> talking about here? AWT currently uses only two window levels: 
>>>> NSNormalWindowLevel is used for regular windows, and 
>>>> NSFloatingWindowLevel for always-on-top windows (see AWTWindow.m). 
>>>> This code is already in the repository, and tests for always-on-top 
>>>> windows do work OK.
>>> The AWT will need to use more levels, and it's not clear how 
>>> parent/child relationships need to affect these window levels. 
>>> NSFloatingWindowLevel is used for palettes (like non-modal parented 
>>> dialogs with small title bars), which do not participate in 
>>> Exposé/Mission Control. NSModalPanelWindowLevel is what app-modal 
>>> dialogs (like Open/Save) live at, which you might want "always on 
>>> top" windows to be on top of too. Obviously popup menus should be at 
>>> NSPopUpMenuWindowLevel. Another complication we encountered in Java 
>>> SE 6 is full-screen exclusive windows, which still expect popups to 
>>> be visible, so we had to push them above the NSScreenSaverWindowLevel 
>>> as a gross hacky workaround.
>>> In your testing have you moved these windows between spaces? Can you 
>>> separate them between spaces? Do the windows show as visible in 
>>> Mission Control? When you right-click on the Dock icon, do they show 
>>> in the window list? Should they? When you press Cmd-~, do they 
>>> participate in the window cycle? These are the manual tests we do 
>>> every time we futz with something like "window level" or "child 
>>> windows" in Java SE 6, which can't easily be automated.
>>>> So back to the fix we're discussing, in reality the oldChildLevel in 
>>>> the CWrapper.addChildWindow() implementation can be either of these 
>>>> two levels, but not anything else. I guess we could simplify the fix 
>>>> and only include these two levels in the ORDERED_LEVELS{} array. I 
>>>> suppose Lubomir just wanted to provide a more generic and complete 
>>>> implementation that won't break if we ever start using any other 
>>>> levels in the future (or e.g. if OS temporarily puts our window on 
>>>> some other level). This is certainly unlikely, of course, however, I 
>>>> see absolutely no problems with an implementation of 
>>>> compareWindowLevels() that can work with all window levels 
>>>> officially supported by Cocoa, not just the two that we use currently.
>>> In the future, I agree, the existing child window level will have to 
>>> be more than two values, but either it should be allowed to inherit 
>>> from it's parent, or it should be reset to the original value. This 
>>> is really a decision to be made based on the Java model state, and 
>>> not an observed property in the AppKit model which is being changed 
>>> behind your back and compared against a table. Right now the only 
>>> decision hooks on if the AOT state of the parent and child, which is 
>>> already present in the styleBits of the window.
>>> See the IS, SET, and MASK macro usage in AWTWindow.m for more info. 
>>> You could make a simple accessor to report if ALWAYS_ON_TOP is set on 
>>> both the parent and the child.
>>>>> The safest thing to do (if it works) is just to simply re-assign 
>>>>> the window level after adding the child to the parent, but you will 
>>>>> have to recursively descend into the child windows of the child, 
>>>>> because setting the window level sets the level of all the children 
>>>>> too (and changes their collection/spaces/cycle behavior). This may 
>>>>> not be an issue if you never change a child window's parent.
>>>> I've clarified above why simple reassignment won't work well.
>>> Got it. I think I've straightened out my thinking on this too, and 
>>> paged back years of hacks and corner cases I would have preferred to 
>>> have forgotten...
>>> Cheers,
>>> Mike Swingler
>>> Apple Inc.
>>>> --
>>>> best regards,
>>>> Anthony
>>>>
>>>>> Regards,
>>>>> Mike Swingler
>>>>> Apple Inc.
>>>>>> Does this clarify the need for the array of window levels? Do you 
>>>>>> have any other concerns regarding the fix?
>>>>>>
>>>>>> [1] 
>>>>>> http://developer.apple.com/library/mac/documentation/GraphicsImaging/Reference/Quartz_Services_Ref/Reference/reference.html#//apple_ref/c/tdef/CGWindowLevelKey
>>>>>>
>>>>>> [2] 
>>>>>> http://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSWindow_Class/Reference/Reference.html#//apple_ref/doc/constant_group/Window_Levels
>>>>>>
>>>>>> --
>>>>>> best regards,
>>>>>> Anthony
>>>>>>
>>>>>>
>>>>>>> Regards,
>>>>>>> Mike Swingler
>>>>>>> Apple Inc.
>>>>>>> On Jan 14, 2012, at 7:40 PM, Mike Swingler wrote:
>>>>>>>> In the course of preparing to contribute our current window 
>>>>>>>> stacking algorithm, I realized that there was a pair of internal 
>>>>>>>> SPI calls it was making. In the next revision we ship of the 
>>>>>>>> JavaRuntimeSupport.framework we can provide cover methods of 
>>>>>>>> these SPI, however that is of little benefit to the OpenJDK port 
>>>>>>>> in the immediate here-and-now.
>>>>>>>>
>>>>>>>> For now, using -addChildWindow: will be sufficient, however, we 
>>>>>>>> can provide an alternate implementation that uses a new pair of 
>>>>>>>> category methods on NSWindow if -respondsToSelector: shows that 
>>>>>>>> they are available:
>>>>>>>> - (void)javaAddToOrderingGroup:(NSWindow *)ownedWindow;
>>>>>>>> - (void)javaRemoveFromOrderingGroup:(NSWindow *)ownedWindow;
>>>>>>>>
>>>>>>>> These functions will allow the window server to move each child 
>>>>>>>> window with respect to it's parent's level, but without being 
>>>>>>>> added to it's movement group.
>>>>>>>>
>>>>>>>> Once customers get an updated JavaNativeFoundation.framework by 
>>>>>>>> either Java update or OS update, the implementation will switch 
>>>>>>>> over dynamically at runtime.
>>>>>>>>
>>>>>>>> How does this sound for a plan?
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Mike Swingler
>>>>>>>> Apple Inc.
>>>>>>>>
>>>>>>>> On Jan 12, 2012, at 2:13 PM, steve.x.northover at oracle.com 
>>>>>>>> <mailto:steve.x.northover at oracle.com> wrote:
>>>>>>>>
>>>>>>>>> We fought this one in SWT and ended up going with the puppy route.
>>>>>>>>>
>>>>>>>>> Steve
>>>>>>>>>
>>>>>>>>> On 12/01/2012 2:57 PM, Anthony Petrov wrote:
>>>>>>>>>> Hi Mike,
>>>>>>>>>>
>>>>>>>>>> I recall we've discussed this issue with you back in 2010. 
>>>>>>>>>> Unfortunately, I wasn't able to implement anything like this 
>>>>>>>>>> that would work reliably back then (and I tried hard, really), 
>>>>>>>>>> that's why we ended up with -addChildWindow:. Note that JCK is 
>>>>>>>>>> very happy with this implementation, and so are we, I presume. 
>>>>>>>>>> As long as child windows receive their respective MOVE events, 
>>>>>>>>>> it seems that everything is all right. Besides, such behavior 
>>>>>>>>>> is very Mac-friendly, making Java apps behave like native apps.
>>>>>>>>>>
>>>>>>>>>> I realize that for some developers this behavior may be 
>>>>>>>>>> inconvenient. But then again, why not listen to MOVE events on 
>>>>>>>>>> the parent frame and compensate for its movement repositioning 
>>>>>>>>>> all its children? ( :) yeah, yeah, I know, sounds weird, 
>>>>>>>>>> but... as Steve says, you gotta do what you gotta do... I 
>>>>>>>>>> mean, there's a workaround at least!)
>>>>>>>>>>
>>>>>>>>>> So, if you or anyone else wish to contribute an alternative 
>>>>>>>>>> implementation for owned windows, that would be greatly 
>>>>>>>>>> appreciated. Otherwise I'm afraid we have to stick with using 
>>>>>>>>>> -addChildWindow: for now since we simply don't have a better 
>>>>>>>>>> solution.
>>>>>>>>>>
>>>>>>>>>> -- 
>>>>>>>>>> best regards,
>>>>>>>>>> Anthony
>>>>>>>>>>
>>>>>>>>>> On 1/12/2012 11:17 PM, Mike Swingler wrote:
>>>>>>>>>>> Also, you should not use -addChildWindow:, because you also 
>>>>>>>>>>> get added to the movement group of the parent (moving the 
>>>>>>>>>>> parent moves all it's children). This is *highly* undesirable 
>>>>>>>>>>> behavior for Java's general use (and you can see it in 
>>>>>>>>>>> Eclipse when the find window follows around the IDE window 
>>>>>>>>>>> like a puppy).
>>>>>>>>>>>
>>>>>>>>>>> In the Java SE 6 AWT we manually restack every Java window in 
>>>>>>>>>>> native with -orderFront: every time a Java window changes 
>>>>>>>>>>> it's level to correctly handle it's children and the 
>>>>>>>>>>> relationships between them. This actually works out ok, since 
>>>>>>>>>>> all the changes happen at once, and when the next turn of the 
>>>>>>>>>>> event loop happens, the new stacking order only has one new 
>>>>>>>>>>> window moving to the background and one new window becoming 
>>>>>>>>>>> key/main.
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Mike Swingler
>>>>>>>>>>> Apple Inc.
>>>>>>>>>>> On Jan 12, 2012, at 6:34 AM, Leonid Romanov wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Bah, I was wrong. The value of NS*WindowLevel is really 
>>>>>>>>>>>> funky, it was a wrong suggestion to rely on it. Sorry for 
>>>>>>>>>>>> wasting your time.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 12.01.2012, at 17:52, Anthony Petrov wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> The values of the NS*WindowLevel macros are not a part of 
>>>>>>>>>>>>> the contract for Cocoa API. Therefore, we shouldn't rely on 
>>>>>>>>>>>>> their current numerical values. The names, however, and 
>>>>>>>>>>>>> their relative z-order are clearly specified in the 
>>>>>>>>>>>>> documentation, and as such we may use them.
>>>>>>>>>>>>>
>>>>>>>>>>>>> -- 
>>>>>>>>>>>>> best regards,
>>>>>>>>>>>>> Anthony
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 01/12/12 17:44, Leonid Romanov wrote:
>>>>>>>>>>>>>> I see… It looks like the higher window level is, the 
>>>>>>>>>>>>>> higher is the value of corresponding NS*WindowLevel macro.
>>>>>>>>>>>>>> Wouldn't it be better to implement compareWindowLevels() 
>>>>>>>>>>>>>> by simply subtracting one value from another?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 12.01.2012, at 17:06, Anthony Petrov wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Leonid,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for taking a look at the fix.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The if check is needed for the following case. If the 
>>>>>>>>>>>>>>> parent window is an always-on-top window, its level is 
>>>>>>>>>>>>>>> NSFloatingWindowLevel. Suppose a child window being added 
>>>>>>>>>>>>>>> to it hasn't been assigned any level explicitly, so its 
>>>>>>>>>>>>>>> default level is NSNormalWindowLevel.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Now, when we call -addChildWindow:, we really want to 
>>>>>>>>>>>>>>> update the level of the child window so that both the 
>>>>>>>>>>>>>>> parent and the child share the same window level. The if 
>>>>>>>>>>>>>>> check ensures that we don't reset the level of the child 
>>>>>>>>>>>>>>> window back to normal in this case.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -- 
>>>>>>>>>>>>>>> best regards,
>>>>>>>>>>>>>>> Anthony
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 01/11/12 21:48, Leonid Romanov wrote:
>>>>>>>>>>>>>>>> Just wondering: what would happen if you simply set 
>>>>>>>>>>>>>>>> oldChildlevel without that "if" check?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 11.01.2012, at 19:22, Anthony Petrov wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Please review a fix for 
>>>>>>>>>>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=7124554 at:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~anthony/x-6-alwaysOnTop.0/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Lubomir Nerad (CC'ed) should be credited for the fix 
>>>>>>>>>>>>>>>>> itself. I'm just going to integrate it into the repository.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> A JWindow object is always a child window with either 
>>>>>>>>>>>>>>>>> an explicit parent, or a shared invisible owner frame. 
>>>>>>>>>>>>>>>>> Therefore, we always call NSWindow -addChildWindow: 
>>>>>>>>>>>>>>>>> when showing a JWindow object. The root cause of the 
>>>>>>>>>>>>>>>>> issue is that the -addChildWindow: resets the level of 
>>>>>>>>>>>>>>>>> the child window to match that of the parent window. 
>>>>>>>>>>>>>>>>> With this fix we restore the level back to its original 
>>>>>>>>>>>>>>>>> value after the -addChildWindow: call, and as such 
>>>>>>>>>>>>>>>>> preserve the always-on-top state of the child window.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I've verified the fix with a test app attached to the 
>>>>>>>>>>>>>>>>> original issue at 
>>>>>>>>>>>>>>>>> http://java.net/jira/browse/MACOSX_PORT-158 , and it 
>>>>>>>>>>>>>>>>> works fine.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> -- 
>>>>>>>>>>>>>>>>> best regards,
>>>>>>>>>>>>>>>>> Anthony
>

More information about the macosx-port-dev mailing list