Re: AWT Dev [9] Review request for 8014755: [TEST_BUG] frames didn't closed after execution some awt/dnd/ tests

[Oleg Pekhovskiy]

Hi Sergey,

thanks you for comments!

Please review the second version of fix here:
http://cr.openjdk.java.net/~bagiras/9/8014755.2/

I've added re-throwing for catch blocks and commented System.exit() 
calls that are used only for child VMs.


Thanks,
Oleg

On 13.05.2014 14:55, Sergey Bylokhov wrote:

Hi. Oleg.
A few notes.
  - Some tests contains empty/non-rethrow catch blocks. I guess we
should rethrow an exception and the test should fail in this case.
  - Some tests use System.exit() which should be replaced by the exception.

On 5/9/14 7:32 PM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/9/8014755.1/
for
https://bugs.openjdk.java.net/browse/JDK-8014755

These tests were moved from the closed workspace.

The main idea of fix is to force termination of child JVM if it's not
exited automatically after the test sequence.

Tests accuracy was improved to cover all error cases.

destroyProcess() method was added to
test.java.awt.regtesthelpers.process.ProcessCommunicator
for termination of child JVM when needed.

Thanks,
Oleg

Re: AWT Dev [9] Review request for 8014755: [TEST_BUG] frames didn't closed after execution some awt/dnd/ tests

[Oleg Pekhovskiy]

Hi Petr,

thank you for the review, here are my thoughts:

1. I added new method that way after analyzing the other methods and 
their usage in jtreg tests. If you look at getExecutionCommand() you 
could see the similar solution. Indeed my changes don't break the 
previous usages of ProcessCommunicator. It's also possible to perform 
bunch run, but of course without the termination ability. Another issue 
is that we must kill a child VM explicitly if it doesn't exit 
automatically, but we must do this only after test sequence is passed.
jtreg doesn't kill child VMs on timeout, this is the known problem 
(that's why this JIRA issue exists).


2. I decided to leave it as is, as main classes in these tests are used 
in two cases: applet itself and the main class for child VM.


3. I found this mistake and added it to the second version of fix.

Regards,
Oleg

On 14.05.2014 14:01, Petr Pchelko wrote:

Hello, Oleg.

1. I'm not sure your about your new API in ProcessCommunicator.
First of all, now the communicator can't be used twice in a single test to open 
and then close a bunch of processes.
Also, you need to modify the tests themselves.
I think it would be better to do the termination implicitly: you can get jtreg 
property with execution timeout, start a timer
thread when you create a process and kill it after the timeout. In case 
doWaitFor finishes you kill the timer thread.

2. We normally remove .html if it's possible to convert test to a standalone 
app.

3. NoFormatsCrashTest lacks .java file.

With best regards. Petr.

On 13 мая 2014 г., at 14:55, Sergey Bylokhov sergey.bylok...@oracle.com wrote:


Hi. Oleg.
A few notes.
- Some tests contains empty/non-rethrow catch blocks. I guess we should rethrow 
an exception and the test should fail in this case.
- Some tests use System.exit() which should be replaced by the exception.

On 5/9/14 7:32 PM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/9/8014755.1/
for
https://bugs.openjdk.java.net/browse/JDK-8014755

These tests were moved from the closed workspace.

The main idea of fix is to force termination of child JVM if it's not exited 
automatically after the test sequence.

Tests accuracy was improved to cover all error cases.

destroyProcess() method was added to
test.java.awt.regtesthelpers.process.ProcessCommunicator
for termination of child JVM when needed.

Thanks,
Oleg




--
Best regards, Sergey.

AWT Dev [9] Review request for 8014755: [TEST_BUG] frames didn't closed after execution some awt/dnd/ tests

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/9/8014755.1/
for
https://bugs.openjdk.java.net/browse/JDK-8014755

These tests were moved from the closed workspace.

The main idea of fix is to force termination of child JVM if it's not 
exited automatically after the test sequence.


Tests accuracy was improved to cover all error cases.

destroyProcess() method was added to
test.java.awt.regtesthelpers.process.ProcessCommunicator
for termination of child JVM when needed.

Thanks,
Oleg

Re: AWT Dev [9] Review Request: 8042007 Javadoc cleanup of javax.sound.sampled.spi package

[Oleg Pekhovskiy]

Hi Sergey,

the fix looks good.

Regards,
Oleg

On 28.04.2014 20:33, Sergey Bylokhov wrote:

Hello.
Please review another one javadoc weekend cleanup in jdk 9 in sound area.
  - 80 column limit
  - tags replaced to @tags
  - empty line after description/before the first tag
  - sets of spaces in the middle of text were deleted
  - @param, @throws, @return now align, to be more readable

Bug: https://bugs.openjdk.java.net/browse/JDK-8042007
Webrev can be found at: http://cr.openjdk.java.net/~serb/8042007/webrev.00

AWT Dev [9] Review request for 8014754: [TEST_BUG] child.exe remains after execution java/awt/dnd/ Win32TYMEDSelectionTest and Win32DropTYMEDSelectionTest

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/9/8014754.1/
for
https://bugs.openjdk.java.net/browse/JDK-8014754

Start of 'child.exe' has no timeout, that's why if dragging fails the 
executable keeps running and prevents temporary folder from being cleared.
So I forcibly terminate the process if it was not exited after dnd 
action (lines 74-76 in Win32TYMEDSelectionTest.java)


PS: Webrev doesn't include child.exe but it's also moved with the others 
from the closed repository.


Thanks,
Oleg

Re: AWT Dev [9] Request for review: 8019990: IM candidate window appears on the South-East corner of the display.

[Oleg Pekhovskiy]

Hi Andrew,

the fix looks good.

Regards,
Oleg

On 02.04.2014 14:45, Andrew Brygin wrote:

Hello,

  could you please review  a fix for 8019990?

  bug: https://bugs.openjdk.java.net/browse/JDK-8019990
  webrev: http://cr.openjdk.java.net/~bae/8019990/9/webrev.00/

  The problem was triggered by the fix for 7024749, which moved
  IME messages handling from awt_Component to awt_Frame. Due
  to this change, the WM_IME_SETCONTEXT never reaches the applet's
  component, and candidate window position is never adjusted.

  Suggested fix reverts a part of the fix for 7024749: messages
  WM_IME_SETCONTEXT and WM_IME_NOTIFY are handled in
  awt_Component again.

  To prevent the crash described in 7024749, I suggest to check
  whether the proxy is enabled before routing messages to it
  (see awt_Component.cpp, line 4090).

  I have verified that the suggested change does not trigger any
  automatic regression test related to IM.

  Unfortunately, there is no new regression test related to the candidate
  window position because this problem can be reproduced in a browser,
  but does not appear in applet-based regression tests.

  Please take a look.

Thanks,
Andrew

Re: AWT Dev [9] Review Request: 8037868 The build is broken after the JDK-8035630

[Oleg Pekhovskiy]

Hi Sergey,

the fix looks good.

Regards,
Oleg

On 19.03.2014 20:45, Sergey Bylokhov wrote:

Please review the small fix:

diff -r fe79a65a51d8 src/windows/native/sun/font/fontpath.c
--- a/src/windows/native/sun/font/fontpath.cWed Mar 19 16:13:59 2014
+0400
+++ b/src/windows/native/sun/font/fontpath.cWed Mar 19 20:40:44 2014
+0400
@@ -155,7 +155,7 @@
  if (fullname == NULL) {
  (*env)-ExceptionClear(env);
  return 1;
-
+}
  fullnameLC = (*env)-CallObjectMethod(env, fullname,
fmi-toLowerCaseMID,
fmi-locale);
  (*env)-CallBooleanMethod(env, fmi-list, fmi-addMID, fullname);

AWT Dev [9] Review request for 8037377: Windows: compilation failed after the fix for 8033712

[Oleg Pekhovskiy]

Hi team,

please review the typo issue fix:
http://cr.openjdk.java.net/~bagiras/9/8037377.1/
for
https://bugs.openjdk.java.net/browse/JDK-8037377


Thanks,
Oleg

Re: AWT Dev [9] Review Request: JDK-8037287 Windows build failed after JDK-8030787

[Oleg Pekhovskiy]

Hi, Petr,

the fix looks good.

Regards,
Oleg

On 13.03.2014 14:57, Petr Pchelko wrote:

Hello, AWT Team.

Sorry, I've broken the Windows build... Compiles fine on Mac and Linux, but 
fails on windows.
The bug: https://bugs.openjdk.java.net/browse/JDK-8037287
The fix: http://cr.openjdk.java.net/~pchelko/9/8037287/webrev/

With best regards. Petr.

Re: AWT Dev [9] Review Request: 8036103 Cleanup of java.awt and java.awt.peer packages

[Oleg Pekhovskiy]

Hi Sergey,

the fix looks good.

Regards,
Oleg.

On 03.03.2014 23:26, Sergey Bylokhov wrote:

Hello.
Please review a weekend cleanup in jdk 9.
typos,final,replacement of old tags, unused imports.

Bug: https://bugs.openjdk.java.net/browse/JDK-8036103
Webrev can be found at: http://cr.openjdk.java.net/~serb/8036103/webrev

Re: AWT Dev [9] Review request for 8031694: [macosx] TwentyThousandTest test intermittently hangs

[Oleg Pekhovskiy]

Thank you, guys!

Regards,
Oleg

On 12.02.2014 21:52, Anthony Petrov wrote:

Oleg, Artem, thank you for the clarifications.

I'm fine with the webrev version .2, too.

--
best regards,
Anthony

On 2/12/2014 4:58 PM, Artem Ananiev wrote:


On 2/10/2014 9:17 PM, Anthony Petrov wrote:

Thanks for the clarifications. Note that given that we re-create the EDT
if there are more events in the queue, I'm still unsure whether we
regress or not. I recall there was a patch submitted on this mailing
list a few years ago that made the EDT die unconditionally and never be
resurrected if it's requested to die. So I'm afraid the code that relies
on this behavior will stop working correctly after your fix because you
will re-create the EDT for all remaining events.


I can barely imagine such a code that kills EDT and expect it to have
died forever :) This is a violation of AWT contract, when event
dispatching is started as soon as a new event is posted. If applications
needs another behavior, it has to make sure no events are posted to AWT
event queue.

Oleg,

webrev .2 looks fine to me.

Thanks,

Artem


What tests did you run with your fix?

--
best regards,
Anthony

On 2/7/2014 8:18 PM, Oleg Pekhovskiy wrote:

Hi Anthony,

there are two points for choosing this solution:
1. If something makes EDT to die, there is a serious reason to do so.
It's a forced action.
So it should be done ASAP. Dying EDT usage for pumping followed events
looks strange because we expect him to die.
Moreover it could happen that events are posted quite frequently to
keep
dying EDT alive for some period of time.

2. Synchronization object for posting events from different threads is
EventQueue.pushPopLock.
it is used in EventQueue. postEventPrivate(), EventQueue.getNextEvent()
and EventQueue. detachDispatchThread().
When dying EDT returns from
EventDispatchThread.pumpEventsForFilter() to
EventDispatchThread.run() and then goes to
getEventQueue().detachDispatchThread(), EventQueue.pushPopLock is not
used, so any other thread could post events.
So if we don't call peekEvent() to recreate a new EDT, we'll just loose
these events as it currently happens.

So the main idea is to make EDT life cycle phases obvious.

Thanks,
Oleg

On 02/07/2014 06:48 PM, Anthony Petrov wrote:

Hi Oleg,

This code is very tricky. I like it that we process any events that
might be posted to the queue after the current EDT dies. However,
could you please clarify how initializing a new EDT is any different
from not letting the old one die? I.e. could we just not kill the old
EDT if we see there are more events in the queue? If not, what exact
difference does you solution bring?

It's not that I'm against your fix, it looks good actually. I'd just
like to understand the difference. Please elaborate.
Also, I recall we've fixed a number of bugs in this area. Are we sure
we don't regress after this fix?

--
best regards,
Anthony

On 2/7/2014 4:31 AM, Oleg Pekhovskiy wrote:

Hi all,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8031694.2/

We with Artem Ananiev had off-line discussion and he offered let the
dying EDT to die
and process unhandled events by forcing another EDT start.

Thanks,
Oleg

On 01/28/2014 05:32 AM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8031694.1/
for
https://bugs.openjdk.java.net/browse/JDK-8031694

During forward-port of JDK-7189350 EDT.doDispatch was not taken into
account when calling EventQueue.detachDispatchThread().
As a result harmful optimization of this method occurred.
So when doDispatch became false, no more events in QventQueue were
handled before EDT shutdown.
I kept the optimization but added the check to
EDT.pumpEventsForFilter() that EventQueue is not empty to keep
pumping.

Thanks,
Oleg

Re: AWT Dev [9] Review request for 8031694: [macosx] TwentyThousandTest test intermittently hangs

[Oleg Pekhovskiy]

Hi Anthony,

I ran only
java/awt/EventDispatchThread
java/awt/EventQueue
and the same from closed.

No difference found.

It would be great if you propose some other tests that could be 
sensitive to the fix.


Thanks,
Oleg

On 10.02.2014 21:17, Anthony Petrov wrote:

Thanks for the clarifications. Note that given that we re-create the EDT
if there are more events in the queue, I'm still unsure whether we
regress or not. I recall there was a patch submitted on this mailing
list a few years ago that made the EDT die unconditionally and never be
resurrected if it's requested to die. So I'm afraid the code that relies
on this behavior will stop working correctly after your fix because you
will re-create the EDT for all remaining events.

What tests did you run with your fix?

--
best regards,
Anthony

On 2/7/2014 8:18 PM, Oleg Pekhovskiy wrote:

Hi Anthony,

there are two points for choosing this solution:
1. If something makes EDT to die, there is a serious reason to do so.
It's a forced action.
So it should be done ASAP. Dying EDT usage for pumping followed events
looks strange because we expect him to die.
Moreover it could happen that events are posted quite frequently to keep
dying EDT alive for some period of time.

2. Synchronization object for posting events from different threads is
EventQueue.pushPopLock.
it is used in EventQueue. postEventPrivate(), EventQueue.getNextEvent()
and EventQueue. detachDispatchThread().
When dying EDT returns from EventDispatchThread.pumpEventsForFilter() to
EventDispatchThread.run() and then goes to
getEventQueue().detachDispatchThread(), EventQueue.pushPopLock is not
used, so any other thread could post events.
So if we don't call peekEvent() to recreate a new EDT, we'll just loose
these events as it currently happens.

So the main idea is to make EDT life cycle phases obvious.

Thanks,
Oleg

On 02/07/2014 06:48 PM, Anthony Petrov wrote:

Hi Oleg,

This code is very tricky. I like it that we process any events that
might be posted to the queue after the current EDT dies. However,
could you please clarify how initializing a new EDT is any different
from not letting the old one die? I.e. could we just not kill the old
EDT if we see there are more events in the queue? If not, what exact
difference does you solution bring?

It's not that I'm against your fix, it looks good actually. I'd just
like to understand the difference. Please elaborate.
Also, I recall we've fixed a number of bugs in this area. Are we sure
we don't regress after this fix?

--
best regards,
Anthony

On 2/7/2014 4:31 AM, Oleg Pekhovskiy wrote:

Hi all,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8031694.2/

We with Artem Ananiev had off-line discussion and he offered let the
dying EDT to die
and process unhandled events by forcing another EDT start.

Thanks,
Oleg

On 01/28/2014 05:32 AM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8031694.1/
for
https://bugs.openjdk.java.net/browse/JDK-8031694

During forward-port of JDK-7189350 EDT.doDispatch was not taken into
account when calling EventQueue.detachDispatchThread().
As a result harmful optimization of this method occurred.
So when doDispatch became false, no more events in QventQueue were
handled before EDT shutdown.
I kept the optimization but added the check to
EDT.pumpEventsForFilter() that EventQueue is not empty to keep
pumping.

Thanks,
Oleg

Re: AWT Dev [9] Review request for 8031694: [macosx] TwentyThousandTest test intermittently hangs

[Oleg Pekhovskiy]

Hi Anthony,

there are two points for choosing this solution:
1. If something makes EDT to die, there is a serious reason to do so. 
It's a forced action.
So it should be done ASAP. Dying EDT usage for pumping followed events 
looks strange because we expect him to die.
Moreover it could happen that events are posted quite frequently to keep 
dying EDT alive for some period of time.


2. Synchronization object for posting events from different threads is 
EventQueue.pushPopLock.
it is used in EventQueue. postEventPrivate(), EventQueue.getNextEvent() 
and EventQueue. detachDispatchThread().
When dying EDT returns from EventDispatchThread.pumpEventsForFilter() to 
EventDispatchThread.run() and then goes to
getEventQueue().detachDispatchThread(), EventQueue.pushPopLock is not 
used, so any other thread could post events.
So if we don't call peekEvent() to recreate a new EDT, we'll just loose 
these events as it currently happens.


So the main idea is to make EDT life cycle phases obvious.

Thanks,
Oleg

On 02/07/2014 06:48 PM, Anthony Petrov wrote:

Hi Oleg,

This code is very tricky. I like it that we process any events that 
might be posted to the queue after the current EDT dies. However, 
could you please clarify how initializing a new EDT is any different 
from not letting the old one die? I.e. could we just not kill the old 
EDT if we see there are more events in the queue? If not, what exact 
difference does you solution bring?


It's not that I'm against your fix, it looks good actually. I'd just 
like to understand the difference. Please elaborate.
Also, I recall we've fixed a number of bugs in this area. Are we sure 
we don't regress after this fix?


--
best regards,
Anthony

On 2/7/2014 4:31 AM, Oleg Pekhovskiy wrote:

Hi all,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8031694.2/

We with Artem Ananiev had off-line discussion and he offered let the
dying EDT to die
and process unhandled events by forcing another EDT start.

Thanks,
Oleg

On 01/28/2014 05:32 AM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8031694.1/
for
https://bugs.openjdk.java.net/browse/JDK-8031694

During forward-port of JDK-7189350 EDT.doDispatch was not taken into
account when calling EventQueue.detachDispatchThread().
As a result harmful optimization of this method occurred.
So when doDispatch became false, no more events in QventQueue were
handled before EDT shutdown.
I kept the optimization but added the check to
EDT.pumpEventsForFilter() that EventQueue is not empty to keep pumping.

Thanks,
Oleg

Re: AWT Dev [9] Review request for 8020443: Frame is not created on the specified GraphicsDevice with two monitors

[Oleg Pekhovskiy]

Hi Alexander,

thank you for your comments, and let me pay attention to the wording:

I wrote struts but not coordinates of the struts.
I did it intentionally: here is the dump of 
XToolkit.getScreenInsetsManually()

method on the second screen:

Root bounds: java.awt.Rectangle[x=0,y=0,width=2048,height=768]
Screen bounds: java.awt.Rectangle[x=1024,y=0,width=1024,height=768]
Window #33554540 bounds[x=1024, y=24, width=1024, height=744] 
struts[left=1089, right=0, top=0, bottom=0]
Window #31457297 bounds[x=1024, y=0, width=1024, height=24] 
struts[left=0, right=0, top=24, bottom=0]


As we see struts could be 0, even if their edge is not the same as the 
root window edge.

So if we are talking about:
left_start_y, left_end_y, right_start_y, right_end_y, top_start_x, 
top_end_x, bottom_start_x

of _NET_WM_STRUT_PARTIAL
I agree with you, but if we are talking about:
left, right, top, bottom
which is the case, I don't, and treat my comments as correct.

Thanks,
Oleg

On 02/06/2014 02:34 PM, Alexander Zvegintsev wrote:

Hi Oleg,

I am a little concerned about this comment:

* struts*could be*relative to root window bounds, so


but specification[1] clearly says:

All coordinates are root window coordinates.
so I think could be should be replaced by are (there is no need 
for a new webrev)


Otherwise, the fix looks good to me.
BTW, I like your idea with frame maximization to check insets.

[1] 
http://standards.freedesktop.org/wm-spec/wm-spec-latest.html#idp6337880

Thanks,

Alexander.
On 02/06/2014 10:21 AM, Oleg Pekhovskiy wrote:

Hi Alexander,

thanks for the comments,
please see the next version of fix:
http://cr.openjdk.java.net/~bagiras/8020443.3/

Updates:
1. Left usage of root window coordinates because they passed to
XToolkit.getScreenInsetsManually() in Rectangle, not just Dimension.
Added comment for struts check.

2. Rewrote the test to make it accurate.
Tested on Ubuntu 12.04, Solaris 11 and Oracle Linux 6.4.

Thanks,
Oleg

On 02/04/2014 07:50 PM, Alexander Zvegintsev wrote:

Oleg,

please see inline

On 02/04/2014 05:00 PM, Oleg Pekhovskiy wrote:

Hi Alexander,

thank you for the review!

1. The problem is that I didn't find that root window ALWAYS starts 
with 0, 0 in X11 docs.

Could you please point such statement out to avoid uncertainty?
Unfortunately, I cannot find any document which clearly specifies 
this. is the root of this hierarchy.
On my understanding root window is the root of windows treehierarchy 
and all other windows are children or descendants of it.
So root window is the origin for all other windows and and I assume 
that it has (0,0) coordinates always and I would

be surprised if it not.

2. Checking insets against screen width and height can fail too. 
Example:

1st screen (0, 0, 1024, 768) - x, y, width, height
2nd screen (1024, 0, 1920, 1080)
Struts for the 2nd screen could be (1074, 0, 20, 0) - left, right, 
top, bottom.

So left strut 1075 fits in 1920 but should be 50 as inset.

Yes, my check will fail on this case.
PS: Just curious: you said that screen (1000, 50, 1000, 1000) is a 
valid case.
Is it valid for testing environment? As I see screens are left-top 
aligned.
It means that if two screens are located from left to right, they 
both have y == 0 regardless of their height.
It is a valid case, screens can be arranged in any order, screens 
even can be overlapped, may have empty spaces between.

(and we definitely will not support last 2 cases).

My previous example was a little abstract, here is the real one with 
screens aligned on bottom edge:

Screen #1 (0,0, 1920, 1080)
Screen #2 (1920, 30, 1680, 1050) with top inset 35

It is unlikely that we will meet such configuration, but who knows.

Since this test will run on machines with environment close to 
default I propose to made an assumption
that insets are not greater that a quarter (or 1/3?) of width and 
height (and add a comment why we do that

and why test may fail):

if (insets.left = bounds.width / 4
|| insets.right = bounds.width / 4
|| insets.top = bounds.height / 4
|| insets.bottom = bounds.height / 4) {


Thanks,

Alexander.


Usually insets are much less than dimensions of a screen and so 
strut minus screen edge should be less

than summary dimension of neighbor screens.
Please, correct me if I'm wrong.

Thanks,
Oleg

04.02.2014 15:28, Alexander Zvegintsev wrote:

Hi Oleg,

I am just curious about adding rootBounds.x and rootBounds.y, it 
looks redundant to me.
Is there a case when a root window have coordinates with non-zero 
values?


MultiScreenInsetsTest.java:

   62 if ((bounds.x != 0  insets.left = bounds.x)
   63 || (bounds.y != 0  insets.top = 
bounds.y)) {
This check will fail for screen with 
[x=1000,y=50,width=1000,height=1000] bounds and top inset 100,

but it is a valid case.

I think we should check insets against screen width and height:
if (insets.left = bounds.width

Re: AWT Dev [9] Review request for 8031694: [macosx] TwentyThousandTest test intermittently hangs

[Oleg Pekhovskiy]

Hi all,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8031694.2/

We with Artem Ananiev had off-line discussion and he offered let the 
dying EDT to die

and process unhandled events by forcing another EDT start.

Thanks,
Oleg

On 01/28/2014 05:32 AM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8031694.1/
for
https://bugs.openjdk.java.net/browse/JDK-8031694

During forward-port of JDK-7189350 EDT.doDispatch was not taken into 
account when calling EventQueue.detachDispatchThread().

As a result harmful optimization of this method occurred.
So when doDispatch became false, no more events in QventQueue were 
handled before EDT shutdown.
I kept the optimization but added the check to 
EDT.pumpEventsForFilter() that EventQueue is not empty to keep pumping.


Thanks,
Oleg

Re: AWT Dev [9] Review request for 8020443: Frame is not created on the specified GraphicsDevice with two monitors

[Oleg Pekhovskiy]

Hi Alexander,

thanks for the comments,
please see the next version of fix:
http://cr.openjdk.java.net/~bagiras/8020443.3/

Updates:
1. Left usage of root window coordinates because they passed to
XToolkit.getScreenInsetsManually() in Rectangle, not just Dimension.
Added comment for struts check.

2. Rewrote the test to make it accurate.
Tested on Ubuntu 12.04, Solaris 11 and Oracle Linux 6.4.

Thanks,
Oleg

On 02/04/2014 07:50 PM, Alexander Zvegintsev wrote:

Oleg,

please see inline

On 02/04/2014 05:00 PM, Oleg Pekhovskiy wrote:

Hi Alexander,

thank you for the review!

1. The problem is that I didn't find that root window ALWAYS starts 
with 0, 0 in X11 docs.

Could you please point such statement out to avoid uncertainty?
Unfortunately, I cannot find any document which clearly specifies 
this. is the root of this hierarchy.
On my understanding root window is the root of windows treehierarchy 
and all other windows are children or descendants of it.
So root window is the origin for all other windows and and I assume 
that it has (0,0) coordinates always and I would

be surprised if it not.

2. Checking insets against screen width and height can fail too. 
Example:

1st screen (0, 0, 1024, 768) - x, y, width, height
2nd screen (1024, 0, 1920, 1080)
Struts for the 2nd screen could be (1074, 0, 20, 0) - left, right, 
top, bottom.

So left strut 1075 fits in 1920 but should be 50 as inset.

Yes, my check will fail on this case.
PS: Just curious: you said that screen (1000, 50, 1000, 1000) is a 
valid case.
Is it valid for testing environment? As I see screens are left-top 
aligned.
It means that if two screens are located from left to right, they 
both have y == 0 regardless of their height.
It is a valid case, screens can be arranged in any order, screens even 
can be overlapped, may have empty spaces between.

(and we definitely will not support last 2 cases).

My previous example was a little abstract, here is the real one with 
screens aligned on bottom edge:

Screen #1 (0,0, 1920, 1080)
Screen #2 (1920, 30, 1680, 1050) with top inset 35

It is unlikely that we will meet such configuration, but who knows.

Since this test will run on machines with environment close to default 
I propose to made an assumption
that insets are not greater that a quarter (or 1/3?) of width and 
height (and add a comment why we do that

and why test may fail):

if (insets.left = bounds.width / 4
|| insets.right = bounds.width / 4
|| insets.top = bounds.height / 4
|| insets.bottom = bounds.height / 4) {


Thanks,

Alexander.


Usually insets are much less than dimensions of a screen and so strut 
minus screen edge should be less

than summary dimension of neighbor screens.
Please, correct me if I'm wrong.

Thanks,
Oleg

04.02.2014 15:28, Alexander Zvegintsev wrote:

Hi Oleg,

I am just curious about adding rootBounds.x and rootBounds.y, it 
looks redundant to me.
Is there a case when a root window have coordinates with non-zero 
values?


MultiScreenInsetsTest.java:

   62 if ((bounds.x != 0  insets.left = bounds.x)
   63 || (bounds.y != 0  insets.top = bounds.y)) {
This check will fail for screen with 
[x=1000,y=50,width=1000,height=1000] bounds and top inset 100,

but it is a valid case.

I think we should check insets against screen width and height:
if (insets.left = bounds.width
|| insets.right = bounds.width
|| insets.top = bounds.height
|| insets.bottom = bounds.height) {


Otherwise fix looks good to me.

Thanks,

Alexander.
On 02/04/2014 10:26 AM, Oleg Pekhovskiy wrote:

Hi All,

please review the second version of fix:
http://cr.openjdk.java.net/~bagiras/8020443.2/

It turned out that struts must be checked and corrected from all sides
to become the proper screen insets.

Thanks,
Oleg

On 01/21/2014 06:31 PM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8020443.1/
for
https://bugs.openjdk.java.net/browse/JDK-8020443

Referring to the standards, we must calculate insets the special 
way for Xinerama:

http://standards.freedesktop.org/wm-spec/1.3/ar01s05.html

_NET_WM_STRUT_PARTIAL
The start and end values associated with each strut allow areas 
to be reserved which do not span the entire width or height of the 
screen. Struts MUST be specified in root window coordinates, that 
is, they are /not/ relative to the edges of any view port or 
Xinerama monitor.


So the fix checks if the insets have absolute values and if so 
makes them relative to each screen.
The issue occurred when the Frame was created with the location by 
default.


Thanks,
Oleg

Re: AWT Dev [9] Review request for 8020443: Frame is not created on the specified GraphicsDevice with two monitors

[Oleg Pekhovskiy]

Hi Alexander,

thank you for the review!

1. The problem is that I didn't find that root window ALWAYS starts with 
0, 0 in X11 docs.

Could you please point such statement out to avoid uncertainty?

2. Checking insets against screen width and height can fail too. Example:
1st screen (0, 0, 1024, 768) - x, y, width, height
2nd screen (1024, 0, 1920, 1080)
Struts for the 2nd screen could be (1074, 0, 20, 0) - left, right, top, 
bottom.

So left strut 1075 fits in 1920 but should be 50 as inset.

PS: Just curious: you said that screen (1000, 50, 1000, 1000) is a valid 
case.

Is it valid for testing environment? As I see screens are left-top aligned.
It means that if two screens are located from left to right, they both 
have y == 0 regardless of their height.
Usually insets are much less than dimensions of a screen and so strut 
minus screen edge should be less

than summary dimension of neighbor screens.
Please, correct me if I'm wrong.

Thanks,
Oleg

04.02.2014 15:28, Alexander Zvegintsev wrote:

Hi Oleg,

I am just curious about adding rootBounds.x and rootBounds.y, it looks 
redundant to me.

Is there a case when a root window have coordinates with non-zero values?

MultiScreenInsetsTest.java:

   62 if ((bounds.x != 0  insets.left = bounds.x)
   63 || (bounds.y != 0  insets.top = bounds.y)) {
This check will fail for screen with 
[x=1000,y=50,width=1000,height=1000] bounds and top inset 100,

but it is a valid case.

I think we should check insets against screen width and height:
if (insets.left = bounds.width
|| insets.right = bounds.width
|| insets.top = bounds.height
|| insets.bottom = bounds.height) {


Otherwise fix looks good to me.

Thanks,

Alexander.
On 02/04/2014 10:26 AM, Oleg Pekhovskiy wrote:

Hi All,

please review the second version of fix:
http://cr.openjdk.java.net/~bagiras/8020443.2/

It turned out that struts must be checked and corrected from all sides
to become the proper screen insets.

Thanks,
Oleg

On 01/21/2014 06:31 PM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8020443.1/
for
https://bugs.openjdk.java.net/browse/JDK-8020443

Referring to the standards, we must calculate insets the special way 
for Xinerama:

http://standards.freedesktop.org/wm-spec/1.3/ar01s05.html

_NET_WM_STRUT_PARTIAL
The start and end values associated with each strut allow areas to 
be reserved which do not span the entire width or height of the 
screen. Struts MUST be specified in root window coordinates, that 
is, they are /not/ relative to the edges of any view port or 
Xinerama monitor.


So the fix checks if the insets have absolute values and if so makes 
them relative to each screen.
The issue occurred when the Frame was created with the location by 
default.


Thanks,
Oleg

Re: AWT Dev [9] Review request for 8020443: Frame is not created on the specified GraphicsDevice with two monitors

[Oleg Pekhovskiy]

Hi All,

please review the second version of fix:
http://cr.openjdk.java.net/~bagiras/8020443.2/

It turned out that struts must be checked and corrected from all sides
to become the proper screen insets.

Thanks,
Oleg

On 01/21/2014 06:31 PM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8020443.1/
for
https://bugs.openjdk.java.net/browse/JDK-8020443

Referring to the standards, we must calculate insets the special way 
for Xinerama:

http://standards.freedesktop.org/wm-spec/1.3/ar01s05.html

_NET_WM_STRUT_PARTIAL
The start and end values associated with each strut allow areas to be 
reserved which do not span the entire width or height of the screen. 
Struts MUST be specified in root window coordinates, that is, they are 
/not/ relative to the edges of any view port or Xinerama monitor.


So the fix checks if the insets have absolute values and if so makes 
them relative to each screen.
The issue occurred when the Frame was created with the location by 
default.


Thanks,
Oleg

Re: AWT Dev [9] Review request for 8013116: Robot moves mouse to point which differs from set in mouseMove on Unity shell

[Oleg Pekhovskiy]

Hi Team,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8013116.3

Javadoc changes were moved to a separate issue JDK-8033128.

Thanks,
Oleg


On 21.01.2014 11:53, Oleg Pekhovskiy wrote:

Hi Sergey,

I tested on Windows, Mac OS X and Ubuntu. As a result I found
calculation error in CRobot.mouseEvent native method.
It was fixed.
I also updated Javadoc for Robot(GraphicsDevice) constructor
(CCC request will be done).

Please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8013116.2

I built JDK8 with my fix and ran JCK runtime test suite for 3 platforms
for 'api.java_applet', 'api.java_awt' and 'api.javax_swing' branches.
There were no differences for Mac OS X and Windows and one test fixed on
Ubuntu
(api/java_awt/Container/getMousePosition_NotHeadless).

Thanks,
Oleg

On 01/14/2014 09:42 PM, Sergey Bylokhov wrote:

On 14.01.2014 21:14, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8013116.1/
for
https://bugs.openjdk.java.net/browse/JDK-8013116

java.awt.Robot.gdLoc field was removed to make Robot's point of
origin be the same as in setters and getters from
GraphicsDevice (GraphicsConfiguration), MouseInfo (PointerInfo) and
Frame.

This makes sense especially on Ubuntu with Xinerama extension where
all screens have common coordinate space.

Did it work on other platfroms before the fix(windows, osx)? If yes, why?


Two helper methods were added to
'test/java/awt/regtesthelpers/Util.java'.
They were used in regression test for this fix. These methods could
be useful for asynchronous window managers.

Thanks,
Oleg

AWT Dev [9] Review request for 8031694: [macosx] TwentyThousandTest test intermittently hangs

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8031694.1/
for
https://bugs.openjdk.java.net/browse/JDK-8031694

During forward-port of JDK-7189350 EDT.doDispatch was not taken into 
account when calling EventQueue.detachDispatchThread().

As a result harmful optimization of this method occurred.
So when doDispatch became false, no more events in QventQueue were 
handled before EDT shutdown.
I kept the optimization but added the check to EDT.pumpEventsForFilter() 
that EventQueue is not empty to keep pumping.


Thanks,
Oleg

Re: AWT Dev [9] Review request for 7033533: realSync() doesn't work with Xfce

[Oleg Pekhovskiy]

Hi Anthony,

thank you for the review,

seems like these lines are redundant for two reasons:
1. We already have waiting cycle before these lines (2418-2429).
2. These lines are entered only on XConvertSelection failure with simple 
X Server (for example when the main Window Manager is crashed or is 
being changed). In this case XConvertSelection doesn't convert VERSION 
property because it exists only for Window Manager (like Compiz). But 
the solution in my fix doesn't expect any property existence and works 
for all popular X Servers/Window Managers (including Mir, LXDM and Xfce).


Thanks,
Oleg

On 1/24/14 3:35 PM, Anthony Petrov wrote:

Hi Oleg,

Thanks for the update. The new version of the fix looks good to me. 
I'm wondering if we should preserve the workaround at original lines 
2430-2442 in XToolkit.java for added safety?


--
best regards,
Anthony

On 1/22/2014 8:31 PM, Oleg Pekhovskiy wrote:

Hi Team,


There was a long break after last comment and now I'm able to continue
discussion.
Please review the second version of fix:
http://cr.openjdk.java.net/~bagiras/7033533.2/

I investigated the order of different events and found that
ConfigureNotify event goes through event queues (X Server and Xlib) the
same way as SelectionNotify.
Moreover it works with and without window manager. Please look at the
native test and its log file attached to the issue in JIRA.
Thus it could unify the way XToolkit.syncNativeQueue() marks the block
of not handled events.

There are several XLib functions that make X server generate
ConfigureNotify event:
XConfigureWindow(), XLowerWindow(), XRaiseWindow(), XRestackWindows(),
XMoveWindow(), XResizeWindow(), XMoveResizeWindow(), XMapRaised(),
XSetWindowBorderWidth().
Invisible XAWTRootWindow could be used as a target for ConfigureNotify
events.
None of mentioned above functions are used for this window and it never
becomes visible.
These functions generate only one event (ConfigureNotify), contrary to
XConvertSelection function which generates two (PropertyNotify,
SelectionNotify).

I chose XMoveWindow(), moving XAWTRootWindow back and forth by 1 pixel
to avoid system optimization.


Thanks,
Oleg

On 10/08/2013 05:09 PM, Anthony Petrov wrote:

Hi Artem,

I'm not sure the testing results below are positive since in his
previous message Yuri wrote:


However some failures are suspicious. Try on Ubuntu with Unity.


when testing on officially supported systems/window managers (see the
quote below).

Also, Leonid and I have concerns regarding reliability of the current
version of the fix, and suggest to implement a solution that would try
executing the current code  of the realSync() first, and only if it
fails fall back to the workaround proposed by Oleg.


What is your opinion on this?


--
best regards,
Anthony


 Original Message 
Subject: Re: AWT Dev [8] Review request for 7033533: realSync()
doesn't work with Xfce
Date: Fri, 04 Oct 2013 18:52:32 +0400
From: Yuri Nesterenko yuri.nestere...@oracle.com
To: Leonid Romanov leonid.roma...@oracle.com,  Anthony Petrov
anthony.pet...@oracle.com
CC: Awt-Dev awt-dev@openjdk.java.net

OK, Ubuntu 13.10 (final beta) with Mir instead of X11
looks surprisingly good. It fails in couple of places
with exceptions like
sun.awt.X11.XException: Cannot write XdndAware property
bug overall the patched build run the awt regression suite
quite OK.

Thanks,
-yan

On 10/04/2013 03:52 PM, Yuri Nesterenko wrote:

So far, I've run some 300+ awt tests with patched build
on Solaris 11 sparcv9 and Ubuntu 13.04 Unity.

Overall passrate is regular.
Most of the failing tests using realSync do fail rather
similarly with patched build and b110.

However some failures are suspicious. Try on Ubuntu with Unity.

test/java/awt/Toolkit/ScreenInsetsTest/ScreenInsetsTest.java
consistently fails some 6 out of 20 runs with patched build but
never in the promoted b110. It may be test issue though, somehow.

event/MouseEvent/SpuriousExitEnter/SpuriousExitEnter_3.java
fails in half of the runs with Incorrect event number but
always pass with b110 (4 or 5 runs).
Event number! It may be superficial resemblance but --

Choice/ChoiceMouseWheelTest/ChoiceMouseWheelTest.java --
coincidence here: Oleg, perhaps you should use
@library ../../regtesthelpers and
@build Util
in these regtests: import test.java.awt.regtesthelpers.Util fails.

Thanks,
-yan

On 10/01/2013 04:27 PM, Yuri Nesterenko wrote:

Sure we can say that Xfce is not complying -- it is not officially
supported by JDK -- neither is LXDE etc. -- but saying that gets
us nowhere.

As to testing, could you suggest a platform selection? I'm afraid
we'll not be able to test Xsun properly but Xorg with Gnome on
Linux and Solaris, Unity and Xfce4 --
all that we can do by the weekend.

Thanks,
-yan

On 10/01/2013 04:01 PM, Leonid Romanov wrote:

By the way, I was reading Inter-Client Communication Conventions
Manual
so I could better understand the fix, and found the following:

4.3. Communication

Re: AWT Dev [9] Review request for JDK-7011513: GTK FileDialog modality issues

[Oleg Pekhovskiy]

Hi Alexander,

Just to clarify some things...

On X11 Frame utilizes XFramePeer and FileDialog utilizes GtkFileDialogPeer.
These peers have common superclass XDecoratedPeer (GtkFileDialogPeer 
through XDialogPeer).


For Frame the following two chains occur:

[#1]
AWT-XAWT
sun.awt.X11.XContentWindow.handleResize(XContentWindow.java:141)
sun.awt.X11.XContentWindow.setContentBounds(XContentWindow.java:129)
sun.awt.X11.XDecoratedPeer.reconfigureContentWindow(XDecoratedPeer.java:662)
sun.awt.X11.XDecoratedPeer.handleConfigureNotifyEvent(XDecoratedPeer.java:763)
sun.awt.X11.XBaseWindow.dispatchEvent(XBaseWindow.java:1135)
sun.awt.X11.XBaseWindow.dispatchToWindow(XBaseWindow.java:1090)
sun.awt.X11.XToolkit.dispatchEvent(XToolkit.java:512)
sun.awt.X11.XToolkit.run(XToolkit.java:621)
sun.awt.X11.XToolkit.run(XToolkit.java:542)
java.lang.Thread.run(Thread.java:744)

This posts ComponentEvent.COMPONENT_RESIZED.

[#2]
AWT-XAWT
sun.awt.X11.XDecoratedPeer.handleMoved(XDecoratedPeer.java:430)
sun.awt.X11.XDecoratedPeer.handleConfigureNotifyEvent(XDecoratedPeer.java:761)
sun.awt.X11.XBaseWindow.dispatchEvent(XBaseWindow.java:1135)
sun.awt.X11.XBaseWindow.dispatchToWindow(XBaseWindow.java:1090)
sun.awt.X11.XToolkit.dispatchEvent(XToolkit.java:512)
sun.awt.X11.XToolkit.run(XToolkit.java:621)
sun.awt.X11.XToolkit.run(XToolkit.java:542)
java.lang.Thread.run(Thread.java:744)

This posts ComponentEvent.COMPONENT_MOVED.

But these chains don't happen for FileDialog.

So the questions are:
Why?
Could we make FileDialog handle event such way too?

Maybe that would be enough and would require less code to implement.

Thanks,
Oleg

On 01/20/2014 07:16 PM, Alexander Zvegintsev wrote:

Hi Sergey,

I've updated the fix:
http://cr.openjdk.java.net/~azvegint/jdk/9/7011513/webrev.01/

Now it returns location and size of a file dialog correctly.

[1] 
https://developer.gnome.org/gtk2/stable/GtkWidget.html#GtkWidget-configure-event

Thanks,

Alexander.
On 01/10/2014 04:32 PM, Sergey Bylokhov wrote:

Hi, Alexander.
The fix looks good.

On 10.01.2014 15:25, Alexander Zvegintsev wrote:

Hello AWT team,

Please review the fix
http://cr.openjdk.java.net/~azvegint/jdk/9/7011513/webrev.00/
for
https://bugs.openjdk.java.net/browse/JDK-7011513

Currently a Gtk file chooser dialog window can be obscured by its 
owner window,
this happens because we do not specify a parent window for obvious 
reason: our owner window in not an instance of GtkWindow.
However it can be workarounded by setting up a WM_TRANSIENT_FOR 
property [1][2].


I tested this on Solaris 10/11, OEL 6, Ubuntu 13.04, all works fine 
for me.


[1] http://tronche.com/gui/x/icccm/sec-4.html#WM_TRANSIENT_FOR
[2] 
http://tronche.com/gui/x/xlib/ICC/client-to-window-manager/XSetTransientForHint.html
[3] 
https://developer.gnome.org/gtk2/stable/GtkWidget.html#gtk-widget-realize
[4] 
https://developer.gnome.org/gdk/stable/gdk-X-Window-System-Interaction.html#gdk-x11-drawable-get-xid


--
Thanks,

Alexander.



--
Best regards, Sergey.

AWT Dev [9] Review request for 7033533: realSync() doesn't work with Xfce

[Oleg Pekhovskiy]

 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?

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.

--
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

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

AWT Dev [9] Review request for 8020443: Frame is not created on the specified GraphicsDevice with two monitors

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8020443.1/
for
https://bugs.openjdk.java.net/browse/JDK-8020443

Referring to the standards, we must calculate insets the special way for 
Xinerama:

http://standards.freedesktop.org/wm-spec/1.3/ar01s05.html

_NET_WM_STRUT_PARTIAL
The start and end values associated with each strut allow areas to be 
reserved which do not span the entire width or height of the screen. 
Struts MUST be specified in root window coordinates, that is, they are 
/not/ relative to the edges of any view port or Xinerama monitor.


So the fix checks if the insets have absolute values and if so makes 
them relative to each screen.

The issue occurred when the Frame was created with the location by default.

Thanks,
Oleg

Re: AWT Dev [9] Review request for 8013116: Robot moves mouse to point which differs from set in mouseMove on Unity shell

[Oleg Pekhovskiy]

Hi Sergey,

I tested on Windows, Mac OS X and Ubuntu. As a result I found
calculation error in CRobot.mouseEvent native method.
It was fixed.
I also updated Javadoc for Robot(GraphicsDevice) constructor
(CCC request will be done).

Please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8013116.2

I built JDK8 with my fix and ran JCK runtime test suite for 3 platforms
for 'api.java_applet', 'api.java_awt' and 'api.javax_swing' branches.
There were no differences for Mac OS X and Windows and one test fixed on 
Ubuntu

(api/java_awt/Container/getMousePosition_NotHeadless).

Thanks,
Oleg

On 01/14/2014 09:42 PM, Sergey Bylokhov wrote:

On 14.01.2014 21:14, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8013116.1/
for
https://bugs.openjdk.java.net/browse/JDK-8013116

java.awt.Robot.gdLoc field was removed to make Robot's point of 
origin be the same as in setters and getters from
GraphicsDevice (GraphicsConfiguration), MouseInfo (PointerInfo) and 
Frame.


This makes sense especially on Ubuntu with Xinerama extension where 
all screens have common coordinate space.

Did it work on other platfroms before the fix(windows, osx)? If yes, why?


Two helper methods were added to 
'test/java/awt/regtesthelpers/Util.java'.
They were used in regression test for this fix. These methods could 
be useful for asynchronous window managers.


Thanks,
Oleg

AWT Dev hg: jdk8/awt/jdk: 7160604: Using non-opaque windows - popups are initially not painted correctly

[oleg . pekhovskiy]

Changeset: 610da7dcd1be
Author:bagiras
Date:  2013-11-26 15:57 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/610da7dcd1be

7160604: Using non-opaque windows - popups are initially not painted correctly
Reviewed-by: serb, alexsch

! src/share/classes/javax/swing/JPopupMenu.java
+ test/javax/swing/JPopupMenu/7160604/bug7160604.html
+ test/javax/swing/JPopupMenu/7160604/bug7160604.java

Re: AWT Dev [8] Review request for 8028995: Write regression test for JDK-8016356

[Oleg Pekhovskiy]

Thanks you guys for such a cognitive review!

Oleg.

On 25.11.2013 13:42, Anthony Petrov wrote:

Thanks for the clarification, Sergey. This sounds reasonable. I'm OK
with the fix then.

--
best regards,
Anthony

On 11/22/2013 08:47 PM, Sergey Bylokhov wrote:

It is not just block of the thread, both threads are synchronized via
wait/notifyAll() on AWTInvocationLock {} inside
EventQueue.invokeAndWait/.

http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html

The |wait| methods of class |Object| (§17.2.1
http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.2.1)

have lock and unlock actions associated with them; their
/happens-before/ relationships are defined by these associated actions.


  *

An unlock on a monitor /happens-before/ every subsequent lock on
that monitor.

  *

A write to a |volatile| field (§8.3.1.4

http://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.3.1.4)

/happens-before/ every subsequent read of that field.



Synchronization is required when we block main thread via Thread.sleep()
and use ActionListeners() on EDT;


On 22.11.2013 20:20, Anthony Petrov wrote:

All I know is as long as you access a variable from multiple threads,
you must use synchronization (locks or volatile). I've never heard
that simply blocking a thread is equivalent to using synchronization
primitives. Are there any documents that specify that?

--
best regards,
Anthony

On 11/22/2013 07:55 PM, Sergey Bylokhov wrote:

On 22.11.2013 18:51, Anthony Petrov wrote:

Hi Oleg,

The frLoc and frSize (and maybe other variables) should be declared
volatile. Generally, if you access a variable from different thread,
you have to synchronize the access using either the volatile modifier
(the easiest way, esp. for tests), or a lock.

I always thought that in such cases two threads already synchronized,
because invokeAndWait is used, which block one thread and wait another.


--
best regards,
Anthony

On 11/22/2013 06:35 PM, Sergey Bylokhov wrote:

Thanks!
The fix looks good.

On 22.11.2013 18:19, Oleg Pekhovskiy wrote:

Hi Sergey,

thanks you for the review,
please take a look at the next version of fix:
http://cr.openjdk.java.net/~bagiras/8028995.2/

Changes:
1. OSInfo used.
2. Location and Dimensions of Frame are retrieved on EDT.

Thanks,
Oleg

On 22.11.2013 17:16, Sergey Bylokhov wrote:

Hi, Oleg.
Usually we check the type of OS via sun.awt.OSInfo
Note that all these things should be on EDT as well:

   // Retrieving the color of window expanded area
   86 p = frame.getLocationOnScreen();
   87 d = frame.getSize();
   88 Insets insets = frame.getInsets();

   91
   92 frame.dispose();


On 22.11.2013 16:37, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8028995.1/
for
https://bugs.openjdk.java.net/browse/JDK-8028995

It's just a regression test for JDK-8016356.

Thanks,
Oleg












--
Best regards, Sergey.

AWT Dev hg: jdk8/awt/jdk: 8028995: Write regression test for JDK-8016356

[oleg . pekhovskiy]

Changeset: e3df535c613f
Author:bagiras
Date:  2013-11-25 14:05 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/e3df535c613f

8028995: Write regression test for JDK-8016356
Reviewed-by: serb, anthony

+ test/javax/swing/JFrame/8016356/bug8016356.java

AWT Dev [8] Review request for 8028995: Write regression test for JDK-8016356

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8028995.1/
for
https://bugs.openjdk.java.net/browse/JDK-8028995

It's just a regression test for JDK-8016356.

Thanks,
Oleg

Re: AWT Dev [8] Review request for 8028995: Write regression test for JDK-8016356

[Oleg Pekhovskiy]

Hi Sergey,

thanks you for the review,
please take a look at the next version of fix:
http://cr.openjdk.java.net/~bagiras/8028995.2/

Changes:
1. OSInfo used.
2. Location and Dimensions of Frame are retrieved on EDT.

Thanks,
Oleg

On 22.11.2013 17:16, Sergey Bylokhov wrote:

Hi, Oleg.
Usually we check the type of OS via sun.awt.OSInfo
Note that all these things should be on EDT as well:

   // Retrieving the color of window expanded area
   86 p = frame.getLocationOnScreen();
   87 d = frame.getSize();
   88 Insets insets = frame.getInsets();

   91
   92 frame.dispose();


On 22.11.2013 16:37, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8028995.1/
for
https://bugs.openjdk.java.net/browse/JDK-8028995

It's just a regression test for JDK-8016356.

Thanks,
Oleg

Re: AWT Dev [8] Review request for 7160604: Using non-opaque windows - popups are initially not painted correctly

[Oleg Pekhovskiy]

Hi Sergey,

thanks you for the review,

please see the next version of fix:
http://cr.openjdk.java.net/~bagiras/7160604.2/

getPopup() method was renamed to showPopup().

Thanks,
Oleg


On 20.11.2013 15:14, Sergey Bylokhov wrote:

Hi, Oleg.
Looks like now getPopup method should be renamed, because it returns
nothing.

On 20.11.2013 15:06, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/7160604.1/
for
https://bugs.openjdk.java.net/browse/JDK-7160604

For now popup menu is painted correctly, so the fix applies to
drop-down list of combo-box only.
BasicComboPopup class implements drop-down list.
BasicComboPopup.isVisible() checks whether BasicComboPopup.popup field
is not null. When the drop-down list is about to show and the
following call-chain occurs: BasicComboPopup.togglePopup() - show()
- setVisible(true) - getPopup()
newPopup.show() method is called before BasicComboPopup.popup field is
updated. Thus when painting occurs for the first time (synchronously,
inside newPopup.show()), BasicComboPopup.isVisible() returns false and
JComponent.paintChildren() skips drawing of BasicComboPopup.
So the fix makes BasicComboPopup.popup field being updated before call
of Popup.show(). Moreover setting of BasicComboPopup.popup field was
moved inside BasicComboPopup.getPopup() method as the same thing
happened after each call of this method.

Thanks,
Oleg

AWT Dev hg: jdk8/awt/jdk: 8027628: JWindow jumps to (0, 0) after mouse clicked

[oleg . pekhovskiy]

Changeset: 3ee121726c17
Author:bagiras
Date:  2013-11-18 23:24 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/3ee121726c17

8027628: JWindow jumps to (0, 0) after mouse clicked
Reviewed-by: anthony, serb

! src/solaris/classes/sun/awt/X11/XDecoratedPeer.java
! src/solaris/classes/sun/awt/X11/XWindowPeer.java
+ test/java/awt/Window/TopLevelLocation/TopLevelLocation.java

Re: AWT Dev [8] Review request for 8027628: JWindow jumps to (0, 0) after mouse clicked

[Oleg Pekhovskiy]

Hi Anthony,

thank you for the review,

the second version of fix is here:
http://cr.openjdk.java.net/~bagiras/8027628.2/

Brief:
1. XBaseWindow changes were reverted.
2. Shared code moved from XDecoratedPeer to XWindowPeer
3. XWindowPeer.handleConfigureNotifyEvent() method improved.

please see my comments below...

Thanks,
Oleg

On 11/08/2013 06:18 PM, Anthony Petrov wrote:
One more point is about the performance. It could be degraded 
significantly if we call toGlobal() all the time unconditionally.


So I suggest to test this using SwingMark to ensure the effect is not 
very severe.


Does it make sense to perform such testing if I made functionality 
changes in XWindowPeer.java only?




--
best regards,
Anthony

On 11/08/2013 06:11 PM, Anthony Petrov wrote:

Hi Oleg,

The XBaseWindow class is not the best place for this code. Firstly, the
XDecoratedPeer.handleConfigureNotifyEvent() never calls its super.*
counterpart. Secondly, all top-level windows use XWindowPeer as an
instance for its peer, so this code actually belongs there (e.g. you can
update x, y after calling the super method.)
I analyzed the code in XDecoratedPeer.handleConfigureNotifyEvent() 
method and found the part that could be shared with
XWindowPeer. I left XDecoratedPeer.handleConfigureNotifyEvent() 
functioanlity intact (just moved the code).
I reverted my changes in XBaseWindow.handleConfigureNotifyEvent() method 
and implemented everything in XWindowPeer.




However, I'd still suggest to analyze why this isn't a problem for
decorated peers in the first place, and consider using the same
mechanism for undecorated peers as well (perhaps we could share some
code in this area and move some common parts to XWindowPeer, for 
example).
It's not a problem for decorated windows because location is corrected 
there too using the way similar to the proposed way

in the first version of my fix. So the code was shared.



Also, the handleConfigureNotifyEvent code is *VERY* fragile. After we
settle on some more-or-less final version of the fix, you should run
most of Window/Frame/Dialog/Layout regression tests (both automatic and
manual, from open and closed repos) to make sure we don't regress. This
needs to be tested on OEL 6 (or which is the minimum supported version
for JDK 8?) and Solaris boxes as well, because the code in this event
handler covers many rare cases only reproducible with specific window
managers found on old systems.

I ran the following tests just on Ubuntu:
test/java/awt/Dialog
test/java/awt/Frame
test/java/awt/Window
closed/test/java/awt/Dialog
closed/ test/java/awt/Frame
closed/ test/java/awt/Layout
closed/ test/java/awt/Window

There were no regressions with my fix even one more previously failing 
test passed.


I also ran SQE functional tests mentioned in JIRA issue description. 
Windows stopped jumping.


Thanks,
Oleg



--
best regards,
Anthony

On 11/08/2013 12:00 PM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8027628.1/
for
https://bugs.openjdk.java.net/browse/JDK-8027628

The issue affects all top-level windows. XConfigureEvent.x and
XConfigureEvent.y fields
from ConfigureNotify event could have wrong values, e.g. when the 
window

is resized by not moved.
That's why manual calculation was added.

This is explained in ICCCM, in 4.1.5. Configuring the Window section:

Advice to Implementors

Clients cannot distinguish between the case where a top-level window is
resized and moved from the case where the window is resized but not
moved, since a real ConfigureNotify event will be received in both
cases. Clients that are concerned with keeping track of the absolute
position of a top-level window should keep a piece of state indicating
whether they are certain of its position. Upon receipt of a real
ConfigureNotify event on the top-level window, the client should note
that the position is unknown. Upon receipt of a synthetic
ConfigureNotify event, the client should note the position as known,
using the position in this event. If the client receives a KeyPress ,
KeyRelease , ButtonPress , ButtonRelease , MotionNotify , EnterNotify ,
or LeaveNotify event on the window (or on any descendant), the client
can deduce the top-level window's position from the difference between
the (event-x, event-y) and (root-x, root-y) coordinates in these 
events.

Only when the position is unknown does the client need to use the
TranslateCoordinates request to find the position of a top-level 
window.


Thanks,
Oleg

AWT Dev [8] Review request for 8028283: Revert JavaDoc changes pushed for JDK-7068423

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8028283.1/
for
https://bugs.openjdk.java.net/browse/JDK-8028283

It's just a JavaDoc reverse changes.

Thanks,
Oleg

AWT Dev hg: jdk8/awt/jdk: 8028283: Revert JavaDoc changes pushed for JDK-7068423

[oleg . pekhovskiy]

Changeset: d6fe4e451dfb
Author:bagiras
Date:  2013-11-13 20:16 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/d6fe4e451dfb

8028283: Revert JavaDoc changes pushed for JDK-7068423
Reviewed-by: art, serb

! src/share/classes/java/awt/GraphicsDevice.java

AWT Dev [8] Review request for 8027628: JWindow jumps to (0, 0) after mouse clicked

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8027628.1/
for
https://bugs.openjdk.java.net/browse/JDK-8027628

The issue affects all top-level windows. XConfigureEvent.x and 
XConfigureEvent.y fields
from ConfigureNotify event could have wrong values, e.g. when the window 
is resized by not moved.

That's why manual calculation was added.

This is explained in ICCCM, in 4.1.5. Configuring the Window section:

Advice to Implementors

Clients cannot distinguish between the case where a top-level window is 
resized and moved from the case where the window is resized but not 
moved, since a real ConfigureNotify event will be received in both 
cases. Clients that are concerned with keeping track of the absolute 
position of a top-level window should keep a piece of state indicating 
whether they are certain of its position. Upon receipt of a real 
ConfigureNotify event on the top-level window, the client should note 
that the position is unknown. Upon receipt of a synthetic 
ConfigureNotify event, the client should note the position as known, 
using the position in this event. If the client receives a KeyPress , 
KeyRelease , ButtonPress , ButtonRelease , MotionNotify , EnterNotify , 
or LeaveNotify event on the window (or on any descendant), the client 
can deduce the top-level window's position from the difference between 
the (event-x, event-y) and (root-x, root-y) coordinates in these events. 
Only when the position is unknown does the client need to use the 
TranslateCoordinates request to find the position of a top-level window.


Thanks,
Oleg

Re: AWT Dev [8] Review request for 8027151: AWT_DnD/Basic_DnD/Automated/DnDMerlinQL/MultipleJVM failing on windows machine

[Oleg Pekhovskiy]

Hi Anthony,

I could refer to the fix for JDK-7075105. the finally {} block was added 
there and had never existed before.


Here's the way InputStream passed to DataTransferer.getInstance().
translateStream() gets closed:

In out case DataTransferer.translateStream() calls 
DataTransferer.translateStreamToInputStream() on line 1776 where
the stream is passed to ReencodingInputStream constructor. It is then 
passed to InputStreamReader constructor. Both classes override close() 
method where ReencodingInputStream closes referenced InputStreamReader and

InputStreamReader closes referenced InputStream.

ReencodingInputStream is returned by 
SunDropTargetContextPeer.getTransferData() method, called by 
DataFlavor.getReaderForText() that returns it to the client code.

So it's a client code obligation to call ReencodingInputStream.close().

This is the use case for the test mentioned in the issue description.
But in general we should investigate each use case to avoid memory 
leaks. As Petr pointed out it makes sense to do it for JDK 9, because it 
could be risky for now.


Thanks,
Oleg

On 29.10.2013 16:41, Anthony Petrov wrote:

Hi Oleg,

I'm not an expert in this code so I may ask some silly questions.

I'm OK with the change #2.

Regarding #1: could you please clarify what code is responsible for
closing the stream now, after your fix? Is this documented/enforced
anywhere (i.e. a finally{} block or something)? Have you run any
regression tests to make sure this change doesn't introduce any memory
leaks? Why was this not a problem before that we decided to fix this
particular piece now, so late in the release?

--
best regards,
Anthony

On 10/28/2013 02:41 AM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8027151.1/
for
https://bugs.openjdk.java.net/browse/JDK-8027151

This issue appeared after the changes made for JDK-7075105.
There were two problems:
1. In drop target code, in SunDropTargetContextPeer.getTransferData()
method inputStream was closed in finally scope but was not yet used in
client code (indirectly). Just its reference stored for the future in
DataTransferer.getInstance().translateStream() call.

2. In drop target code, in DataTransferer.translateStream() there was no
String object handler (it was moved from
DataTransferer.translateBytesOrStream() only to
DataTransferer.translateBytes() method).

Thanks,
Oleg

Re: AWT Dev [8] Review request for 8027151: AWT_DnD/Basic_DnD/Automated/DnDMerlinQL/MultipleJVM failing on windows machine

[Oleg Pekhovskiy]

Thank you for the review, Anthony,

I created new issue:
https://bugs.openjdk.java.net/browse/JDK-8027469
and linked it together with 8027452.

Thanks,
Oleg

On 29.10.2013 21:00, Anthony Petrov wrote:

Thanks for the details, Oleg. I agree that it's a good idea to
investigate the close() issue in JDK 9.

I've also noticed that DataFlavor.getReaderForText() doesn't mention
that user code is responsible for close()'ing the returned Reader. I
suggest to update the specification of this method and mention that
(please add a note about the spec change to 8027452 which you've just
filed).

With this done, the fix looks good to me.

--
best regards,
Anthony

On 10/29/2013 06:39 PM, Oleg Pekhovskiy wrote:

Hi Anthony,

I could refer to the fix for JDK-7075105. the finally {} block was added
there and had never existed before.

Here's the way InputStream passed to DataTransferer.getInstance().
translateStream() gets closed:

In out case DataTransferer.translateStream() calls
DataTransferer.translateStreamToInputStream() on line 1776 where
the stream is passed to ReencodingInputStream constructor. It is then
passed to InputStreamReader constructor. Both classes override close()
method where ReencodingInputStream closes referenced InputStreamReader
and
InputStreamReader closes referenced InputStream.

ReencodingInputStream is returned by
SunDropTargetContextPeer.getTransferData() method, called by
DataFlavor.getReaderForText() that returns it to the client code.
So it's a client code obligation to call ReencodingInputStream.close().

This is the use case for the test mentioned in the issue description.
But in general we should investigate each use case to avoid memory
leaks. As Petr pointed out it makes sense to do it for JDK 9, because it
could be risky for now.

Thanks,
Oleg

On 29.10.2013 16:41, Anthony Petrov wrote:

Hi Oleg,

I'm not an expert in this code so I may ask some silly questions.

I'm OK with the change #2.

Regarding #1: could you please clarify what code is responsible for
closing the stream now, after your fix? Is this documented/enforced
anywhere (i.e. a finally{} block or something)? Have you run any
regression tests to make sure this change doesn't introduce any memory
leaks? Why was this not a problem before that we decided to fix this
particular piece now, so late in the release?

--
best regards,
Anthony

On 10/28/2013 02:41 AM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8027151.1/
for
https://bugs.openjdk.java.net/browse/JDK-8027151

This issue appeared after the changes made for JDK-7075105.
There were two problems:
1. In drop target code, in SunDropTargetContextPeer.getTransferData()
method inputStream was closed in finally scope but was not yet used in
client code (indirectly). Just its reference stored for the future in
DataTransferer.getInstance().translateStream() call.

2. In drop target code, in DataTransferer.translateStream() there
was no
String object handler (it was moved from
DataTransferer.translateBytesOrStream() only to
DataTransferer.translateBytes() method).

Thanks,
Oleg

AWT Dev hg: jdk8/awt/jdk: 8027151: AWT_DnD/Basic_DnD/Automated/DnDMerlinQL/MultipleJVM failing on windows machine

[oleg . pekhovskiy]

Changeset: a2b42e558211
Author:bagiras
Date:  2013-10-29 21:46 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/a2b42e558211

8027151: AWT_DnD/Basic_DnD/Automated/DnDMerlinQL/MultipleJVM failing on windows 
machine
Reviewed-by: anthony, pchelko

! src/share/classes/sun/awt/datatransfer/DataTransferer.java
! src/share/classes/sun/awt/dnd/SunDropTargetContextPeer.java

AWT Dev [8] Review request for 8027151: AWT_DnD/Basic_DnD/Automated/DnDMerlinQL/MultipleJVM failing on windows machine

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8027151.1/
for
https://bugs.openjdk.java.net/browse/JDK-8027151

This issue appeared after the changes made for JDK-7075105.
There were two problems:
1. In drop target code, in SunDropTargetContextPeer.getTransferData() 
method inputStream was closed in finally scope but was not yet used in 
client code (indirectly). Just its reference stored for the future in 
DataTransferer.getInstance().translateStream() call.


2. In drop target code, in DataTransferer.translateStream() there was no 
String object handler (it was moved from 
DataTransferer.translateBytesOrStream() only to 
DataTransferer.translateBytes() method).


Thanks,
Oleg

AWT Dev [8] Review request for 2228674: Fix failed for CR 7162144

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/2228674.1/
for
https://bugs.openjdk.java.net/browse/JDK-2228674

The fix covers two scenarios:
1. User code calls EventDispatchThread.interrupt() and then 
EventDispatchThread.interrupted() which clears interrupted state and

EDT doesn't stop.

2. EventDispatchThread.interrupt() is called without clearing the 
interrupted state (e.g. invocation of AppContext.dispose()) that makes 
EDT terminate.


The other scenario, in which AppContext.dispose() is called from the 
thread other than EDT and after that EventDispatchThread.interrupted() 
is called from EDT preventing EDT from termination, is treated like an 
architecture bug.


Some dead code was also removed because detaching of EDT is always forced.

Thanks,
Oleg

Re: AWT Dev [8] Review request for 2228674: Fix failed for CR 7162144

[Oleg Pekhovskiy]

Hi Anthony,

thank you for the review.

As for your question I could just refer to your fix for JDK 7 where the 
following changes we made in

src/share/classes/java/awt/EventDispatchThread.java:

@@ -100,7 +94,12 @@ class EventDispatchThread extends Thread
 }
 });
 } finally {
-if(getEventQueue().detachDispatchThread(this, shutdown)) {
+// 7189350: doDispatch is reset from stopDispatching(),
+//on InterruptedException, or ThreadDeath. Either way,
+//this indicates that we must force shutting down.
+if (getEventQueue().detachDispatchThread(this,
+!doDispatch || isInterrupted()))
+{
 break;
 }
 }

Condition !doDispatch || isInterrupted() is always true and as a 
result forced detach is always performed. That's why in 
EventQueue.detachDispatchThread() method in condition
!forceDetach  (peekEvent() != null) we never reach peekEvent() != 
null and therefore we could simply remove it.


Thanks,
Oleg

On 16.10.2013 15:29, Anthony Petrov wrote:

Hi Oleg,

The fix looks good to me. However, I don't recall all details about this
machinery, and removing a call to peekEvent() in
EventQueue.detachDispatchThread() looks a bit scary.

Could Artem or you clarify if AWTAutoShutdown() recreates the EDT if
there are still events in the queue, or we simply don't care about them?

--
best regards,
Anthony

On 10/16/2013 02:31 PM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/2228674.1/
for
https://bugs.openjdk.java.net/browse/JDK-2228674

The fix covers two scenarios:
1. User code calls EventDispatchThread.interrupt() and then
EventDispatchThread.interrupted() which clears interrupted state and
EDT doesn't stop.

2. EventDispatchThread.interrupt() is called without clearing the
interrupted state (e.g. invocation of AppContext.dispose()) that makes
EDT terminate.

The other scenario, in which AppContext.dispose() is called from the
thread other than EDT and after that EventDispatchThread.interrupted()
is called from EDT preventing EDT from termination, is treated like an
architecture bug.

Some dead code was also removed because detaching of EDT is always
forced.

Thanks,
Oleg

AWT Dev hg: jdk8/awt/jdk: 2228674: Fix failed for CR 7162144

[oleg . pekhovskiy]

Changeset: 229b10e97bd2
Author:bagiras
Date:  2013-10-16 19:02 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/229b10e97bd2

2228674: Fix failed for CR 7162144
Reviewed-by: art, anthony

! src/share/classes/java/awt/EventDispatchThread.java
! src/share/classes/java/awt/EventQueue.java

AWT Dev hg: jdk8/awt/jdk: 8016356: Any swing frame resizes ugly.

[oleg . pekhovskiy]

Changeset: 84c766f6796b
Author:bagiras
Date:  2013-10-09 14:12 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/84c766f6796b

8016356: Any swing frame resizes ugly.
Reviewed-by: art, anthony

! src/windows/native/sun/windows/awt_Window.cpp

AWT Dev hg: jdk8/awt/jdk: 8000425: FileDialog documentation should be enhanced

[oleg . pekhovskiy]

Changeset: 85a72bb00d74
Author:bagiras
Date:  2013-10-08 16:04 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/85a72bb00d74

8000425: FileDialog documentation should be enhanced
Reviewed-by: serb, anthony

! src/share/classes/java/awt/FileDialog.java

AWT Dev hg: jdk8/awt/jdk: 7068423: Spec for java.awt.GraphicsDevice.getFullScreenWindow() needs clarification

[oleg . pekhovskiy]

Changeset: 01607de6265d
Author:bagiras
Date:  2013-10-08 16:56 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/01607de6265d

7068423: Spec for java.awt.GraphicsDevice.getFullScreenWindow() needs 
clarification
Reviewed-by: art, anthony

! src/share/classes/java/awt/GraphicsDevice.java

AWT Dev hg: jdk8/awt/jdk: 7199196: Incremental transfer is broken because of a typo

[oleg . pekhovskiy]

Changeset: 184b16f4e61f
Author:bagiras
Date:  2013-10-08 17:00 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/184b16f4e61f

7199196: Incremental transfer is broken because of a typo
Reviewed-by: anthony, serb

! src/solaris/classes/sun/awt/X11/XSelection.java

Re: AWT Dev [8] Review request for 8016356: Any swing frame resizes ugly.

[Oleg Pekhovskiy]

Hi Artem, Anthony,

thank you for the review...

I've added more details in the source code comments and bug comments, 
plus changed else location.


Please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8016356.3/

Thanks,
Oleg

On 07.10.2013 16:02, Artem Ananiev wrote:

Hi, Oleg,

could you provide information (here and in bug comments), what are the
values returned by GetWindowRect() and getWindowPlacement(), while the
window is being snapped and after it has been snapped, please?

Thanks,

Artem

On 10/7/2013 2:17 PM, Oleg Pekhovskiy wrote:

Hi All,

Please review the second version of fix
http://cr.openjdk.java.net/~bagiras/8016356.2/
for
http://bugs.sun.com/view_bug.do?bug_id=8016356

I found quite robust way to determine Windows Snap-feature that relies
upon the difference between the real (GetWindowRect()) and the normal
placement of the window (GetWindowPlacement()).
When a window goes snapped the actual and normal rectangles are
different. In this case WindowResized() method is called to send
ComponentResized event and update window layout.

This check was placed in the handler of WM_SYSCOMMAND message right
after SIZE-MOVE loop that works inside AwtWindow::DefWindowProc().

Thanks,
Oleg


On 01.09.2013 10:32, Oleg Pekhovskiy wrote:

Hi Anthony,

On 29.08.2013 17:16, Anthony Petrov wrote:

Hi Oleg,

On 08/28/13 01:35, Oleg Pekhovskiy wrote:

On 26.08.2013 17:20, Anthony Petrov wrote:

The fix looks somewhat fragile to me. The sequence of events may
change
in a future version of Windows, and the fix will fail then.


I agree, the fix is not elegant.
As for the future: I tested it on Windows 8.1 - it worked.


I didn't mean *the* Win 8.1, I meant *a* future version. I suppose you
can't test with Win 9 or Win 19 yet, can you? :)



Sure, no guarantee...




Are there any peculiarities for the WM_SIZE messages being posted
when a
window is snapped? I'm referring to [1] for example, and they suggest
that a proper SC_ flag might be specified when snapping occurs.
So, if
we could detect that, we might unblock the WindowResized() call at
the
end of the WmSize() method in the AwtWindow class, which would fix
the
issue.


Unfortunately there is not direct way to determine the snapping.
My experience says not to play with sending system events manually on
Windows. That is much more fragile and nobody knows how
DefWindowProc()
would behave.


I totally agree.


I chose WM_SYSCOMMAND handler in AwtWindow::WindowProc() because it's
synchronous during window resize (looping in
AwtWindow::DefWindowProc())
So WindowResized() would be called only when the user releases mouse
button after resizing. But WM_SIZE message is called each time the
size
of the window is changed while resizing.


Aren't the position/size of a window change noticeably greater from one
WM_SIZE message to the next one when the snapping occurs, as opposed to
normal resizing? Could we introduce a threshold and only call
WindowResized from WmSize() if the size/position change is greater than
the threshold?


Unfortunately, that's impossible in, at least, to cases:
1. The height of window is near the height of desktop, so snapping
increases the height of window just for a few pixels.
2. The mouse is moved quite fast during the simple resizing (I got 100
pixel increase for one WM_SIZE message).



I realize that such a solution is no more elegant than your original
one, but at least it doesn't rely on a particular sequence of different
messages, which may change in future versions of Windows.

--
best regards,
Anthony



You might also want to explore the WM_WINDOWPOSCHANGED and

WM_WINDOWPOSCHANGING events that the window receives during snapping,
perhaps they have some characteristics allowing us to ignore the
IsResizing() and call the WindowResized() nonetheless.


These messages are sent as a result of a call to the SetWindowPos
function or another window-management function and never appears
during
window resizing with the mouse.



[1]
http://stackoverflow.com/questions/9321549/handling-aerosnap-message-in-wndproc








This article is an attempt to handle maximize/restore snapping, but
this
issue deals with vertical snapping, when top or bottom edge of the
window moved to the corresponding edge of the screen.



--
best regards,
Anthony

On 08/25/13 16:40, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8016356.1/
for
http://bugs.sun.com/view_bug.do?bug_id=8016356

Windows 7 has a feature that makes window being automatically
arranged
when moved to the edge of the screen. And there is no specific
system
notification when that happens. From the other side AwtWindow class
has
some optimization algorithm for resizing that doesn't take into
account
such the arranging. I found indirect way to determine the
arranging by
tracking the sequence of WM_GETMINMAXINFO and WM_SIZE before the
end of
resizing routine, and call WindowResized() when needed to update the
layout.

Thanks,
Oleg

Re: AWT Dev [8] Review request for 8016356: Any swing frame resizes ugly.

[Oleg Pekhovskiy]

Hi All,

Please review the second version of fix
http://cr.openjdk.java.net/~bagiras/8016356.2/
for
http://bugs.sun.com/view_bug.do?bug_id=8016356

I found quite robust way to determine Windows Snap-feature that relies 
upon the difference between the real (GetWindowRect()) and the normal 
placement of the window (GetWindowPlacement()).
When a window goes snapped the actual and normal rectangles are 
different. In this case WindowResized() method is called to send 
ComponentResized event and update window layout.


This check was placed in the handler of WM_SYSCOMMAND message right 
after SIZE-MOVE loop that works inside AwtWindow::DefWindowProc().


Thanks,
Oleg


On 01.09.2013 10:32, Oleg Pekhovskiy wrote:

Hi Anthony,

On 29.08.2013 17:16, Anthony Petrov wrote:

Hi Oleg,

On 08/28/13 01:35, Oleg Pekhovskiy wrote:

On 26.08.2013 17:20, Anthony Petrov wrote:

The fix looks somewhat fragile to me. The sequence of events may change
in a future version of Windows, and the fix will fail then.


I agree, the fix is not elegant.
As for the future: I tested it on Windows 8.1 - it worked.


I didn't mean *the* Win 8.1, I meant *a* future version. I suppose you
can't test with Win 9 or Win 19 yet, can you? :)



Sure, no guarantee...




Are there any peculiarities for the WM_SIZE messages being posted
when a
window is snapped? I'm referring to [1] for example, and they suggest
that a proper SC_ flag might be specified when snapping occurs. So, if
we could detect that, we might unblock the WindowResized() call at the
end of the WmSize() method in the AwtWindow class, which would fix the
issue.


Unfortunately there is not direct way to determine the snapping.
My experience says not to play with sending system events manually on
Windows. That is much more fragile and nobody knows how DefWindowProc()
would behave.


I totally agree.


I chose WM_SYSCOMMAND handler in AwtWindow::WindowProc() because it's
synchronous during window resize (looping in AwtWindow::DefWindowProc())
So WindowResized() would be called only when the user releases mouse
button after resizing. But WM_SIZE message is called each time the size
of the window is changed while resizing.


Aren't the position/size of a window change noticeably greater from one
WM_SIZE message to the next one when the snapping occurs, as opposed to
normal resizing? Could we introduce a threshold and only call
WindowResized from WmSize() if the size/position change is greater than
the threshold?


Unfortunately, that's impossible in, at least, to cases:
1. The height of window is near the height of desktop, so snapping
increases the height of window just for a few pixels.
2. The mouse is moved quite fast during the simple resizing (I got 100
pixel increase for one WM_SIZE message).



I realize that such a solution is no more elegant than your original
one, but at least it doesn't rely on a particular sequence of different
messages, which may change in future versions of Windows.

--
best regards,
Anthony



You might also want to explore the WM_WINDOWPOSCHANGED and

WM_WINDOWPOSCHANGING events that the window receives during snapping,
perhaps they have some characteristics allowing us to ignore the
IsResizing() and call the WindowResized() nonetheless.


These messages are sent as a result of a call to the SetWindowPos
function or another window-management function and never appears during
window resizing with the mouse.



[1]
http://stackoverflow.com/questions/9321549/handling-aerosnap-message-in-wndproc






This article is an attempt to handle maximize/restore snapping, but this
issue deals with vertical snapping, when top or bottom edge of the
window moved to the corresponding edge of the screen.



--
best regards,
Anthony

On 08/25/13 16:40, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8016356.1/
for
http://bugs.sun.com/view_bug.do?bug_id=8016356

Windows 7 has a feature that makes window being automatically
arranged
when moved to the edge of the screen. And there is no specific system
notification when that happens. From the other side AwtWindow class
has
some optimization algorithm for resizing that doesn't take into
account
such the arranging. I found indirect way to determine the arranging by
tracking the sequence of WM_GETMINMAXINFO and WM_SIZE before the
end of
resizing routine, and call WindowResized() when needed to update the
layout.

Thanks,
Oleg

Re: AWT Dev Bug 7107490 affecting JDK7 and JDK8, Bugfix included

[Oleg Pekhovskiy]

Hi Alexander,

that's right and that proposal was made in issue description.
Victor asked to push those changes. I'm doing it right now.
Don't you mind? ;)

Thanks,
Oleg



On 04.10.2013 15:45, Alexander Zvegintsev wrote:

Hi Anthony,
I think the given bugfix is:

diff -r 0cc00c11e17e src/solaris/classes/sun/awt/X11/XSelection.java
--- a/src/solaris/classes/sun/awt/X11/XSelection.javaTue Sep 10
20:42:15 2013 +0400
+++ b/src/solaris/classes/sun/awt/X11/XSelection.javaFri Oct 04
15:32:36 2013 +0400
@@ -375,7 +375,7 @@
  XToolkit.awtUnlock();
  }

-validateDataGetter(dataGetter);
+validateDataGetter(incrDataGetter);

  if (incrDataGetter.getActualFormat() != 8) {
  throw new IOException(Unsupported
data format:  +

It looks like we validating a wrong dataGetter.

--
Thanks,
Alexander.

On 10/04/2013 02:29 PM, Artem Ananiev wrote:


On 9/25/2013 2:46 PM, Jiaz wrote:

Hi,
could anyone with commit rights please apply the given bugfix in
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7107490


I don't see the given bugfix, probably it was stripped by OpenJDK
mailer... According to the bug report, it's considered to be a
duplicate of another one:

https://bugs.openjdk.java.net/browse/JDK-7199196

JDK-7199196: Incremental transfer is broken because of a typo

Thanks,

Artem


Best Regards,
Daniel Wilhelm, JDownloader-Team

AWT Dev [8] Review request for 7199196: Incremental transfer is broken because of a typo

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/7199196.1/
for
https://bugs.openjdk.java.net/browse/JDK-7199196

Just a typo fix.

Thanks,
Oleg

Re: AWT Dev [8] Review request for 7068423: Spec for java.awt.GraphicsDevice.getFullScreenWindow() needs clarification

[Oleg Pekhovskiy]

Hi All,

please review the second version of fix:
http://cr.openjdk.java.net/~bagiras/7068423.2/

Reason:
Discussed with Artem and Andrew, came to conclusion that there were 
situations when returned object could be also set by the system, and not 
by the client application through setFullScreenWindow().


Thanks,
Oleg

On 01.10.2013 22:34, Anthony Petrov wrote:

Looks fine.

--
best regards,
Anthony

On 10/01/2013 04:37 PM, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/7068423.1/
for
https://bugs.openjdk.java.net/browse/JDK-7068423

It's just a Javadoc fix.

Thanks,
Oleg

AWT Dev [8] Review request for 8013553: [macosx] java.awt.FileDialog removes file extensions

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8013553.1/
for
https://bugs.openjdk.java.net/browse/JDK-8013553

setExtensionHidden(NO) method of NSSavePanel doesn't work as expected 
and that's a known issue mentioned on several forums.
But there is another solution based on Application system settings that 
allows to control showing of extension for file dialog.


Thanks,
Oleg

Re: AWT Dev [8] Review request for 8013553: [macosx] java.awt.FileDialog removes file extensions

[Oleg Pekhovskiy]

Thank you, Leonid!

Oleg.

On 03.10.2013 15:08, Leonid Romanov wrote:

Looks good to me.

On 03.10.2013, at 13:07, Oleg Pekhovskiy oleg.pekhovs...@oracle.com wrote:


Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8013553.1/
for
https://bugs.openjdk.java.net/browse/JDK-8013553

setExtensionHidden(NO) method of NSSavePanel doesn't work as expected and 
that's a known issue mentioned on several forums.
But there is another solution based on Application system settings that allows 
to control showing of extension for file dialog.

Thanks,
Oleg

Re: AWT Dev [8] Review request for 8013553: [macosx] java.awt.FileDialog removes file extensions

[Oleg Pekhovskiy]

Hi Sergey, thank you for the review,

please review the second version of fix:
http://cr.openjdk.java.net/~bagiras/8013553.2/

I split the line and added more comments.

Oleg.

On 03.10.2013 15:19, Sergey Bylokhov wrote:

Hi, Oleg.
Can you split the line and add the comment that setExtensionHidden is
not applicable here.

On 03.10.2013 15:08, Leonid Romanov wrote:

Looks good to me.

On 03.10.2013, at 13:07, Oleg Pekhovskiy oleg.pekhovs...@oracle.com
wrote:


Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8013553.1/
for
https://bugs.openjdk.java.net/browse/JDK-8013553

setExtensionHidden(NO) method of NSSavePanel doesn't work as expected
and that's a known issue mentioned on several forums.
But there is another solution based on Application system settings
that allows to control showing of extension for file dialog.

Thanks,
Oleg

AWT Dev hg: jdk8/awt/jdk: 8013553: [macosx] java.awt.FileDialog removes file extensions

[oleg . pekhovskiy]

Changeset: 998578a87c0e
Author:bagiras
Date:  2013-10-03 16:51 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/998578a87c0e

8013553: [macosx] java.awt.FileDialog removes file extensions
Reviewed-by: leonidr, serb

! src/macosx/native/sun/awt/CFileDialog.m

Re: AWT Dev [8] Review request for 7033533: realSync() doesn't work with Xfce

[Oleg Pekhovskiy]

Hi Anthony,

thank you for the review, please see my comment below...

On 01.10.2013 15:29, Anthony Petrov wrote:

Hi Oleg,

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.



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.

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.




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

Re: AWT Dev [8] Review request for 7033533: realSync() doesn't work with Xfce

[Oleg Pekhovskiy]

Hi Leonid,

On 01.10.2013 16:01, Leonid Romanov wrote:

By the way, I was reading Inter-Client Communication Conventions Manual
so I could better understand the fix, and found the following:

4.3. Communication with the Window Manager by Means of Selections

For each screen they manage, window managers will acquire ownership of a
selection named WM_Sn, where n is the screen number, as described in
section 1.2.6. Window managers should comply with the conventions for
Manager Selections described in section 2.8. The intent is for clients
to be able to request a variety of information or services by issuing
conversion requests on this selection.

Considering this, can we say that Xfce is not complying to the spec?


not really, because Xfce HAS VESRION target for WM_S0 selection, but 
in spite of the fast we request this atom as a target, we ask for OOPS 
property which is not mentioned anywhere.


Thanks,
Oleg



On 10/1/2013 15:29, Anthony Petrov wrote:

Hi Oleg,

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.

A minor comment is, lines 2404 - 2407 should be moved to the nearest
try{} block at line 2409.

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?

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.

--
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

Re: AWT Dev [8] Review request for 7033533: realSync() doesn't work with Xfce

[Oleg Pekhovskiy]

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

AWT Dev [8] Review request for 8000425: FileDialog documentation should be enhanced

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8000425.1/
for
https://bugs.openjdk.java.net/browse/JDK-8000425

It's just a Javadoc fix.

Thanks,
Oleg

AWT Dev [8] Review request for 7033533: realSync() doesn't work with Xfce

[Oleg Pekhovskiy]

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

AWT Dev hg: jdk8/awt/jdk: 8003965: Toolkit.beep() documentation is ambiguous

[oleg . pekhovskiy]

Changeset: 04fbd34fda7b
Author:bagiras
Date:  2013-09-12 14:56 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/04fbd34fda7b

8003965: Toolkit.beep() documentation is ambiguous
Reviewed-by: anthony

! src/share/classes/java/awt/Toolkit.java

AWT Dev hg: jdk8/awt/jdk: 7064312: Cleanup: avoid using unsafe string function

[oleg . pekhovskiy]

Changeset: def1fa9854f7
Author:bagiras
Date:  2013-09-12 15:50 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/def1fa9854f7

7064312: Cleanup: avoid using unsafe string function
Reviewed-by: serb, pchelko

! src/windows/native/sun/windows/awt_FileDialog.cpp
! src/windows/native/sun/windows/awt_Font.cpp
! src/windows/native/sun/windows/awt_PrintControl.cpp
! src/windows/native/sun/windows/awt_Toolkit.cpp
! src/windows/native/sun/windows/awt_TrayIcon.cpp
! src/windows/native/sun/windows/awt_ole.cpp

Re: AWT Dev [8] Review request for 8003965: Toolkit.beep() documentation is ambiguous

[Oleg Pekhovskiy]

Hi Team,

please review the second version of fix here:
http://cr.openjdk.java.net/~bagiras/8003965.2/

Thanks,
Oleg


On 29.08.2013 16:05, Anthony Petrov wrote:

Note that this change will also need a CCC request filed/approved before
the fix may be pushed.

--
best regards,
Anthony

On 08/29/13 16:03, Anthony Petrov wrote:

Hi Oleg,

I believe that a javadoc shouldn't mention any particular native APIs
used to implement a particular functionality, because:

1. There are many more platforms where Java runs, and they also may
implement the beep() method.

2. In the future we may need to change the underlying implementation,
and we certainly don't want to also change the javadoc to describe the
low-level changes.

Hence, I suggest to only state something like the following:

Tells the native system to emit a beep. Emitting depends on system
settings and hardware capabilities.

--
best regards,
Anthony

On 08/29/13 14:52, Oleg Pekhovskiy wrote:

Hi all,

please review the fix for JDK-8003965 (not available on bugs.sun.com):
http://cr.openjdk.java.net/~bagiras/8003965.1/

It's just a Javadoc fix.

Thanks,
Oleg

Re: AWT Dev [8] Review request for 8016356: Any swing frame resizes ugly.

[Oleg Pekhovskiy]

Hi Anthony,

On 29.08.2013 17:16, Anthony Petrov wrote:

Hi Oleg,

On 08/28/13 01:35, Oleg Pekhovskiy wrote:

On 26.08.2013 17:20, Anthony Petrov wrote:

The fix looks somewhat fragile to me. The sequence of events may change
in a future version of Windows, and the fix will fail then.


I agree, the fix is not elegant.
As for the future: I tested it on Windows 8.1 - it worked.


I didn't mean *the* Win 8.1, I meant *a* future version. I suppose you
can't test with Win 9 or Win 19 yet, can you? :)



Sure, no guarantee...




Are there any peculiarities for the WM_SIZE messages being posted when a
window is snapped? I'm referring to [1] for example, and they suggest
that a proper SC_ flag might be specified when snapping occurs. So, if
we could detect that, we might unblock the WindowResized() call at the
end of the WmSize() method in the AwtWindow class, which would fix the
issue.


Unfortunately there is not direct way to determine the snapping.
My experience says not to play with sending system events manually on
Windows. That is much more fragile and nobody knows how DefWindowProc()
would behave.


I totally agree.


I chose WM_SYSCOMMAND handler in AwtWindow::WindowProc() because it's
synchronous during window resize (looping in AwtWindow::DefWindowProc())
So WindowResized() would be called only when the user releases mouse
button after resizing. But WM_SIZE message is called each time the size
of the window is changed while resizing.


Aren't the position/size of a window change noticeably greater from one
WM_SIZE message to the next one when the snapping occurs, as opposed to
normal resizing? Could we introduce a threshold and only call
WindowResized from WmSize() if the size/position change is greater than
the threshold?


Unfortunately, that's impossible in, at least, to cases:
1. The height of window is near the height of desktop, so snapping 
increases the height of window just for a few pixels.
2. The mouse is moved quite fast during the simple resizing (I got 100 
pixel increase for one WM_SIZE message).




I realize that such a solution is no more elegant than your original
one, but at least it doesn't rely on a particular sequence of different
messages, which may change in future versions of Windows.

--
best regards,
Anthony



You might also want to explore the WM_WINDOWPOSCHANGED and

WM_WINDOWPOSCHANGING events that the window receives during snapping,
perhaps they have some characteristics allowing us to ignore the
IsResizing() and call the WindowResized() nonetheless.


These messages are sent as a result of a call to the SetWindowPos
function or another window-management function and never appears during
window resizing with the mouse.



[1]
http://stackoverflow.com/questions/9321549/handling-aerosnap-message-in-wndproc





This article is an attempt to handle maximize/restore snapping, but this
issue deals with vertical snapping, when top or bottom edge of the
window moved to the corresponding edge of the screen.



--
best regards,
Anthony

On 08/25/13 16:40, Oleg Pekhovskiy wrote:

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8016356.1/
for
http://bugs.sun.com/view_bug.do?bug_id=8016356

Windows 7 has a feature that makes window being automatically arranged
when moved to the edge of the screen. And there is no specific system
notification when that happens. From the other side AwtWindow class has
some optimization algorithm for resizing that doesn't take into account
such the arranging. I found indirect way to determine the arranging by
tracking the sequence of WM_GETMINMAXINFO and WM_SIZE before the end of
resizing routine, and call WindowResized() when needed to update the
layout.

Thanks,
Oleg

AWT Dev [8] Review request for 8003965: Toolkit.beep() documentation is ambiguous

[Oleg Pekhovskiy]

Hi all,

please review the fix for JDK-8003965 (not available on bugs.sun.com):
http://cr.openjdk.java.net/~bagiras/8003965.1/

It's just a Javadoc fix.

Thanks,
Oleg

AWT Dev [8] Review request for 8016356: Any swing frame resizes ugly.

[Oleg Pekhovskiy]

Hi all,

please review the fix
http://cr.openjdk.java.net/~bagiras/8016356.1/
for
http://bugs.sun.com/view_bug.do?bug_id=8016356

Windows 7 has a feature that makes window being automatically arranged 
when moved to the edge of the screen. And there is no specific system 
notification when that happens. From the other side AwtWindow class has 
some optimization algorithm for resizing that doesn't take into account 
such the arranging. I found indirect way to determine the arranging by 
tracking the sequence of WM_GETMINMAXINFO and WM_SIZE before the end of 
resizing routine, and call WindowResized() when needed to update the layout.


Thanks,
Oleg

Re: AWT Dev [8] Review request for 7171412: awt Choice doesn't fire ItemStateChange when selecting item after select() call - approved

[Oleg Pekhovskiy]

, intersection)
  4. review result in TextArea result
  5. change the text in left or right or both of the areas
  6. select the SAME operation again from the choice.
  In J6 and lower, it will perform the operation on the new inputs.
  In J7, nothing will happen and there is no way to know that the
  user has attempted something.
 
  For step 6 to work in Java7 even after the patch for 7171412, I will
  have to switch to a different item and then switch back to the 
desired

  item.
  For upgrading the application to work reasonably with Java7 I will
  need to add a separate evaluate button to fire the choice or 
else

  change the choice items into individual buttons.
 
  Thanks for looking into this. With all the recent press on the
  security items recently, I wasn't sure when someone would get a 
chance
  to look into it. (My Personal Rant about security: Why do people 
allow

  untrusted sites to run active X or applets in the first place? duh?)
  I thank you all for your work on this,
  Tim English
 
 
   Date: Thu, 4 Oct 2012 13:33:59 +0400
   From: oleg.pekhovs...@oracle.com
   To: denis.fo...@oracle.com
   CC: artem.anan...@oracle.com; awt-dev@openjdk.java.net;
  tim_engl...@hotmail.com
   Subject: Re: [8] Review request for 7171412: awt Choice doesn't 
fire

  ItemStateChange when selecting item after select() call - approved
  
   Hi Denis,
  
   there are behavior differences for Choice across the platforms.
   on Windows - if we choose the same item twice ItemStateChange 
is not

   fired twice but for other platform it is so.
   There is a separate issue about that 7159935, so all platform 
should

   behave like Windows does.
  
   BTW, native Choice controls fire event always on all platforms.
  
   Thanks,
   Oleg
  
   10/3/2012 5:47 PM, Denis S. Fokin wrote:
Hi Oleg,
   
the fix looks good. It was interesting to verify the 
functionality on

Linux. On my Ubuntu everything works properly.
   
Thank you,
Denis.
   
On 10/2/2012 6:48 PM, Artem Ananiev wrote:
Hi, Oleg,
   
the new version looks fine.
   
Thanks,
   
Artem
   
On 10/2/2012 4:30 PM, Oleg Pekhovskiy wrote:
Hi Artem,
   
thank you for the review, I made changes you proposed there:
http://cr.openjdk.java.net/~bagiras/8/7171412.2
   
Please tell if everything is ok.
   
Thanks,
Oleg
   
10/1/2012 2:23 PM, Artem Ananiev wrote:
Hi, Oleg,
   
(adding Tim to copy)
   
the fix looks fine. Could you please make selectedIndexID 
just a

static variable in awt_Choice.cpp instead of AwtChoice member?
   
Thanks,
   
Artem
   
On 10/1/2012 2:09 PM, Oleg Pekhovskiy wrote:
Hi!
   
Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7171412
   
Webrev:
http://cr.openjdk.java.net/~bagiras/8/7171412.1/
   
I left the idea of the fix CR 6770017 but refused of using
  doubling
native variable for storing previously selected index
(that also caused the problem described in the current 
issue).

   
Thanks,
Oleg
   
   
  




--
Best regards, Sergey.

AWT Dev hg: jdk8/awt/jdk: 8000486: REGRESSION: Three java2d tests fail since jdk8b58 on Windows 7 with NullPointerException

[oleg . pekhovskiy]

Changeset: c27efe7615f8
Author:bagiras
Date:  2012-10-25 09:55 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/c27efe7615f8

8000486: REGRESSION: Three java2d tests fail since jdk8b58 on Windows 7 with 
NullPointerException
Reviewed-by: flar, art

! src/windows/classes/sun/java2d/ScreenUpdateManager.java

AWT Dev [8] Review request for 7082294: nsk/regression/b4265661 crashes on windows

[Oleg Pekhovskiy]

Hi!

Please review a 100% backport from 7u4 for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7082294

Webrev:
http://cr.openjdk.java.net/~bagiras/8/7082294.1/

Changeset:
http://hg.openjdk.java.net/jdk7u/jdk7u-dev/jdk/rev/c5618ec76f11

Thanks,
Oleg

AWT Dev [8] Review request for 8000486: REGRESSION: Three java2d tests fail since jdk8b58 on Windows 7 with NullPointerException

[Oleg Pekhovskiy]

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8000486

Webrev:
http://cr.openjdk.java.net/~bagiras/8/8000486.1/

Comments:
This is an 100% port for already approved 
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8000486


Thanks,
Oleg

Re: AWT Dev [7u12] Review request for 8000492: REGRESSION: Three java2d tests fail since jdk8b58 on Windows 7 with NullPointerException

[Oleg Pekhovskiy]

Hi Jim!

this is exactly the situation!

Thanks,
Oleg

10/20/2012 2:05 AM, Jim Graham wrote:
It sounds like what you are saying is that it gets a null because the 
SD is disposed?  If so, then the fix looks fine.  If I'm still not 
understanding then I'd like to have a better idea of what is going on 
there...


...jim

On 10/19/12 3:40 AM, Oleg Pekhovskiy wrote:
Hi Jim,

that was my first idea too, but then I found out that calling
peer.replaceSurfaceData() didn't change null statement for those tests,
because peer.replaceSurfaceData() was called after .dispose() method.

Thanks,
Oleg

10/18/2012 3:52 AM, Jim Graham wrote:

Hi Oleg,

What are the cases when it returns null? Depending on those
conditions, the right fix might be to pass through to call
peer.replaceSurfaceData() instead of returning the null...

...jim

On 10/17/12 1:47 PM, Oleg Pekhovskiy wrote:

Hi again,

as issues that start with '8' are still unavailable on bugs.sun.com,
please use JIRA link:
https://jbs.oracle.com/bugs/browse/JDK-8000492

Thanks,
Oleg

10/18/2012 12:22 AM, Oleg Pekhovskiy wrote:

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8000492

Webrev:
http://cr.openjdk.java.net/~bagiras/7u12/8000492.1/

Comments:
It is a normal situation when
ScreenUpdateManager.getReplacementScreenSurface() returns null taking
it from the peer.
That's why we should check surfaceData for null before validity
check.

Thanks,
Oleg

AWT Dev [7u12] Review request for 8000492: REGRESSION: Three java2d tests fail since jdk8b58 on Windows 7 with NullPointerException

[Oleg Pekhovskiy]

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8000492

Webrev:
http://cr.openjdk.java.net/~bagiras/7u12/8000492.1/

Comments:
It is a normal situation when 
ScreenUpdateManager.getReplacementScreenSurface() returns null taking it 
from the peer.

That's why we should check surfaceData for null before validity check.

Thanks,
Oleg

Re: AWT Dev [8] Review request for 7171412: awt Choice doesn't fire ItemStateChange when selecting item after select() call - approved

[Oleg Pekhovskiy]

Hi Tim,

I'm researching changes made for 6770017 in order to decide whether we 
could revert them
or do it another way and return back firing of ItemStateChange always 
for all platforms.


Thanks,
Oleg

10/8/2012 4:39 PM, Tim English wrote:

Oleg,
I just saw your earlier email as well. I apologize that I do not have 
an automated test. I did review the .2 change and I am glad that you 
all went with the better fix by eliminating the duplicate/triplicate 
state.  Thank you for keeping me in the loop directly as I have not 
been keeping up with the digests.

All,

Thought in hindsight... maybe the original enhancement request should 
have just been rejected. If the user keeps picking the same item from 
the list, they are probably expecting the software to do something! It 
is possible for an event listener to check against previous state and 
ignore extra messages(work around possible), it is NOT possible for an 
event listener to react to an event that is NEVER fired (no work 
around, must redesign UI).

 BTW, native Choice controls fire event always on all platforms.
Similar reasoning might lie behind why the native platforms choose to 
always fire: more flexibility to the developer.
Another possiblity would be to add a new control state to the Choice 
control [ set/isFireAlreadySelected() ] and/or the selection Event  [ 
isAlreadySelected() ].  Default state for isFireAlreadySelecteded() 
defaults to true to retain compatibility for older JVMs.  User 
requesting the original enhacement could set this to false to silence 
the repeated selection methods.
Note that the original enhancement requester could have created a 
Choice subclass to solve the duplicate firing result. (pseudo code)

class SingleFireChoice extends Choice {
Listeners singleFirelisteners;
addSingleFireListener(Listener onlyWantsToKnowIfChanged);
... code to filter out duplicate selects
}

I normally consider that the behavior between radio groups and choice 
lists should operate on the same rules. (Just 2 different views of the 
same information) I wonder if radio groups fire an extra message if 
you keep clicking the same radio button over and over again? It might 
be an interesting experiment.


I just happened to run an old Java utility that I wrote in 2000.  I 
had not run it since Java7 has been released.
It is still using an old 1.4 runtime, and as I was running it, I 
realized that it will NOT work with the Java 7 JVM since it performs 
an operation when a choice item is selected. The user might want to 
perform the same operation repeatedly via the choice on different inputs.


Basic test case that will now fail in the application is
1. enter some text in TextArea left
2. enter some text in TextArea right
3. select an operation from the choice (left difference, right 
difference, symmetric difference, union, intersection)

4. review result in TextArea result
5. change the text in left or right or both of the areas
6. select the SAME operation again from the choice.
In J6 and lower, it will perform the operation on the new inputs.
In J7, nothing will happen and there is no way to know that the 
user has attempted something.


For step 6 to work in Java7 even after the patch for 7171412, I will 
have to switch to a different item and then switch back to the desired 
item.
For upgrading the application to work reasonably with Java7 I will 
need to add a separate evaluate button to fire the choice or else 
change the choice items into individual buttons.


Thanks for looking into this.  With all the recent press on the 
security items recently, I wasn't sure when someone would get a chance 
to look into it. (My Personal Rant about security: Why do people allow 
untrusted sites to run active X or applets in the first place? duh?)

I thank you all for your work on this,
Tim English


 Date: Thu, 4 Oct 2012 13:33:59 +0400
 From: oleg.pekhovs...@oracle.com
 To: denis.fo...@oracle.com
 CC: artem.anan...@oracle.com; awt-dev@openjdk.java.net; 
tim_engl...@hotmail.com
 Subject: Re: [8] Review request for 7171412: awt Choice doesn't fire 
ItemStateChange when selecting item after select() call - approved


 Hi Denis,

 there are behavior differences for Choice across the platforms.
 on Windows - if we choose the same item twice ItemStateChange is not
 fired twice but for other platform it is so.
 There is a separate issue about that 7159935, so all platform should
 behave like Windows does.

 BTW, native Choice controls fire event always on all platforms.

 Thanks,
 Oleg

 10/3/2012 5:47 PM, Denis S. Fokin wrote:
  Hi Oleg,
 
  the fix looks good. It was interesting to verify the functionality on
  Linux. On my Ubuntu everything works properly.
 
  Thank you,
  Denis.
 
  On 10/2/2012 6:48 PM, Artem Ananiev wrote:
  Hi, Oleg,
 
  the new version looks fine.
 
  Thanks,
 
  Artem
 
  On 10/2/2012 4:30 PM, Oleg Pekhovskiy wrote:
  Hi Artem,
 
  thank you for the review, I made changes you proposed there:
  http

Re: AWT Dev [8] Review request for 7199708 FileChooser crashs when opening large folder

[Oleg Pekhovskiy]

Hi Alexander,

the fix looks good to me.

Thanks,
Oleg

10/3/2012 7:37 PM, Alexander Scherbatiy wrote:


bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7199708
webrev: http://cr.openjdk.java.net/~alexsch/7199708/webrev.00

The Win32ShellFolder2 can dispose the pIShellFolder after passing it 
to the ColumnComparator.
The fix store the Win32ShellFolder2 reference to the ColumnComparator 
class to prevent the pIShellFolder disposing.


Thanks,
Alexandr.

Re: AWT Dev [7u12] Review request for 7197619 Using modifiers for the dead key detection on Windows

[Oleg Pekhovskiy]

Hi Alexander,

the fix looks good to me.

Thanks,
Oleg

03.10.2012 16:50, Alexander Scherbatiy wrote:


This is a direct backport of the fix from the JDK 8 to JDK 7u12

bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7197619
webrev: http://cr.openjdk.java.net/~alexsch/7197619/webrev.03/

Only virtual key code is used for the dead key detection on Windows in 
the current implementation.
This does not take into account that dead keys can be pressed with 
modifiers like ctrl+alt+2 (caron dead key) on Hungarian keyoard.


The fix gets isDeadKey flag and a character from the ToAsciiEx method 
and used them for the windows dead key to java key translation.


Thanks,
Alexandr.

AWT Dev [8] Review request for 7171412: awt Choice doesn't fire ItemStateChange when selecting item after select() call

[Oleg Pekhovskiy]

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7171412

Webrev:
http://cr.openjdk.java.net/~bagiras/8/7171412.1/

I left the idea of the fix CR 6770017 but refused of using doubling 
native variable for storing previously selected index

(that also caused the problem described in the current issue).

Thanks,
Oleg

Re: AWT Dev [8] Review request for 7197619 Using modifiers for the dead key detection on Windows

[Oleg Pekhovskiy]

Looks good to me.

Oleg.

9/27/2012 5:06 PM, Alexander Scherbatiy wrote:


Could you review the updated fix:
  http://cr.openjdk.java.net/~alexsch/7197619/webrev.03/

The only difference is that the formatting is updated for the line
   3176 if(isDeadKey){

Thanks,
Alexandr.


On 9/25/2012 10:46 AM, Leonid Romanov wrote:

3176 if(isDeadKey){

Please, add space here. Otherwise looks OK, although I'm not an 
expert in

this area.


-Original Message-
From: awt-dev-boun...@openjdk.java.net [mailto:awt-dev-
boun...@openjdk.java.net] On Behalf Of Alexander Scherbatiy
Sent: Thursday, September 20, 2012 6:48 PM
To: Oleg Pekhovskiy; awt-dev@openjdk.java.net; Alexey Utkin
Subject: Re:AWT Dev  [8] Review request for 7197619 Using 
modifiers for

the dead key detection on Windows


Could you review the updated fix:
 http://cr.openjdk.java.net/~alexsch/7197619/webrev.02/

- cast to BOOL type is fixed for the isDeadKey
- isDeadKey parameter is passed by reference in the 
WindowsKeyToJavaChar

method

Thanks,
Alexandr.

On 9/19/2012 8:16 PM, Oleg Pekhovskiy wrote:

Hi Alexander,

let me advice you further changes:

3397 UINT AwtComponent::WindowsKeyToJavaChar(UINT wkey, UINT
modifiers, TransOps ops, BOOL *isDeadKey)
3398 {
3399 static Hashtable transTable(VKEY translations);
3400 static Hashtable deadKeyFlagTable(Dead Key Flags);
3401 *isDeadKey = FALSE;
3402
3403 // Try to translate using last saved translation
3404 if (ops == LOAD) {
3405void* deadKeyFlag =

deadKeyFlagTable.remove(reinterpret_castvoid*(static_castINT_PTR(wke 


y)));

3406void* value =
transTable.remove(reinterpret_castvoid*(static_castINT_PTR(wkey))); 


3407if (value != NULL) {
3408*isDeadKey =
static_castUINT(reinterpret_castBOOL(deadKeyFlag));
3409return
static_castUINT(reinterpret_castINT_PTR(value));
3410}
3411 }

1. Casting at row 3408 should be corrected to:

static_castBOOL(reinterpret_castINT_PTR(deadKeyFlag))


2. Seems like it makes sense to use a reference instead of a pointer

for:

BOOL *isDeadKey

as this is a required parameter.

Thanks,
Oleg

9/19/2012 5:42 PM, Alexander Scherbatiy wrote:

Could you review the updated fix:
   http://cr.openjdk.java.net/~alexsch/7197619/webrev.01/

- The WindowsKeyToJavaChar method returns a char as it was before the
fix
- ToAsciiEx is changed to ToUnicodeEx as it is suggested in the

comment.

  Thanks,
  Alexandr.


On 9/14/2012 9:11 PM, Oleg Pekhovskiy wrote:

Hi Alexander,

here are my comments:
1. Why did you decide to make WindowsKeyToJavaChar void? Maybe it
takes sense to leave signature closer to its original look?
2. Seems like WindowsKeyToJavaChar method could be simplifed
(ToAsciiEx -  ToUnicodeEx, remove MultiByteToWideChar),
like this:

3504 WORD wChar[2];
3505 UINT scancode = ::MapVirtualKey(wkey, 0);
3506 int converted = ::ToUnicodeEx(wkey, scancode, 
keyboardState,

3507wChar, 2, 0, GetKeyboardLayout());
3508
3509 UINT translation;
3510 BOOL deadKeyFlag = (converted == 2);
3511
3512 // Dead Key
3513 if (converted   0) {
3514 translation = java_awt_event_KeyEvent_CHAR_UNDEFINED;
3515 } else
3516 // No translation available -- try known conversions or
else punt.
3517 if (converted == 0) {
3518 if (wkey == VK_DELETE) {
3519 translation = '\177';
3520 } else
3521 if (wkey= VK_NUMPAD0   wkey= VK_NUMPAD9) {
3522 translation = '0' + wkey - VK_NUMPAD0;
3523 } else {
3524 translation = 
java_awt_event_KeyEvent_CHAR_UNDEFINED;

3525 }
3526 } else
3527 // the caller expects a Unicode character.
3528 if (converted   0) {
3533 translation = wChar[0];
3534 }


Thanks,
Oleg


9/11/2012 5:24 PM, Alexander Scherbatiy wrote:

bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7197619
webrev: http://cr.openjdk.java.net/~alexsch/7197619/webrev.00/

Only a virtual key code is used for the dead key detection on
Windows in the current implementation.
This does not take into account that dead keys can be pressed with
modifiers like ctrl+alt+2 (caron dead key) on Hungarian keyboard.

The fix gets isDeadKey flag and a character from the ToAsciiEx
method and uses them for the windows dead key to java key

translation.

Thanks,
Alexandr.

Re: AWT Dev [8] Review request for 7197619 Using modifiers for the dead key detection on Windows

[Oleg Pekhovskiy]

Looks good to me.

Thanks,
Oleg

9/20/2012 6:47 PM, Alexander Scherbatiy wrote:


Could you review the updated fix:
   http://cr.openjdk.java.net/~alexsch/7197619/webrev.02/

- cast to BOOL type is fixed for the isDeadKey
- isDeadKey parameter is passed by reference in the 
WindowsKeyToJavaChar method


Thanks,
Alexandr.

On 9/19/2012 8:16 PM, Oleg Pekhovskiy wrote:

Hi Alexander,

let me advice you further changes:

3397 UINT AwtComponent::WindowsKeyToJavaChar(UINT wkey, UINT 
modifiers, TransOps ops, BOOL *isDeadKey)

3398 {
3399 static Hashtable transTable(VKEY translations);
3400 static Hashtable deadKeyFlagTable(Dead Key Flags);
3401 *isDeadKey = FALSE;
3402
3403 // Try to translate using last saved translation
3404 if (ops == LOAD) {
3405void* deadKeyFlag = 
deadKeyFlagTable.remove(reinterpret_castvoid*(static_castINT_PTR(wkey)));
3406void* value = 
transTable.remove(reinterpret_castvoid*(static_castINT_PTR(wkey)));

3407if (value != NULL) {
3408*isDeadKey = 
static_castUINT(reinterpret_castBOOL(deadKeyFlag));
3409return 
static_castUINT(reinterpret_castINT_PTR(value));

3410}
3411 }

1. Casting at row 3408 should be corrected to:

static_castBOOL(reinterpret_castINT_PTR(deadKeyFlag))


2. Seems like it makes sense to use a reference instead of a pointer 
for:


BOOL *isDeadKey

as this is a required parameter.

Thanks,
Oleg

9/19/2012 5:42 PM, Alexander Scherbatiy wrote:


Could you review the updated fix:
  http://cr.openjdk.java.net/~alexsch/7197619/webrev.01/

- The WindowsKeyToJavaChar method returns a char as it was before 
the fix
- ToAsciiEx is changed to ToUnicodeEx as it is suggested in the 
comment.


 Thanks,
 Alexandr.


On 9/14/2012 9:11 PM, Oleg Pekhovskiy wrote:

Hi Alexander,

here are my comments:
1. Why did you decide to make WindowsKeyToJavaChar void? Maybe it 
takes sense to leave signature closer to its original look?
2. Seems like WindowsKeyToJavaChar method could be simplifed 
(ToAsciiEx - ToUnicodeEx, remove MultiByteToWideChar),

like this:

3504 WORD wChar[2];
3505 UINT scancode = ::MapVirtualKey(wkey, 0);
3506 int converted = ::ToUnicodeEx(wkey, scancode, keyboardState,
3507wChar, 2, 0, GetKeyboardLayout());
3508
3509 UINT translation;
3510 BOOL deadKeyFlag = (converted == 2);
3511
3512 // Dead Key
3513 if (converted  0) {
3514 translation = java_awt_event_KeyEvent_CHAR_UNDEFINED;
3515 } else
3516 // No translation available -- try known conversions or 
else punt.

3517 if (converted == 0) {
3518 if (wkey == VK_DELETE) {
3519 translation = '\177';
3520 } else
3521 if (wkey= VK_NUMPAD0  wkey= VK_NUMPAD9) {
3522 translation = '0' + wkey - VK_NUMPAD0;
3523 } else {
3524 translation = java_awt_event_KeyEvent_CHAR_UNDEFINED;
3525 }
3526 } else
3527 // the caller expects a Unicode character.
3528 if (converted  0) {
3533 translation = wChar[0];
3534 }


Thanks,
Oleg


9/11/2012 5:24 PM, Alexander Scherbatiy wrote:


bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7197619
webrev: http://cr.openjdk.java.net/~alexsch/7197619/webrev.00/

Only a virtual key code is used for the dead key detection on 
Windows in the current implementation.
This does not take into account that dead keys can be pressed with 
modifiers like ctrl+alt+2 (caron dead key) on Hungarian keyboard.


The fix gets isDeadKey flag and a character from the ToAsciiEx 
method and uses them for the windows dead key to java key 
translation.


Thanks,
Alexandr.

Re: AWT Dev [8] Review request for 7197619 Using modifiers for the dead key detection on Windows

[Oleg Pekhovskiy]

Hi Alexander,

let me advice you further changes:

3397 UINT AwtComponent::WindowsKeyToJavaChar(UINT wkey, UINT modifiers, 
TransOps ops, BOOL *isDeadKey)
3398 {
3399 static Hashtable transTable(VKEY translations);
3400 static Hashtable deadKeyFlagTable(Dead Key Flags);
3401 *isDeadKey = FALSE;
3402
3403 // Try to translate using last saved translation
3404 if (ops == LOAD) {
3405void* deadKeyFlag = 
deadKeyFlagTable.remove(reinterpret_castvoid*(static_castINT_PTR(wkey)));
3406void* value = 
transTable.remove(reinterpret_castvoid*(static_castINT_PTR(wkey)));
3407if (value != NULL) {
3408*isDeadKey = 
static_castUINT(reinterpret_castBOOL(deadKeyFlag));
3409return static_castUINT(reinterpret_castINT_PTR(value));
3410}
3411 }

1. Casting at row 3408 should be corrected to:

static_castBOOL(reinterpret_castINT_PTR(deadKeyFlag))


2. Seems like it makes sense to use a reference instead of a pointer for:

BOOL *isDeadKey

as this is a required parameter.

Thanks,
Oleg

9/19/2012 5:42 PM, Alexander Scherbatiy wrote:


Could you review the updated fix:
  http://cr.openjdk.java.net/~alexsch/7197619/webrev.01/

- The WindowsKeyToJavaChar method returns a char as it was before the fix
- ToAsciiEx is changed to ToUnicodeEx as it is suggested in the comment.

 Thanks,
 Alexandr.


On 9/14/2012 9:11 PM, Oleg Pekhovskiy wrote:

Hi Alexander,

here are my comments:
1. Why did you decide to make WindowsKeyToJavaChar void? Maybe it 
takes sense to leave signature closer to its original look?
2. Seems like WindowsKeyToJavaChar method could be simplifed 
(ToAsciiEx - ToUnicodeEx, remove MultiByteToWideChar),

like this:

3504 WORD wChar[2];
3505 UINT scancode = ::MapVirtualKey(wkey, 0);
3506 int converted = ::ToUnicodeEx(wkey, scancode, keyboardState,
3507wChar, 2, 0, GetKeyboardLayout());
3508
3509 UINT translation;
3510 BOOL deadKeyFlag = (converted == 2);
3511
3512 // Dead Key
3513 if (converted  0) {
3514 translation = java_awt_event_KeyEvent_CHAR_UNDEFINED;
3515 } else
3516 // No translation available -- try known conversions or else 
punt.

3517 if (converted == 0) {
3518 if (wkey == VK_DELETE) {
3519 translation = '\177';
3520 } else
3521 if (wkey= VK_NUMPAD0  wkey= VK_NUMPAD9) {
3522 translation = '0' + wkey - VK_NUMPAD0;
3523 } else {
3524 translation = java_awt_event_KeyEvent_CHAR_UNDEFINED;
3525 }
3526 } else
3527 // the caller expects a Unicode character.
3528 if (converted  0) {
3533 translation = wChar[0];
3534 }


Thanks,
Oleg


9/11/2012 5:24 PM, Alexander Scherbatiy wrote:


bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7197619
webrev: http://cr.openjdk.java.net/~alexsch/7197619/webrev.00/

Only a virtual key code is used for the dead key detection on 
Windows in the current implementation.
This does not take into account that dead keys can be pressed with 
modifiers like ctrl+alt+2 (caron dead key) on Hungarian keyboard.


The fix gets isDeadKey flag and a character from the ToAsciiEx 
method and uses them for the windows dead key to java key translation.


Thanks,
Alexandr.

Re: AWT Dev [8] Review request for 7197619 Using modifiers for the dead key detection on Windows

[Oleg Pekhovskiy]

Hi Alexander,

here are my comments:
1. Why did you decide to make WindowsKeyToJavaChar void? Maybe it takes 
sense to leave signature closer to its original look?
2. Seems like WindowsKeyToJavaChar method could be simplifed (ToAsciiEx 
- ToUnicodeEx, remove MultiByteToWideChar),

like this:

3504 WORD wChar[2];
3505 UINT scancode = ::MapVirtualKey(wkey, 0);
3506 int converted = ::ToUnicodeEx(wkey, scancode, keyboardState,
3507wChar, 2, 0, GetKeyboardLayout());
3508
3509 UINT translation;
3510 BOOL deadKeyFlag = (converted == 2);
3511
3512 // Dead Key
3513 if (converted  0) {
3514 translation = java_awt_event_KeyEvent_CHAR_UNDEFINED;
3515 } else
3516 // No translation available -- try known conversions or else punt.
3517 if (converted == 0) {
3518 if (wkey == VK_DELETE) {
3519 translation = '\177';
3520 } else
3521 if (wkey= VK_NUMPAD0  wkey= VK_NUMPAD9) {
3522 translation = '0' + wkey - VK_NUMPAD0;
3523 } else {
3524 translation = java_awt_event_KeyEvent_CHAR_UNDEFINED;
3525 }
3526 } else
3527 // the caller expects a Unicode character.
3528 if (converted  0) {
3533 translation = wChar[0];
3534 }


Thanks,
Oleg


9/11/2012 5:24 PM, Alexander Scherbatiy wrote:


bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7197619
webrev: http://cr.openjdk.java.net/~alexsch/7197619/webrev.00/

Only a virtual key code is used for the dead key detection on Windows 
in the current implementation.
This does not take into account that dead keys can be pressed with 
modifiers like ctrl+alt+2 (caron dead key) on Hungarian keyboard.


The fix gets isDeadKey flag and a character from the ToAsciiEx method 
and uses them for the windows dead key to java key translation.


Thanks,
Alexandr.

Re: AWT Dev [8] Review request for 7186109: Simplify lock machinery for PostEventQueue EventQueue

[Oleg Pekhovskiy]

Hi Artem, Anthony, David, Peter,

thank you all for your attention, comments and ideas!
They were important to look at the task from different points of view
and prepare an optimized solution.

Best regards,
Oleg

9/13/2012 7:20 PM, Artem Ananiev wrote:

Hi, Peter,

thank you very much for deep analysis! It makes everybody better 
understand what's going, what's wrong and what can be improved.


Your particular example below is absolutely valid: there are non-zero 
chances e2 is posted to EventQueue before e1. I don't see any ways to 
fix that except to pass the event to post to flushPendingEvents().


However, this is not a side effect of the current fix. Before Oleg's 
changes, e2 is still possible to land in EventQueue before e1. 
Therefore it should be considered a separate problem and fixed 
separately.


Oleg,

the latest .5 version of the fix looks fine to me.

Thanks,

Artem

On 9/12/2012 9:46 AM, Peter Levart wrote:

Hi Oleg,

The reasoning in my previous message, that I made just before going 
to bed,
was not entirely correct. I made a mistake which was obvious in the 
morning.
But even if .notifyAll() is correctly interpreted, the results are 
similar.

So, once again:

Imagine two threads (t1, t2) calling concurrently 
EventQueue.postEvent() -

each with it's own event (e1, e2):

It can happen that t1:

- acquires the monitor in PostEventQueue 1st,
- establishes the flushThread variable,
- takes 1st snapshot of events,
- releases the monitor,
- flushes 1st snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via notifyAll(),
- releases the monitor, ...

when t2 that was waiting, wakes up:

- acquires the monitor
- establishes the flushThread variable,
- takes a 2nd snapshot of events,
- releases the monitor,
- flushes 2nd snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via notifyAll(),
- releases the monitor,
- posts e2 to the EventQueue, ...

and only after that t1 gets a chance to:

- post e1 to the EventQueue


The order of events that end up in EventQueue would still be as follows:

1st snapshot (taken by t1) of toolkit events,
2nd snapshot (taken by t2) of toolkit events,
e2 (posted by t2),
e1 (posted by t1)

where 2nd snapshot are toolkit events that were born after e1.

Probability of such a scenario is very low, but a higher probability 
is that
at least some events from 2nd snapshot get inserted into EventQueue 
before e1.


The point I was trying to make is that as it is done currently, 
events that
end up in the EventQueue are only roughly ordered according to the 
time at

which they are posted to the queues by different threads. Ordering among
threads is not specified (the events don't have a timestamp or a 
sequence

allocated) Ordering is only maintained among events posted from the same
thread.

Different approaches are only more or less fair as to the ordering by 
real

time.

Regards, Peter

On Wednesday, September 12, 2012 12:02:17 AM Peter Levart wrote:

Hi Oleg,

On Tuesday, September 11, 2012 04:03:01 PM Oleg Pekhovskiy wrote:

Hi Peter,

please, see my comments below...

Thanks,
Oleg

9/11/2012 9:42 AM, Peter Levart wrote:

On Tuesday, September 11, 2012 01:25:39 AM Oleg Pekhovskiy wrote:

Hi Peter,

your idea might work if drainTo() is used before while-cycle,
otherwise the order of events could be broken sometimes.


I don't know how. The flush() is synchronized! No two threads can
execute
while-poll at the same time. And LinkedBlockingQueue is used as 
FIFO, so
it does not matter if toolkit thread posts any events concurrently 
- the

order of events is the same.


Imagine the situation when flushing caused by EventQueue.postEvent()
(called from any thread) is in progress.
Toolkit thread calls PostEventQueue.postEvent() while poll() is being
calling over and over again.
As a result the message posted by Toolkit thread would be posted to
EventQueue within current flushing cycle.
That would happen before posting of the event that caused flushing 
(see

EventQueue.postEvent()) and would violate the order of messages.


There's no absolute guarantee anyway since there's allways a window 
between
SunToolkit.flushPendingEvents() and acquire-ing a pushPopLock where 
anything

can happen...

Imagine two threads (t1, t2) calling concurrently 
EventQueue.postEvent() -

each with it's own event (e1, e2):

It can happen that t1:

- acquires the monitor in PostEventQueue 1st,
- establishes the flushThread variable,
- takes 1st snapshot of events,
- releases the monitor,
- flushes 1st snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via notifyAll() which temporarily releases the 
monitor, ...


when t2 that was waiting, wakes up:

- acquires the monitor
- establishes the flushThread variable,
- takes a 2nd snapshot of events,
- releases the monitor,
- flushes 2nd snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable

AWT Dev hg: jdk8/awt/jdk: 7186109: Simplify lock machinery for PostEventQueue EventQueue

[oleg . pekhovskiy]

Changeset: aa35dc4d3f70
Author:bagiras
Date:  2012-09-13 19:53 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/aa35dc4d3f70

7186109: Simplify lock machinery for PostEventQueue  EventQueue
Reviewed-by: art, anthony, dholmes

! src/macosx/classes/sun/lwawt/macosx/LWCToolkit.java
! src/share/classes/java/awt/EventQueue.java
! src/share/classes/sun/awt/SunToolkit.java
+ test/java/awt/EventQueue/PostEventOrderingTest/PostEventOrderingTest.java

AWT Dev [8] Review request for 7198318: SunToolkitSubclass.java should be removed

[Oleg Pekhovskiy]

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7198318

Webrev:
http://cr.openjdk.java.net/~bagiras/8/7198318.1/

That's a simple removing of redundant file.

Thanks,
Oleg

AWT Dev hg: jdk8/awt/jdk: 7198318: SunToolkitSubclass.java should be removed

[oleg . pekhovskiy]

Changeset: 5b7ad5bedbd7
Author:bagiras
Date:  2012-09-13 21:23 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/5b7ad5bedbd7

7198318: SunToolkitSubclass.java should be removed
Reviewed-by: serb

- src/macosx/classes/sun/awt/SunToolkitSubclass.java

Re: AWT Dev [8] Review request for 7186109: Simplify lock machinery for PostEventQueue EventQueue

[Oleg Pekhovskiy]

Hi Peter,

I understood the case you described. That case is possible.

But... my task for now is to preserve the functionality we had before.
That is: Toolkit events posted BEFORE any call of EventQueue.postEvent() 
must go FIRST.

And we have a special test for that.

Thanks,
Oleg

9/12/2012 9:46 AM, Peter Levart wrote:

Hi Oleg,

The reasoning in my previous message, that I made just before going to bed,
was not entirely correct. I made a mistake which was obvious in the morning.
But even if .notifyAll() is correctly interpreted, the results are similar.
So, once again:

Imagine two threads (t1, t2) calling concurrently EventQueue.postEvent() -
each with it's own event (e1, e2):

It can happen that t1:

- acquires the monitor in PostEventQueue 1st,
- establishes the flushThread variable,
- takes 1st snapshot of events,
- releases the monitor,
- flushes 1st snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via notifyAll(),
- releases the monitor, ...

when t2 that was waiting, wakes up:

- acquires the monitor
- establishes the flushThread variable,
- takes a 2nd snapshot of events,
- releases the monitor,
- flushes 2nd snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via notifyAll(),
- releases the monitor,
- posts e2 to the EventQueue, ...

and only after that t1 gets a chance to:

- post e1 to the EventQueue


The order of events that end up in EventQueue would still be as follows:

1st snapshot (taken by t1) of toolkit events,
2nd snapshot (taken by t2) of toolkit events,
e2 (posted by t2),
e1 (posted by t1)

where 2nd snapshot are toolkit events that were born after e1.

Probability of such a scenario is very low, but a higher probability is that
at least some events from 2nd snapshot get inserted into EventQueue before e1.

The point I was trying to make is that as it is done currently, events that
end up in the EventQueue are only roughly ordered according to the time at
which they are posted to the queues by different threads. Ordering among
threads is not specified (the events don't have a timestamp or a sequence
allocated) Ordering is only maintained among events posted from the same
thread.

Different approaches are only more or less fair as to the ordering by real
time.

Regards, Peter

On Wednesday, September 12, 2012 12:02:17 AM Peter Levart wrote:

Hi Oleg,

On Tuesday, September 11, 2012 04:03:01 PM Oleg Pekhovskiy wrote:

Hi Peter,

please, see my comments below...

Thanks,
Oleg

9/11/2012 9:42 AM, Peter Levart wrote:

On Tuesday, September 11, 2012 01:25:39 AM Oleg Pekhovskiy wrote:

Hi Peter,

your idea might work if drainTo() is used before while-cycle,
otherwise the order of events could be broken sometimes.

I don't know how. The flush() is synchronized! No two threads can
execute
while-poll at the same time. And LinkedBlockingQueue is used as FIFO, so
it does not matter if toolkit thread posts any events concurrently - the
order of events is the same.

Imagine the situation when flushing caused by EventQueue.postEvent()
(called from any thread) is in progress.
Toolkit thread calls PostEventQueue.postEvent() while poll() is being
calling over and over again.
As a result the message posted by Toolkit thread would be posted to
EventQueue within current flushing cycle.
That would happen before posting of the event that caused flushing (see
EventQueue.postEvent()) and would violate the order of messages.

There's no absolute guarantee anyway since there's allways a window between
SunToolkit.flushPendingEvents() and acquire-ing a pushPopLock where anything
can happen...

Imagine two threads (t1, t2) calling concurrently EventQueue.postEvent() -
each with it's own event (e1, e2):

It can happen that t1:

- acquires the monitor in PostEventQueue 1st,
- establishes the flushThread variable,
- takes 1st snapshot of events,
- releases the monitor,
- flushes 1st snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via notifyAll() which temporarily releases the monitor, ...

when t2 that was waiting, wakes up:

- acquires the monitor
- establishes the flushThread variable,
- takes a 2nd snapshot of events,
- releases the monitor,
- flushes 2nd snapshot of events,
- re-acquires the monitor,
- clears the flushThread variable,
- broadcasts via notifyAll() which temporarily releases the monitor,
- reacquires the monitor,
- releases the monitor
- posts e2 to the EventQueue, ...

and only after that t1 gets a chance to:

- re-acquire the monitor
- release the monitor
- post e1 to the EventQueue


The order of events that end up in EventQueue is then as follows:

1st snapshot (taken by t1) of toolkit events,
2nd snapshot (taken by t2) of toolkit events,
e2 (posted by t2),
e1 (posted by t1)

where 2nd snapshot are toolkit events that were born after e1.


Nobody guarantees that the event posted by EventQueue.postEvent will be the
1st event to arrive into the EventQueue

Re: AWT Dev [8] Review request for 7186109: Simplify lock machinery for PostEventQueue EventQueue

[Oleg Pekhovskiy]

Hi Peter,

please, see my comments below...

Thanks,
Oleg

9/11/2012 9:42 AM, Peter Levart wrote:

On Tuesday, September 11, 2012 01:25:39 AM Oleg Pekhovskiy wrote:

Hi Peter,

your idea might work if drainTo() is used before while-cycle,
otherwise the order of events could be broken sometimes.

I don't know how. The flush() is synchronized! No two threads can execute 
while-poll at the same time. And LinkedBlockingQueue is used as FIFO, so it 
does not matter if toolkit thread posts any events concurrently - the order of 
events is the same.
Imagine the situation when flushing caused by EventQueue.postEvent() 
(called from any thread) is in progress.
Toolkit thread calls PostEventQueue.postEvent() while poll() is being 
calling over and over again.
As a result the message posted by Toolkit thread would be posted to 
EventQueue within current flushing cycle.
That would happen before posting of the event that caused flushing (see 
EventQueue.postEvent()) and would violate the order of messages.



Also, usage of LinkedBlockingQueue could lead to performance decrease

Internally it uses ReentrantLock, which in flush while-poll loop is acquired 
once per poll. Uncontended acquire is just a CAS. I don't think that in this 
context (GUI events) it presents any difference. So any approach is good-enough.


(especially with drainTo()).

The class has more complicated logic entailing pitfalls in future.

That might be true:

http://stackoverflow.com/questions/12349881/why-linkedblockingqueuepoll-may-hang-up
I'm not denying that CAS in most cases is faster than locking. But if 
you look at the implementation of LinkedBlockingQueue you'll see some 
unnecessary things (for our case) as this class is quite common.



Thanks,
Oleg

Regards, Peter


08.09.2012 21:39, Peter Levart wrote:

Hi Oleg,

I still think that there is room for simplification. Now that the flush
can be synchronized again (because EventQueue.detatchDispatchThread is
not calling SunToolkit.PostEventQueue's synchronized methods while
holding pushPopLock), we just have to make sure that toolkit thread is
not blocked by flushing.

Here's a simplified PostEventQueue that does that:

class PostEventQueue {

 private final QueueAWTEvent  queue = new LinkedBlockingQueue(); //
 unbounded private final EventQueue eventQueue;

 private boolean isFlushing;

 PostEventQueue(EventQueue eq) {

eventQueue = eq;

 }

 public synchronized void flush() {

if (isFlushing)

   return;

isFlushing = true;
try {

   AWTEvent event;
   while ((event = queue.poll()) != null)

  eventQueue.postEvent(event);

}
finally {

   isFlushing = false;

}

 }

 /*
 * Enqueue an AWTEvent to be posted to the Java EventQueue.
 */
 void postEvent(AWTEvent event) {

queue.offer(event);
SunToolkit.wakeupEventQueue(eventQueue, event.getSource() ==
AWTAutoShutdown.getInstance());
 }

}



This implementation also finishes the flush in the presence of
interrupts...

Peter

On Saturday, September 08, 2012 04:09:40 AM Oleg Pekhovskiy wrote:

Artem, Anthony, David,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.4/

What's new in comparison with the previous version:

1. Removed isThreadLocalFlushing and isFlashing, created
flushThread instead
(stores the thread that currently performs flushing).
2. Implemented both recursion and multi-thread block on flushThread
only.
3. Added Thread.interrupt() to the catch block as outside we would like
to know what happened in PostEventQueue.flush().
We are not able to rethrow the exception because we couldn't change the
signature (by adding throwns)
for all related methods (e.g. EventQueue.postEvent()).

The fix successfully passed
closed/java/awt/EventQueue/PostEventOrderingTest.java test.

IMHO, the code became clearer.

Looking forward to your comments!

Thanks,
Oleg

8/30/2012 9:19 PM, Oleg Pekhovskiy wrote:

There are also other 2 methods that will require 'throws
InterruptedException' or try-catch:
1. EventQueue.postEvent()
2. EventQueue.removeSourceEvents()

Thanks,
Oleg

8/30/2012 9:01 PM, Oleg Pekhovskiy wrote:

Hi,

I got another idea preparing the next version of fix.

Previously we didn't catch InterruptedException and stack unwinding
took place right up to
try-catch section in EventDispatchThread.pumpOneEventForFilters().

So seems like it would be correct not eating that exception in
PostEventQueue.flush()
but just check the state using isInterrupted() method and add 'throws
InterruptedException'
to PostEventQueue.flush() and SunToolkit.flushPendingEvents() methods.

Thoughts?

Thanks,
Oleg

8/30/2012 5:20 PM, Anthony Petrov wrote:

I see. After giving it more thought I don't see an easy solution for
this issue, too. As long as we guarantee that the EDT is recreated
should more events be posted, I don't see a big problem with this.
So

Re: AWT Dev [8] Review request for 7186109: Simplify lock machinery for PostEventQueue EventQueue

[Oleg Pekhovskiy]

Hi Peter,

your idea might work if drainTo() is used before while-cycle,
otherwise the order of events could be broken sometimes.

Also, usage of LinkedBlockingQueue could lead to performance decrease
(especially with drainTo()).

The class has more complicated logic entailing pitfalls in future.

Thanks,
Oleg


08.09.2012 21:39, Peter Levart wrote:

Hi Oleg,

I still think that there is room for simplification. Now that the flush can be 
synchronized again (because EventQueue.detatchDispatchThread is not calling 
SunToolkit.PostEventQueue's synchronized methods while holding pushPopLock), we 
just have to make sure that toolkit thread is not blocked by flushing.

Here's a simplified PostEventQueue that does that:

class PostEventQueue {
private final QueueAWTEvent queue = new LinkedBlockingQueue(); // 
unbounded
private final EventQueue eventQueue;

private boolean isFlushing;

PostEventQueue(EventQueue eq) {
   eventQueue = eq;
}

public synchronized void flush() {
   if (isFlushing)
  return;

   isFlushing = true;
   try {
  AWTEvent event;
  while ((event = queue.poll()) != null)
 eventQueue.postEvent(event);
   }
   finally {
  isFlushing = false;
   }
}

/*
* Enqueue an AWTEvent to be posted to the Java EventQueue.
*/
void postEvent(AWTEvent event) {
   queue.offer(event);
   SunToolkit.wakeupEventQueue(eventQueue, event.getSource() == 
AWTAutoShutdown.getInstance());
}
}



This implementation also finishes the flush in the presence of interrupts...

Peter


On Saturday, September 08, 2012 04:09:40 AM Oleg Pekhovskiy wrote:

Artem, Anthony, David,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.4/

What's new in comparison with the previous version:

1. Removed isThreadLocalFlushing and isFlashing, created
flushThread instead
(stores the thread that currently performs flushing).
2. Implemented both recursion and multi-thread block on flushThread only.
3. Added Thread.interrupt() to the catch block as outside we would like
to know what happened in PostEventQueue.flush().
We are not able to rethrow the exception because we couldn't change the
signature (by adding throwns)
for all related methods (e.g. EventQueue.postEvent()).

The fix successfully passed
closed/java/awt/EventQueue/PostEventOrderingTest.java test.

IMHO, the code became clearer.

Looking forward to your comments!

Thanks,
Oleg

8/30/2012 9:19 PM, Oleg Pekhovskiy wrote:

There are also other 2 methods that will require 'throws
InterruptedException' or try-catch:
1. EventQueue.postEvent()
2. EventQueue.removeSourceEvents()

Thanks,
Oleg

8/30/2012 9:01 PM, Oleg Pekhovskiy wrote:

Hi,

I got another idea preparing the next version of fix.

Previously we didn't catch InterruptedException and stack unwinding
took place right up to
try-catch section in EventDispatchThread.pumpOneEventForFilters().

So seems like it would be correct not eating that exception in
PostEventQueue.flush()
but just check the state using isInterrupted() method and add 'throws
InterruptedException'
to PostEventQueue.flush() and SunToolkit.flushPendingEvents() methods.

Thoughts?

Thanks,
Oleg

8/30/2012 5:20 PM, Anthony Petrov wrote:

I see. After giving it more thought I don't see an easy solution for
this issue, too. As long as we guarantee that the EDT is recreated
should more events be posted, I don't see a big problem with this.
So let's stick with the Minimize... approach then.

On 08/30/12 00:18, Oleg Pekhovskiy wrote:

Hi Anthony,

I see your concerns.

As PostEventQueue.flush() method left unsynchronized,
we potentially could return PostEventQueue.noEvents()
and return check in EventQueue.detachDispatchThread()
back to the condition.
But is could increase the possibility of deadlock in future
(with PostEventQueue  pushPopLock).

Artem, what do you think?

Thanks,
Oleg

29.08.2012 15:22, Anthony Petrov wrote:

On 8/29/2012 3:08 PM, Anthony Petrov wrote:

Hi Oleg,

I'm still concerned about the following:

detachDispatchThread()
{
flush();
lock();
// possibly detach
unlock();
}

at EventQueue.java. What if an even get posted to the queue after
the

A typo: s/even get/event gets/.


flush() returns but before we even acquired the lock? We may still
end up with a situation when we detach the thread while in fact
there
are some pending events present, which actually contradicts the
current logic of the detach() method. I see that you say Minimize
discard possibility in the comment instead of Prevent ..., but I
feel uncomfortable with this actually.

What exactly prevents us from adding some synchronization to ensure
that the detaching only happens when there's really no pending
events?

SunToolkit.java:

2120 Boolean b = isThreadLocalFlushing.get();
2121 if (b != null  b) {
2122 return;
2123 }
2124 2125 isThreadLocalFlushing.set(true);
2126 try {

How does access to the isThreadLocalFlushing

Re: AWT Dev [8] Review request for 7186109: Simplify lock machinery for PostEventQueue EventQueue

[Oleg Pekhovskiy]

Artem, Anthony, David,

please review the next version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.4/

What's new in comparison with the previous version:

1. Removed isThreadLocalFlushing and isFlashing, created 
flushThread instead

(stores the thread that currently performs flushing).
2. Implemented both recursion and multi-thread block on flushThread only.
3. Added Thread.interrupt() to the catch block as outside we would like 
to know what happened in PostEventQueue.flush().
We are not able to rethrow the exception because we couldn't change the 
signature (by adding throwns)

for all related methods (e.g. EventQueue.postEvent()).

The fix successfully passed 
closed/java/awt/EventQueue/PostEventOrderingTest.java test.


IMHO, the code became clearer.

Looking forward to your comments!

Thanks,
Oleg

8/30/2012 9:19 PM, Oleg Pekhovskiy wrote:
There are also other 2 methods that will require 'throws 
InterruptedException' or try-catch:

1. EventQueue.postEvent()
2. EventQueue.removeSourceEvents()

Thanks,
Oleg

8/30/2012 9:01 PM, Oleg Pekhovskiy wrote:

Hi,

I got another idea preparing the next version of fix.

Previously we didn't catch InterruptedException and stack unwinding 
took place right up to

try-catch section in EventDispatchThread.pumpOneEventForFilters().

So seems like it would be correct not eating that exception in 
PostEventQueue.flush()
but just check the state using isInterrupted() method and add 'throws 
InterruptedException'

to PostEventQueue.flush() and SunToolkit.flushPendingEvents() methods.

Thoughts?

Thanks,
Oleg

8/30/2012 5:20 PM, Anthony Petrov wrote:
I see. After giving it more thought I don't see an easy solution for 
this issue, too. As long as we guarantee that the EDT is recreated 
should more events be posted, I don't see a big problem with this. 
So let's stick with the Minimize... approach then.


--
best regards,
Anthony

On 08/30/12 00:18, Oleg Pekhovskiy wrote:

Hi Anthony,

I see your concerns.

As PostEventQueue.flush() method left unsynchronized,
we potentially could return PostEventQueue.noEvents()
and return check in EventQueue.detachDispatchThread()
back to the condition.
But is could increase the possibility of deadlock in future
(with PostEventQueue  pushPopLock).

Artem, what do you think?

Thanks,
Oleg

29.08.2012 15:22, Anthony Petrov wrote:

On 8/29/2012 3:08 PM, Anthony Petrov wrote:

Hi Oleg,

I'm still concerned about the following:

detachDispatchThread()
{
flush();
lock();
// possibly detach
unlock();
}

at EventQueue.java. What if an even get posted to the queue after 
the


A typo: s/even get/event gets/.

--
best regards,
Anthony


flush() returns but before we even acquired the lock? We may still
end up with a situation when we detach the thread while in fact 
there

are some pending events present, which actually contradicts the
current logic of the detach() method. I see that you say Minimize
discard possibility in the comment instead of Prevent ..., but I
feel uncomfortable with this actually.

What exactly prevents us from adding some synchronization to ensure
that the detaching only happens when there's really no pending 
events?


SunToolkit.java:

2120 Boolean b = isThreadLocalFlushing.get();
2121 if (b != null  b) {
2122 return;
2123 }
2124 2125 isThreadLocalFlushing.set(true);
2126 try {


How does access to the isThreadLocalFlushing synchronized? What
happens if the flush() is being invoked from two different threads
for the same post event queue? Why do we have two isFlushing 
flags?
Can we collapse them into one? Why is the isFlushing set/reset in 
two

disjunct synchronized(){} blocks?

Overall, I find the current synchronization scheme in flush() very,
*very* (and I mean it) confusing. Can we simplify it somehow?

--
best regards,
Anthony

On 8/28/2012 6:33 PM, Oleg Pekhovskiy wrote:

Hi Artem, Anthony,

thank you for your proposals!

We with Artem also had off-line discussion,
so as a result I prepared improved version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.3/

What was done:
1. EventQueue.detachDispatchThread(): moved
SunToolkit.flushPnedingEvents() above the comments and added a
separate comment to it.
2. Moved SunToolkitSubclass.flushPendingEvents(AppContext) 
method to

SunToolkit. Deleted SunToolkitSubclass.
3. Moved isFlushingPendingEvents to PostEventQueue with the new 
name

- isThreadLocalFlushing and made it ThreadLocal.
4. Left PostEventQueue.flush() unsynchronized and created
wait()-notifyAll() synchronization mechanism to avoid blocking of
PostEventQueue.postEvent().

Looking forward to your comments!

Thanks,
Oleg

20.08.2012 20:20, Artem Ananiev wrote:

Hi, Oleg,

here are a few comments:

1. What is the reason of keeping isFlushingPendingEvents in
SunToolkit, given that PEQ.flush() is synchronized (and therefore
serialized) anyway?

2. flushPendingEvents(AppContext) may be moved directly to
SunToolkit, so we don't need a separate sun-class for that.

3. EQ.java:1035-1040

AWT Dev hg: jdk8/awt/jdk: 7153339: InternalError when drawLine with Xor and Antialiasing

[oleg . pekhovskiy]

Changeset: d14dc0ae1c2c
Author:bagiras
Date:  2012-09-06 17:57 +0400
URL:   http://hg.openjdk.java.net/jdk8/awt/jdk/rev/d14dc0ae1c2c

7153339: InternalError when drawLine with Xor and Antialiasing
Reviewed-by: prr, flar

! src/windows/classes/sun/java2d/ScreenUpdateManager.java

Re: AWT Dev [7u10] Review request for 7194469 Pressing the Enter key results in an alert tone beep when focus is TextField

[Oleg Pekhovskiy]

The fix looks good to me.

Thanks,
Oleg

9/6/2012 6:26 PM, Alexander Scherbatiy wrote:


This is the same fix backported from the JDK 8 to JDK 7u10 without 
changes:

bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7194469
webrev: http://cr.openjdk.java.net/~alexsch/7194469/webrev.01/


The TextField control starts beeping on some keys pressing (enter, up, 
down...) after switching the EDIT control to RICHEDIT.


The fix disables and enables the beeper by SystemParametersInfo method 
with the zero value in the fWinIni param for the RICHEDIT control if 
requested keys are pressed.



Thanks,
Alexandr.

AWT Dev [7u10] Review request for 7153339: InternalError when drawLine with Xor and Antialiasing

[Oleg Pekhovskiy]

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153339

Webrev:
http://cr.openjdk.java.net/~bagiras/7u10/7153339.3/

JDK 8 changeset:
http://hg.openjdk.java.net/jdk8/awt/jdk/rev/d14dc0ae1c2c

This is a direct backport from JDK 8.

Thanks,
Oleg

AWT Dev [7u10] Review request for 7188708 - REGRESSION: closed/java/awt/EventQueue/PostEventOrderingTest.java fails

[Oleg Pekhovskiy]

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7188708

Webrev:
http://cr.openjdk.java.net/~bagiras/7u10/7188708.1/

The reason is that isFlushingPendingEvents in 
SunToolkit.flushPendingEvents() is reset
after the first recursive call of flushPendingEvents(). Thus, if there 
are several pending events in PostEventQueue,
recursion would be rejected only for the first event, allowing nested 
call of PostEventQueue.flush() after.


To resolve the problem I added the counter flushNestingLevel instead 
of the flag isFlushingPendingEvents.
It's increased each time entering SunToolkit.flushPendingEvents() and 
decreased on exit.
So, PostEventQueue.flush() method is called only when we enter 
SunToolkit.flushPendingEvents() for the first time

within one thread.

That fix was prepared ONLY for 7u10.
For JDK 8 the fix for CR7186109 - Simplify lock machinery for 
PostEventQueue  EventQueue should cover this case.


Thanks,
Oleg

Re: AWT Dev [8] Review request for 7153339: InternalError when drawLine with Xor and Antialiasing

[Oleg Pekhovskiy]

Hi all,

please review the second version of fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153339

Webrev:
http://cr.openjdk.java.net/~bagiras/8/7153339.2/

To avoid influence to the platforms other than Windows and guarantee 
SurfaceData.getReplacement() returns valid surface
I made changes in ScreenUpdateManager.getReplacementScreenSurface() 
where WComponentPeer.replaceSurfaceData()

is called to make peer's surfaceData valid.

Thanks,
Oleg

8/25/2012 3:07 AM, Jim Graham wrote:

Hi Oleg,

Let me clarify, I'm not referring to a better way to fix this. 
Actually, the fix you have prepared has a problem.  Once the SD is 
invalid due to switching to software and the SG2D decides to install 
a null SD instead, then it will never recover.  But, no change to a 
drawable should happen (other than it going away) to cause that state. 
The SG2D's were designed to survive changes in state to the underlying 
drawable, as long as it remains viable as a drawing destination, but 
here you are giving up on it when that (externally not really very 
obvious) state change occurs.  Thread A could be happily rendering to 
the surface on the screen, thread B decides to use XOR thereby 
changing the state, but thread A's G2D now stops working?  Thread A 
would be very very confused.


So, while there may be a fix we could add to SG2D to work around the 
problem, your fix isn't the right solution.  I believe that the right 
solution is to have the D3DSurface return the correct new surface, but 
it being what I think is the right solution is just part of the 
issue with your currently proposed fix - the proposed fix violates a 
long-standing behavior of G2D objects to remain viable until the 
surface is gone...


...jim

On 8/24/2012 3:01 AM, Oleg Pekhovskiy wrote:

Hi Jim,

first of all I should say, that I prepared that fix for 7u as the most
safe, with minimum changes.
I agree that getReplacement() should return a valid sufrace or null, but
it doesn't happen during that switch.

That CR was assigned to me because my previous changes for:
7112642 Incorrect checking for graphics rendering object
7121482 some sun/java2d and sun/awt tests failed with 
InvalidPipeException

discovered the problem with getReplacement() and were somehow related.

But maybe it would be better to create a separate CR for
getReplacement() issue and assign it to Java2d Team?
I also could add a comment for !surfaceData.isValid() reffering to
CR7153339.

What do you think?

Thanks,
Oleg

24.08.2012 4:04, Jim Graham wrote:

Hi Oleg,

getReplacement should not be returning an invalid surface. If the
current D3DSD is invalid, then it should return a valid replacement.
The only time it should return null is when the window is gone. It
sounds like the window isn't gone here, it has just switched over to a
non-D3D type of SurfaceData and return that...

...jim

On 8/23/2012 4:37 PM, Oleg Pekhovskiy wrote:

Hi Phil, Jim,

thank you for pointing out the testing work that should be performed.
I tested my fix with the following regression tests:
test/java/awt/Graphics
test/java/awt/Graphics2D
test/java/awt/GraphicsDevice
test/java/awt/GraphicsEnvironment
test/sun/java2d

Plus I tested performance differences on:
demo/jfc/Java2D/Java2Demo.jar

Testing was done on Windows 7  Ubuntu 12.04 LTS.
No differences were found.

I also asked Yuri Nesterenko to test all that stuff on Mac.

Thanks,
Oleg


11.08.2012 3:10, Phil Race wrote:

Oleg,
This looks OK to me but since this is a shared code change I have
to ask what testing you've done ?

Also why not provide a regression test ? The provided test was
interactive but I think it can be automated.

You need another review and I'd like Jim to take a look.

-phil.

On 8/10/2012 2:26 PM, Oleg Pekhovskiy wrote:

Hi,

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153339

Webrev:
http://cr.openjdk.java.net/~bagiras/8/7153339.1/

Comments:
XOR is not supported for D3D (see comments inside
D3DSurfaceData.validatePipe()) and
software rendering is used invalidating current D3DSurfaceData.
So we have situation when component's peer has invalid SurfaceData
and it's retrieved in
SunGraphics2D.revalidateAll() through SurfaceData.getReplacement()
without any check.
That's why I added validity check there.

Thanks,
Oleg

http://cr.openjdk.java.net/%7Ebagiras/8/7153339.1/

Re: AWT Dev [8] Review request for 7186109: Simplify lock machinery for PostEventQueue EventQueue

[Oleg Pekhovskiy]

Hi Peter,

making PostEventQueue.flush() method 'synchronized' will block Toolkit 
thread during PostEventQueue.postEvent()
call, that is bad. In our case synchronization monitor is released on 
wait(), thus no blocking occurs.


Thanks,
Oleg

31.08.2012 1:01, Peter Levart wrote:


If I'm right, then instead of using thread-local flag for 
recursion-prevention, you can just re-instate a boolean flag:


private boolean isFlushing = false;

public synchronized void flush() {

if (isFlushing) {

// every EventQueue.postEvent calls us back - prevent recursion

return;

}

isFlushing = true;

try {

EventQueueItem tempQueue = queueHead;

queueHead = queueTail = null;

while (tempQueue != null) {

eventQueue.postEvent(tempQueue.event);

tempQueue = tempQueue.next;

}

}

}

finally {

isFlushing = false;

}

}

Regards, Peter

On Thursday, August 30, 2012 10:39:03 PM Peter Levart wrote:

Hi Oleg,

Now that SunToolkit is never called from EventQueue while holding 
pushPopLock (not even from detatchDispatchThread - I saw you removed 
SunToolkit.isPostEventQueueEmpty() check), there's no need for 
flushing loop in PostEventQueue not to be simply synchronized again 
and be done with InterruptedExceptions and handlers, Am I right?


Regards, Peter

On Thursday, August 30, 2012 04:42:00 PM Artem Ananiev wrote:

 Hi, Anthony,



 On 8/29/2012 3:08 PM, Anthony Petrov wrote:

  Hi Oleg,

 

  I'm still concerned about the following:

 

  detachDispatchThread()

  {

  flush();

  lock();

  // possibly detach

  unlock();

  }

 

  at EventQueue.java. What if an even get posted to the queue after the

  flush() returns but before we even acquired the lock? We may still end

  up with a situation when we detach the thread while in fact there are

  some pending events present, which actually contradicts the current

  logic of the detach() method. I see that you say Minimize discard

  possibility in the comment instead of Prevent ..., but I feel

  uncomfortable with this actually.



 yes, this is a known issue: we don't guarantee that no new events are

 posted between flush() and lock(). If this happens, we'll re-create

 event dispatch thread.



  What exactly prevents us from adding some synchronization to 
ensure that


  the detaching only happens when there's really no pending events?



 As Oleg wrote, this is exactly the deadlock we're trying to resolve.

 EQ.detachDispatchThread() was the only place where the order of locks

 was pushPopLock-flushLock, while in other cases we flush events without

 pushPopLock.



  SunToolkit.java:

  2120 Boolean b = isThreadLocalFlushing.get();

  2121 if (b != null  b) {

  2122 return;

  2123 }

  2124 2125 isThreadLocalFlushing.set(true);

  2126 try {

 

  How does access to the isThreadLocalFlushing synchronized? What 
happens


  if the flush() is being invoked from two different threads for the 
same


  post event queue? Why do we have two isFlushing flags? Can we 
collapse


  them into one? Why is the isFlushing set/reset in two disjunct

  synchronized(){} blocks?



 As David correctly wrote, isThreadLocalFlushing is a ThreadLocal object,

 which is thread-safe. isFlushing is used to synchronize access from

 multiple threads, isThreadLockFlushing is to prevent EQ.postEvent() to

 be called recursively.



 The only valid comment is that isThreadLocalFlushing should be set to

 false in the finally block. Oleg will include this change into the

 next version of the fix.



  Overall, I find the current synchronization scheme in flush() very,

  *very* (and I mean it) confusing. Can we simplify it somehow?



 The current Oleg's fix is the simplest yet (almost) backwards compatible

 solution we've found. If you have another ideas, please, let us know :)



 Thanks,



 Artem



  --

  best regards,

  Anthony

 

  On 8/28/2012 6:33 PM, Oleg Pekhovskiy wrote:

  Hi Artem, Anthony,

 

  thank you for your proposals!

 

  We with Artem also had off-line discussion,

  so as a result I prepared improved version of fix:

  http://cr.openjdk.java.net/~bagiras/8/7186109.3/

 

  What was done:

  1. EventQueue.detachDispatchThread(): moved

  SunToolkit.flushPnedingEvents() above the comments and added a

  separate comment to it.

  2. Moved SunToolkitSubclass.flushPendingEvents(AppContext) method to

  SunToolkit. Deleted SunToolkitSubclass.

  3. Moved isFlushingPendingEvents to PostEventQueue with the new 
name -


  isThreadLocalFlushing and made it ThreadLocal.

  4. Left PostEventQueue.flush() unsynchronized and created

  wait()-notifyAll() synchronization mechanism to avoid blocking of

  PostEventQueue.postEvent().

 

  Looking forward to your comments!

 

  Thanks,

  Oleg

 

  20.08.2012 20:20, Artem Ananiev wrote:

  Hi, Oleg,

 

  here are a few comments:

 

  1. What is the reason of keeping isFlushingPendingEvents in

  SunToolkit, given that PEQ.flush() is synchronized (and therefore

  serialized) anyway?

 

  2. flushPendingEvents

Re: AWT Dev [8] Review request for 7186109: Simplify lock machinery for PostEventQueue EventQueue

[Oleg Pekhovskiy]

Anthony, David,
thank you for the review!

Your comments are reasonable,
I'll try to apply them both.

Thanks,
Oleg

29.08.2012 16:12, David Holmes wrote:

 Ah, I see. Thanks for the insight. It now looks much clearer. I think
 that the final isThreadLocalFlushing.set(false); must be in the
 finally{} block, though.

Right!

Also there is a problem with the InterruptedException handling. Let 
thread A set isFlushing and be busy flushing. Then let Thread B call 
wait() but be interrupted. Thread B will enter the finally block grab 
the lock and set isFlushing to false, even though Thread A is actively 
flushing! We don't want the finally block to execute if 
InterruptedException is caught.


David

On 29/08/2012 10:02 PM, Anthony Petrov wrote:

Hi David,

On 8/29/2012 3:45 PM, David Holmes wrote:

I took a look at the last incarnation of this so let me see if I can
follow the new scheme.

On 29/08/2012 9:08 PM, Anthony Petrov wrote:

Hi Oleg,

I'm still concerned about the following:

detachDispatchThread()
{
flush();
lock();
// possibly detach
unlock();
}

at EventQueue.java. What if an even get posted to the queue after the
flush() returns but before we even acquired the lock? We may still end
up with a situation when we detach the thread while in fact there are
some pending events present, which actually contradicts the current
logic of the detach() method. I see that you say Minimize discard
possibility in the comment instead of Prevent ..., but I feel
uncomfortable with this actually.


If a new event is posted before the lock() then within the locked
region won't peekEvent() see it and so avoid the detach?


peekEvent() checks the event queue only, while the posted event may be
stuck in the PostEventQueue. The flushPendingEvents() actually posts the
events from the PostEventQueue to the EventQueue.


What exactly prevents us from adding some synchronization to ensure 
that

the detaching only happens when there's really no pending events?

SunToolkit.java:

2120 Boolean b = isThreadLocalFlushing.get();
2121 if (b != null  b) {
2122 return;
2123 }
2124 2125 isThreadLocalFlushing.set(true);
2126 try {


How does access to the isThreadLocalFlushing synchronized? What 
happens
if the flush() is being invoked from two different threads for the 
same
post event queue? Why do we have two isFlushing flags? Can we 
collapse

them into one? Why is the isFlushing set/reset in two disjunct
synchronized(){} blocks?


isThreadLocalFlushing is a ThreadLocal so no synchronization is
needed. I presume it is used to prevent re-entrant/recursive calls to
flush() when calling postEvent.

The isFlushing variable is the global flag to coordinate flushing
across multiple threads. It has to be set and cleared in synchronized
blocks, but the synchronization lock has to be dropped when calling
postEvent to avoid deadlocks. So a thread acquires the lock and checks
if flushing is in progress, and if so it waits. Else/then it updates
isFlushing to indicate if that thread is doing flushing and releases
the lock. It then does any flushing needed, reacquires the lock, sets
isFlushing to false and notifies any other threads who may be waiting.


Overall, I find the current synchronization scheme in flush() very,
*very* (and I mean it) confusing. Can we simplify it somehow?


This seems like a reasonable protocol to coordinate multiple flushers
whilst dropping the synchronization lock when posting events. The
actual coordination might be simpler to understand if expressed using
a Semaphore - but I think the semantics would be the same.


Ah, I see. Thanks for the insight. It now looks much clearer. I think
that the final isThreadLocalFlushing.set(false); must be in the
finally{} block, though.

--
best regards,
Anthony




David


--
best regards,
Anthony

On 8/28/2012 6:33 PM, Oleg Pekhovskiy wrote:

Hi Artem, Anthony,

thank you for your proposals!

We with Artem also had off-line discussion,
so as a result I prepared improved version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.3/

What was done:
1. EventQueue.detachDispatchThread(): moved
SunToolkit.flushPnedingEvents() above the comments and added a
separate comment to it.
2. Moved SunToolkitSubclass.flushPendingEvents(AppContext) method to
SunToolkit. Deleted SunToolkitSubclass.
3. Moved isFlushingPendingEvents to PostEventQueue with the new 
name -

isThreadLocalFlushing and made it ThreadLocal.
4. Left PostEventQueue.flush() unsynchronized and created
wait()-notifyAll() synchronization mechanism to avoid blocking of
PostEventQueue.postEvent().

Looking forward to your comments!

Thanks,
Oleg

20.08.2012 20:20, Artem Ananiev wrote:

Hi, Oleg,

here are a few comments:

1. What is the reason of keeping isFlushingPendingEvents in
SunToolkit, given that PEQ.flush() is synchronized (and therefore
serialized) anyway?

2. flushPendingEvents(AppContext) may be moved directly to
SunToolkit, so we don't need a separate sun-class for that.

3. EQ.java:1035-1040 - this comment

Re: AWT Dev [8] Review request for 7186109: Simplify lock machinery for PostEventQueue EventQueue

[Oleg Pekhovskiy]

Hi Artem, Anthony,

thank you for your proposals!

We with Artem also had off-line discussion,
so as a result I prepared improved version of fix:
http://cr.openjdk.java.net/~bagiras/8/7186109.3/

What was done:
1. EventQueue.detachDispatchThread(): moved 
SunToolkit.flushPnedingEvents() above the comments and added a separate 
comment to it.
2. Moved SunToolkitSubclass.flushPendingEvents(AppContext) method to 
SunToolkit. Deleted SunToolkitSubclass.
3. Moved isFlushingPendingEvents to PostEventQueue with the new name - 
isThreadLocalFlushing and made it ThreadLocal.
4. Left PostEventQueue.flush() unsynchronized and created 
wait()-notifyAll() synchronization mechanism to avoid blocking of 
PostEventQueue.postEvent().


Looking forward to your comments!

Thanks,
Oleg

20.08.2012 20:20, Artem Ananiev wrote:

Hi, Oleg,

here are a few comments:

1. What is the reason of keeping isFlushingPendingEvents in 
SunToolkit, given that PEQ.flush() is synchronized (and therefore 
serialized) anyway?


2. flushPendingEvents(AppContext) may be moved directly to SunToolkit, 
so we don't need a separate sun-class for that.


3. EQ.java:1035-1040 - this comment is obsolete and must be replaced 
by another one.


Thanks,

Artem

On 8/17/2012 4:49 PM, Oleg Pekhovskiy wrote:

Hi!

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7186109

Webrev:
http://cr.openjdk.java.net/~bagiras/8/7186109.1/

The following changes were made:
1. Removed flushLock from SunToolkit.flushPendingEvent()
2. Returned method PostEventQueue.flush() as 'synchronized' back
3. Added call of SunToolkit.flushPendingEvents() to
EventQueue.detachDispatchThread(),
right before pushPopLock.lock()
4. Removed !SunToolkit.isPostEventQueueEmpty() check from
EventQueue.detachDispatchThread()
5. Removed SunToolkit.isPostEventQueueEmpty()  
PostEventQueue.noEvents();


Thanks,
Oleg
http://cr.openjdk.java.net/%7Ebagiras/8/7186109.1/

Re: AWT Dev [8] Review request for 7153339: InternalError when drawLine with Xor and Antialiasing

[Oleg Pekhovskiy]

Hi Phil,

I forgot to mention the test creation problem.

The manual test proposed in CR's description just
relies on specific call sequence after getReplacement() returns invalid 
surfaceData.


So the test doesn't catch InvalidPipeException when invalid surfaceData 
found,

because this exception is cought in sun.java2d.SunGraphics2D.drawLine().

But it just catches java.lang.InternalError that is thrown on catch of 
Throwable exception

(indeed InvalidPipeException) that is thrown from:
sun.java2d.SunGraphics2D.validatePipe(SunGraphics2D.java:380)
with the stack:
sun.java2d.SunGraphics2D.revalidateAll(SunGraphics2D.java:2363)
sun.java2d.SunGraphics2D.getCompClip(SunGraphics2D.java:496)
sun.java2d.pipe.LoopPipe.getStrokeSpans(LoopPipe.java:270)
sun.java2d.pipe.LoopPipe.draw(LoopPipe.java:201)
sun.java2d.pipe.PixelToShapeConverter.drawLine(PixelToShapeConverter.java:52)
sun.java2d.pipe.ValidatePipe.drawLine(ValidatePipe.java:62)
sun.java2d.SunGraphics2D.drawLine(SunGraphics2D.java:2137)

Could we rely on such indirect error in our case?
As I remember you had plans to refactor Graphics related stuff, so the 
sequence could change

making test's expectations odd.

Thanks,
Oleg

24.08.2012 3:37, Oleg Pekhovskiy wrote:

Hi Phil, Jim,

thank you for pointing out the testing work that should be performed.
I tested my fix with the following regression tests:
test/java/awt/Graphics
test/java/awt/Graphics2D
test/java/awt/GraphicsDevice
test/java/awt/GraphicsEnvironment
test/sun/java2d

Plus I tested performance differences on:
demo/jfc/Java2D/Java2Demo.jar

Testing was done on Windows 7  Ubuntu 12.04 LTS.
No differences were found.

I also asked Yuri Nesterenko to test all that stuff on Mac.

Thanks,
Oleg


11.08.2012 3:10, Phil Race wrote:

Oleg,
This looks OK to me but since this is a shared code change I have
to ask what testing you've done ?

Also why not provide a regression test ? The provided test was
interactive but I think it can be automated.

You need another review and I'd like Jim to take a look.

-phil.

On 8/10/2012 2:26 PM, Oleg Pekhovskiy wrote:

Hi,

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153339

Webrev:
http://cr.openjdk.java.net/~bagiras/8/7153339.1/

Comments:
XOR is not supported for D3D (see comments inside 
D3DSurfaceData.validatePipe()) and

software rendering is used invalidating current D3DSurfaceData.
So we have situation when component's peer has invalid SurfaceData 
and it's retrieved in
SunGraphics2D.revalidateAll() through SurfaceData.getReplacement() 
without any check.

That's why I added validity check there.

Thanks,
Oleg

http://cr.openjdk.java.net/%7Ebagiras/8/7153339.1/

Re: AWT Dev [8] Review request for 7153339: InternalError when drawLine with Xor and Antialiasing

[Oleg Pekhovskiy]

Hi Phil, Jim,

thank you for pointing out the testing work that should be performed.
I tested my fix with the following regression tests:
test/java/awt/Graphics
test/java/awt/Graphics2D
test/java/awt/GraphicsDevice
test/java/awt/GraphicsEnvironment
test/sun/java2d

Plus I tested performance differences on:
demo/jfc/Java2D/Java2Demo.jar

Testing was done on Windows 7  Ubuntu 12.04 LTS.
No differences were found.

I also asked Yuri Nesterenko to test all that stuff on Mac.

Thanks,
Oleg


11.08.2012 3:10, Phil Race wrote:

Oleg,
This looks OK to me but since this is a shared code change I have
to ask what testing you've done ?

Also why not provide a regression test ? The provided test was
interactive but I think it can be automated.

You need another review and I'd like Jim to take a look.

-phil.

On 8/10/2012 2:26 PM, Oleg Pekhovskiy wrote:

Hi,

Please review the fix for CR:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153339

Webrev:
http://cr.openjdk.java.net/~bagiras/8/7153339.1/

Comments:
XOR is not supported for D3D (see comments inside 
D3DSurfaceData.validatePipe()) and

software rendering is used invalidating current D3DSurfaceData.
So we have situation when component's peer has invalid SurfaceData 
and it's retrieved in
SunGraphics2D.revalidateAll() through SurfaceData.getReplacement() 
without any check.

That's why I added validity check there.

Thanks,
Oleg

http://cr.openjdk.java.net/%7Ebagiras/8/7153339.1/

Re: AWT Dev [7u8] Review request for 7189350: Fix failed for CR 7162144

[Oleg Pekhovskiy]

Looks fine to me.

Thanks,
Oleg

20.08.2012 19:24, Anthony Petrov wrote:

Hello,

Please review a fix for 
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7189350 at:


http://cr.openjdk.java.net/~anthony/7u8-1-missingAWTThread-7189350.0/

With this fix we resolve an issue originally described in 7162144 by 
properly handling thread interruption requests. We no longer override 
the Thread.interrupt() for the EDT, and hence don't clash with 
NetBeans using thread interruption for its own purposes.


Note that there's still not a regression test since NetBeans' test is 
very heavy (over 100MB) and impractical for our purposes. However, the 
NetBeans team has tested a developer build with this fix and found no 
regressions.


PS. We aren't yet sure if we want the same fix for JDK 8, hence we fix 
it for 7u8 first.


--
best regards,
Anthony