<AWT Dev> Review request: 6689983 (reevaluate our inset-related code in XAWT)
Anthony.Petrov at Sun.COM
Fri Jul 31 09:00:50 PDT 2009
On 07/22/2009 11:18 AM, Oleg Sukhodolsky wrote:
> Ok, so there are two main purposes:
> - to move most of insets-guessing code to XDecorated Peer.
> - better support for modern WMs
Well... The CR might be honestly renamed to "Re-factor the
insets-related code... completely". :) While I agree that this is far
from perfect to rewrite such a big chunk of code, I should also admit
that the existing code is hardly maintainable.
> 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.
I agree with the point: the smaller the changes, the easier to review.
But due to the nature of the current state of the fix I doubt if it is
doable now. E.g., I could drop the XWM.reset() method and its invocation
from the XDecPeer, but is it worthwhile to file a separate CR for just
about 5-10 lines of quite clear code that is quite related to the aims
of the fix? Note that all these small pieces would reduce the fix size
insignificantly (maybe ~3-5% at most?), thus not actually reducing the
complexity of the fix.
PS. To keep you up to date: the fix has been tweaked quite a lot since
the version #0. Apart from applying the suggestions expressed on the
mailing list, some problems were fixed when running on a remote DISPLAY
on the Cygwin X server with their "native" window manager. Though we do
not officially support this configuration, we still want to work
acceptably in some most popular cases (notably, when running the
NetBeans IDE). Currently I'm in the process of re-running the regression
tests. Hopefully the next version will follow sometime next week. Please
stay tuned. And I want to thank you again for your passionate,
unquenchable interest in this area! This is very appreciated!
More information about the awt-dev