On 10/02/2013 02:52 PM, Oleg Pekhovskiy wrote:
On 01.10.2013 15:29, Anthony Petrov wrote:
I second to Leonid: you should add a comment and explain why you expect
exactly 4 (or more) events to be processed. Preferably, you should list
each event to clearly understand this.
I'll investigate why the previous number was 2...
A minor comment is, lines 2404 - 2407 should be moved to the nearest
try{} block at line 2409.
Agree.
Thanks.
A major concern is that I'm not sure the new solution is reliable in all
cases. Previously, we expected to get a notification from another
process (the WM). Now we process the notification ourselves and are the
owners of the selection. I don't know for sure, but suppose some xlib
implementations could optimize this scenario and process events locally
w/o even sending them to the X server. In which case there wouldn't be
any real synchronization of the native event queue. That's why
communicating with another process was an important part of this
procedure.
How about we try the original method first, and only if it fails, then
try this workaround solution?
In my fix XSendEvent is used to send SelectionNotify event and in the
doc http://www.x.org/archive/X11R7.5/doc/man/man3/XSendEvent.3.html
there is a paragraph saying that XSendEvent always works with the server:
"The event in the XEvent structure must be one of the core events or one
of the events defined by an extension (or a BadValue error results) so
that the X server can correctly byte-swap the contents as necessary. The
contents of the event are otherwise unaltered and unchecked by the X
server except to force send_event to True in the forwarded event and to
set the serial number in the event correctly; therefore these fields and
the display field are ignored by XSendEvent."
So no optimization expected in this case.
I don't see how this statement follows from the spec. An Xlib
implementation could still optimize requests when they manipulate
entities belonging to the same app.
Leaving the original method along with the new one will complicate the
code and could produce pitfalls. IMHO it would be better to thoroughly
test my fix.
That would be fine to do early in JDK 8, or early in JDK 9. But at this
point it is too risky for JDK 8. That's the reason Leonid and I suggest
to make it a workaround rather than the main method for syncing the
native event queue.
--
best regards,
Anthony
Also, this is a very sensitive method because a lot of code relies on
it. I suggest running all automatic regression tests for AWT and Swing
areas to make sure we don't introduce a regression with this fix.
As I mentioned Yuri ran tests on XFCE and LXDE. But we could do even
more of course.
--
best regards,
Anthony
On 09/26/2013 11:56 AM, Oleg Pekhovskiy wrote:
Hi Leonid,
I did it mostly logically, because my fix added one more service event
(SelectionRequest), that should be excluded from the number of
dispatched native events.
IMHO, the previous number "2" should have been commented, but, that did
not happen.
Thanks,
Oleg
On 25.09.2013 18:11, Leonid Romanov wrote:
Hi,
I'm not an expert in X11 programming, so I can't comment about the fix
in general, but I think the line 2436, "return getEventNumber() -
event_number > 3", really asks for a comment.
On 9/25/2013 16:38, Oleg Pekhovskiy wrote:
Hi all,
please review the fix
http://cr.openjdk.java.net/~bagiras/7033533.1/
for
https://bugs.openjdk.java.net/browse/JDK-7033533
Previous implementation of XToolkit.syncNativeQueue() relied upon
WM_S0 atom existence and that it was owned by current window manager.
But several WMs (like XFCE and LXDE) don't send SelectionNotify event
to the client on XConvertSelection() for that atom. That led
XToolkit.syncNativeQueue() to hang until TimeOutException.
I decided to keep XConvertSelection() usage, but make root toolkit
window as an owner for selection atom (with another name), and handle
SelectionRequest event from X Server, sending SelectionNotify in
response (as window manager is supposed to do).
Tested on both XFCE and LXDE.
Thanks,
Oleg
