On 10/22/2013 07:16 PM, Sergey Bylokhov wrote:
Hi, Anthony.
On 22.10.2013 17:38, Anthony Petrov wrote:
Hi Sergey,
I'm comparing the changes to XCanvasPeer and XComponentPeer, and I see
that previously we would trigger target.repaint() if the current
background of the peer is null (regardless of the argument value
passed to XCanvasPeer.setBackground().
The XComponentPeer.setBackground() won'd trigger a repaint if both the
current background color and the argument are nulls. Also, it doesn't
call target.repaint() directly as the former method in XCanvasPeer
did. I see that in XWindow.repaint() there's some logic that might
send a repaint event asynchronously depending on the current thread,
but still may not do so.
- This method was added to the XCanvasPeer in this 4947530 to stop
kind of recursion postRepaint->paint->postRepaint.
- Usage of target repaint is incorrect here, it should be used from
the user's code only, because it trigger UPDATE event instead of PAINT
event.
Also, XListPeer now dispatches target's paint() asynchronously through
an event whereas previously this method has always been called
directly. I believe I understand the reason for this: we want to post
an event instead of calling the method directly to avoid a
StackOverflowError. And this looks fine then.However, in case of the
XCanvasPeer I'm not sure we trigger target's repainting in all cases
depending on the current thread. I feel there's some change in logic
which is not properly documented or may even be a bug that could cause
some regressions in user code dealing with canvases.
What does it mean "not sure we trigger target's repainting in all cases
depending on the current thread"? If background is changed on the EDT we
See above: if you call Canvas.setBackground(null), currently this
triggers repainting of the canvas even if the background hasn't been set
previously. After your fix removes XCanvasPeer.setBackground(), this is
no longer true. So, -1 time we repaint the canvas. This is what I mean
under "not sure...". Let's hope this won't break user code (though it
would be worthwhile to investigate the previous logic and understand why
this was done...)
paint a native/target, if background changed on other thread we paint
native part and post PAINT event, which repaint the whole component later.
Ah, so in this case we actually use the XComponentPeer.paint() which
does paint the target synchronously. Now I see this. Could you please
add a comment just above the line 503 in XWindow.java about that to
clarify this? I'm OK with the fix with such a comment.
Another point is regarding the XWindow.reapint() changes alone.
Previously it has called paint(), and the latter simply called
paintPeer w/o repainting the target. Only the XComponentPeer repainted
the target in its overridden repaint() method. And this does seem
logical since it's the XComponentPeer that should maintain the link
between a peer and its target. So my question is, would it be more
logical to implement the if (dispatchThread) logic in
XComponentPeer.paint() instead of XWindow.repaint()?
It is not possible, because a paint() method can be called from the
Component.paintAll() on any thread and it should paint all parts of the
component(native/user's) synchronously.
I see. This looks weird (at least, method naming-wise), but let it be
so. Thanks for the clarification.
--
best regards,
Anthony
--
best regards,
Anthony
On 10/22/2013 03:19 PM, Sergey Bylokhov wrote:
New version here:
http://cr.openjdk.java.net/~serb/7090424/webrev.02
On 22.10.2013 13:52, Anthony Petrov wrote:
Hi Sergey,
Sorry, but I can't review a fix w/o a webrev.
What kind of problems do you experience with the tool? The cr.openjdk
should now have enough free space to host new webrevs. Please try to
re-upload the new webrev.
--
best regards,
Anthony
On 10/22/2013 12:28 PM, Sergey Bylokhov wrote:
I have a problem with the review tool. I assume that version .00 with
removed volatiles and renamed handleexpose event is ok for everybody?
On 10/21/13 4:53 PM, Sergey Bylokhov wrote:
Hi, Anthony.
Since cr have some issues with the space. I upload 2 files, where
handleExposeEvent was renamed to postPaintEvent():
http://cr.openjdk.java.net/~serb/7090424/webrev%2c01/src/solaris/classes/sun/awt/X11/XWindow.java.sdiff.html
http://cr.openjdk.java.net/~serb/7090424/webrev%2c01/src/solaris/classes/sun/awt/X11/XContentWindow.java.sdiff.html
On 18.10.2013 15:47, Sergey Bylokhov wrote:
Hi, Anthony.
On 18.10.2013 15:17, Anthony Petrov wrote:
Hi Sergey,
In XAWT, we usually use the StateLock to synchronize access to peer
fields (such as background, label, etc.) I don't think that
switching to volatile is a good idea since it prevents us from
performing atomic read/writes to the fields. And this is exactly
what we need for this fix, actually. In other words, the following
pattern works perfectly:
synchronized (lock) {
if (a != b) {
a = b;
// do stuff, or set a flag to do it later w/o the lock
}
}
whereas the following doesn't:
volatile a;
if (a != b) {
a = b;
// do stuff
}
The latter doesn't work because the value of 'a' may change from
another thread after the if() statement in the first thread is
executed.
If it will be changed that's ok. It is safe in this context since
there is no races. a will be the latest setted value and repaint
action will be done after a was set. Non trivial setters(like
setLabel/setText) are called under the locks in shared code.
Please note that this is critical for AWT because it is a
multi-threaded GUI toolkit.
src/solaris/classes/sun/awt/X11/XListPeer.java
- target.paint(g);
+ handleExposeEvent(target, 0, 0, getWidth(),
getHeight());
(the same applies to XWindow.repaint): can you please rename
XWindow.handleExposeEvent(Component...) to postPaintEvent() and
make
it final? A good javadoc for this method would also be appreciated,
because currently seeing the name handle*() I'd think it needs
to be
done under the awtLock().
Will do.
Also, for the corresponding changes in XWindow.repaint(), could you
please elaborate a bit more? Looking at the code I see that
XWindow.paint() calls paintPeer(). And in repaint(), you either
call
paint() or paintPeer() depending on the current thread. Why is it
needed? Can we just call paintPeer() (or paint() for that matter)
unconditionally since they both seem to result in the same call?
paintPeer is used to draw the native part of the peer(text of the
label, border of the button etc).
paint is a part of Component.paintAll(). And as a result of it call
it should paint the native part of the peer and it should call
appropriate paint method from the target.
Also, why don't we post an event if reapint() is invoked on the
EDT?
We do this in osx, but in xawt there are "smart" caches, which are
initialized during the paint of the peer. See
XLabelPeer&cachedFontMetrics for example . Note that this cache is
completely broken.
--
best regards,
Anthony
On 10/16/2013 06:00 PM, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk 8.
The fix has two parts
- Repaint method in the peer now paint the component in place
if it
was called on EDT only.
- Most of setters were changed to stop recursion if they were
called
on EDT.
Bug: https://bugs.openjdk.java.net/browse/JDK-7090424
Webrev can be found at:
http://cr.openjdk.java.net/~serb/7090424/webrev.00
