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).
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<[email protected]> 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<[email protected]>
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
