Hi Anthony, sorry for delay (I was on vacation).
On Wed, Jul 8, 2009 at 6:05 PM, Anthony Petrov<[email protected]> 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<[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 >>>>> >>> >>> >
