<AWT Dev> Review request: 6689983 (reevaluate our inset-related code in XAWT)

[Oleg Sukhodolsky]

Hi Anthony,

sorry for delay (I was on vacation).

On Wed, Jul 8, 2009 at 6:05 PM, Anthony Petrov<Anthony.Petrov at sun.com> wrote:
> Hello Oleg,
>
> On 06/29/2009 12:01 PM, Oleg Sukhodolsky wrote:
>>
>> I've finally read through rest of the changes :)  Although I found
>
> Thanks much!
>
>> nothing suspicious in them, I have to say about
>> strange feeling I've got after the reading :(  For me (I hope I'm
>> wrong and I've missed something rather important)
>> it looks like you have changed some defaults, and some assumptions,
>> rewritten some code, to change the behavior
>> a bit :(  I mean amount of changes for me looks inadequate to
>> behavior's changes they introduce.  Moreover the code
>> doesn't become much clearer :(  As I said before I hope I've missed
>> something important and you will point these
>> pieces to me :)
>>
>> So, the question is why do you think that current code is noticeably
>> better than the old one?
>> You have added support for situation when WM at runtime (this is plus,
>> but it is not directly related to problems with insets)
>> It looks like a new code has better support for changes of insets
>> during toplevel's lifetime (it is good too)
>> But for it is still unclear that these two points are worth all
>> changes you suggest :(
>
> The third one is the clarity/maintainability of the code (which may appear
> quite subjective, so we need more opinions to get some objective valuation
> of this point).

Ok, so there are two main purposes:
- to move most of insets-guessing code to XDecorated Peer.
- better support for modern WMs

Both look nice and I agree that it is worth to implement both.  But I
would suggest to do them separately.
This way it would be easier to review/understand changes.  Also I have
noticed several other changes
which are not related to main purposes (e.g. support for restarting
WM, support for changing insets on-fly),
and I think it would be better to do these changes as separate commits
too. Small changes are easier to test and review, and,
I believe, you will find some tests (not necessary automatic) for
these changes ;)

I understand, that such approach increase amount of work you need to
do, but it simplifies reviewing and
so improve quality of the code.  This why I suggest it.

Regards, Oleg.

> Anyways, as I mentioned in my extended description of the purpose of the fix
> (see the quoted text below), the intention was to support all modern WMs
> (which support the NET protocol and are otherwise "good"), and do our best
> to support the rest (since there're obviously people using exotic WMs, and
> we certainly don't want to break Java for that people).
>
> If we were to drop support for that exotic cases, we could actually only
> skip a piece of the code from the XDecoratedPeer.retrieveNativeInsets()
> method. Almost all the rest of the code needs to be in place, IMO.
>
>> If you believe that the changes resolves some other issues please
>> share them with me.  Some buggy scenarios would help me to
>> understand your motivation.
>
> There were no any specific buggy scenarios that we had in mind while
> engineering the code. This was more like factoring and enhancing.
>
>> Another question I have is why the code doesn't try to request extents
>> just before showing a toplevel?
>> This way we would minimize size dancing after showing the toplevel?
>
> Reading the EWMH specification I don't find any evidence that the extents
> received just before making the window visible are the real insets that
> would persist after the window becomes visible.
>
> Now suppose we got some insets for a not-yet-mapped window that differ from
> the ones we currently consider as the current native insets (for whatever
> reason). This would mean we need to perform the adjustment (see
> XDecPeer.adjustBounds()). After it finishes, it resets the flag making
> adjustments disabled for further updates of the insets (or, more generally,
> the bounds).
>
> Since we don't have a guarantee that the extents persist after mapping the
> window, we may end up with an incorrectly sized/positioned window when it
> pops up on the screen.
>
> We could possibly perform the adjustment twice in this case, but this
> doesn't seem to clarify the paradigm, does it?
>
>> I hope the message is not too pessimistic and your answer help me find
>> missed pieces of the masterpiece I'me trying to review ;)
>
> Well, at least I wouldn't call it over-optimistic for sure. ;)
> However, if you have any constructive ideas on how we could improve the fix,
> I would really appreciate to discuss them.
>
> Thanks again for reviewing that!
>
> --
> best regards,
> Anthony
>
>>
>> Regards, Oleg.
>>
>> On Wed, Jun 17, 2009 at 5:48 PM, Anthony Petrov<Anthony.Petrov at sun.com>
>> wrote:
>>>
>>> Hi Oleg,
>>>
>>> You're right. This is indeed should have been done. Here it follows:
>>>
>>> The intention is to support all modern window managers well (Metacity,
>>> KWin,
>>> Compiz), and try our best supporting other WMs.
>>>
>>> One could think that this is an unimportant problem at all: just show a
>>> frame, and whatever decorations it gets from the window manager - that
>>> are
>>> the insets we need. However a Java program might previously set some
>>>  desired parameters to the frame, notably:
>>>
>>>  a) location,
>>>  b) size,
>>>  c) client area size (via pack()).
>>>
>>> Some details regarding the last case: the pack() method may be called
>>> before
>>> showing the frame - at this point the peer can report incorrect insets -
>>> we
>>> only get real values for the insets after the window has been shown.
>>> Which
>>> means that the preferred size set by the Window.pack() method may be
>>> calculated with the incorrect insets, and therefore we need to resize the
>>> frame upon showing it so that the size of the client area (that actually
>>> has
>>> been "packed") be preserved.
>>>
>>> Upon showing the frame we would like to preserve the desired settings
>>> making
>>> the frame appear on the screen as close as possible to what the user
>>> application wants.
>>>
>>> On the other hand, window managers may "dance" with their decorations
>>> differently: some preserve the location of the frame, but, or course,
>>> enlarge the size of the window when decorating it. Others may preserve
>>> the
>>> size, but change the location of the frame, because the decorations add
>>> some
>>> exterior space to the window bounds. Third may keep the location and the
>>> size of the frame, however the client area size will obviously shrink. Et
>>> cetera.
>>>
>>> Hence the main responsibility of the insets-related code is to identify
>>> the
>>> moment when the window manager has finished its "dancing", then figure
>>> out
>>> what exact parameters we want to preserve, and finally readjust the
>>> bounds
>>> of the frame accordingly. And of course it would also be extremely nice
>>> to
>>> report some actual values to the user space via the Frame.getInsets()
>>> method. :)
>>>
>>> The code is centered around one window property and two event handlers
>>> mostly:
>>>
>>> 1. The frame extents property (in most cases the XA_NET_FRAME_EXTENTS):
>>> modern window managers may be requested to provide the current frame
>>> extents
>>> (== insets in Java terminology) as values in this property. Some can also
>>> automatically update the values when the extents change (like when the
>>> theme
>>> changes). So we listen to the PropertyNotify event and update the locally
>>> cached value of the extents (named "native insets" in the terms of the
>>> current fix). Sometimes the code resets the native insets to null
>>> (because
>>> we think they may have been changed). In these cases we request the
>>> window
>>> manager to update the property and then read its value (see the
>>> XDecoratedPeer.retrieveNativeInsets() method).
>>>
>>> 2. The ReparentNotify event: most window managers reparent user windows
>>> in
>>> order to decorate them. Some do that event twice - by reparenting the
>>> already parented frame - creating two levels of parenting from the user's
>>> window to the root window. Mostly the event is used to reset the locally
>>> cached native insets because we usually don't need to get the insets
>>> immediately after reparenting, and we also need to indicate that we need
>>> to
>>> get them later. We'll have a better chance upon receiving subsequent
>>> ConfigureNotify events. Besides there even exist non-reparenting window
>>> managers (like Compiz), so we better postpone the task.
>>>
>>> 3. The ConfigureNotify event: that is when we actually do our dirty job.
>>> In
>>> a nutshell: we skip some ConfigureNotify events since they do not carry
>>> any
>>> useful information for us. Then we calculate the current bounds of the
>>> frame
>>> based on the values in the XEvent structure and pass the info to the
>>> XDecoratedPeer.setWindowDimensions() method. The method checks if the
>>> bounds
>>> of the frame need to be adjusted (i.e. if we just got shown, and we need
>>> to
>>> preserve the location and/or size of the frame). If we need to do that,
>>> we
>>> just call the reshape() methods with the desired location/size (see the
>>> adjustBounds() method), and stop processing the request. If the bounds
>>> have
>>> already been adjusted (which happens only once per each showing of the
>>> frame), the method updates the dimensions, sends Java events, and does
>>> other
>>> house keeping stuff.
>>>
>>> Basically that is what the code does. A lot more details can be found in
>>> the
>>> comment sections inline.
>>>
>>> On 06/16/2009 11:44 PM, Oleg Sukhodolsky wrote:
>>>>
>>>> perhaps I'm a lazy guy, but I think that some short description of
>>>> main ideas you use
>>>> and decisions you've made would help to review the changes.
>>>
>>> I hope the description above is short enough for you to get started. :)
>>> Please feel free to ask any further questions.
>>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>>> Oleg.
>>>>
>>>> On Tue, Jun 16, 2009 at 7:06 PM, Anthony Petrov<Anthony.Petrov at sun.com>
>>>> wrote:
>>>>>
>>>>> Hello.
>>>>>
>>>>> The insets-related code in the XToolkit has been a nightmare to
>>>>> maintain
>>>>> for
>>>>> quite a long time. This fix is a try to rewrite the code making it more
>>>>> understandable and maintainable.
>>>>>
>>>>> Testing: all more-or-less related automatic regression tests have been
>>>>> run.
>>>>> Categories include but not limited to: top-level tests, layouts tests,
>>>>> embedded frame tests, mouse events tests, focus tests, menu/popup menu
>>>>> tests, and some other. Nearly 60 tests were found failing with a clean
>>>>> build, so I filed the corresponding CRs. The rest pass with this fix
>>>>> on:
>>>>>
>>>>> linux-i586: Gnome/Metacity 2.24, Gnome/Compiz 2.24/0.7.8, KDE/KWin
>>>>> 4.1.3/3.0
>>>>>
>>>>> solaris-sparc: Gnome/Metacity 2.8, CDE/DTWM Solaris 10
>>>>>
>>>>> Please review the code at:
>>>>> http://cr.openjdk.java.net/~anthony/7-16-insets-6689983.0/
>>>>>
>>>>> Suggestions are welcome. Thanks!
>>>>>
>>>>> --
>>>>> best regards,
>>>>> Anthony
>>>>>
>>>
>>>
>