Re: AWT Dev [9] Review Request: 8132355 Incorrect guard block in HPkeysym.h, awt_Event.h
+1 On 27.07.2015 15:16, Alexander Zvegintsev wrote: Looks fine. Thanks, Alexander. On 07/27/2015 02:36 PM, Sergey Bylokhov wrote: Hello. Please review the fix for a typo in jdk9. The HPkeysym.h is from external library, so I have filed the bug in upstream too: https://bugs.freedesktop.org/show_bug.cgi?id=91469 Bug: https://bugs.openjdk.java.net/browse/JDK-8132355 Webrev can be found at: http://cr.openjdk.java.net/~serb/8132355/webrev.00
Re: AWT Dev [8u-dev] Review request for 8130776: Remove EmbeddedFrame.requestFocusToEmbedder() method
Looks fine to me. Regards, Anton. On 23.07.2015 17:09, Alexey Ivanov wrote: Hello AWT team, Could you please review the backport of JDK-8130776 to jdk8u-dev: http://cr.openjdk.java.net/~aivanov/8130776/jdk8/webrev.00/ The patch from JDK9 does not apply cleanly because of different mechanism to access peer in WEmbeddedFrame.java. Logically there are no changes. bug: https://bugs.openjdk.java.net/browse/JDK-8130776 jdk9 webrev: http://cr.openjdk.java.net/~aivanov/8130776/jdk9/webrev.00/ jdk9 review thread: http://mail.openjdk.java.net/pipermail/awt-dev/2015-July/009667.html jdk9 changeset: http://hg.openjdk.java.net/jdk9/client/jdk/rev/207a6ebae49d Thanks, Alexey
Re: AWT Dev [9] Review request for 8130776: Remove EmbeddedFrame.requestFocusToEmbedder() method
Hi Alexey, Looks fine to me. Regards, Anton. On 21.07.2015 13:17, Alexey Ivanov wrote: Hello AWT team, Could you please review the following fix: bug: https://bugs.openjdk.java.net/browse/JDK-8130776 webrev: http://cr.openjdk.java.net/~aivanov/8130776/jdk9/webrev.00/ Description: Cleaning up the unused method. It was added under JDK-8056915 Focus lost in applet when browser window is minimized and restored. Later the code has been changed, and this method, requestFocusToEmbedder(), is not used any more. Thanks, Alexey
Re: AWT Dev [9] Review Request: 8067093 Fix windows-specific deprecation warnings in the java.desktop module
Looks fine. Regards, Anton. On 22.07.2015 1:01, Sergey Bylokhov wrote: Hello. Please review the fix for jdk9. In the fix I replace the usage of the deprecated api in WListPeer. Most of the other cases in the bug description are related to getPeer(), which was already removed. The usage of deprecated constructor of Win32GraphicsConfig in its subclasses was not changed. Bug: https://bugs.openjdk.java.net/browse/JDK-8067093 Webrev can be found at: http://cr.openjdk.java.net/~serb/8067093/webrev.01
Re: AWT Dev [9] Review Request: 7178683 [macosx] The default directory for open dialog is different for FileDialogOpenDirTest.html
Looks fine to me. Anton. On 09.06.2015 22:34, Sergey Bylokhov wrote: Hello. Please review the small fix for jdk9. The test was created for the JDK-4974135 and checks specific to xawt/mawt behavior. It is not applicable to the osx as well as windows. The test was moved to the open, but the diff is provided [1]. [1] http://cr.openjdk.java.net/~serb/7178683/diff/ Bug: https://bugs.openjdk.java.net/browse/JDK-7178683 Webrev can be found at: http://cr.openjdk.java.net/~serb/7178683/webrev.00
Re: AWT Dev [9] Review Request: 8025492 Hand cursor does not use Windows' system cursor
Looks fine to me. Anton. On 09.06.2015 20:03, Sergey Bylokhov wrote: Hello. Please review the small fix for jdk9. The fix was contributed-by morvan.lemes...@gmail.com [1] For historical reasons jdk uses its own hand cursor(which is actually the same as a default win cursor) instead of native cursor[2]. But on current win we can use the system cursor without problems. [1] http://mail.openjdk.java.net/pipermail/awt-dev/2013-April/004675.html [2] http://mail.openjdk.java.net/pipermail/awt-dev/2013-April/004714.html Bug: https://bugs.openjdk.java.net/browse/JDK-8025492 Webrev can be found at: http://cr.openjdk.java.net/~serb/8025492/webrev.00
Re: AWT Dev Awt Dev [9] Review Request for 8017487: filechooser in Windows-Libraries folder: columns are mixed up
I looked through and found nothing bad (I'm not aware of the details of this Win32 API). Regards, Anton. On 01.06.2015 11:45, Semyon Sadetsky wrote: Hi Sergey, Not 100% sure that Libraries is not localized. So, I have excluded it: http://cr.openjdk.java.net/~ssadetsky/8017487/webrev.01/ --Semyon On 5/29/2015 1:28 PM, Sergey Bylokhov wrote: Hi, The fix looks fine, except Libraries text. Does it mean that the same text is used in all locales? On 26.05.15 8:09, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8017487 webrev: http://cr.openjdk.java.net/~ssadetsky/8017487/webrev.00/ File details view columns obtained by the legacy special folder API for the virtual Windows libraries are not consistent with child files inside those libraries. The details can be obtained only using new Windows APIs since MS stops to support the old API for the new Windows libraries. The fix redirects column details requests for Windows Libraries to their real file system locations. --Semyon
Re: AWT Dev Awt Dev [9] Review Request for 8022057: JFileChooser blocks EDT in Win32ShellFolder2.getIcon
Ok, good. Thanks. Anton. On 01.06.2015 18:02, Semyon Sadetsky wrote: Hi Anton, On my laptop both calls are always very fast even when I open a USB drive dir with many files. Probably it is very rare situation but Netbeans reported it several times and people complains on delays of tens of seconds. I could not reproduce that. I made a guess that following MS recommendations can eliminate delays + I added Windows libraries icons. --Semyon On 6/1/2015 5:41 PM, Anton V. Tarasov wrote: Hi Semyon, The idea of the fix looks ok to me. On 28.05.2015 9:44, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8022057 webrev: http://cr.openjdk.java.net/~ssadetsky/8022057/webrev.00/ The full story can be found in the jira's comments and NetBeans tracker (https://netbeans.org/bugzilla/show_bug.cgi?id=188001). It seems the bug proposes to change the design of the AWT shell support on Windows platform. But instead I tried to eliminate the user experience issue it can be a good step to improve the situation. The user experience issue is the JFileChooser spontaneous delays caused by getIcon(): I could not reproduce this under Win7 and jdk8/9. But I found in MSDN that ExtractIcon Win32 API call can take significant amount of time in some cases. Mostly when the file is an executable or a link and its icon is not cached yet. MS propose a way how to avoid that: use asynchronous flag GIL_ASYNC with GetIconLocation call which then may return E_PENDING which means consequent ExtractIcon call can take time. There are several ways to handle E_PENDING return I propose just to use the default icon for the file which can be obtained with GIL_DEFAULTICON flag and should be much faster. Since I cannot reproduce the issue I don't know how effective it will be. But did you simply try to load with GIL_DEFAULTICON for a sanity check? Is it really much faster? Regards, Anton. Also in the fix I added possibility to get Windows-Libraries icons, which were not available before in the JFileChooser. --Semyon
Re: AWT Dev Awt Dev [9] Review Request for 8022057: JFileChooser blocks EDT in Win32ShellFolder2.getIcon
Hi Semyon, The idea of the fix looks ok to me. On 28.05.2015 9:44, Semyon Sadetsky wrote: Hello, Please review fix for JDK9: bug: https://bugs.openjdk.java.net/browse/JDK-8022057 webrev: http://cr.openjdk.java.net/~ssadetsky/8022057/webrev.00/ The full story can be found in the jira's comments and NetBeans tracker (https://netbeans.org/bugzilla/show_bug.cgi?id=188001). It seems the bug proposes to change the design of the AWT shell support on Windows platform. But instead I tried to eliminate the user experience issue it can be a good step to improve the situation. The user experience issue is the JFileChooser spontaneous delays caused by getIcon(): I could not reproduce this under Win7 and jdk8/9. But I found in MSDN that ExtractIcon Win32 API call can take significant amount of time in some cases. Mostly when the file is an executable or a link and its icon is not cached yet. MS propose a way how to avoid that: use asynchronous flag GIL_ASYNC with GetIconLocation call which then may return E_PENDING which means consequent ExtractIcon call can take time. There are several ways to handle E_PENDING return I propose just to use the default icon for the file which can be obtained with GIL_DEFAULTICON flag and should be much faster. Since I cannot reproduce the issue I don't know how effective it will be. But did you simply try to load with GIL_DEFAULTICON for a sanity check? Is it really much faster? Regards, Anton. Also in the fix I added possibility to get Windows-Libraries icons, which were not available before in the JFileChooser. --Semyon
Re: AWT Dev [9] Review Request: 8041654 OutOfMemoryError: RepainManager doesn't clean up cache of volatile images
Hi Sergey,
The fix looks fine to me. However, I don't clearly understand what did you mean
by these comments?
1689 // Empty non private constructor was added because access to this
1690 // class shouldn't be emulated by a synthetic accessor method.
1691 DisplayChangedHandler() {
1692 super();
1693 }
I would expect something like we need to be able to instantiate the class, but why you refer to
accessors?...
Also, do you think the super() call is necessary here?
Reagards,
Anton.
On 21.05.2015 15:11, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk9.
RepainManager adds a listener to the SGE.DisplayChangedListener() and it don't hold a strong
reference to this listener. The problem is that SGE holds a listeners in the WeakHashMap so
sometimes the listeners can be cleared before they are called. Same issue exists in XToolkit.
Bug: https://bugs.openjdk.java.net/browse/JDK-8041654
Webrev can be found at: http://cr.openjdk.java.net/~serb/8041654/webrev.02
Re: AWT Dev [9] Review request for 8003399: JFileChooser gives wrong path to selected file when saving to Libraries folder on Windows 7
Hi Semyon, I'm fine with it, but don't you want to define a simple macro for this: +jfieldID field_guid = env-GetFieldID(cl, guid, Ljava/lang/String;); +DASSERT(field_guid != NULL); +CHECK_NULL_RETURN(field_guid, NULL); To call it like: DEFINE_FIELD_ID(field_guid, cl, guid, Ljava/lang/String;); You would reduce the code a lot and make it more readable. Regards, Anton. On 19.05.2015 18:45, Semyon Sadetsky wrote: Hi Anton, here is an updated version: http://cr.openjdk.java.net/~ssadetsky/8003399/webrev.01/ --Semyon On 5/8/2015 5:01 PM, Semyon Sadetsky wrote: On 5/8/2015 3:45 PM, Sergey Bylokhov wrote: On 07.05.15 15:29, Semyon Sadetsky wrote: Hi Sergey, Yes, after the fix filedialog produces usual filesystem paths for libraries which are readable for java.io. Just to clarify: after the fix, both Open and Save dialog works? Open file in library was not a problem, because an exicting file has real FS path already. But there are no possibility to reference files in libraries directly using new File(library link). --Semyon On 5/7/2015 11:26 AM, Sergey Bylokhov wrote: Hi, Semyon. Can you please raise the supportness of this in the java.io on the core-libs alias. Does the open filedialog will work after the fix? On 07.05.15 11:14, Semyon Sadetsky wrote: Hello, Please review fix for JDK9. webrev: http://cr.openjdk.java.net/~ssadetsky/8003399/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8003399 ***THE ROOT CAUSE JDK uses legacy WINAPI special folders calls while MS introduced a new interfaces IKnownFolder and IShellLibrary to manage special folder locations and the new Libraries functionality in Windows 7 is not backward compatible with old special folders CSIDL. ***SOLUTION Since it is too expensive to migrate AWT shell to the new interfaces and still they are not supported in java.io the solution is to map virtual folder PIDL to the Known Folder GUID and replace libraries links with the default library save location. Thus the File save dialog will be able to work with any Libraries registered in the system (Windows Libraries concept assumes that Libraries can be added arbitrary). The resulting code should be compatible with older Windows versions because the new COM interfaces are called only if they are available and a Libraries link has been actually requested. ***TESTING A test scenario is added to check that all available Libraries links are converted into filesystem paths. --Semyon
Re: AWT Dev [9] Review Request: 8071306 GUI perfomance are very slow compared java 1.6.0_45
On 15.05.2015 1:57, Sergey Bylokhov wrote: Hi, Anton. + * Determines the bounds of a visible part of the component relative to its + * parents. Did you mean to its parent? Yep, new version: http://cr.openjdk.java.net/~serb/8071306/webrev.05/ Looks good to me. Regards, Anton. 2. 100 * The components in this container. 101 * @see #add 102 * @see #getComponents 103 */ 104 private java.util.ListComponent component = new ArrayList(); May be it's worth to rename the field? The component name is odd... I suppose it wasn't changed, because this name is used in the serialization for a long time. Plus there are a bunch of the similar vars like: tmpComponent etc. I can do it later for jdk9 only. Ok, thanks. It's up to you. Regards, Anton. Regards, Anton. On 07.05.2015 3:39, Sergey Bylokhov wrote: Hello. Please review the fix for a jdk9. I plan to backport it to jdk8u60. Description. An UIworks really slowly, when an application has a lot of components in one container, and these components should be disabled one by one. The reason is the next sequence of methods calls: Component.setEnabled-updateCursorImmediately()- some cursor related staff-GlobalCursorManager._updateCursor-Container.findComponentAt()-iteration over all components in the container.- twice You can imagine how it works in case of 1 components in the container. Note that in the bug report described difference jdk6 vs jdk8 - 1sec vs 6 sec. This was caused by the two fixes, one of which adds checkTreeLock() and in another one a simple array of components was replaced by the ArrayList. Since code was added to the really hot method we got so big slowdown. To fix the problem I suggest two different approaches: - Container.java: Fix a general case, by eliminating a second iteration in the hot loop. - Component.java: Totally eliminates cursor machinery, if component cannot affect current cursor. Some speedup measurements on my local system: - Simple removing of checkTreeLock() will partly solve a regression reported by the user(12 sec - 5 sec). - Changes in the loop will speedup the code in the worse case(5-2 sec) - The changes in the Component.java will change the performance from 2 sec to 100 ms Test case which was added was improved from 10 seconds to 80 ms. JMH test: http://cr.openjdk.java.net/~serb/8071306/SetEnabledPerformanceTest.java Fixedversion: Benchmark Mode Cnt Score Error Units SetEnabledPerformanceTest.testContains thrpt5 301300,813 ± 17338,045 ops/ms SetEnabledPerformanceTest.testFindComponentAt thrpt5 20,521 ± 0,269 ops/ms SetEnabledPerformanceTest.testGetComponentAt thrpt5 22,297 ± 1,264 ops/ms SetEnabledPerformanceTest.testSetEnabled thrpt5 711,120 ±19,837 ops/ms Base version: Benchmark Mode Cnt Score Error Units SetEnabledPerformanceTest.testContains thrpt5 299145,642 ± 2120,183 ops/ms SetEnabledPerformanceTest.testFindComponentAt thrpt5 1,101 ±0,012 ops/ms SetEnabledPerformanceTest.testGetComponentAt thrpt5 6,792 ±0,097 ops/ms SetEnabledPerformanceTest.testSetEnabled thrpt5 0,464 ±0,020 ops/ms Bug: https://bugs.openjdk.java.net/browse/JDK-8071306 Webrev can be found at: http://cr.openjdk.java.net/~serb/8071306/webrev.03 -- Best regards, Sergey. -- Best regards, Sergey. -- Best regards, Sergey.
Re: AWT Dev internal API usage: sun.awt.CausedFocusEvent
Hi Alan, Appropriate RFE is targeted to jdk9: https://bugs.openjdk.java.net/browse/JDK-8080395 Regards, Anton. On 13.05.2015 20:40, Alan Snyder wrote: I have been using sun.awt.CausedFocusEvent to implement special behavior in response to an explicit user-initiated focus traversal to a component. The main point is that I want to inhibit this behavior for all other kinds of focus gained events. Will there be some way of doing this in JDK9?
Re: AWT Dev [9] Review Request: 8071306 GUI perfomance are very slow compared java 1.6.0_45
Hi Sergey,
On 09.05.2015 3:41, Sergey Bylokhov wrote:
Hi, Anton.
On 08.05.15 12:23, Anton V. Tarasov wrote:
1314 /**
1315 * Determines the bounds which will be displayed on the screen.
1316 *
1317 * @return the visible part of bounds in the coordinate space of this
comp
1318 */
1319 private Rectangle getRecursivelyVisibleBounds() {
Could you please clarify the comment, it's not quite clear from the first
glance. Something like:
the bounds of a visible part of the component relative to...
The patch updated:
http://cr.openjdk.java.net/~serb/8071306/webrev.04
+ * Determines the bounds of a visible part of the component relative to its
+ * parents.
Did you mean to its parent? (If so, I don't mind fixing it in a commit
changeset only).
2.
100 * The components in this container.
101 * @see #add
102 * @see #getComponents
103 */
104 private java.util.ListComponent component = new ArrayList();
May be it's worth to rename the field? The component name is odd...
I suppose it wasn't changed, because this name is used in the serialization for a long time. Plus
there are a bunch of the similar vars like: tmpComponent etc. I can do it later for jdk9 only.
Ok, thanks. It's up to you.
Regards,
Anton.
Regards,
Anton.
On 07.05.2015 3:39, Sergey Bylokhov wrote:
Hello.
Please review the fix for a jdk9. I plan to backport it to jdk8u60.
Description.
An UIworks really slowly, when an application has a lot of components in one container, and
these components should be disabled one by one.
The reason is the next sequence of methods calls:
Component.setEnabled-updateCursorImmediately()- some cursor related
staff-GlobalCursorManager._updateCursor-Container.findComponentAt()-iteration over all
components in the container.- twice
You can imagine how it works in case of 1 components in the container.
Note that in the bug report described difference jdk6 vs jdk8 - 1sec vs 6 sec. This was caused
by the two fixes, one of which adds checkTreeLock() and in another one a simple array of
components was replaced by the ArrayList. Since code was added to the really hot method we got
so big slowdown.
To fix the problem I suggest two different approaches:
- Container.java: Fix a general case, by eliminating a second iteration in the
hot loop.
- Component.java: Totally eliminates cursor machinery, if component cannot
affect current cursor.
Some speedup measurements on my local system:
- Simple removing of checkTreeLock() will partly solve a regression reported by the user(12 sec
- 5 sec).
- Changes in the loop will speedup the code in the worse case(5-2 sec)
- The changes in the Component.java will change the performance from 2 sec to
100 ms
Test case which was added was improved from 10 seconds to 80 ms.
JMH test:
http://cr.openjdk.java.net/~serb/8071306/SetEnabledPerformanceTest.java
Fixedversion:
Benchmark Mode Cnt Score
Error Units
SetEnabledPerformanceTest.testContains thrpt5 301300,813 ±
17338,045 ops/ms
SetEnabledPerformanceTest.testFindComponentAt thrpt 5 20,521 ± 0,269
ops/ms
SetEnabledPerformanceTest.testGetComponentAt thrpt 5 22,297 ± 1,264
ops/ms
SetEnabledPerformanceTest.testSetEnabled thrpt 5 711,120 ±19,837
ops/ms
Base version:
Benchmark Mode Cnt Score Error
Units
SetEnabledPerformanceTest.testContains thrpt5 299145,642 ± 2120,183
ops/ms
SetEnabledPerformanceTest.testFindComponentAt thrpt 5 1,101 ±0,012
ops/ms
SetEnabledPerformanceTest.testGetComponentAt thrpt 5 6,792 ±0,097
ops/ms
SetEnabledPerformanceTest.testSetEnabled thrpt 5 0,464 ±0,020
ops/ms
Bug: https://bugs.openjdk.java.net/browse/JDK-8071306
Webrev can be found at: http://cr.openjdk.java.net/~serb/8071306/webrev.03
--
Best regards, Sergey.
--
Best regards, Sergey.
Re: AWT Dev [9] Review Request: 8071306 GUI perfomance are very slow compared java 1.6.0_45
Hi Sergey,
The perf improvment is great! I have some minor comments only:
1.
1314 /**
1315 * Determines the bounds which will be displayed on the screen.
1316 *
1317 * @return the visible part of bounds in the coordinate space of this
comp
1318 */
1319 private Rectangle getRecursivelyVisibleBounds() {
Could you please clarify the comment, it's not quite clear from the first
glance. Something like:
the bounds of a visible part of the component relative to...
2.
100 * The components in this container.
101 * @see #add
102 * @see #getComponents
103 */
104 private java.util.ListComponent component = new ArrayList();
May be it's worth to rename the field? The component name is odd...
Regards,
Anton.
On 07.05.2015 3:39, Sergey Bylokhov wrote:
Hello.
Please review the fix for a jdk9. I plan to backport it to jdk8u60.
Description.
An UIworks really slowly, when an application has a lot of components in one container, and these
components should be disabled one by one.
The reason is the next sequence of methods calls:
Component.setEnabled-updateCursorImmediately()- some cursor related
staff-GlobalCursorManager._updateCursor-Container.findComponentAt()-iteration over all
components in the container.- twice
You can imagine how it works in case of 1 components in the container.
Note that in the bug report described difference jdk6 vs jdk8 - 1sec vs 6 sec. This was caused by
the two fixes, one of which adds checkTreeLock() and in another one a simple array of components
was replaced by the ArrayList. Since code was added to the really hot method we got so big slowdown.
To fix the problem I suggest two different approaches:
- Container.java: Fix a general case, by eliminating a second iteration in the
hot loop.
- Component.java: Totally eliminates cursor machinery, if component cannot
affect current cursor.
Some speedup measurements on my local system:
- Simple removing of checkTreeLock() will partly solve a regression reported by the user(12 sec
- 5 sec).
- Changes in the loop will speedup the code in the worse case(5-2 sec)
- The changes in the Component.java will change the performance from 2 sec to
100 ms
Test case which was added was improved from 10 seconds to 80 ms.
JMH test:
http://cr.openjdk.java.net/~serb/8071306/SetEnabledPerformanceTest.java
Fixedversion:
Benchmark Mode Cnt Score
Error Units
SetEnabledPerformanceTest.testContains thrpt5 301300,813 ±
17338,045 ops/ms
SetEnabledPerformanceTest.testFindComponentAt thrpt5 20,521 ± 0,269
ops/ms
SetEnabledPerformanceTest.testGetComponentAt thrpt5 22,297 ± 1,264
ops/ms
SetEnabledPerformanceTest.testSetEnabled thrpt5 711,120 ±19,837
ops/ms
Base version:
Benchmark Mode Cnt Score Error
Units
SetEnabledPerformanceTest.testContains thrpt5 299145,642 ± 2120,183
ops/ms
SetEnabledPerformanceTest.testFindComponentAt thrpt 5 1,101 ±0,012
ops/ms
SetEnabledPerformanceTest.testGetComponentAt thrpt 5 6,792 ±0,097
ops/ms
SetEnabledPerformanceTest.testSetEnabled thrpt 5 0,464 ±0,020
ops/ms
Bug: https://bugs.openjdk.java.net/browse/JDK-8071306
Webrev can be found at: http://cr.openjdk.java.net/~serb/8071306/webrev.03
--
Best regards, Sergey.
Re: AWT Dev [9] Review request for 8003399: JFileChooser gives wrong path to selected file when saving to Libraries folder on Windows 7
Hi Semyon,
Some comments:
- Please, correct the name to KnownfolderDefinition, or better
KnownFolderDefinition:
+static class KnownfolderDefenition {
- Should JNI exceptions be asserted in
Java_sun_awt_shell_Win32ShellFolder2_loadKnownFolders?
Regards,
Anton.
On 07.05.2015 11:14, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK9.
webrev: http://cr.openjdk.java.net/~ssadetsky/8003399/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8003399
***THE ROOT CAUSE
JDK uses legacy WINAPI special folders calls while MS introduced a new interfaces IKnownFolder and
IShellLibrary to manage special folder locations and the new Libraries functionality in Windows 7
is not backward compatible with old special folders CSIDL.
***SOLUTION
Since it is too expensive to migrate AWT shell to the new interfaces and still they are not
supported in java.io the solution is to map virtual folder PIDL to the Known Folder GUID and
replace libraries links with the default library save location. Thus the File save dialog will be
able to work with any Libraries registered in the system (Windows Libraries concept assumes that
Libraries can be added arbitrary).
The resulting code should be compatible with older Windows versions because the new COM interfaces
are called only if they are available and a Libraries link has been actually requested.
***TESTING
A test scenario is added to check that all available Libraries links are converted into filesystem
paths.
--Semyon
Re: AWT Dev [9] Review request for 8077982: GIFLIB upgrade
Hi Alexander,
I've just checked a diff b/w the original gif lib sources and your updated
sources.
The only diff (except the bool and headers changes) is this:
dgif_lib.c
+ /* Sanity check for corrupted file */
+ if (GifFile-ImageCount == 0) {
+ GifFile-Error = D_GIF_ERR_NO_IMAG_DSCR;
+ return(GIF_ERROR);
+ }
Is this an intentional insert?
Regards,
Anton.
On 17.04.2015 21:10, Alexander Zvegintsev wrote:
Hello,
please review the fix
http://cr.openjdk.java.net/~azvegint/jdk/9/8077982/00/
for the issue
https://bugs.openjdk.java.net/browse/JDK-8077982
This fix is GIFLIB upgrade to the latest version 5.1.1 [1]
webrev against GIFLIB 5.1.1 available here [2]
Please see some explanations below:
GIFLIB 5.0.0+ supports interlaced images properly,
so we don't need to handle it in splashscreen_gif.c anymore (pass = 4; npass =
5).
We compile JDK with Microsoft's compilers on Windows, there is no unistd.h[3]
(but Cygwin/MinGW has).
stdbool.h also isn't supported [4]. However I got a lot of strange build errors on Solaris with
included stdbool.h and -std=gnu99.
So it was replaced with own definition of bool.
[1] http://sourceforge.net/projects/giflib/files/giflib-5.1.1.tar.gz/download
[2] http://cr.openjdk.java.net/~azvegint/jdk/9/8077982/giflib/00/
[3]
http://stackoverflow.com/questions/341817/is-there-a-replacement-for-unistd-h-for-windows-visual-c/1759731#1759731
[4]
http://stackoverflow.com/questions/8548521/trying-to-use-include-stdbool-h-in-vs-2010/8549206#8549206
Re: AWT Dev [9] Review request for 8077982: GIFLIB upgrade
Ok, thanks. No more concerns.
Regards
Anton.
On 29.04.2015 12:10, Alexander Zvegintsev wrote:
Hi Anton,
this change is intentional:
http://sourceforge.net/p/giflib/code/ci/dfc5b4de5b1cc619d90afc8d2731d31145897aee/tree/lib/dgif_lib.c?diff=63352c4fa592db103f7300790a3383d638dd1edc
Thanks,
Alexander.
On 29/04/15 10:49, Anton V. Tarasov wrote:
Hi Alexander,
I've just checked a diff b/w the original gif lib sources and your updated
sources.
The only diff (except the bool and headers changes) is this:
dgif_lib.c
+ /* Sanity check for corrupted file */
+ if (GifFile-ImageCount == 0) {
+ GifFile-Error = D_GIF_ERR_NO_IMAG_DSCR;
+ return(GIF_ERROR);
+ }
Is this an intentional insert?
Regards,
Anton.
On 17.04.2015 21:10, Alexander Zvegintsev wrote:
Hello,
please review the fix
http://cr.openjdk.java.net/~azvegint/jdk/9/8077982/00/
for the issue
https://bugs.openjdk.java.net/browse/JDK-8077982
This fix is GIFLIB upgrade to the latest version 5.1.1 [1]
webrev against GIFLIB 5.1.1 available here [2]
Please see some explanations below:
GIFLIB 5.0.0+ supports interlaced images properly,
so we don't need to handle it in splashscreen_gif.c anymore (pass = 4; npass =
5).
We compile JDK with Microsoft's compilers on Windows, there is no unistd.h[3] (but Cygwin/MinGW
has).
stdbool.h also isn't supported [4]. However I got a lot of strange build errors on Solaris with
included stdbool.h and -std=gnu99.
So it was replaced with own definition of bool.
[1] http://sourceforge.net/projects/giflib/files/giflib-5.1.1.tar.gz/download
[2] http://cr.openjdk.java.net/~azvegint/jdk/9/8077982/giflib/00/
[3]
http://stackoverflow.com/questions/341817/is-there-a-replacement-for-unistd-h-for-windows-visual-c/1759731#1759731
[4]
http://stackoverflow.com/questions/8548521/trying-to-use-include-stdbool-h-in-vs-2010/8549206#8549206
Re: AWT Dev [9] Review Request: 4703110 java.awt.Canvas(GraphicaConfiguration): null reaction
+1 On 17.04.2015 15:07, Alexander Zvegintsev wrote: looks fine to me. Thanks, Alexander. On 04/16/2015 07:23 PM, Sergey Bylokhov wrote: Hello. Please review the fix for jdk 9. Small documentation update that java.awt.Canvas(GraphicaConfiguration) will not throw NPE if null is passed. ccc request will be files after review. Bug: https://bugs.openjdk.java.net/browse/JDK-4703110 Webrev can be found at: http://cr.openjdk.java.net/~serb/4703110/webrev.00
Re: AWT Dev [9] Review Request: 8078115 Applets now require modifyThread permission to exit on windows
+1 (awesome bug : ) Thanks Anton. On 22.04.2015 16:03, Alexander Scherbatiy wrote: The fix looks good to me. Thanks, Alexandr. On 4/21/2015 6:22 PM, Alexander Zvegintsev wrote: Hello Sergey, the fix looks good to me. Thanks, Alexander. On 04/21/2015 05:49 PM, Sergey Bylokhov wrote: Hello. Please review a small fix for jdk 9. After one of the latest fix, we lost an ability to change the priority of our toolkit thread with default permissions. We have to use doPrivileged block for this(we already do this on linux/osx) Bug: https://bugs.openjdk.java.net/browse/JDK-8078115 Webrev can be found at: http://cr.openjdk.java.net/~serb/8078115/webrev.00
Re: AWT Dev [9] Review Request JDK-6980209: Make tracking SecondaryLoop.enter/exit methods easier
I'm ok with the change.
Anton.
On 21.04.2015 13:12, Semyon Sadetsky wrote:
Hi Sergey,
Actually it's not needed and I've removed it for the no enter() brunch.
http://cr.openjdk.java.net/~ssadetsky/6980209/webrev.03/
--Semyon
the exit() is called and it detects that there was no enter() after that
scheduler
On 4/20/2015 2:17 AM, Sergey Bylokhov wrote:
Hi Semyon,
Why we call wakeupEDT in the exit method after the fix unconditionally? Please add a comment to
the code to describe this briefly.
On 01.04.15 14:50, Anton V. Tarasov wrote:
Hi Semyon,
This looks much clearer now. I'm fine with it!
Thanks,
Anton.
On 01.04.2015 14:14, Semyon Sadetsky wrote:
Hi Anton,
the updated webrev: http://cr.openjdk.java.net/~ssadetsky/6980209/webrev.02/
--Semyon
On 3/30/2015 6:29 PM, Anton V. Tarasov wrote:
Hi Semyon,
The new version looks fine to me, however it seems to have a flaw currently:
- Suppose exit is called when enter is executing on EDT, at line 181. In this case
afterExit won't be reset to false by enter.
- Suppose, exit is called when enter is executing on non-dispatch thread, at line 263.
Same problem with afterExit.
Is that corrent? If so, then I'd say that afterExit should be reset to false by enter when
it is awaken, either from an event pump or sleep.
Also, please put a space b/w if and brace at the line 177.
Thanks,
Anton.
On 30.03.2015 16:07, Semyon Sadetsky wrote:
Hi Anton,
the code was changed according to our offline discussion:
http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6980209/webrev.01/
added a test case for deterministic scenario
bug root cause description extended
--Semyon
On 3/27/2015 5:32 PM, Anton V. Tarasov wrote:
Hi Semyon,
It would be fine if you describe the case when the enter/exit methods are not thread safe,
that is the bug you're fixing. It's always helpful to track all the essential details in the
bug report.
AFAICS, the case is this:
a) enter is called on non-dispatch thread
b) the thread scheduler suspends it at, say, the point right after keepBlockingEDT is set
to true (line 177).
c) exit is called, keepBlockingEDT is set to false, wakingRunnable is posted to EDT
and gets executed on EDT, keepBlockingCT is set to false
b) enter continues execution, keepBlockingCT is set to true, getTreeLock().wait()
potentially makes the thread sleep endlessly
Is this correct?
WRT the fix. It's actually a question when exit is considered to be prematurely called.
Consider the following:
a) exit is called first on EDT
b) at line 305 keepBlockingEDT is checked as false
c) say, at line 310 enter is called on EDT and is starting pumping events
d) exit continues and wakes enter up
From my point of view, this situation looks functionally the same as if enter and exit
would be called in the correct order.
What about simplifying this logic?
public boolean exit() {
boolean res = false;
synchronized (getTreeLock()) {
res = keepBlockingEDT.getAndSet(false);
afterExit.set(true); // new atomic var
}
wakeupEDT();
return res;
}
Will this scheme work, provided that you use afterExit in enter
appropriately?
getTreeLock() should be called at a narrow possible scope. At least I wouldn't include
wakeupEDT() into it, unless it's justified. Even then, I would consider adding another
private lock for the sake of synchronizing enter and exit.
Also, what's the reason of the call at the following line?
325 keepBlockingEDT.set(false);
And the same question here:
326 if (keepBlockingCT.get()) {
327 // Notify only if enter() waiting in non-dispatch thread
What's wrong if you call getTreeLock().notifyAll() without checking
keepBlockingCT?
WRT the test. Does it really call enter/exit in different order, on your machine at least?
Shouldn't be some test cases where the order is guaranted?
Thanks,
Anton.
On 26.03.2015 17:49, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK9.
Bug: https://bugs.openjdk.java.net/browse/JDK-6980209#comment-13623256
Webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6980209/webrev.00/
***The root cause:
There are 2 issues in WaitDispatchSupport:
1. If exit() is called before the secondary loop just silently never ends. That is pointed
in the summary.
2. In the same spec it is proposed to call exit() and enter() concurrently but they are not
thread-safe. That is an additional issue.
To fix 1: I support Artem's proposal for enter() to return false immidiately in case if
disordered exit() detected.
To fix 2: WaitDispatchSupport need to be reworked to allow concurrent execution.
Unfortunately we cannot synchronize EDT with another thread, so the secondary loop
launching cannot be run atomically. And this is very sensitive area because of potential
possibility to block EDT. Right testing of the fix is very desirable.
***Solution description:
1. User immediately get false as result of enter() if it is detected that exit() has been
run
Re: AWT Dev [9] Review request for 7155957: closed/java/awt/MenuBar/MenuBarStress1/MenuBarStress1.java hangs on win 64 bit with jdk8
Hi Semyon, The fix looks fine to me. Please, add its description to JIRA as well. Regards, Anton. On 15.04.2015 17:38, Semyon Sadetsky wrote: Hello, Please review fix for JDK9. webrev: http://cr.openjdk.java.net/~ssadetsky/7155957/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-7155957 *THE ROOT CAUSE A number of concurrency bugs in AWT Toolkit. When menus are modified concurrently there is a big chance that the internal Windows menu events are handled at the same time on the AWT-Windows thread which is not synchronized. If the internal event processing on AWT-Windows calls Java side methods the JVM can die silently. *SOLUTIONS A number of fixes in various Java and C++ classes are introduced to eliminate (with finite probability) concurrency problems : 1. java.awt.Menu.remove(int) The peer.delItem(index) is called after mi.removeNotify(). That means that dispose event will be sent earlier than the menu remove WinAPI call. This causes Access Violation exception because Windows events may come with deallocated references. The solution is to call them in the right order. 2. java.awt.MenuBar.remove(int) Same error as in 1 for menu bar. 3. java.awt.MenuComponent.serFont(Font) This method should hold tree lock while running otherwise its concurrent execution causes Access Violation in number of places and JVM is crashed. 4. awt_Menu.cpp AwtMenu::GetItem(jobject target, jint index), AwtMenu::DrawItems(), AwtMenu::MeasureItems( Calling java.awt.Menu.getItem() during internal windows event processing can throw ArrayIndexOutOfBoundException because number of menu items could be changed concurrently and the index is not in range. This causes a hidden exception which is only seen in debug mode as an Assertion error. Another issue here is request to GetPeerForTarget() for the menu item peer which can be concurrently deleted on the Java side as result of Menu.delNotify() execution. It causes NPE peer which is also hidden and only reveals itself as an Assertion error in debug mode. The solution for all is to abort and return windows event callback if the menu structure was changed concurrently. 5. awt_MenuBar.cpp AwtMenuBar::GetItem() Same solution as 4 for menu bar in similar situations. 6. awt_MenuItem.cpp AwtMenuItem::Dispose() The destroyed filed should be set for the peer before pData is set to NULL otherwise NPE null pData can be thrown in various concurrent situations. 7. awt_new.cpp safe_ExceptionOccurred() This routine is called evrywere in the code to check exceptions. It only stops execution and prints to console if OOE happened, but other exceptions are re-thrown silently and execution continues without any warnings. If the call is initiated by an internal Windows event callback the exceptions are hidden in the release mode and shown in the debug mode as the Assertion Error message box, but in the last case without any useful information because GetLastError() is always 0 in such situations. I have added env-ExceptionDescribe() to print exception stack trace on the console for all debug and release modes. This should help to detect internal toolkit issues during testing by JCK and jtreg. Later before the JDK9 release we can leave it for debug mode only. *A KNOWN PROBLEM DID NOT FIXED When font is assigned to a menu item concurrently there is a big chance that menu item size will be calculated with one font while drawing of the item will be performed with another font. In such situation label of the menu item does not fit its size or vice versa. This is due to nature how the Windows OS handles owner-drawn menus. --Semyon
Re: AWT Dev [9] Review Request: 8074757 Remove java.awt.Toolkit methods which return peer types
Hi Sergey, I'm fine with it. Regards, Anton. On 10.04.2015 18:08, Sergey Bylokhov wrote: Hello, The new version of the fix: - @deprecated tag was removed - the message was changed to UI components are unsupported by: + toolkit http://cr.openjdk.java.net/~serb/8074757/webrev.05 On 10.04.15 11:52, Anton V. Tarasov wrote: On 07.04.2015 17:28, Sergey Bylokhov wrote: On 03.04.15 20:14, Phil Race wrote: It does not need to be deprecated. It can be 'undeprecated' It was deprecated only because it was the public Toolkit method that is now gone .. Ok, I'll update it. So perhaps there's just a small adjustment needed in the case of where we use createComponent() ?? It is used in 3 places: - Indirectly in Canvas and Panel where our headless toolkits creates NullComponentPeer instead of the native peer. So the question is this is implementation detail of our headless toolkit or all such toolkits should use the same things. - In Component class I can reuse NullComponentPeer, but it is unclear how we survive this later when external tollkit is in use. If nobody objects then I suggest for now to use this new error as an assertion to find possible usage of these methods, instead of silent usage of some empty stub, and fail sometime later with unclear reason. Hi Sergey, I don't object throwing AWTError. However, if we still claim to support external toolkits at least in headless env, then doesn't it make sense to clarify the error message? +throw new AWTError(Unsupported toolkit: + toolkit); to something like Unsupported headful toolkit or Only headless custom toolkits are supported? Thanks, Anton. -phil. -phil. On 04/02/2015 08:15 AM, Sergey Bylokhov wrote: Hello. Please review the fix for jdk 9. There are a number of public methods in the java.awt.Toolkit class, which reference the unsupported java.awt.dnd.peer and java.awt.peer interfaces. There is a decision to remove these references as described: http://mail.openjdk.java.net/pipermail/awt-dev/2015-February/008924.html Changes description: - All such methods were moved from Toolkit.java to the ComponentFactory.java. Note that all our toolkits implement ComponentFactory interface. - HToolkit, HeadlessToolkit, SunToolkit were cleared because they have the same implementation of these methods as in ComponentFactory. - The questionable moment is that I throw an AWTError in a some places if a default toolkit not implements ComponentFactory interface. Bug: https://bugs.openjdk.java.net/browse/JDK-8074757 Webrev can be found at: http://cr.openjdk.java.net/~serb/8074757/webrev.04 -- Best regards, Sergey. -- Best regards, Sergey. -- Best regards, Sergey.
Re: AWT Dev [9] Review request for 8073453: Focus doesn't move when pressing Shift + Tab keys
Hi Dmitry, Well, the fix seems correct to me. I tried to thought of any possible regressions but nothing came to my mind (let's suppose this was really a mistake in the code). However, wouldn't you like to do the same for swing's SortingFocusTraversalPolicy? And also, include it into the test scenario? (Hope you've run all the focus related regression tests). Thanks, Anton. On 06.04.2015 10:14, dmitry markov wrote: Hello, Could you review the fix for jdk9, please? bug: https://bugs.openjdk.java.net/browse/JDK-8073453 webrev: http://cr.openjdk.java.net/~dmarkov/8073453/jdk9/webrev.00/ Problem description: The method ContainerOrderFocusTraversalPolicy.getLastComponent() always returns null if the last component is a container with focus traversal policy and does not have any sub-components. In some cases such behaviour of getLastComponent() causes failure during reverse focus transition, (i.e. focus stays on the selected component when SHIFT+TAB is pressed). Fix: If the last component is a container with focus traversal policy and does not have any sub-components, the method getLastComponent() should return a previous component instead of null. Please note: the same approach is already implemented for ContainerOrderFocusTraversalPolicy.getFirstComponent(). Thanks, Dmitry
Re: AWT Dev [9] Review request for 7042645: Numerous api/java_awt jck tests fail - AWT Assertion Failure on fastdebug ri bundles b138 win7 x86
Hi Semyon, Looks fine to me. Regards, Anton. On 06.04.2015 15:11, Semyon Sadetsky wrote: Hello, Please review fix for JDK9. webrev: http://cr.openjdk.java.net/~ssadetsky/7042645/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-7042645 *ROOT CAUSE: The assertion fails message triggered in awt_Button.cpp because a WinAPI function used to draw button focus rectangle returns zero. Tests run successfully on the release build just because assertions are ignored in it but they are in failure as well. MSDN documentation for the function return value: Return value If the function succeeds, the return value is nonzero. If the function fails, the return value is zero. is wrong, because it returns zero when window is not visible (for example, outside visible screen area). The discrepancy reveals itself when the function returns zero which means error but subsequent ::GetLastError() call returns error code 0 which means successful execution. The same exists in awt_Checkbox.cpp and awt_Component.cpp *SOLUTION ::GetlastError() returning error code is asserted to be 0 in case if ::DrawFocusRect() returns zero. *TESTING This is a JCK test failure issue. No extra regression testing needed. --Semyon
Re: AWT Dev [9] Review Request JDK-6980209: Make tracking SecondaryLoop.enter/exit methods easier
Hi Semyon,
This looks much clearer now. I'm fine with it!
Thanks,
Anton.
On 01.04.2015 14:14, Semyon Sadetsky wrote:
Hi Anton,
the updated webrev: http://cr.openjdk.java.net/~ssadetsky/6980209/webrev.02/
--Semyon
On 3/30/2015 6:29 PM, Anton V. Tarasov wrote:
Hi Semyon,
The new version looks fine to me, however it seems to have a flaw currently:
- Suppose exit is called when enter is executing on EDT, at line 181. In this case
afterExit won't be reset to false by enter.
- Suppose, exit is called when enter is executing on non-dispatch thread, at line 263. Same
problem with afterExit.
Is that corrent? If so, then I'd say that afterExit should be reset to false by enter when it
is awaken, either from an event pump or sleep.
Also, please put a space b/w if and brace at the line 177.
Thanks,
Anton.
On 30.03.2015 16:07, Semyon Sadetsky wrote:
Hi Anton,
the code was changed according to our offline discussion:
http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6980209/webrev.01/
added a test case for deterministic scenario
bug root cause description extended
--Semyon
On 3/27/2015 5:32 PM, Anton V. Tarasov wrote:
Hi Semyon,
It would be fine if you describe the case when the enter/exit methods are not thread safe, that
is the bug you're fixing. It's always helpful to track all the essential details in the bug
report.
AFAICS, the case is this:
a) enter is called on non-dispatch thread
b) the thread scheduler suspends it at, say, the point right after keepBlockingEDT is set to
true (line 177).
c) exit is called, keepBlockingEDT is set to false, wakingRunnable is posted to EDT and
gets executed on EDT, keepBlockingCT is set to false
b) enter continues execution, keepBlockingCT is set to true, getTreeLock().wait()
potentially makes the thread sleep endlessly
Is this correct?
WRT the fix. It's actually a question when exit is considered to be prematurely called.
Consider the following:
a) exit is called first on EDT
b) at line 305 keepBlockingEDT is checked as false
c) say, at line 310 enter is called on EDT and is starting pumping events
d) exit continues and wakes enter up
From my point of view, this situation looks functionally the same as if enter and exit
would be called in the correct order.
What about simplifying this logic?
public boolean exit() {
boolean res = false;
synchronized (getTreeLock()) {
res = keepBlockingEDT.getAndSet(false);
afterExit.set(true); // new atomic var
}
wakeupEDT();
return res;
}
Will this scheme work, provided that you use afterExit in enter
appropriately?
getTreeLock() should be called at a narrow possible scope. At least I wouldn't include
wakeupEDT() into it, unless it's justified. Even then, I would consider adding another private
lock for the sake of synchronizing enter and exit.
Also, what's the reason of the call at the following line?
325 keepBlockingEDT.set(false);
And the same question here:
326 if (keepBlockingCT.get()) {
327 // Notify only if enter() waiting in non-dispatch thread
What's wrong if you call getTreeLock().notifyAll() without checking
keepBlockingCT?
WRT the test. Does it really call enter/exit in different order, on your machine at least?
Shouldn't be some test cases where the order is guaranted?
Thanks,
Anton.
On 26.03.2015 17:49, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK9.
Bug: https://bugs.openjdk.java.net/browse/JDK-6980209#comment-13623256
Webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6980209/webrev.00/
***The root cause:
There are 2 issues in WaitDispatchSupport:
1. If exit() is called before the secondary loop just silently never ends. That is pointed in
the summary.
2. In the same spec it is proposed to call exit() and enter() concurrently but they are not
thread-safe. That is an additional issue.
To fix 1: I support Artem's proposal for enter() to return false immidiately in case if
disordered exit() detected.
To fix 2: WaitDispatchSupport need to be reworked to allow concurrent execution. Unfortunately
we cannot synchronize EDT with another thread, so the secondary loop launching cannot be run
atomically. And this is very sensitive area because of potential possibility to block EDT.
Right testing of the fix is very desirable.
***Solution description:
1. User immediately get false as result of enter() if it is detected that exit() has been run
either before or concurrently. For that purpose a new AtomicBoolean field prematureExit is
introduced.
2. The exit() and enter() are reworked to be capable to run concurrently and avoid EDT jam in
the secondary loop. See comments to the code for details.
***Testing:
Test launches the secondary loop upon a button press action triggered by the Robot and
simultaneously call exit() in a separate thread. The Robot sends a big number of events
(number is adjustable by ATTEMPTS constant) to cover different concurrent states randomly.
Along
Re: AWT Dev [9] Review request for 8071668: [macosx] Clipboard does not work with 3rd parties Clipboard Managers
Hi Anton, Currently JFXPanel calls checkPasteboard via reflection. I had filed JDK-8061315 to provide a public method instead. But with your fix it's not needed. So, I've closed JDK-8061315. Also, I will back out the related fix in JFXPanel: https://javafx-jira.kenai.com/browse/RT-38922 Thanks, Anton. On 30.03.2015 14:39, Sergey Bylokhov wrote: The fix looks good. I recall that checkPasteboard can be used outside of awt in fx. Anton T. can clarify this. 26.03.15 20:32, Anton Nashatyrev wrote: Hi Sergey, yes it looks like I've missed the notifyChanged. you are right, this will remove the unnecessary AWT upcall from Toolkit thread (was it your intention?) Please take a look at the new fix version (the test has also been updated to check flavorsChanged): http://cr.openjdk.java.net/~anashaty/8071668/9/webrev.01/ http://cr.openjdk.java.net/%7Eanashaty/8071668/9/webrev.01/ Thanks! Anton. On 26.03.2015 18:10, Sergey Bylokhov wrote: Hi, Anton. Should we call CClipboard.notifyChanged when the owner was changed?(at least CClipboard.checkPasteboard call it). If yes then it seems that the previous fix(8010925) for this bug can be reworked in pure java using checkPasteboardWithoutNotification? 26.03.15 17:29, Anton Nashatyrev wrote: Hello, could you please review the following fix: fix: http://cr.openjdk.java.net/~anashaty/8071668/9/webrev.00/ http://cr.openjdk.java.net/%7Eanashaty/8071668/9/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8071668 Problem: On Mac Java doesn't see external clipboard changes. Fix: since there is still no Cocoa pasteboard changes notification mechanism, check the Cocoa clipboard counter each time the contents is requested from the Java Clipboard. Thanks! Anton. -- Best regards, Sergey. -- Best regards, Sergey.
Re: AWT Dev [9] Review Request: 8074763 Remove API references to java.awt.dnd.peer
Hi Kevin,
Right, I just assumed we would need some coordination.
Thanks,
Anton.
On 30.03.2015 17:48, Kevin Rushforth wrote:
Hi Anton,
Yes, there are concerns regarding this, mainly due to the timing and build issues. After this week
we are no longer auto-syncing changes from 8u-dev into 9, so we are at a good point to do this,
but it will need to be done carefully. I expect that we will need at least 2 weeks to switch our
Hudson build systems to build FX 9 with JDK 9 (we currently build with JDK 8), so we will need to
coordinate this.
-- Kevin
Anton V. Tarasov wrote:
Hi Sergey, Kevin,
This method is called from JFX/interop:
DropTargetContext.java
-public void addNotify(DropTargetContextPeer dtcp) {
An accessor is introduced. So, we will have to pick it up in JFX/interop once
the fix is in the ws.
This means we won't be able to run jfx9 atop of jdk8.
@Kevin,
Do you have any concerns with regard to this fact?
Thanks,
Anton.
On 25.03.2015 17:35, Sergey Bylokhov wrote:
Hello,
Please review an updated version of the fix.
http://cr.openjdk.java.net/~serb/8074763/webrev.02
DropTargetContext.addNotify/removeNotify were renamed and access was changed to a package
private. A necessary methods were added to the AWTAccessor.
18.03.15 23:47, Phil Race wrote:
I think its preferable to remove (hide) the method rather than leave one that
no application code can (or should) call because they can't provide a parameter
of the required type.
-phil.
On 03/18/2015 09:24 AM, Sergey Bylokhov wrote:
Hi, Anton.
The problem is that this method is called when the peer itself change the information in the
DropTargetContext. So this method works like a setter. I can make this method private, and get
an access to it via accessor. Will it be better?
18.03.15 8:27, Anton V. Tarasov wrote:
Hi Sergey,
The only dependency on JFX/interop is this method in DropTargetContext.java:
98 public void addNotify(final Object dtcp) throws IllegalArgumentException
{
Was that the reason why you left the parameter?
Is it technically possible to retrieve the peer via the ComponentAccessor.getPeer(component)
method where the component is dropTarget.getComponent()?
Thanks,
Anton.
On 16.03.2015 21:30, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk 9.
There are a number of public API which reference the unsupported java.awt.
dnd.peer interfaces.
protected
java.awt.dnd.DragSource.createDragSourceContext(java.awt.dnd.peer.DragSourceContextPeer, ...)
public java.awt.dnd.DragSourceContext(java.awt.dnd.peer.DragSourceContextPeer,
...) constructor
public java.awt.dnd.DropTarget.addNotify(ComponentPeer peer) and removeNotify(ComponentPeer
peer)
public java.awt.dnd.DropTargetContext.addNotify(DropTargetContextPeer dtcp)
There is a decision to remove these references as described:
http://mail.openjdk.java.net/pipermail/awt-dev/2015-February/008924.html
Changes description:
* DragSource.java, DragSourceContext.java, DropTarget.java : In all of these
methods the
peers are used as a parameters. In most of the cases these parameters are
not necessary,
because the peer can be accessed using the reference to the shared
object(Component/DropTarget etc). Since these methods can be useful I did
not remove
them, but remove one parameter only.
* DropTargetContext.java: addNotify() is called when we cannot get the
information about a
peer so I change type of the parameter and documentation of the method. It
seems that
these methods DropTargetContext.addNotify/removeNotify are not useful and I
can change
them by private version, but I don't know which way will be better.
Bug: https://bugs.openjdk.java.net/browse/JDK-8074763
Webrev can be found at: http://cr.openjdk.java.net/~serb/8074763/webrev.01
--
Best regards, Sergey.
--
Best regards, Sergey.
--
Best regards, Sergey.
Re: AWT Dev [9] Review Request JDK-6980209: Make tracking SecondaryLoop.enter/exit methods easier
Hi Semyon,
The new version looks fine to me, however it seems to have a flaw currently:
- Suppose exit is called when enter is executing on EDT, at line 181. In this case afterExit
won't be reset to false by enter.
- Suppose, exit is called when enter is executing on non-dispatch thread, at line 263. Same
problem with afterExit.
Is that corrent? If so, then I'd say that afterExit should be reset to false by enter when it is
awaken, either from an event pump or sleep.
Also, please put a space b/w if and brace at the line 177.
Thanks,
Anton.
On 30.03.2015 16:07, Semyon Sadetsky wrote:
Hi Anton,
the code was changed according to our offline discussion:
http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6980209/webrev.01/
added a test case for deterministic scenario
bug root cause description extended
--Semyon
On 3/27/2015 5:32 PM, Anton V. Tarasov wrote:
Hi Semyon,
It would be fine if you describe the case when the enter/exit methods are not thread safe, that
is the bug you're fixing. It's always helpful to track all the essential details in the bug report.
AFAICS, the case is this:
a) enter is called on non-dispatch thread
b) the thread scheduler suspends it at, say, the point right after keepBlockingEDT is set to
true (line 177).
c) exit is called, keepBlockingEDT is set to false, wakingRunnable is posted to EDT and
gets executed on EDT, keepBlockingCT is set to false
b) enter continues execution, keepBlockingCT is set to true, getTreeLock().wait() potentially
makes the thread sleep endlessly
Is this correct?
WRT the fix. It's actually a question when exit is considered to be prematurely called.
Consider the following:
a) exit is called first on EDT
b) at line 305 keepBlockingEDT is checked as false
c) say, at line 310 enter is called on EDT and is starting pumping events
d) exit continues and wakes enter up
From my point of view, this situation looks functionally the same as if enter and exit would
be called in the correct order.
What about simplifying this logic?
public boolean exit() {
boolean res = false;
synchronized (getTreeLock()) {
res = keepBlockingEDT.getAndSet(false);
afterExit.set(true); // new atomic var
}
wakeupEDT();
return res;
}
Will this scheme work, provided that you use afterExit in enter
appropriately?
getTreeLock() should be called at a narrow possible scope. At least I wouldn't include
wakeupEDT() into it, unless it's justified. Even then, I would consider adding another private
lock for the sake of synchronizing enter and exit.
Also, what's the reason of the call at the following line?
325 keepBlockingEDT.set(false);
And the same question here:
326 if (keepBlockingCT.get()) {
327 // Notify only if enter() waiting in non-dispatch thread
What's wrong if you call getTreeLock().notifyAll() without checking
keepBlockingCT?
WRT the test. Does it really call enter/exit in different order, on your machine at least?
Shouldn't be some test cases where the order is guaranted?
Thanks,
Anton.
On 26.03.2015 17:49, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK9.
Bug: https://bugs.openjdk.java.net/browse/JDK-6980209#comment-13623256
Webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6980209/webrev.00/
***The root cause:
There are 2 issues in WaitDispatchSupport:
1. If exit() is called before the secondary loop just silently never ends. That is pointed in
the summary.
2. In the same spec it is proposed to call exit() and enter() concurrently but they are not
thread-safe. That is an additional issue.
To fix 1: I support Artem's proposal for enter() to return false immidiately in case if
disordered exit() detected.
To fix 2: WaitDispatchSupport need to be reworked to allow concurrent execution. Unfortunately
we cannot synchronize EDT with another thread, so the secondary loop launching cannot be run
atomically. And this is very sensitive area because of potential possibility to block EDT. Right
testing of the fix is very desirable.
***Solution description:
1. User immediately get false as result of enter() if it is detected that exit() has been run
either before or concurrently. For that purpose a new AtomicBoolean field prematureExit is
introduced.
2. The exit() and enter() are reworked to be capable to run concurrently and avoid EDT jam in
the secondary loop. See comments to the code for details.
***Testing:
Test launches the secondary loop upon a button press action triggered by the Robot and
simultaneously call exit() in a separate thread. The Robot sends a big number of events (number
is adjustable by ATTEMPTS constant) to cover different concurrent states randomly. Along with
that the Robot sends a number of key events to ensure that UI is kept responsive and all events
are dispatched by EDT. The number of the sent key events is tested to be equal to the number of
processed events.
The test is run in different modes to test secondary loop
Re: AWT Dev [9] Review Request JDK-6980209: Make tracking SecondaryLoop.enter/exit methods easier
Hi Semyon,
It would be fine if you describe the case when the enter/exit methods are not thread safe, that is
the bug you're fixing. It's always helpful to track all the essential details in the bug report.
AFAICS, the case is this:
a) enter is called on non-dispatch thread
b) the thread scheduler suspends it at, say, the point right after keepBlockingEDT is set to true
(line 177).
c) exit is called, keepBlockingEDT is set to false, wakingRunnable is posted to EDT and gets
executed on EDT, keepBlockingCT is set to false
b) enter continues execution, keepBlockingCT is set to true, getTreeLock().wait() potentially
makes the thread sleep endlessly
Is this correct?
WRT the fix. It's actually a question when exit is considered to be prematurely called. Consider
the following:
a) exit is called first on EDT
b) at line 305 keepBlockingEDT is checked as false
c) say, at line 310 enter is called on EDT and is starting pumping events
d) exit continues and wakes enter up
From my point of view, this situation looks functionally the same as if enter and exit would be
called in the correct order.
What about simplifying this logic?
public boolean exit() {
boolean res = false;
synchronized (getTreeLock()) {
res = keepBlockingEDT.getAndSet(false);
afterExit.set(true); // new atomic var
}
wakeupEDT();
return res;
}
Will this scheme work, provided that you use afterExit in enter
appropriately?
getTreeLock() should be called at a narrow possible scope. At least I wouldn't include wakeupEDT()
into it, unless it's justified. Even then, I would consider adding another private lock for the sake
of synchronizing enter and exit.
Also, what's the reason of the call at the following line?
325 keepBlockingEDT.set(false);
And the same question here:
326 if (keepBlockingCT.get()) {
327 // Notify only if enter() waiting in non-dispatch thread
What's wrong if you call getTreeLock().notifyAll() without checking
keepBlockingCT?
WRT the test. Does it really call enter/exit in different order, on your machine at least? Shouldn't
be some test cases where the order is guaranted?
Thanks,
Anton.
On 26.03.2015 17:49, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK9.
Bug: https://bugs.openjdk.java.net/browse/JDK-6980209#comment-13623256
Webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/6980209/webrev.00/
***The root cause:
There are 2 issues in WaitDispatchSupport:
1. If exit() is called before the secondary loop just silently never ends. That is pointed in the
summary.
2. In the same spec it is proposed to call exit() and enter() concurrently but they are not
thread-safe. That is an additional issue.
To fix 1: I support Artem's proposal for enter() to return false immidiately in case if disordered
exit() detected.
To fix 2: WaitDispatchSupport need to be reworked to allow concurrent execution. Unfortunately we
cannot synchronize EDT with another thread, so the secondary loop launching cannot be run
atomically. And this is very sensitive area because of potential possibility to block EDT. Right
testing of the fix is very desirable.
***Solution description:
1. User immediately get false as result of enter() if it is detected that exit() has been run
either before or concurrently. For that purpose a new AtomicBoolean field prematureExit is
introduced.
2. The exit() and enter() are reworked to be capable to run concurrently and avoid EDT jam in the
secondary loop. See comments to the code for details.
***Testing:
Test launches the secondary loop upon a button press action triggered by the Robot and
simultaneously call exit() in a separate thread. The Robot sends a big number of events (number is
adjustable by ATTEMPTS constant) to cover different concurrent states randomly. Along with that
the Robot sends a number of key events to ensure that UI is kept responsive and all events are
dispatched by EDT. The number of the sent key events is tested to be equal to the number of
processed events.
The test is run in different modes to test secondary loop launching from EDT
and non-EDT threads.
--Semyon
Re: AWT Dev [9] Review Request: 8074763 Remove API references to java.awt.dnd.peer
Hi Sergey, Kevin,
This method is called from JFX/interop:
DropTargetContext.java
-public void addNotify(DropTargetContextPeer dtcp) {
An accessor is introduced. So, we will have to pick it up in JFX/interop once
the fix is in the ws.
This means we won't be able to run jfx9 atop of jdk8.
@Kevin,
Do you have any concerns with regard to this fact?
Thanks,
Anton.
On 25.03.2015 17:35, Sergey Bylokhov wrote:
Hello,
Please review an updated version of the fix.
http://cr.openjdk.java.net/~serb/8074763/webrev.02
DropTargetContext.addNotify/removeNotify were renamed and access was changed to a package private.
A necessary methods were added to the AWTAccessor.
18.03.15 23:47, Phil Race wrote:
I think its preferable to remove (hide) the method rather than leave one that
no application code can (or should) call because they can't provide a parameter
of the required type.
-phil.
On 03/18/2015 09:24 AM, Sergey Bylokhov wrote:
Hi, Anton.
The problem is that this method is called when the peer itself change the information in the
DropTargetContext. So this method works like a setter. I can make this method private, and get
an access to it via accessor. Will it be better?
18.03.15 8:27, Anton V. Tarasov wrote:
Hi Sergey,
The only dependency on JFX/interop is this method in DropTargetContext.java:
98 public void addNotify(final Object dtcp) throws IllegalArgumentException
{
Was that the reason why you left the parameter?
Is it technically possible to retrieve the peer via the ComponentAccessor.getPeer(component)
method where the component is dropTarget.getComponent()?
Thanks,
Anton.
On 16.03.2015 21:30, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk 9.
There are a number of public API which reference the unsupported java.awt.
dnd.peer interfaces.
protected
java.awt.dnd.DragSource.createDragSourceContext(java.awt.dnd.peer.DragSourceContextPeer, ...)
public java.awt.dnd.DragSourceContext(java.awt.dnd.peer.DragSourceContextPeer,
...) constructor
public java.awt.dnd.DropTarget.addNotify(ComponentPeer peer) and
removeNotify(ComponentPeer peer)
public java.awt.dnd.DropTargetContext.addNotify(DropTargetContextPeer dtcp)
There is a decision to remove these references as described:
http://mail.openjdk.java.net/pipermail/awt-dev/2015-February/008924.html
Changes description:
* DragSource.java, DragSourceContext.java, DropTarget.java : In all of these
methods the
peers are used as a parameters. In most of the cases these parameters are
not necessary,
because the peer can be accessed using the reference to the shared
object(Component/DropTarget etc). Since these methods can be useful I did
not remove them,
but remove one parameter only.
* DropTargetContext.java: addNotify() is called when we cannot get the
information about a
peer so I change type of the parameter and documentation of the method. It
seems that
these methods DropTargetContext.addNotify/removeNotify are not useful and I
can change
them by private version, but I don't know which way will be better.
Bug: https://bugs.openjdk.java.net/browse/JDK-8074763
Webrev can be found at: http://cr.openjdk.java.net/~serb/8074763/webrev.01
--
Best regards, Sergey.
--
Best regards, Sergey.
--
Best regards, Sergey.
Re: AWT Dev [9] Review request for 8074481: [macosx] Menu items are appearing on top of other windows
Hi Anton, I'm ok with the fix. Please, run all the focus regression tests available. Thanks, Anton. On 18.03.2015 20:55, Anton Nashatyrev wrote: Hello, could you please review the following fix: fix: http://cr.openjdk.java.net/~anashaty/8074481/9/webrev.00/ http://cr.openjdk.java.net/%7Eanashaty/8074481/9/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8074481 Problem: applet popup windows are not closed when other native window activated Fix: another variant to fix the JDK-8001161 https://bugs.openjdk.java.net/browse/JDK-8001161 which causes such behavior (thanks Anton Tarasov for suggested fix idea) Thanks! Anton.
Re: AWT Dev [9] Review request for 8075244 [macosx] The fix for JDK-8043869 should be reworked
Hi Alexander, Sergey, I can't say exactly if this is ok to init AWT from that point, but even if it is, this seems to introduce new risks. At the same time, AFAICS, the root of the original focus problem is unknown. Is that the case? Should we try to understand it, instead? It is possible that fixing the root of the issue would be less risky. Thanks, Anton. On 17.03.2015 14:27, Alexander Scherbatiy wrote: Hello, Could you review the fix: bug: https://bugs.openjdk.java.net/browse/JDK-8075244 webrev: http://cr.openjdk.java.net/~alexsch/8075244/webrev.00 [NSApplicationAWT sharedApplication] call is added for the application initialization. Thanks, Alexandr.
Re: AWT Dev [9] Review request for 8074481: [macosx] Menu items are appearing on top of other windows
Ok, great! Thanks, Anton. On 19.03.2015 14:09, Anton Nashatyrev wrote: Hi Anton, thanks for review! I've run all [closed/]java/awt/Focus tests: no regressions were found Regards, Anton. On 19.03.2015 11:33, Anton V. Tarasov wrote: Hi Anton, I'm ok with the fix. Please, run all the focus regression tests available. Thanks, Anton. On 18.03.2015 20:55, Anton Nashatyrev wrote: Hello, could you please review the following fix: fix: http://cr.openjdk.java.net/~anashaty/8074481/9/webrev.00/ http://cr.openjdk.java.net/%7Eanashaty/8074481/9/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8074481 Problem: applet popup windows are not closed when other native window activated Fix: another variant to fix the JDK-8001161 https://bugs.openjdk.java.net/browse/JDK-8001161 which causes such behavior (thanks Anton Tarasov for suggested fix idea) Thanks! Anton.
Re: AWT Dev [9] Review Request: 8074763 Remove API references to java.awt.dnd.peer
Hi Sergey,
The only dependency on JFX/interop is this method in DropTargetContext.java:
98 public void addNotify(final Object dtcp) throws IllegalArgumentException
{
Was that the reason why you left the parameter?
Is it technically possible to retrieve the peer via the ComponentAccessor.getPeer(component) method
where the component is dropTarget.getComponent()?
Thanks,
Anton.
On 16.03.2015 21:30, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk 9.
There are a number of public API which reference the unsupported java.awt.
dnd.peer interfaces.
protected java.awt.dnd.DragSource.createDragSourceContext(java.awt.dnd.peer.DragSourceContextPeer,
...)
public java.awt.dnd.DragSourceContext(java.awt.dnd.peer.DragSourceContextPeer,
...) constructor
public java.awt.dnd.DropTarget.addNotify(ComponentPeer peer) and
removeNotify(ComponentPeer peer)
public java.awt.dnd.DropTargetContext.addNotify(DropTargetContextPeer dtcp)
There is a decision to remove these references as described:
http://mail.openjdk.java.net/pipermail/awt-dev/2015-February/008924.html
Changes description:
* DragSource.java, DragSourceContext.java, DropTarget.java : In all of these
methods the peers
are used as a parameters. In most of the cases these parameters are not
necessary, because the
peer can be accessed using the reference to the shared
object(Component/DropTarget etc). Since
these methods can be useful I did not remove them, but remove one parameter
only.
* DropTargetContext.java: addNotify() is called when we cannot get the
information about a peer
so I change type of the parameter and documentation of the method. It seems
that these methods
DropTargetContext.addNotify/removeNotify are not useful and I can change
them by private
version, but I don't know which way will be better.
Bug: https://bugs.openjdk.java.net/browse/JDK-8074763
Webrev can be found at: http://cr.openjdk.java.net/~serb/8074763/webrev.01
--
Best regards, Sergey.
Re: AWT Dev [9] Review request for 7081580: Specification for MouseInfo.getNumberOfButtons() doesn't contain info about awt.mouse.numButtons
Hi Semyon,
As a minimalistic description of the property, this looks ok to me. So, if there's nothing else to
say about it, I'm fine with the fix.
Regards,
Anton.
On 16.03.2015 16:22, Semyon Sadetsky wrote:
Hi!
Thank you Anton!
The updated webrev is:
http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/7081580/webrev.01/
--Semyon
On 3/12/2015 1:42 PM, Anton V. Tarasov wrote:
Hi Semyon, Sergey,
I agree with that the modified javadoc is not good.
1. When you say something is done by calling A.b(), it means I can write exactly A.b() in my
code and this will do the job. However, that's not the case with Toolkit.getDesktopProperty() (it
won't be compiled).
In order to refer to a method, you can use either of the following
constructions:
a) the A.b() method
b) {@link A#b}
c) the {@link A#b} method
b/c is preferrable.
2. When you say The value is obtained by calling something, it's not quite clear what or who
obtains the value. The method itself? Or this is an alternative way to get it for a user?
3. If this is the only place in the spec where the property is introduced, then you should
somehow reflect this fact. For instance, like this:
The value is set by the awt.mouse.numButtons property, which can be obtained directly with the
{@link Toolkit#getDesktopProperty} method.
You don't have to _specify_ the way getNumberOfButtons() obtains the property, unless this
implementation detail should really be specified. (For instance, if it was obtained by a method
which could be overriden in an application.)
Thanks,
Anton.
On 12.03.2015 11:42, Semyon Sadetsky wrote:
Sorry, Sergey. Still don't understand what you mean.
The issue is about*to do**mention* awt.mouse.numButtons.
Now you are saying that there is no value to mention it for the first time in this spec. Doesn't
it contradict to the request itself?
You couldn't be more specific on what do you want, could you?
The fix just adds one short statement to the spec. Maybe you'll find it to be more productive
to just rephrase as you want and write here.
Thank you!
--Semyon
On 3/12/2015 11:11 AM, Sergey Bylokhov wrote:
Hi, Semyon.
That's a specification which should be read as written. But if you mean this is not the same
things, then it is unclear what value will be added to the description of
awt.mouse.numButtons property, which mentions in the specification for the first time. Since
getNumberOfButtons obtain something not specified from the getToolkit, modify it somehow(w/o
specification) and returns. See for example Toolkit.getToolkit and Toolkit.
areExtraMouseButtonsEnabled(). It is not necessary write so specific specification but at least
it should be clear.
It would be good to rephrase it somehow.
12.03.15 0:09, Semyon Sadetsky wrote:
Hi Sergey,
I didn't find any mention in the new text that the method returns the same value as
Tolkit.get... returns.
I'm not an expert in English but in my opinion obtained by verb doesn't state that the same
value is returned without any handling.
Maybe you've mixed it up with proxy?
Thanks,
--Semyon
On 3/12/2015 9:47 AM, Sergey Bylokhov wrote:
Hi, Semyon.
The fix in general is correct, but it adds an assertion that this method should return the
same values as Toolkit.get...
And this is incorrect, and we can get a new CR that implementation don't follow the
specification. Probably we can simplify it and state that we use numeric value from desktop
property or something like that?
11.03.15 22:52, Semyon Sadetsky wrote:
Hello,
please review fix for jdk9.
Webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/7081580/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-7081580
Thanks,
--Semyon
--
Best regards, Sergey.
Re: AWT Dev Swing Dev [9] Review Request: 8074668 [macosx] Mac 10.10: Application run with splash screen has focus issues
Hi Sergey, Looks good to me. Regards, Anton. On 12.03.2015 4:40, Sergey Bylokhov wrote: Hello. Please review the fix for jdk 8/9. Lots of issues(hangs or focus lost) occurred after we call NSScreen.backingScaleFactor on a Appkit Thread in time of splash screen initialization. This is a regression of two fixes: https://bugs.openjdk.java.net/browse/JDK-8043869 https://bugs.openjdk.java.net/browse/JDK-8049198 Current issue occures after the second fix. Bug: https://bugs.openjdk.java.net/browse/JDK-8074668 As a fix I suggest to disable functionality which was added in JDK-8049198 https://bugs.openjdk.java.net/browse/JDK-8049198 and JDK-8043869 https://bugs.openjdk.java.net/browse/JDK-8043869. Because if I try to revert them back I should change more files which I consider will be danger. -- Best regards, Sergey.
Re: AWT Dev [9] Review request for 7081580: Specification for MouseInfo.getNumberOfButtons() doesn't contain info about awt.mouse.numButtons
Hi Semyon, Sergey,
I agree with that the modified javadoc is not good.
1. When you say something is done by calling A.b(), it means I can write exactly A.b() in my
code and this will do the job. However, that's not the case with Toolkit.getDesktopProperty() (it
won't be compiled).
In order to refer to a method, you can use either of the following
constructions:
a) the A.b() method
b) {@link A#b}
c) the {@link A#b} method
b/c is preferrable.
2. When you say The value is obtained by calling something, it's not quite clear what or who
obtains the value. The method itself? Or this is an alternative way to get it for a user?
3. If this is the only place in the spec where the property is introduced, then you should somehow
reflect this fact. For instance, like this:
The value is set by the awt.mouse.numButtons property, which can be obtained directly with the
{@link Toolkit#getDesktopProperty} method.
You don't have to _specify_ the way getNumberOfButtons() obtains the property, unless this
implementation detail should really be specified. (For instance, if it was obtained by a method
which could be overriden in an application.)
Thanks,
Anton.
On 12.03.2015 11:42, Semyon Sadetsky wrote:
Sorry, Sergey. Still don't understand what you mean.
The issue is about*to do**mention* awt.mouse.numButtons.
Now you are saying that there is no value to mention it for the first time in this spec. Doesn't
it contradict to the request itself?
You couldn't be more specific on what do you want, could you?
The fix just adds one short statement to the spec. Maybe you'll find it to be more productive to
just rephrase as you want and write here.
Thank you!
--Semyon
On 3/12/2015 11:11 AM, Sergey Bylokhov wrote:
Hi, Semyon.
That's a specification which should be read as written. But if you mean this is not the same
things, then it is unclear what value will be added to the description of awt.mouse.numButtons
property, which mentions in the specification for the first time. Since getNumberOfButtons obtain
something not specified from the getToolkit, modify it somehow(w/o specification) and returns.
See for example Toolkit.getToolkit and Toolkit. areExtraMouseButtonsEnabled(). It is not
necessary write so specific specification but at least it should be clear.
It would be good to rephrase it somehow.
12.03.15 0:09, Semyon Sadetsky wrote:
Hi Sergey,
I didn't find any mention in the new text that the method returns the same value as
Tolkit.get... returns.
I'm not an expert in English but in my opinion obtained by verb doesn't state that the same
value is returned without any handling.
Maybe you've mixed it up with proxy?
Thanks,
--Semyon
On 3/12/2015 9:47 AM, Sergey Bylokhov wrote:
Hi, Semyon.
The fix in general is correct, but it adds an assertion that this method should return the same
values as Toolkit.get...
And this is incorrect, and we can get a new CR that implementation don't follow the
specification. Probably we can simplify it and state that we use numeric value from desktop
property or something like that?
11.03.15 22:52, Semyon Sadetsky wrote:
Hello,
please review fix for jdk9.
Webrev: http://cr.openjdk.java.net/~alexsch/semyon-sadetsky/7081580/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-7081580
Thanks,
--Semyon
--
Best regards, Sergey.
Re: AWT Dev [9] Review Request: 8043393 NullPointerException and no event received when clipboard data flavor changes
Hi Sergey, I suppose formatArrayAsDataFlavorSet() didn't produce any meaningful side effect currently not taken into account? If so, I'm fine with the change. Regards, Anton. On 24.02.2015 15:20, Sergey Bylokhov wrote: Hello. Please review a fix for jdk 9. The NPE occurs, because we call getDefaultFlavorMap() on the Toolkit thread (formatArrayAsDataFlavorSet-getDefaultFlavorTable-SystemFlavorMap.getDefaultFlavorMap()). But after the fix of JDK-8030679 a DefaultFlavorMap is stored per AppContext, so we cannot use it on the TK thread. As a fix I suggest to store raw formats instead of SetDataFlavor. Bug: https://bugs.openjdk.java.net/browse/JDK-8043393 Webrev can be found at: http://cr.openjdk.java.net/~serb/8043393/webrev.00
Re: AWT Dev [9] Review Request: 8043393 NullPointerException and no event received when clipboard data flavor changes
On 24.02.2015 19:10, Sergey Bylokhov wrote: On 24.02.2015 18:49, Anton V. Tarasov wrote: Hi Sergey, I suppose formatArrayAsDataFlavorSet() didn't produce any meaningful side effect currently not taken into account? Seems yes, other than some default initialization if it was not called before. Ok, provided that the default initialization doesn't play a part here... Anton. If so, I'm fine with the change. Regards, Anton. On 24.02.2015 15:20, Sergey Bylokhov wrote: Hello. Please review a fix for jdk 9. The NPE occurs, because we call getDefaultFlavorMap() on the Toolkit thread (formatArrayAsDataFlavorSet-getDefaultFlavorTable-SystemFlavorMap.getDefaultFlavorMap()). But after the fix of JDK-8030679 a DefaultFlavorMap is stored per AppContext, so we cannot use it on the TK thread. As a fix I suggest to store raw formats instead of SetDataFlavor. Bug: https://bugs.openjdk.java.net/browse/JDK-8043393 Webrev can be found at: http://cr.openjdk.java.net/~serb/8043393/webrev.00
Re: AWT Dev [9] Review request for 8056915: Focus lost in applet when browser window is minimized and restored
Hi Alexey, The fix looks fine to me. Regards, Anton. On 10.02.2015 14:45, Alexey Ivanov wrote: Hello AWT team, Could you please review the fix: bug: https://bugs.openjdk.java.net/browse/JDK-8056915 webrev: http://cr.openjdk.java.net/~aivanov/8056915/jdk9/webrev.00/ Description: After user minimizes the browser window and then restores it, the focus does not return to the applet. The fix preserves the latest focus owner in the browser window so that applet gains focus when the browser window is restored. Thanks, Alexey
Re: AWT Dev [9] Review request for 8072088: [PIT] NPE in DnD tests apparently because of the fix to JDK-8061636
Looks fine to me. Regards, Anton. On 02.02.2015 19:44, Alexander Zvegintsev wrote: Hello, please review the fix: http://cr.openjdk.java.net/~azvegint/jdk/9/8072088/00/ for the issue: https://bugs.openjdk.java.net/browse/JDK-8072088 Removed WeakReference nullification.
Re: AWT Dev [9] Review Request: 8062738 Test java/awt/datatransfer/MissedHtmlAndRtfBug/MissedHtmlAndRtfBug fails in Windows
Looks fine to me. Regards, Anton. On 02.02.2015 16:52, Sergey Bylokhov wrote: Hello. Please review a fix for jdk 9. Description: In the fix for JDK-8051449 was an incorrect assumption that String.replace() replaces only the first text occurrence, and it was changed to replaceAll(). Bug: https://bugs.openjdk.java.net/browse/JDK-8062738 Webrev can be found at: http://cr.openjdk.java.net/~serb/8062738/webrev.00
Re: AWT Dev [9] Review Request: 7185221 [macosx] Regtest should not throw exception if a suitable display mode found
Looks fine to me. Regards, Anton. On 21.01.2015 20:12, Sergey Bylokhov wrote: Hello. Please review the fix for jdk 9. The test modify a supported list of DisplayModes and try to check that IllegalArgumentException will be thrown. But the problem occurs, when the test tries to change a supported DisplayMode which refresh rate is unknown. This happens because we match any refresh rate to a REFRESH_RATE_UNKNOWN. Bug: https://bugs.openjdk.java.net/browse/JDK-7185221 Webrev can be found at: http://cr.openjdk.java.net/~serb/7185221/webrev.00/webrev Diff: http://cr.openjdk.java.net/~serb/7185221/webrev.00/diff
Re: AWT Dev Review Request: 8069015 Re-examine Solaris/Linux java.desktop dependency on java.logging
Looks fine ot me. Regards Anton. On 22.01.2015 20:11, Sergey Bylokhov wrote: Hello. Please review the fix for jdk 9. Additional information about the problem in https://bugs.openjdk.java.net/browse/JDK-6879044 There are many classes using java.util.logging to log messages to enhance the diagnosability. java.util.logging is a good candidate to be separated out as a module. We need to eliminate the dependency of logging from certain core classes for JDK modularization. In the fix XEmbeddedFrame was changed to use Platformlogger and unused XAWTFormatter was removed. Bug: https://bugs.openjdk.java.net/browse/JDK-8069015 Webrev can be found at: http://cr.openjdk.java.net/~serb/8069015/webrev.00
Re: AWT Dev [9] Review request for 8068283: Mac OS Incompatibility between JDK 6 and 8 regarding input method handling
Looks fine! Thanks, Anton. On 20.01.2015 18:42, Anton Nashatyrev wrote: Hello Alexander, Anton could you please review the following fix: fix: http://cr.openjdk.java.net/~anashaty/8068283/9/webrev.00/ http://cr.openjdk.java.net/%7Eanashaty/8068283/9/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8068283 Problem: No ways for keyboard shortcuts like Alt + Char on Mac OS Fix: Change the fix approach for the bug JDK-7144063 https://bugs.openjdk.java.net/browse/JDK-7144063 which has changed the behavior of the original Apple JDK port. Thanks! Anton.
Re: AWT Dev Review Request for 8065709: Deadlock in awt/logging apparently introduced by 8019623
Hi Mikhail, It seems the problem is not limited to EventQueue only. Even if you solve it for EventQueue, the EventQueue ctor may cause a chain of calls where some other AWT class is first accessed and initialized. What if it contains the same getLogger() call in a static block? If you agree with this, then there should be a generic solution for the deadlock. What comes to my mind is the following. The deadlock happens due to a reverse lock taking order. Can we change it? In LogManager.getUserContext() it seems like the javaAwtAccess lock scope can be narrowed down. Namely, we can move the javaAwtAccess.getAppletContext() line out of it. In which case the method implementation should be additionally synchronized. For instance, we can use the getAppContextLock for the whole body of the method. Is the method implemented somewhere else except the AppContext class? Will it work from your point of view? (Probably, I didn't take into account all the details.) Regards, Anton. On 16.01.2015 17:18, mikhail cherkasov wrote: Hi all, JBS: https://bugs.openjdk.java.net/browse/JDK-8065709 Webrev: http://cr.openjdk.java.net/~mcherkas/8065709/webrev.00/ AppContext creation is guarder by getAppContextLockand, but during AppContext creation we also call EventQueue initialization, during EQ initialization logger initialization happens it acquires javaAwtAccess. if javaAwtAccess is acquired by other it can lead to deadlock: T1 T2 start AppContext creation acquire javaAwtAccess acquire getAppContextLock init EQ call getAppContext init Logger waiting for getAppContextLock waiting for javaAwtAccess I applied the fix suggested in jbs comments by Petr. I replaced eager logger initialization in EQ with lazy, so we won't invoke Logger during EQ initialization as result no deadlock. Thanks, Mikhail.
Re: AWT Dev [9] Review Request: 6475361 Attempting to remove help menu from java.awt.MenuBar throws NullPointerException
Hi Sergey, Looks fine to me. Regards, Anton. On 12.01.2015 20:16, Sergey Bylokhov wrote: Hello. Please review a fix for jdk 9. All problems mentioned in the CR were fixed: - Menu.isHelpMenu will be changed to false when a menu is removed from the MenuBar. - MenuBar.helpMenu will be changed to null when a menu is removed from the MenuBar. - MenuBar.setHelpMenu now handle null w/o NPE. (Initially this code behaved in the same way, but long long time ago in 1996 this m.parent code was added outside of null check) Bug: https://bugs.openjdk.java.net/browse/JDK-6475361 Webrev can be found at: http://cr.openjdk.java.net/~serb/6475361/webrev.00
Re: AWT Dev [9] Review request for 7155963: Deadlock in SystemFlavorMap.getFlavorsForNative and SunToolkit.awtLock
Hi Alexander, The fix looks fine for me. Regards Anton. On 11.12.2014 15:06, Alexander Zvegintsev wrote: Hello, please review the fix http://cr.openjdk.java.net/~azvegint/jdk/9/7155963/00/ for the issue https://bugs.openjdk.java.net/browse/JDK-7155963 SunClipboard.checkChange() does not require an awtLock, so we can safely release awtLock for this call. I've run related tests(including JCK), it works fine. -- Thanks, Alexander.
Re: AWT Dev [9] Review Request: 8059998 Broken link in java.awt.event Interface KeyListener
Approved. Regards, Anton On 05.12.2014 12:05, Sergey Bylokhov wrote: Hello. Please review the fix for jdk 9. The broken link was fixed. Bug: https://bugs.openjdk.java.net/browse/JDK-8059998 Webrev can be found at: http://cr.openjdk.java.net/~serb/8059998/webrev.00
Re: AWT Dev [7] Review Request for 8047961: [TESTBUG] closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html fails intermittently
Hi Mikhail, The modified test won't do anything weired, it will test something that meets the spec. However, I agree that it won't test the original incident found in 1.4 comparing to jdk 1.3. But since we agreed not to treat it as a regression against the spec, I'm ok with the test removal. Thanks, Anton. On 28.11.2014 19:09, mikhail cherkasov wrote: Hi Anton, Oleg. I vote for removing the test. Rewriting test just will produce something weired that won't test what original regression does. A new test won't be a regression test that check problem was found in jdk 1.4. I believe jdk4 and 7 has a lot of difference in focus system and due those changes the test became irrelevant. (It's not as different as 1.3 comparing to 1.7, taking into account the fact that focus had been drastically refactored in 1.4.) Thanks, Mikhail. On 11/17/2014 5:42 PM, Anton V. Tarasov wrote: On 17.11.2014 17:38, Oleg Sukhodolsky wrote: On Mon, Nov 17, 2014 at 5:31 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: On 17.11.2014 17:21, Oleg Sukhodolsky wrote: On Mon, Nov 17, 2014 at 4:34 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: On 17.11.2014 15:01, Oleg Sukhodolsky wrote: Hi Anton, the bug was a regression introduced in 1.4 (comparing with 1.3.1) this is why it was fixed and the test was written. Indeed the spec doesn't guarantee that the test will work but at the time we were working on the ticket it was decided that we should not allow such regressions. If (according to the current policy) the spec is more important than compatibility in this case I'd suggest to remove the test completely since its new version doesn't test for the regression we had earlier but just test spec in a very strange/complicated way. The test passes on Windows, but fails on Mac and I suppose Linux. I recall I made attemps to fix the failure on Linux (in 1.6 or 1.7) but that was quite complicated to make something reliable, due to the fact that in XToolkit even native focus is asynchronous, like in CToolkit. So, from my point of view hacking it doesn't worth the candles, taking into account all I've said below. I still like the idea of replacing requestFocus with requestFocusInWindow. We can add comments saying that the original test was modified to avoid the endless loop. It didn't actually test for the endless loop, as otherwise it would have had a timer. It tested for focus gains count per window, that remains relevant even in its new form. That's my personal opinion, I won't insist on keeping it if you both vote for the opposite... if you do not plan to fix the problem the test reproduces then I'd vote for removing the test (you have enough tests in regression tests suite ;) I'm afraid I don't, as I don't think this is a real problem :) so just remove the test, you do not need it ;) No, I won't. I'd leave it at Mikhail's discretion, as he is the author of the request. Mikhail, you have a chance to save the test :) Regards, Anton. Oleg. Regards, Anton. Regards, Oleg. Thanks, Anton. So, it is you whole should make the final decision but I just do not see a reason to keep a test which doesn't test for the particular regression in regression tests suite. Regards, Oleg. On Mon, Nov 17, 2014 at 2:01 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: Hi Oleg, Glad to hear from you :) On 14.11.2014 18:24, Oleg Sukhodolsky wrote: Sorry to interrupt you but since I do know the test let me say that requestFocus() is an important part of the test, if you are going to replace it with requestFocusInWindow() you (effectively) remove it from list of regression tests. Please check 4369903 bug for more information. In the bug I see the description of why the infinite loop happens and a suggestion to use requestFocusInWindow to prevent it. From my point of view, the behavior is expected taking into account the asynchronous nature of java focus. By default, when a toplevel window gains activation, KeyboardFocusManager requests focus in it by means of calling the requestFocusInWindow method. Thus, the test case creates an artificial situation with unclear goals. It doesn't seem to reflect a real use case (I can see the bug was filed by an internal engineer, not refering to any customer or user application). In case a developer wants to alter the component receiving focus at the toplevels's activation, he/she can achieve this by means of customizing the focus traversal policy, for instance. The javadoc [1] for the JComponent.requestFocus method warns developers about its usage: Note that the use of this method is discouraged because its behavior is platform dependent. Instead we recommend the use of requestFocusInWindow(). What I'm trying to say is that we'd rather avoid hacking the focus subsystem in order to just solve the infinite loop problem. However, originally the reporter of the bug complains about the following: Setting focus during window activation often sends focus to the wrong
Re: AWT Dev [7] Review Request for 8047961: [TESTBUG] closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html fails intermittently
Hi Oleg, Glad to hear from you :) On 14.11.2014 18:24, Oleg Sukhodolsky wrote: Sorry to interrupt you but since I do know the test let me say that requestFocus() is an important part of the test, if you are going to replace it with requestFocusInWindow() you (effectively) remove it from list of regression tests. Please check 4369903 bug for more information. In the bug I see the description of why the infinite loop happens and a suggestion to use requestFocusInWindow to prevent it. From my point of view, the behavior is expected taking into account the asynchronous nature of java focus. By default, when a toplevel window gains activation, KeyboardFocusManager requests focus in it by means of calling the requestFocusInWindow method. Thus, the test case creates an artificial situation with unclear goals. It doesn't seem to reflect a real use case (I can see the bug was filed by an internal engineer, not refering to any customer or user application). In case a developer wants to alter the component receiving focus at the toplevels's activation, he/she can achieve this by means of customizing the focus traversal policy, for instance. The javadoc [1] for the JComponent.requestFocus method warns developers about its usage: Note that the use of this method is discouraged because its behavior is platform dependent. Instead we recommend the use of requestFocusInWindow(). What I'm trying to say is that we'd rather avoid hacking the focus subsystem in order to just solve the infinite loop problem. However, originally the reporter of the bug complains about the following: Setting focus during window activation often sends focus to the wrong place, or to multiple places. This behavior is incorrect, indeed. But we can verify it with the test case. When a component gains focus we can check that: 1. it is the current focus owner reported by KFM, and the current focused window is its container 2. the opposite component has lost focus prior to that (that is, every FOCUS_GAINED precedes a FOCUS_LOST received by the opposite component) 3. the same about the WINDOW_LOST_FOCUS/WINDOW_GAINED_FOCUS pair The test should be ready for the infinite loop and I can suggest to simply stop requesting focus after a couple of iterations. This could be a compromize. What do you think, Oleg, Mikhail? Thanks, Anton. [1] https://docs.oracle.com/javase/8/docs/api/javax/swing/JComponent.html#requestFocus-- Regards, Oleg. P.S. feel free to ask more questions if you need. On Fri, Nov 14, 2014 at 5:07 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: Hi Mikhail, Looks fine for me. Thanks! This was an old one... Do you have any plans to fix it in 8/9 as well? Regards, Anton. On 14.11.2014 18:57, mikhail cherkasov wrote: Hello all, Could you please review a simple fix of closed test, the test was moved to open repo and requestFocus was replaced requestFocusInWindow. Because requestFocus cause infinite war for focus between two windows, however this behavior is correct and doesn't violet spec. [TESTBUG] closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html fails intermittently bug: https://bugs.openjdk.java.net/browse/JDK-8047961 Webrev: http://cr.openjdk.java.net/~mcherkas/8047961/webrev.00/ Thanks, Mikhail.
Re: AWT Dev [7] Review Request for 8047961: [TESTBUG] closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html fails intermittently
On 17.11.2014 15:01, Oleg Sukhodolsky wrote: Hi Anton, the bug was a regression introduced in 1.4 (comparing with 1.3.1) this is why it was fixed and the test was written. Indeed the spec doesn't guarantee that the test will work but at the time we were working on the ticket it was decided that we should not allow such regressions. If (according to the current policy) the spec is more important than compatibility in this case I'd suggest to remove the test completely since its new version doesn't test for the regression we had earlier but just test spec in a very strange/complicated way. The test passes on Windows, but fails on Mac and I suppose Linux. I recall I made attemps to fix the failure on Linux (in 1.6 or 1.7) but that was quite complicated to make something reliable, due to the fact that in XToolkit even native focus is asynchronous, like in CToolkit. So, from my point of view hacking it doesn't worth the candles, taking into account all I've said below. I still like the idea of replacing requestFocus with requestFocusInWindow. We can add comments saying that the original test was modified to avoid the endless loop. It didn't actually test for the endless loop, as otherwise it would have had a timer. It tested for focus gains count per window, that remains relevant even in its new form. That's my personal opinion, I won't insist on keeping it if you both vote for the opposite... Thanks, Anton. So, it is you whole should make the final decision but I just do not see a reason to keep a test which doesn't test for the particular regression in regression tests suite. Regards, Oleg. On Mon, Nov 17, 2014 at 2:01 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: Hi Oleg, Glad to hear from you :) On 14.11.2014 18:24, Oleg Sukhodolsky wrote: Sorry to interrupt you but since I do know the test let me say that requestFocus() is an important part of the test, if you are going to replace it with requestFocusInWindow() you (effectively) remove it from list of regression tests. Please check 4369903 bug for more information. In the bug I see the description of why the infinite loop happens and a suggestion to use requestFocusInWindow to prevent it. From my point of view, the behavior is expected taking into account the asynchronous nature of java focus. By default, when a toplevel window gains activation, KeyboardFocusManager requests focus in it by means of calling the requestFocusInWindow method. Thus, the test case creates an artificial situation with unclear goals. It doesn't seem to reflect a real use case (I can see the bug was filed by an internal engineer, not refering to any customer or user application). In case a developer wants to alter the component receiving focus at the toplevels's activation, he/she can achieve this by means of customizing the focus traversal policy, for instance. The javadoc [1] for the JComponent.requestFocus method warns developers about its usage: Note that the use of this method is discouraged because its behavior is platform dependent. Instead we recommend the use of requestFocusInWindow(). What I'm trying to say is that we'd rather avoid hacking the focus subsystem in order to just solve the infinite loop problem. However, originally the reporter of the bug complains about the following: Setting focus during window activation often sends focus to the wrong place, or to multiple places. This behavior is incorrect, indeed. But we can verify it with the test case. When a component gains focus we can check that: 1. it is the current focus owner reported by KFM, and the current focused window is its container 2. the opposite component has lost focus prior to that (that is, every FOCUS_GAINED precedes a FOCUS_LOST received by the opposite component) 3. the same about the WINDOW_LOST_FOCUS/WINDOW_GAINED_FOCUS pair The test should be ready for the infinite loop and I can suggest to simply stop requesting focus after a couple of iterations. This could be a compromize. What do you think, Oleg, Mikhail? Thanks, Anton. [1] https://docs.oracle.com/javase/8/docs/api/javax/swing/JComponent.html#requestFocus-- Regards, Oleg. P.S. feel free to ask more questions if you need. On Fri, Nov 14, 2014 at 5:07 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: Hi Mikhail, Looks fine for me. Thanks! This was an old one... Do you have any plans to fix it in 8/9 as well? Regards, Anton. On 14.11.2014 18:57, mikhail cherkasov wrote: Hello all, Could you please review a simple fix of closed test, the test was moved to open repo and requestFocus was replaced requestFocusInWindow. Because requestFocus cause infinite war for focus between two windows, however this behavior is correct and doesn't violet spec. [TESTBUG] closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html fails intermittently bug: https://bugs.openjdk.java.net/browse/JDK-8047961 Webrev: http://cr.openjdk.java.net/~mcherkas/8047961/webrev.00/ Thanks, Mikhail.
Re: AWT Dev [7] Review Request for 8047961: [TESTBUG] closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html fails intermittently
On 17.11.2014 17:21, Oleg Sukhodolsky wrote: On Mon, Nov 17, 2014 at 4:34 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: On 17.11.2014 15:01, Oleg Sukhodolsky wrote: Hi Anton, the bug was a regression introduced in 1.4 (comparing with 1.3.1) this is why it was fixed and the test was written. Indeed the spec doesn't guarantee that the test will work but at the time we were working on the ticket it was decided that we should not allow such regressions. If (according to the current policy) the spec is more important than compatibility in this case I'd suggest to remove the test completely since its new version doesn't test for the regression we had earlier but just test spec in a very strange/complicated way. The test passes on Windows, but fails on Mac and I suppose Linux. I recall I made attemps to fix the failure on Linux (in 1.6 or 1.7) but that was quite complicated to make something reliable, due to the fact that in XToolkit even native focus is asynchronous, like in CToolkit. So, from my point of view hacking it doesn't worth the candles, taking into account all I've said below. I still like the idea of replacing requestFocus with requestFocusInWindow. We can add comments saying that the original test was modified to avoid the endless loop. It didn't actually test for the endless loop, as otherwise it would have had a timer. It tested for focus gains count per window, that remains relevant even in its new form. That's my personal opinion, I won't insist on keeping it if you both vote for the opposite... if you do not plan to fix the problem the test reproduces then I'd vote for removing the test (you have enough tests in regression tests suite ;) I'm afraid I don't, as I don't think this is a real problem :) Regards, Anton. Regards, Oleg. Thanks, Anton. So, it is you whole should make the final decision but I just do not see a reason to keep a test which doesn't test for the particular regression in regression tests suite. Regards, Oleg. On Mon, Nov 17, 2014 at 2:01 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: Hi Oleg, Glad to hear from you :) On 14.11.2014 18:24, Oleg Sukhodolsky wrote: Sorry to interrupt you but since I do know the test let me say that requestFocus() is an important part of the test, if you are going to replace it with requestFocusInWindow() you (effectively) remove it from list of regression tests. Please check 4369903 bug for more information. In the bug I see the description of why the infinite loop happens and a suggestion to use requestFocusInWindow to prevent it. From my point of view, the behavior is expected taking into account the asynchronous nature of java focus. By default, when a toplevel window gains activation, KeyboardFocusManager requests focus in it by means of calling the requestFocusInWindow method. Thus, the test case creates an artificial situation with unclear goals. It doesn't seem to reflect a real use case (I can see the bug was filed by an internal engineer, not refering to any customer or user application). In case a developer wants to alter the component receiving focus at the toplevels's activation, he/she can achieve this by means of customizing the focus traversal policy, for instance. The javadoc [1] for the JComponent.requestFocus method warns developers about its usage: Note that the use of this method is discouraged because its behavior is platform dependent. Instead we recommend the use of requestFocusInWindow(). What I'm trying to say is that we'd rather avoid hacking the focus subsystem in order to just solve the infinite loop problem. However, originally the reporter of the bug complains about the following: Setting focus during window activation often sends focus to the wrong place, or to multiple places. This behavior is incorrect, indeed. But we can verify it with the test case. When a component gains focus we can check that: 1. it is the current focus owner reported by KFM, and the current focused window is its container 2. the opposite component has lost focus prior to that (that is, every FOCUS_GAINED precedes a FOCUS_LOST received by the opposite component) 3. the same about the WINDOW_LOST_FOCUS/WINDOW_GAINED_FOCUS pair The test should be ready for the infinite loop and I can suggest to simply stop requesting focus after a couple of iterations. This could be a compromize. What do you think, Oleg, Mikhail? Thanks, Anton. [1] https://docs.oracle.com/javase/8/docs/api/javax/swing/JComponent.html#requestFocus-- Regards, Oleg. P.S. feel free to ask more questions if you need. On Fri, Nov 14, 2014 at 5:07 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: Hi Mikhail, Looks fine for me. Thanks! This was an old one... Do you have any plans to fix it in 8/9 as well? Regards, Anton. On 14.11.2014 18:57, mikhail cherkasov wrote: Hello all, Could you please review a simple fix of closed test, the test was moved to open repo and requestFocus was replaced requestFocusInWindow
Re: AWT Dev [7] Review Request for 8047961: [TESTBUG] closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html fails intermittently
On 17.11.2014 17:38, Oleg Sukhodolsky wrote: On Mon, Nov 17, 2014 at 5:31 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: On 17.11.2014 17:21, Oleg Sukhodolsky wrote: On Mon, Nov 17, 2014 at 4:34 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: On 17.11.2014 15:01, Oleg Sukhodolsky wrote: Hi Anton, the bug was a regression introduced in 1.4 (comparing with 1.3.1) this is why it was fixed and the test was written. Indeed the spec doesn't guarantee that the test will work but at the time we were working on the ticket it was decided that we should not allow such regressions. If (according to the current policy) the spec is more important than compatibility in this case I'd suggest to remove the test completely since its new version doesn't test for the regression we had earlier but just test spec in a very strange/complicated way. The test passes on Windows, but fails on Mac and I suppose Linux. I recall I made attemps to fix the failure on Linux (in 1.6 or 1.7) but that was quite complicated to make something reliable, due to the fact that in XToolkit even native focus is asynchronous, like in CToolkit. So, from my point of view hacking it doesn't worth the candles, taking into account all I've said below. I still like the idea of replacing requestFocus with requestFocusInWindow. We can add comments saying that the original test was modified to avoid the endless loop. It didn't actually test for the endless loop, as otherwise it would have had a timer. It tested for focus gains count per window, that remains relevant even in its new form. That's my personal opinion, I won't insist on keeping it if you both vote for the opposite... if you do not plan to fix the problem the test reproduces then I'd vote for removing the test (you have enough tests in regression tests suite ;) I'm afraid I don't, as I don't think this is a real problem :) so just remove the test, you do not need it ;) No, I won't. I'd leave it at Mikhail's discretion, as he is the author of the request. Mikhail, you have a chance to save the test :) Regards, Anton. Oleg. Regards, Anton. Regards, Oleg. Thanks, Anton. So, it is you whole should make the final decision but I just do not see a reason to keep a test which doesn't test for the particular regression in regression tests suite. Regards, Oleg. On Mon, Nov 17, 2014 at 2:01 PM, Anton V. Tarasov anton.tara...@oracle.com wrote: Hi Oleg, Glad to hear from you :) On 14.11.2014 18:24, Oleg Sukhodolsky wrote: Sorry to interrupt you but since I do know the test let me say that requestFocus() is an important part of the test, if you are going to replace it with requestFocusInWindow() you (effectively) remove it from list of regression tests. Please check 4369903 bug for more information. In the bug I see the description of why the infinite loop happens and a suggestion to use requestFocusInWindow to prevent it. From my point of view, the behavior is expected taking into account the asynchronous nature of java focus. By default, when a toplevel window gains activation, KeyboardFocusManager requests focus in it by means of calling the requestFocusInWindow method. Thus, the test case creates an artificial situation with unclear goals. It doesn't seem to reflect a real use case (I can see the bug was filed by an internal engineer, not refering to any customer or user application). In case a developer wants to alter the component receiving focus at the toplevels's activation, he/she can achieve this by means of customizing the focus traversal policy, for instance. The javadoc [1] for the JComponent.requestFocus method warns developers about its usage: Note that the use of this method is discouraged because its behavior is platform dependent. Instead we recommend the use of requestFocusInWindow(). What I'm trying to say is that we'd rather avoid hacking the focus subsystem in order to just solve the infinite loop problem. However, originally the reporter of the bug complains about the following: Setting focus during window activation often sends focus to the wrong place, or to multiple places. This behavior is incorrect, indeed. But we can verify it with the test case. When a component gains focus we can check that: 1. it is the current focus owner reported by KFM, and the current focused window is its container 2. the opposite component has lost focus prior to that (that is, every FOCUS_GAINED precedes a FOCUS_LOST received by the opposite component) 3. the same about the WINDOW_LOST_FOCUS/WINDOW_GAINED_FOCUS pair The test should be ready for the infinite loop and I can suggest to simply stop requesting focus after a couple of iterations. This could be a compromize. What do you think, Oleg, Mikhail? Thanks, Anton. [1] https://docs.oracle.com/javase/8/docs/api/javax/swing/JComponent.html#requestFocus-- Regards, Oleg. P.S. feel free to ask more questions if you need. On Fri, Nov 14, 2014 at 5:07 PM, Anton V. Tarasov anton.tara
Re: AWT Dev [7] Review Request for 8001633: Wrong alt processing during switching between windows.
Looks fine for me. Regards, Anton. On 13.11.2014 22:17, mikhail cherkasov wrote: Hello all, Could you please review a backport to jdk7 for the following bug: bug: https://bugs.openjdk.java.net/browse/JDK-8001633 Webrev: http://cr.openjdk.java.net/~mcherkas/8001633/jdk7/webrev.00/ review for jdk8: http://mail.openjdk.java.net/pipermail/awt-dev/2012-October/003732.html It's direct backport except a test case, the test case was replaced with a new one, because jdk8's test case doesn't reproduce the problem for jdk7. Thanks, Mikhail.
Re: AWT Dev [7] Review Request for 8047961: [TESTBUG] closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html fails intermittently
Hi Mikhail, Looks fine for me. Thanks! This was an old one... Do you have any plans to fix it in 8/9 as well? Regards, Anton. On 14.11.2014 18:57, mikhail cherkasov wrote: Hello all, Could you please review a simple fix of closed test, the test was moved to open repo and requestFocus was replaced requestFocusInWindow. Because requestFocus cause infinite war for focus between two windows, however this behavior is correct and doesn't violet spec. [TESTBUG] closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html fails intermittently bug: https://bugs.openjdk.java.net/browse/JDK-8047961 Webrev: http://cr.openjdk.java.net/~mcherkas/8047961/webrev.00/ Thanks, Mikhail.
AWT Dev [9] Review Request for JDK-8004148: NPE in sun.awt.SunToolkit.getWindowDeactivationTime
Hello, Please, review the fix. https://bugs.openjdk.java.net/browse/JDK-8004148 http://cr.openjdk.java.net/~ant/JDK-8004148/webrev.0 There's no a test case for the original issue. However, there's another test case which reproduces NPE with the same stack trace. The problem was fixed in jdk8 in JDK-8001633. However, I suggest to do a sanity check for null. I check for null AppContext which covers the case for null Window as well. Thanks, Anton.
Re: AWT Dev Focus regression in Java8 caused by JDK-7160604
Hi Clemens, Sure, please file the issue. If you have an account in JIRA, please file it right there. Otherwise, tell us please the ID of the incident you will file and we'll accept it. Thanks! Anton. On 05.11.2014 22:46, Clemens Eisserer wrote: Hi Anton, I can't say exactly how the fix for JDK-7160604 could change the behavior like you described (this requires some more time to investigate). However, an obvious question comes to my mind - why don't you make your popup non-focusable? But still, making the popup simply non-focusable looks better. Exactly, this is the solution I chose to fix the issue. However, as JDK-7160604 broke an application which was working fine since Java-1.3 days I thought reporting it wouldn't harm. Should I open a bug-report? If so, in the jira bug-tracker or using the official form? Also, just a side note. As a straightforward workaround to the focus issue, you could invoke your requestFocus later, either via calling invokeLater() Funny thing is, I've tried this approach first - and it leads to a very strange phenomenon and breaks in a very odd way. When I call requestFocus inside a runnable executed via SwingUtilities.invokeLater(), the JTextField doesn't receive any keyboard events, even after the JPopup is closed and I click on the JTextField again. This also seems to be a side-effect of JDK-7160604, before it this also just works. Best regards, Clemens
Re: AWT Dev [7u] Review request for 8061954: 7u76 - deployment warning dialogs do not work on Linux
Hi Anton, The fix looks fine for me (taking into account you've launched all the focus reg tests). However, I have a concern about the issue itself. As far as I know, any deploy warning dialog should belong to the deploy Thread Group which does have its own AppContext. The only TG which has null AppContext is the system thread group. However, no UI code should be invoked in that TG. Am I right? If so, then it seems to me we should identify and fix the root cause of the issue. What do you think? Regards, Anton. On 31.10.2014 14:39, Anton Litvinov wrote: Hello, Could you please review the following fix for P1 bug. Bug: https://bugs.openjdk.java.net/browse/JDK-8061954 Webrev: http://cr.openjdk.java.net/~alitvinov/8061954/webrev.00 The bug consists in the fact that java.lang.NullPointerException is thrown from java.awt.KeyboardFocusManager.getCurrentKeyboardFocusManager and a button does not get the focus on mouse click, when the button is located on a warning dialog which is shown during Java applet loading on Linux OS. The solution consists of 2 following parts: 1. Reversal of the fix for JDK-6993873 in order to resolve this bug (file XContentWindow.java). 2. Backport of only Linux/Solaris parts of the fix for JDK-6981400 from JDK 8 in order to again resolve JDK-6993873 (the failing test jdk/test/java/awt/Focus/FocusOwnerFrameOnClick/FocusOwnerFrameOnClick.java). The next jtreg regression tests were run on JDK 7 without/with the fix and no new test failures were defined: jdk/test/java/awt/Focus jdk/test/java/awt/KeyboardFocusmanager jdk/test/javax/swing/KeyboardManager Analogous tests from closed parts of JDK 7. Thank you, Anton
Re: AWT Dev Focus regression in Java8 caused by JDK-7160604
Hi Clemens, I can't say exactly how the fix for JDK-7160604 could change the behavior like you described (this requires some more time to investigate). However, an obvious question comes to my mind - why don't you make your popup non-focusable? I suppose you manage navigation through the content of your auto-completion list somehow by intercepting the up/down keys on the textfiled, right? In that case, you don't need your popup be a focusable component. Does it work for you? Also, just a side note. As a straightforward workaround to the focus issue, you could invoke your requestFocus later, either via calling invokeLater() or by invoking it at the time the popup is shown (I'm not sure you can listen for the componentShown(), but at least, you can listen for focusGained() on its content). This would make the things happen in a fixed order: popup_show then request_focus. (Currently, it seems the order is broken somehow.) But still, making the popup simply non-focusable looks better. Regards, Anton. On 02.11.2014 17:52, Clemens Eisserer wrote: HI, Starting with the rollout of Java8 via the auto-updater we received several reports that one of our applications has servere focus issues. The application registers a KeyListener on a JTextField showing an auto-completion JPopupMenu every time a character is typed. Every time a key is typed, the following sequence is run: - Hide existing JPopupMenu in case there is one - Create new JPopupMenu and populate with contents (in this case a JTable) - JPopupMenu.show() below the JTextField which caused the keyTyped-event - JTextField.requestFocus(), so the user can continue typing. I've created a self-contained testcase available here: http://pastebin.com/GEQ1ZW28 This worked fine with Java6 Java7, however since JDK8-b119 it failes when the popup is heavyweight - for lightweight popups it still succeeds (unfourtunately, we need heavyweight ones). I tried to analyze what happend and registered a global focus listener: This is the focus-history without the fix for JDK-7160604 - once the JTextField got it's focus no changes can be observed, despite new JPopups are being opened: javax.swing.JTextField[,241,115,197x19,... javax.swing.JRootPane[,5,27,955x384,. javax.swing.JTextField[,241,115,197x19,... (focus stays here, despite new JPopups are beeing opened and old ones closed) With the fix, the focus history looks like this: javax.swing.JTextField[,241,115,197x19, javax.swing.JTextField[,241,115,197x19, javax.swing.JRootPane[,5,27,938x588,...(focus stays here, no further keyEvents are received by the JTextField) Beside some refactorings, the only functional change I can spot is that the order of the assignment of the popup-member and popup.show() have changed in JPopupMenu.showPopup(): Popup newPopup = getUI().getPopup(this, desiredLocationX, desiredLocationY); popupFactory.setPopupType(PopupFactory.LIGHT_WEIGHT_POPUP); popup = newPopup; newPopup.show(); If I change the order of newPopup.show() and popup=newPopup, the old behaviour is restored. At least it looks somehow suspicious the original author introduced a variable newPopup (and the patch still uses it without a good reason, popup could be assigned directly), so at first sight it looks intentional to assign popup only after show() was called. During the call to newPopup.show() the method JPopupMenu.isVisible() is called several times. Before the fix for JDK-7160604 it would return false, now it returns true - the focus issue mentioned is triggered by returning true at the following call-stack (full one available at http://pastebin.com/4qG8h7Td). at javax.swing.JPopupMenu.isVisible(JPopupMenu.java:854) at javax.swing.SortingFocusTraversalPolicy.enumerateCycle(SortingFocusTraversalPolicy.java:141) at javax.swing.SortingFocusTraversalPolicy.enumerateCycle(SortingFocusTraversalPolicy.java:156) at javax.swing.SortingFocusTraversalPolicy.enumerateCycle(SortingFocusTraversalPolicy.java:156) at javax.swing.SortingFocusTraversalPolicy.enumerateCycle(SortingFocusTraversalPolicy.java:156) at javax.swing.SortingFocusTraversalPolicy.enumerateCycle(SortingFocusTraversalPolicy.java:156) at javax.swing.SortingFocusTraversalPolicy.enumerateAndSortCycle(SortingFocusTraversalPolicy.java:135) at javax.swing.SortingFocusTraversalPolicy.getFocusTraversalCycle(SortingFocusTraversalPolicy.java:110) at javax.swing.SortingFocusTraversalPolicy.getFirstComponent(SortingFocusTraversalPolicy.java:445) at javax.swing.LayoutFocusTraversalPolicy.getFirstComponent(LayoutFocusTraversalPolicy.java:166) at javax.swing.SortingFocusTraversalPolicy.getDefaultComponent(SortingFocusTraversalPolicy.java:535) at java.awt.Window.isFocusableWindow(Window.java:2496) If I introspect the call-stack and return false only when
Re: AWT Dev [9] Review request : 8038919, Requesting focus to a modeless dialog doesn't work on Safari
Hi Mikhail, (sorry, I was away yesterday) On 07.10.2014 19:37, mikhail cherkasov wrote: Hi Anton, But anyway, I'll have to use instanceof to check the type returned for DefaultKeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow().getPeer() . or does it always return only LWWindowPeer? That's right, on Mac a toplevel's peer is of LWWindowPeer type. Also, I think you should check for the active window to be not null (it's possible when all the toplevels are set non-focusable). However, you can omit a check for peer==null, as an active window may not have a null peer. Regards, Anton. Thanks, Mikhail. On 10/6/2014 12:56 PM, Anton V. Tarasov wrote: Hi Mikhail, Ok, thank you for the clarification. Still, let's make another attempt to avoid the instanceof. In LWWindowPeer we have a good public method - getPeerType() - which should return PeerType.EMBEDDED_FRAME for EF. Can it be used in the code spot? Regards, Anton. On 06.10.2014 11:08, mikhail cherkasov wrote: Hi Anton, it's window problem, toFront doesn't work for windows when embedded frame in browser is active. Mac ignores toFront, because it recognizes browser is active application and doesn't allow to java process activate its window. So the problem occurs only when we call toFront on Windows and embedded frame is active, hence fix should be done in Window's toFront method. Thanks, Mikhail. On 10/3/2014 5:48 PM, Anton V. Tarasov wrote: Hi Mikhail, Can we get rid of that instance of? I think it makes sense to create a new LWEmbeddedFramePeer class with toFront overriden. Then we can do all the verifications gracefully and call platformWindow.toFrontIgnoringOtherApps() (new method). I would also rename LWCToolkit.activateApplication to match its native API call. What do you think? Regards, Anton. On 16.09.2014 15:39, mikhail cherkasov wrote: Hello all, please review the fix http://cr.openjdk.java.net/~mcherkas/8038919/webrev.01/ bug: https://bugs.openjdk.java.net/browse/JDK-8038919 The problem appears if we trying to call toFront when embedded window is active in browser, this call is ignored, because for macosx the browser process is active and it ignores [nsWindow orderFront:nsWindow] call to java process windows. To fix this issue I use [NSApp activateIgnoringOtherApps:YES]; before [nsWindow orderFront:nsWindow] if an embedded frame is active window. Thanks, Mikhail.
Re: AWT Dev [9] Review request : 8038919, Requesting focus to a modeless dialog doesn't work on Safari
Hi Mikhail, Ok, thank you for the clarification. Still, let's make another attempt to avoid the instanceof. In LWWindowPeer we have a good public method - getPeerType() - which should return PeerType.EMBEDDED_FRAME for EF. Can it be used in the code spot? Regards, Anton. On 06.10.2014 11:08, mikhail cherkasov wrote: Hi Anton, it's window problem, toFront doesn't work for windows when embedded frame in browser is active. Mac ignores toFront, because it recognizes browser is active application and doesn't allow to java process activate its window. So the problem occurs only when we call toFront on Windows and embedded frame is active, hence fix should be done in Window's toFront method. Thanks, Mikhail. On 10/3/2014 5:48 PM, Anton V. Tarasov wrote: Hi Mikhail, Can we get rid of that instance of? I think it makes sense to create a new LWEmbeddedFramePeer class with toFront overriden. Then we can do all the verifications gracefully and call platformWindow.toFrontIgnoringOtherApps() (new method). I would also rename LWCToolkit.activateApplication to match its native API call. What do you think? Regards, Anton. On 16.09.2014 15:39, mikhail cherkasov wrote: Hello all, please review the fix http://cr.openjdk.java.net/~mcherkas/8038919/webrev.01/ bug: https://bugs.openjdk.java.net/browse/JDK-8038919 The problem appears if we trying to call toFront when embedded window is active in browser, this call is ignored, because for macosx the browser process is active and it ignores [nsWindow orderFront:nsWindow] call to java process windows. To fix this issue I use [NSApp activateIgnoringOtherApps:YES]; before [nsWindow orderFront:nsWindow] if an embedded frame is active window. Thanks, Mikhail.
Re: AWT Dev [9] Review request : 8038919, Requesting focus to a modeless dialog doesn't work on Safari
Hi Mikhail, Can we get rid of that instance of? I think it makes sense to create a new LWEmbeddedFramePeer class with toFront overriden. Then we can do all the verifications gracefully and call platformWindow.toFrontIgnoringOtherApps() (new method). I would also rename LWCToolkit.activateApplication to match its native API call. What do you think? Regards, Anton. On 16.09.2014 15:39, mikhail cherkasov wrote: Hello all, please review the fix http://cr.openjdk.java.net/~mcherkas/8038919/webrev.01/ bug: https://bugs.openjdk.java.net/browse/JDK-8038919 The problem appears if we trying to call toFront when embedded window is active in browser, this call is ignored, because for macosx the browser process is active and it ignores [nsWindow orderFront:nsWindow] call to java process windows. To fix this issue I use [NSApp activateIgnoringOtherApps:YES]; before [nsWindow orderFront:nsWindow] if an embedded frame is active window. Thanks, Mikhail.
Re: AWT Dev CFV: New AWT group member: Alexander Zvegintsev
Vote: YES On 03.09.2014 14:44, Artem Ananiev wrote: I hereby nominate Alexander Zvegintsev (OpenJDK user name: azvegint) to Membership in the AWT Group. Alexander is a member of AWT/Swing group at Oracle. He has contributed 20+ fixes to JDK9 so far, and JDK8/8u contribution is significant as well. Votes are due by Sep 17, 2014. Only current Members of the AWT Group [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list For Lazy Consensus voting instructions, see [2]. [1] http://openjdk.java.net/census#awt [2] http://openjdk.java.net/groups#group-member Thanks, Artem
Re: AWT Dev Review request for 8047288: [macosx] Endless loop in EDT on Mac
Hi Artem, I'm Ok with the fix, provided that LWWindowPeer.setVisibleImpl() is called on EDT for applets (if I'm not mistaken). Otherwise, we still have a client method (getTarget().isFocusableWindow()) called on the toolkit thread, which is generally no good. Regards, Anton. On 24.07.2014 10:25, artem malinko wrote: Thank you, Petr. Could anyone else review the fix, please? On 7/23/2014 10:30 PM, Petr Pchelko wrote: Hello, Artem. The new version (it’s .00 for some reason) looks good. With best regards. Petr. On Jul 23, 2014, at 6:55 PM, artem malinko artem.mali...@oracle.com mailto:artem.mali...@oracle.com wrote: Hi, Petr. I ran focus regression tests and jck tests on awt. For fixed jdk results is the same. Except my new test, of course which is not passed on not fixed jdk:) And also I fixed issues in test. New webrev: http://cr.openjdk.java.net/~bae/8047288/9/webrev.00/ On 7/22/2014 8:23 PM, Petr Pchelko wrote: Hello, Artem. A couple of comments: 1. LWWindowPeer: 268 - please remove an empty line. 2. LWWIndowPeer. isTargetFocusable - the method is not needed at all. 3. I’m concerned about the test. Do you really need the close button? 4. frame and window variables are set from main thread and read from EDT. They should be declared volatile. Also please run all focus regression and JCK tests, because this area is very sensitive. With best regards. Petr. On Jul 22, 2014, at 8:04 PM, artem malinko artem.mali...@oracle.com mailto:artem.mali...@oracle.com wrote: Hello, AWT Team. Please review a fix for the issue: https://bugs.openjdk.java.net/browse/JDK-8047288 The fix is available at: http://cr.openjdk.java.net/~mcherkas/artem/8047288/webrev.01/ http://cr.openjdk.java.net/%7Emcherkas/artem/8047288/webrev.01/ Window.isFocusableWindow() could lead to deadlock if it is invoked on AppKit thread. Fix caches result of Window.isFocusableWindow() on a peer level and method is not invoked on AppkitThread. Thank you.
Re: AWT Dev Review request for 8047288: [macosx] Endless loop in EDT on Mac
On 24.07.2014 13:48, artem malinko wrote: Hi Anton. Sorry, didn't understand you well. Do you mean we should check(and test) that this method(isFocusableWindow()) is also called on EDT if we run applet? IMHO, it's enough to just make sure (via inspecting the sources) that Window.setVisible() is called on EDT in case of an applet. That logic is triggered on the plugin side. Anton. On 7/24/2014 11:42 AM, Anton V. Tarasov wrote: Hi Artem, I'm Ok with the fix, provided that LWWindowPeer.setVisibleImpl() is called on EDT for applets (if I'm not mistaken). Otherwise, we still have a client method (getTarget().isFocusableWindow()) called on the toolkit thread, which is generally no good. Regards, Anton. On 24.07.2014 10:25, artem malinko wrote: Thank you, Petr. Could anyone else review the fix, please? On 7/23/2014 10:30 PM, Petr Pchelko wrote: Hello, Artem. The new version (it’s .00 for some reason) looks good. With best regards. Petr. On Jul 23, 2014, at 6:55 PM, artem malinko artem.mali...@oracle.com mailto:artem.mali...@oracle.com wrote: Hi, Petr. I ran focus regression tests and jck tests on awt. For fixed jdk results is the same. Except my new test, of course which is not passed on not fixed jdk:) And also I fixed issues in test. New webrev: http://cr.openjdk.java.net/~bae/8047288/9/webrev.00/ On 7/22/2014 8:23 PM, Petr Pchelko wrote: Hello, Artem. A couple of comments: 1. LWWindowPeer: 268 - please remove an empty line. 2. LWWIndowPeer. isTargetFocusable - the method is not needed at all. 3. I’m concerned about the test. Do you really need the close button? 4. frame and window variables are set from main thread and read from EDT. They should be declared volatile. Also please run all focus regression and JCK tests, because this area is very sensitive. With best regards. Petr. On Jul 22, 2014, at 8:04 PM, artem malinko artem.mali...@oracle.com mailto:artem.mali...@oracle.com wrote: Hello, AWT Team. Please review a fix for the issue: https://bugs.openjdk.java.net/browse/JDK-8047288 The fix is available at: http://cr.openjdk.java.net/~mcherkas/artem/8047288/webrev.01/ http://cr.openjdk.java.net/%7Emcherkas/artem/8047288/webrev.01/ Window.isFocusableWindow() could lead to deadlock if it is invoked on AppKit thread. Fix caches result of Window.isFocusableWindow() on a peer level and method is not invoked on AppkitThread. Thank you.
Re: AWT Dev [7] Review request for 6993873: java/awt/Focus/FocusOwnerFrameOnClick/FocusOwnerFrameOnClick.java test indicates .a frame wasn't focused on click jdk7 issue on linux
Looks fine to me. Regards, Anton. On 02.07.2014 22:36, anton nashatyrev wrote: Hello, could you please review the following fix: fix: http://cr.openjdk.java.net/~anashaty/6993873/7/webrev.00/ http://cr.openjdk.java.net/%7Eanashaty/6993873/7/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-6993873 This is actually a partial port of the JDK-6886678 https://bugs.openjdk.java.net/browse/JDK-6886678 fix from jdk6 to jdk7 (the port had been made but not completely) Since jdk8 the problem is not reproducible since was fixed with the bug JDK-6981400 https://bugs.openjdk.java.net/browse/JDK-6981400, but again without backport to the jdk7. Thanks! Anton.
Re: AWT Dev [9] Review request for 8046495: KeyEvent can not be accepted in quick mouse clicking
On 02.07.2014 11:44, Petr Pchelko wrote: Hello, Anton. I'm not sure I have a detailed understanding of what's happening. Before your fix the timestamp of the event represented the time when the event was created, and now it's the time when it's sent to java. This might be important if some events cause other events to be issued on the java side. So suppose the following: Toolkit thread: InputEvent#1 created - timestamp 0 Toolkit thread: InputEvent#2 created - timestamp 1 Toolkit thread: InputEvent#1 sent - timestamp 2 EDT: InputEvent#1 dispatched - timestamp 3 EDT: FocusEvent created- timestamp 4 Toolkit thread: InputEvent#2 sent - timestamp 5 Before you fix we had the following event order: InputEvent#1(ts=0), InputEvent#2(ts=1), FocusEvent(ts=4). But after your fix we will have a different order: InputEvent#1(ts=2), FocusEvent(ts=4), InputEvent#2(ts=5). So now we would have troubles, because the Input Event will go to the new focused component instead of the old one. Do you think that my arguments are correct? I understand that the likelihood of such a situation is very low, but for me it looks possible? Am I missing something? A timestamp for a FocusEvent is taken from the_most_recent_KeyEvent_time which is set on EDT when the event is dispatched. So the fix shouldn't affect it. Also, in awt_Window.cpp, when a TimedWindowEvent is created it is passed a timestamp got with TimeHelper::getMessageTimeUTC(). It appears, that the fix makes key events times even more consistent with it. So, from the kbd focus perspective, it shouldn't make any harm. Anton, could you just please run the following tests, at least: test/java/awt/Focus/6981400/* test/java/awt/KeyboardFocusManager/TypeAhead/* Thanks, Anton. Another thing I do not understand is why we were used to use the complicated formula instead of initializing the msg.time field with the JVM current time and using it when sending the event? Wouldn't that resolve both your issue and the issue the original fix was made for? I have a couple of comments about the code, but let's postpone that until we decide on the approach. Thank you. With best regards. Petr. On 01 июля 2014 г., at 21:20, anton nashatyrev anton.nashaty...@oracle.com mailto:anton.nashaty...@oracle.com wrote: Hello, could you please review the following fix: fix: http://cr.openjdk.java.net/~anashaty/8046495/9/webrev.00/ http://cr.openjdk.java.net/%7Eanashaty/8046495/9/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8046495 *Problem:* On Windows the later InputEvent may have earlier timestamp (getWhen()), which results in incorrect processing of enqueued input events and may also potentially be the reason of other artifacts *Evaluation*: On Windows we have some algorithm for calculating input event timestamp: jdk/src/windows/native/sun/windows/awt_Component.cpp:2100 Shortly the timestamp is calculated by the following formula: when = JVM_CurrentTimeMillis() - (GetTickCount() - GetMessageTime()) Where: JVM_CurrentTimeMillis() - the same as System.currentTimeMillis() GetTickCount() - Win32 function, current millis from boot time GetMessageTime() - Win32 function, millis from boot time of the latest native Message In theory the formula looks good: we are trying our best to calculate the actual time of the OS message by taking the current JVM time (JVM_CurrentTimeMillis) and adjusting it with the offset (GetTickCount - GetMessageTime) which should indicate how many milliseconds ago from the current moment (GetTickCount) the message has been actually issued (GetMessageTime). In practice due to usage of different system timers by the JVM_CurrentTimeMillis and GetTickCount their values are not updated synchronously and we may get an earlier timestamp for the later event. *Fix*: Just to use JVM_CurrentTimeMillis only as events timestamp. On Mac this is done in exactly the same way: CPlatformResponder.handleMouse/KeyEvent() The message time offset calculation has been introduced with the fix for JDK-4434193 https://bugs.openjdk.java.net/browse/JDK-4434193 and it seems that the issue has addressed very similar problem (At times getWhen()in ActionEvent returns higher value than the CurrentSystemTime) which has not been completely resolved in fact. I also didn't find any benefits in using the existing approach: - all the usages of the TimerHelper are in fact reduced to the mentioned formula: when = JVM_CurrentTimeMillis() - (GetTickCount() - GetMessageTime()) - GetMessageTime() always increases (except of the int overflow moments), thus we couldn't get earlier OS messages after later ones - TimerHelper::windowsToUTC(DWORD windowsTime) doesn't guarantee returning the same timestamp across different calls for the same message time Thanks! Anton.
Re: AWT Dev [9] Review request for 8046495: KeyEvent can not be accepted in quick mouse clicking
On 02.07.2014 19:28, anton nashatyrev wrote: Hello, Anton On 02.07.2014 18:13, Anton V. Tarasov wrote: On 02.07.2014 11:44, Petr Pchelko wrote: Hello, Anton. I'm not sure I have a detailed understanding of what's happening. Before your fix the timestamp of the event represented the time when the event was created, and now it's the time when it's sent to java. This might be important if some events cause other events to be issued on the java side. So suppose the following: Toolkit thread: InputEvent#1 created - timestamp 0 Toolkit thread: InputEvent#2 created - timestamp 1 Toolkit thread: InputEvent#1 sent - timestamp 2 EDT: InputEvent#1 dispatched - timestamp 3 EDT: FocusEvent created- timestamp 4 Toolkit thread: InputEvent#2 sent - timestamp 5 Before you fix we had the following event order: InputEvent#1(ts=0), InputEvent#2(ts=1), FocusEvent(ts=4). But after your fix we will have a different order: InputEvent#1(ts=2), FocusEvent(ts=4), InputEvent#2(ts=5). So now we would have troubles, because the Input Event will go to the new focused component instead of the old one. Do you think that my arguments are correct? I understand that the likelihood of such a situation is very low, but for me it looks possible? Am I missing something? A timestamp for a FocusEvent is taken from the_most_recent_KeyEvent_time which is set on EDT when the event is dispatched. So the fix shouldn't affect it. Also, in awt_Window.cpp, when a TimedWindowEvent is created it is passed a timestamp got with TimeHelper::getMessageTimeUTC(). It appears, that the fix makes key events times even more consistent with it. So, from the kbd focus perspective, it shouldn't make any harm. Anton, could you just please run the following tests, at least: test/java/awt/Focus/6981400/* test/java/awt/KeyboardFocusManager/TypeAhead/* I've tested with the following set: [closed]/java/awt/event/* [closed]/java/awt/Focus/* [closed]java/awt/KeyboardFocusManager/* : no unexpected failures here. I've also verified that my regression test which comes with the fix works fine on Linux and Mac (despite the fix is Win specific). Great, thanks! Anton. Thanks for review! Anton. Thanks, Anton. Another thing I do not understand is why we were used to use the complicated formula instead of initializing the msg.time field with the JVM current time and using it when sending the event? Wouldn't that resolve both your issue and the issue the original fix was made for? I have a couple of comments about the code, but let's postpone that until we decide on the approach. Thank you. With best regards. Petr. On 01 июля 2014 г., at 21:20, anton nashatyrev anton.nashaty...@oracle.com mailto:anton.nashaty...@oracle.com wrote: Hello, could you please review the following fix: fix: http://cr.openjdk.java.net/~anashaty/8046495/9/webrev.00/ http://cr.openjdk.java.net/%7Eanashaty/8046495/9/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8046495 *Problem:* On Windows the later InputEvent may have earlier timestamp (getWhen()), which results in incorrect processing of enqueued input events and may also potentially be the reason of other artifacts *Evaluation*: On Windows we have some algorithm for calculating input event timestamp: jdk/src/windows/native/sun/windows/awt_Component.cpp:2100 Shortly the timestamp is calculated by the following formula: when = JVM_CurrentTimeMillis() - (GetTickCount() - GetMessageTime()) Where: JVM_CurrentTimeMillis() - the same as System.currentTimeMillis() GetTickCount() - Win32 function, current millis from boot time GetMessageTime() - Win32 function, millis from boot time of the latest native Message In theory the formula looks good: we are trying our best to calculate the actual time of the OS message by taking the current JVM time (JVM_CurrentTimeMillis) and adjusting it with the offset (GetTickCount - GetMessageTime) which should indicate how many milliseconds ago from the current moment (GetTickCount) the message has been actually issued (GetMessageTime). In practice due to usage of different system timers by the JVM_CurrentTimeMillis and GetTickCount their values are not updated synchronously and we may get an earlier timestamp for the later event. *Fix*: Just to use JVM_CurrentTimeMillis only as events timestamp. On Mac this is done in exactly the same way: CPlatformResponder.handleMouse/KeyEvent() The message time offset calculation has been introduced with the fix for JDK-4434193 https://bugs.openjdk.java.net/browse/JDK-4434193 and it seems that the issue has addressed very similar problem (At times getWhen()in ActionEvent returns higher value than the CurrentSystemTime) which has not been completely resolved in fact. I also didn't find any benefits in using the existing approach: - all the usages of the TimerHelper are in fact reduced to the mentioned formula: when
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Sergey, Thanks for the update. I'm fine with the fix, except one thing. (I'm sorry that I didn't say that earlier). My concern is that we have the LightweightContent iface which is used to communicate to the client app. And so the method LightweightFrame.getHostBounds() is better to be a method of that iface which the client (SwingNode) will implement on its side. In that case we won't need the LightweightFrame.setHostBounds() method. This would look consistent from my perspective. Thanks, Anton. On 22.05.2014 22:05, Sergey Bylokhov wrote: On 5/22/14 5:58 PM, Anton V. Tarasov wrote: On 22.05.2014 15:36, Sergey Bylokhov wrote: On 5/22/14 11:47 AM, Anton V. Tarasov wrote: Hi Sergey, On 22.05.2014 1:44, Sergey Bylokhov wrote: On 5/21/14 10:13 PM, Anthony Petrov wrote: Hi Sergey, The original fix provides some updates and clarifications to the javadoc for the LightweightContent.imageBufferReset() method, but they are missing from your fix. Is this intentional? Nope. I just missed this update. I looked at this method closely and got a question: do we need this scale parameter? Why we cannot use w,h + scanstride here an skip all clarification about logical coordinates? Originally, Jim suggested to generalize the API: Rather than imply any parameters, I think specifying a very exact set of parameters gives the most flexibility. Even if the relationships you characterize above are true, xywh,scan or off,wh,scan both provide the flexibility to supply the data in those formats without the client having to guess dimensions or scan size. Any API that specifies an array containing data should always provide the flexibility of specifying an offset (and x,y is a way of specifying an offset for rectangular data and using a nio Buffer can implicitly imply an offset based on its position) and when that data is a rectangle of data then it should also supply independent w,h and scan strides. If the offset is always 0, and if the scanstride is always w in the implementation's that choose the data storage then it may seem like overkill, but it provides the flexibility of switching to a more sophisticated buffer re-use strategy later without having to track down every client and update them... and so we provide all the coordinates. I understand why we need x/y/w/h/scanstride but why we need scale, because our buffer is pixel based anyway? In this case we have to convert w/h/x/y/scanstride from logical to pixels and back. The reasoning for that if the following. On the client side (SwingNode), during the rendering of the image, there's a need to have logical bounds of the image. So, this would require conversion (devision) for what the client would need to know the scale factor for what it would need to ask for it, separately. This would bring another code path dependencies (for instance, b/w SwingNode and its prism peer). Currently, there's only one parameter of a method for that purpose. Ok. Here is an updated version: http://cr.openjdk.java.net/~serb/8029455/webrev.02 Thanks, Anton. Thanks, Anton. BTW, I've applied your fix and tested it with the latest version of FX changes, and everything works smoothly on my Mac, including the display change listener. -- best regards, Anthony On 5/21/2014 7:46 PM, Sergey Bylokhov wrote: Hello, Everybody. Please review an updated version of this fix. This is a minimum possible fix. changes in shared code of jdk are minimized and can be enhanced in the future. The fix is covering hdpi support in SwingNode on osx + system look and feel(Aqua). http://cr.openjdk.java.net/~serb/8029455/webrev.01 Notes: - This fix depends from two other fixes: JDK- 8041129 and JDK-8041644. Both are under review on 2d alias. On 5/13/14 9:29 PM, Anthony Petrov wrote: Hi Jim, Sergey, and Anton, I'd like to revive this old thread and finally push this fix, which has been reviewed and approved on this mailing list back in February. The only additional change that I want to introduce, is the addition of default implementations for the LightweightContent.imageBufferReset() methods. This allows clients of the API (namely, JavaFX) to run with both the old and the new JDK w/o any changes or side-effects. Here's the updated webrev: http://cr.openjdk.java.net/~anthony/9-2-hiDPISwingNode-8029455.0/ It literally only adds the default methods and doesn't make any other changes to the rest of the already reviewed code. I've tested this on both hiDPI and loDPI displays, with both old and hiDPI-aware JavaFX builds, and it works fine in all the cases. The current plan is to push this fix to JDK 9, and then back-port the changes to 8u20. -- best regards, Anthony On 2/21/2014 2:47 AM, Jim Graham wrote: Yes, approved. ...jim On 2/17/14 6:09 AM, Anton V. Tarasov wrote: Jim, so this is ready for a push then. Thanks! Anton. On 15.02.2014 5:01, Jim Graham wrote: I don't need to see an update for that. I didn't read the entire webrev
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
On 23.05.2014 14:47, Anthony Petrov wrote:
Hi Anton,
I disagree, and here's my arguments:
1. The host bounds are not related to the /content/. Hence, adding this method to the
LightweightContent interface would look inconsistent from API perspective.
It's not strictly about content (the name of the interface is not good enough).
32 * The interface by means of which the {@link JLightweightFrame} class
33 * communicates to its client application.
2. Given the requirement to keep backward compatibility, the default implementation of the method
would return 'null', so the calling code would have to check the return value and fall back to
calling LF.getBounds() manually. Currently this logic is encapsulated in the LightweightFrame
class itself, which looks reasonable to me.
3. SwingNode already calls other APIs on LF, such as notifyDisplayChanged() (and again, this
particular notification is unrelated to the /content/ itself.) So adding the setHostBounds() to LF
looks consistent from this perspective, too.
4. The current implementation of the getHostBounds() method simply returns a new rectangles using
cached values. If we implement your suggestion, then every call to CPLWW.getGraphicsDevice() would
involve an additional call to the SwingNode code, which may impact the performance slightly.
5. I was almost ready to push the FX part of the fix today, and let's admit it, this fix is very
well overdue. I'd prefer if we don't modify the interface anymore.
Ok, this is about a matter of taste. The 5th point sounds strong enough for me, so I'm fine with the
current version.
Thanks!
Anton.
--
best regards,
Anthony
On 5/23/2014 2:11 PM, Anton V. Tarasov wrote:
Hi Sergey,
Thanks for the update. I'm fine with the fix, except one thing. (I'm
sorry that I didn't say that earlier).
My concern is that we have the LightweightContent iface which is used to
communicate to the client app. And so the method
LightweightFrame.getHostBounds()
is better to be a method of that iface which the client (SwingNode) will
implement on its side. In that case we won't need the
LightweightFrame.setHostBounds() method.
This would look consistent from my perspective.
Thanks,
Anton.
On 22.05.2014 22:05, Sergey Bylokhov wrote:
On 5/22/14 5:58 PM, Anton V. Tarasov wrote:
On 22.05.2014 15:36, Sergey Bylokhov wrote:
On 5/22/14 11:47 AM, Anton V. Tarasov wrote:
Hi Sergey,
On 22.05.2014 1:44, Sergey Bylokhov wrote:
On 5/21/14 10:13 PM, Anthony Petrov wrote:
Hi Sergey,
The original fix provides some updates and clarifications to the
javadoc for the LightweightContent.imageBufferReset() method, but
they are missing from your fix. Is this intentional?
Nope. I just missed this update. I looked at this method closely
and got a question: do we need this scale parameter? Why we cannot
use w,h + scanstride here an skip all clarification about logical
coordinates?
Originally, Jim suggested to generalize the API:
Rather than imply any parameters, I think specifying a very exact
set of parameters gives the most flexibility. Even if the
relationships you characterize above are true, xywh,scan or
off,wh,scan both provide the flexibility to supply the data in
those formats without the client having to guess dimensions or scan
size. Any API that specifies an array containing data should always
provide the flexibility of specifying an offset (and x,y is a way
of specifying an offset for rectangular data and using a nio Buffer
can implicitly imply an offset based on its position) and when that
data is a rectangle of data then it should also supply independent
w,h and scan strides. If the offset is always 0, and if the
scanstride is always w in the implementation's that choose the data
storage then it may seem like overkill, but it provides the
flexibility of switching to a more sophisticated buffer re-use
strategy later without having to track down every client and update
them...
and so we provide all the coordinates.
I understand why we need x/y/w/h/scanstride but why we need scale,
because our buffer is pixel based anyway? In this case we have to
convert w/h/x/y/scanstride from logical to pixels and back.
The reasoning for that if the following. On the client side
(SwingNode), during the rendering of the image, there's a need to
have logical bounds of the image. So, this would require conversion
(devision) for what the client would need to know the scale factor
for what it would need to ask for it, separately. This would bring
another code path dependencies (for instance, b/w SwingNode and its
prism peer). Currently, there's only one parameter of a method for
that purpose.
Ok. Here is an updated version:
http://cr.openjdk.java.net/~serb/8029455/webrev.02
Thanks,
Anton.
Thanks,
Anton.
BTW, I've applied your fix and tested it with the latest version
of FX changes, and everything works smoothly on my Mac, including
the display change listener.
--
best regards,
Anthony
On 5/21/2014 7:46
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Ok, good!
Anton.
On 23.05.2014 15:18, Anthony Petrov wrote:
On 5/23/2014 3:12 PM, Anton V. Tarasov wrote:
On 23.05.2014 14:47, Anthony Petrov wrote:
1. The host bounds are not related to the /content/. Hence, adding
this method to the LightweightContent interface would look
inconsistent from API perspective.
It's not strictly about content (the name of the interface is not good
enough).
32 * The interface by means of which the {@link JLightweightFrame}
class
33 * communicates to its client application.
Right. We might want to file a follow-up issue targeted for JDK/FX 9 and rename the
interface/rearrange some APIs. However, personally, I don't feel this is strictly necessary at
this time.
PS. I'll be pushing the FX part of the fix today. So we should consider the current interface
frozen for now.
--
best regards,
Anthony
2. Given the requirement to keep backward compatibility, the default
implementation of the method would return 'null', so the calling code
would have to check the return value and fall back to calling
LF.getBounds() manually. Currently this logic is encapsulated in the
LightweightFrame class itself, which looks reasonable to me.
3. SwingNode already calls other APIs on LF, such as
notifyDisplayChanged() (and again, this particular notification is
unrelated to the /content/ itself.) So adding the setHostBounds() to
LF looks consistent from this perspective, too.
4. The current implementation of the getHostBounds() method simply
returns a new rectangles using cached values. If we implement your
suggestion, then every call to CPLWW.getGraphicsDevice() would involve
an additional call to the SwingNode code, which may impact the
performance slightly.
5. I was almost ready to push the FX part of the fix today, and let's
admit it, this fix is very well overdue. I'd prefer if we don't modify
the interface anymore.
Ok, this is about a matter of taste. The 5th point sounds strong enough
for me, so I'm fine with the current version.
Thanks!
Anton.
--
best regards,
Anthony
On 5/23/2014 2:11 PM, Anton V. Tarasov wrote:
Hi Sergey,
Thanks for the update. I'm fine with the fix, except one thing. (I'm
sorry that I didn't say that earlier).
My concern is that we have the LightweightContent iface which is used to
communicate to the client app. And so the method
LightweightFrame.getHostBounds()
is better to be a method of that iface which the client (SwingNode) will
implement on its side. In that case we won't need the
LightweightFrame.setHostBounds() method.
This would look consistent from my perspective.
Thanks,
Anton.
On 22.05.2014 22:05, Sergey Bylokhov wrote:
On 5/22/14 5:58 PM, Anton V. Tarasov wrote:
On 22.05.2014 15:36, Sergey Bylokhov wrote:
On 5/22/14 11:47 AM, Anton V. Tarasov wrote:
Hi Sergey,
On 22.05.2014 1:44, Sergey Bylokhov wrote:
On 5/21/14 10:13 PM, Anthony Petrov wrote:
Hi Sergey,
The original fix provides some updates and clarifications to the
javadoc for the LightweightContent.imageBufferReset() method, but
they are missing from your fix. Is this intentional?
Nope. I just missed this update. I looked at this method closely
and got a question: do we need this scale parameter? Why we cannot
use w,h + scanstride here an skip all clarification about logical
coordinates?
Originally, Jim suggested to generalize the API:
Rather than imply any parameters, I think specifying a very exact
set of parameters gives the most flexibility. Even if the
relationships you characterize above are true, xywh,scan or
off,wh,scan both provide the flexibility to supply the data in
those formats without the client having to guess dimensions or scan
size. Any API that specifies an array containing data should always
provide the flexibility of specifying an offset (and x,y is a way
of specifying an offset for rectangular data and using a nio Buffer
can implicitly imply an offset based on its position) and when that
data is a rectangle of data then it should also supply independent
w,h and scan strides. If the offset is always 0, and if the
scanstride is always w in the implementation's that choose the data
storage then it may seem like overkill, but it provides the
flexibility of switching to a more sophisticated buffer re-use
strategy later without having to track down every client and update
them...
and so we provide all the coordinates.
I understand why we need x/y/w/h/scanstride but why we need scale,
because our buffer is pixel based anyway? In this case we have to
convert w/h/x/y/scanstride from logical to pixels and back.
The reasoning for that if the following. On the client side
(SwingNode), during the rendering of the image, there's a need to
have logical bounds of the image. So, this would require conversion
(devision) for what the client would need to know the scale factor
for what it would need to ask for it, separately. This would bring
another code path dependencies (for instance, b/w SwingNode and its
prism peer). Currently
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Sergey,
On 22.05.2014 1:44, Sergey Bylokhov wrote:
On 5/21/14 10:13 PM, Anthony Petrov wrote:
Hi Sergey,
The original fix provides some updates and clarifications to the javadoc for the
LightweightContent.imageBufferReset() method, but they are missing from your fix. Is this
intentional?
Nope. I just missed this update. I looked at this method closely and got a question: do we need
this scale parameter? Why we cannot use w,h + scanstride here an skip all clarification about
logical coordinates?
Originally, Jim suggested to generalize the API:
Rather than imply any parameters, I think specifying a very exact set of parameters gives the most
flexibility. Even if the relationships you characterize above are true, xywh,scan or off,wh,scan
both provide the flexibility to supply the data in those formats without the client having to guess
dimensions or scan size. Any API that specifies an array containing data should always provide the
flexibility of specifying an offset (and x,y is a way of specifying an offset for rectangular data
and using a nio Buffer can implicitly imply an offset based on its position) and when that data is a
rectangle of data then it should also supply independent w,h and scan strides. If the offset is
always 0, and if the scanstride is always w in the implementation's that choose the data storage
then it may seem like overkill, but it provides the flexibility of switching to a more sophisticated
buffer re-use strategy later without having to track down every client and update them...
and so we provide all the coordinates.
Thanks,
Anton.
BTW, I've applied your fix and tested it with the latest version of FX changes, and everything
works smoothly on my Mac, including the display change listener.
--
best regards,
Anthony
On 5/21/2014 7:46 PM, Sergey Bylokhov wrote:
Hello, Everybody.
Please review an updated version of this fix. This is a minimum possible
fix. changes in shared code of jdk are minimized and can be enhanced in
the future.
The fix is covering hdpi support in SwingNode on osx + system look and
feel(Aqua).
http://cr.openjdk.java.net/~serb/8029455/webrev.01
Notes:
- This fix depends from two other fixes: JDK- 8041129 and JDK-8041644.
Both are under review on 2d alias.
On 5/13/14 9:29 PM, Anthony Petrov wrote:
Hi Jim, Sergey, and Anton,
I'd like to revive this old thread and finally push this fix, which
has been reviewed and approved on this mailing list back in February.
The only additional change that I want to introduce, is the addition
of default implementations for the
LightweightContent.imageBufferReset() methods. This allows clients of
the API (namely, JavaFX) to run with both the old and the new JDK w/o
any changes or side-effects. Here's the updated webrev:
http://cr.openjdk.java.net/~anthony/9-2-hiDPISwingNode-8029455.0/
It literally only adds the default methods and doesn't make any other
changes to the rest of the already reviewed code. I've tested this on
both hiDPI and loDPI displays, with both old and hiDPI-aware JavaFX
builds, and it works fine in all the cases.
The current plan is to push this fix to JDK 9, and then back-port the
changes to 8u20.
--
best regards,
Anthony
On 2/21/2014 2:47 AM, Jim Graham wrote:
Yes, approved.
...jim
On 2/17/14 6:09 AM, Anton V. Tarasov wrote:
Jim, so this is ready for a push then.
Thanks!
Anton.
On 15.02.2014 5:01, Jim Graham wrote:
I don't need to see an update for that. I didn't read the entire
webrev, but I looked at this one piece of code and if that was the
only thing changed then I think that dealt with the outstanding
issues...
...jim
On 2/13/14 11:12 PM, Anton V. Tarasov wrote:
On 14.02.2014 2:52, Jim Graham wrote:
On 2/13/14 5:03 AM, Anton V. Tarasov wrote:
Hi Jim,
Please, look at the update:
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.5
Here I'm correcting the rect after the transform in SG2D:
2123 // In case of negative scale transform, reflect the
rect
coords.
2124 if (w 0) {
2125 w *= -1;
2126 x -= w;
2127 }
2128 if (h 0) {
2129 h *= -1;
2130 y -= h;
2131 }
The blit direction (dx, dy) remains transformed. Is this the right
behavior from your perspective?
Yes, that looks good. I wonder if the w *= -1 results in a
multiply
byte code whereas w = -w would avoid the multiply?
...jim
Jim,
Yes, this indeed results in different byte code instructions (imult
ineg). Just for curiosity I did some measuring which showed
negatioation
worked 10% faster :)
Well, I'll fix it but let me please not send an update...
Thanks!
Anton.
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
On 22.05.2014 15:36, Sergey Bylokhov wrote:
On 5/22/14 11:47 AM, Anton V. Tarasov wrote:
Hi Sergey,
On 22.05.2014 1:44, Sergey Bylokhov wrote:
On 5/21/14 10:13 PM, Anthony Petrov wrote:
Hi Sergey,
The original fix provides some updates and clarifications to the javadoc for the
LightweightContent.imageBufferReset() method, but they are missing from your fix. Is this
intentional?
Nope. I just missed this update. I looked at this method closely and got a question: do we need
this scale parameter? Why we cannot use w,h + scanstride here an skip all clarification about
logical coordinates?
Originally, Jim suggested to generalize the API:
Rather than imply any parameters, I think specifying a very exact set of parameters gives the
most flexibility. Even if the relationships you characterize above are true, xywh,scan or
off,wh,scan both provide the flexibility to supply the data in those formats without the client
having to guess dimensions or scan size. Any API that specifies an array containing data should
always provide the flexibility of specifying an offset (and x,y is a way of specifying an offset
for rectangular data and using a nio Buffer can implicitly imply an offset based on its position)
and when that data is a rectangle of data then it should also supply independent w,h and scan
strides. If the offset is always 0, and if the scanstride is always w in the implementation's
that choose the data storage then it may seem like overkill, but it provides the flexibility of
switching to a more sophisticated buffer re-use strategy later without having to track down every
client and update them...
and so we provide all the coordinates.
I understand why we need x/y/w/h/scanstride but why we need scale, because our buffer is pixel
based anyway? In this case we have to convert w/h/x/y/scanstride from logical to pixels and back.
The reasoning for that if the following. On the client side (SwingNode), during the rendering of the
image, there's a need to have logical bounds of the image. So, this would require conversion
(devision) for what the client would need to know the scale factor for what it would need to ask
for it, separately. This would bring another code path dependencies (for instance, b/w SwingNode
and its prism peer). Currently, there's only one parameter of a method for that purpose.
Thanks,
Anton.
Thanks,
Anton.
BTW, I've applied your fix and tested it with the latest version of FX changes, and everything
works smoothly on my Mac, including the display change listener.
--
best regards,
Anthony
On 5/21/2014 7:46 PM, Sergey Bylokhov wrote:
Hello, Everybody.
Please review an updated version of this fix. This is a minimum possible
fix. changes in shared code of jdk are minimized and can be enhanced in
the future.
The fix is covering hdpi support in SwingNode on osx + system look and
feel(Aqua).
http://cr.openjdk.java.net/~serb/8029455/webrev.01
Notes:
- This fix depends from two other fixes: JDK- 8041129 and JDK-8041644.
Both are under review on 2d alias.
On 5/13/14 9:29 PM, Anthony Petrov wrote:
Hi Jim, Sergey, and Anton,
I'd like to revive this old thread and finally push this fix, which
has been reviewed and approved on this mailing list back in February.
The only additional change that I want to introduce, is the addition
of default implementations for the
LightweightContent.imageBufferReset() methods. This allows clients of
the API (namely, JavaFX) to run with both the old and the new JDK w/o
any changes or side-effects. Here's the updated webrev:
http://cr.openjdk.java.net/~anthony/9-2-hiDPISwingNode-8029455.0/
It literally only adds the default methods and doesn't make any other
changes to the rest of the already reviewed code. I've tested this on
both hiDPI and loDPI displays, with both old and hiDPI-aware JavaFX
builds, and it works fine in all the cases.
The current plan is to push this fix to JDK 9, and then back-port the
changes to 8u20.
--
best regards,
Anthony
On 2/21/2014 2:47 AM, Jim Graham wrote:
Yes, approved.
...jim
On 2/17/14 6:09 AM, Anton V. Tarasov wrote:
Jim, so this is ready for a push then.
Thanks!
Anton.
On 15.02.2014 5:01, Jim Graham wrote:
I don't need to see an update for that. I didn't read the entire
webrev, but I looked at this one piece of code and if that was the
only thing changed then I think that dealt with the outstanding
issues...
...jim
On 2/13/14 11:12 PM, Anton V. Tarasov wrote:
On 14.02.2014 2:52, Jim Graham wrote:
On 2/13/14 5:03 AM, Anton V. Tarasov wrote:
Hi Jim,
Please, look at the update:
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.5
Here I'm correcting the rect after the transform in SG2D:
2123 // In case of negative scale transform, reflect the
rect
coords.
2124 if (w 0) {
2125 w *= -1;
2126 x -= w;
2127 }
2128 if (h 0) {
2129 h *= -1;
2130 y -= h;
2131
Re: AWT Dev [9] Review Request: JDK-8032872: [macosx] Cannot select from JComboBox in a JWindow
Hi Dmitry, Actually, I meant to add a _simple_ case to the test, or otherwise it becomes overly complicated. Sorry, if I didn't make that clear. So, the simple case is to have a hierarchy: frame - window-1 - window-2, where window-1 is grabbed. Then you click in window-2 and check if it doesn't cause ungrab event. Right? Thanks, Anton. On 05.03.2014 12:55, dmitry markov wrote: Hi Anton, I updated java/awt/Window/Grab/GrabTest.java as you requested. Please find new webrev here: http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.02/ Thanks, Dmitry On 04/03/2014 19:49, Anton V. Tarasov wrote: Hi Dmitry, The fix looks fine for me, but I'm still voting for adding this case to java/awt/Window/Grab/GrabTest.java Thanks, Anton. On 04.03.2014 17:26, dmitry markov wrote: Hello, Could you review the updated fix, please? webrev: http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.01/ - Fixed problem in changeFocusedWindow() - Added description for getOwnerFrameDialog() Thanks, Dmitry On 03/03/2014 20:19, Sergey Bylokhov wrote: On 3/3/14 6:15 PM, Anton V. Tarasov wrote: Hi all, The fix looks fine for me. The usage of getOwnerFrameDialog() in the mentioned cases indeed seems incorrect. I've looked at the rest of the code and found yet another incorrect usage, in LWWindowPeer.changeFocusedWindow line 1265. Please, fix it the same way. All the other use cases of the method should be fine as they relate to an activation (a simple window can't be an active window). Then it would be good to add appropriate javadoc to getOwnerFrameDialog to mention that it returns owner which can be activated(Frame or D ialog and not a WIndow). Also, I'd recommend to add a new testcase to java/awt/Window/Grab/GrabTest.java Thanks, Anton. On 03.03.2014 16:49, Sergey Bylokhov wrote: On 3/3/14 2:22 PM, dmitry markov wrote: If I get it right, getOwnerFrameDialog() is designed for another purpose. Also it is NOT only used in notifyMouseEvent() and notifyNCMouseDown(). Probably other places don't work also? I see that these usages are related to the focus staff. Anton do you know why LWWindowPeer.getOwnerFrameDialog() checks Frame and Dialog only? So I think we should stay this method as is to avoid any problems in future. It is really necessary to add new method isOneOfOwnersOf(). BTW this approach is already used on Windows platform, see awt_Window.cpp for details. Thanks, Dmitry On 03/03/2014 13:54, Petr Pchelko wrote: Hello, Dmitry. I've investigated a similar issue a while ago (https://bugs.openjdk.java.net/browse/JDK-8029686), could you please check if this issue is also resolved by your fix? In other words the current implementation assumes that the grabbingWindow must be an instance of Frame or Dialog and does not handle the case when the grabbingWindow is JWindow. When I was investigating this I did not understand why that was done that way. Do you know the reason? May be it's better not in introduce a new function but replace the getOwnerFrameDialog with your new implementation? Thank you. With best regards. Petr. On 03.03.2014, at 13:45, dmitry markov dmitry.mar...@oracle.com wrote: Hi Sergey, The current implementation of LWWindowPeer.getOwnerFrameDialog() may return an instance of Frame or Dialog. The returned value used to check whether the grabbingWindow is owner of mouse event target or not. If JComboBox is added to JFrame, the grabbingWindow is JFrame and getOwnerFrameDialog() returns the same JFrame object, (i.e. the check passes). If JCombobox is added to JWindow which is owned by JFrame, the grabbingWindow is JWindow but getOwnerFrameDialog() returns the JFrame, (i.e. the check fails). In other words the current implementation assumes that the grabbingWindow must be an instance of Frame or Dialog and does not handle the case when the grabbingWindow is JWindow. Thanks, Dmitry On 03/03/2014 13:01, Sergey Bylokhov wrote: Hi, Dmitry. Why the problem is reproduced in JWindow? Why it works in JFrame? On 3/3/14 10:40 AM, dmitry markov wrote: Hello, Could you review the fix for jdk9, please? bug: https://bugs.openjdk.java.net/browse/JDK-8032872 webrev: http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.00/ Problem description: It is impossible to make a selection in JComboBox added to JWindow via the mouse. The problem is caused by incorrect mouse events handling in LWWindowPeer class. When LWWindowPeer receives a mouse event intended for a popup window, it checks whether the current grabbingWindow is owner of the popup using getOwnerFrameDialog() method. This approach always fails for the JComboBox added to JWindow. As a result an UngrabEvent is send to the popup window. Fix: Introduce new private method LWWindowPeer.isOneOfOwnersOf(LWWindowPeer peer). The method will be invoked on grabbingWindow object to test whether it is owner of current mouse event target or not. The usage of getOwnerFrameDialog() should be replaced
Re: AWT Dev [9] Review Request: JDK-8032872: [macosx] Cannot select from JComboBox in a JWindow
Thanks, Dmitry! Looks great. Regards, Anton. On 05.03.2014 15:09, dmitry markov wrote: Anton, I updated the test. New version is located at http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.03/ Thanks, Dmitry On 05/03/2014 13:13, Anton V. Tarasov wrote: Hi Dmitry, Actually, I meant to add a _simple_ case to the test, or otherwise it becomes overly complicated. Sorry, if I didn't make that clear. So, the simple case is to have a hierarchy: frame - window-1 - window-2, where window-1 is grabbed. Then you click in window-2 and check if it doesn't cause ungrab event. Right? Thanks, Anton. On 05.03.2014 12:55, dmitry markov wrote: Hi Anton, I updated java/awt/Window/Grab/GrabTest.java as you requested. Please find new webrev here: http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.02/ Thanks, Dmitry On 04/03/2014 19:49, Anton V. Tarasov wrote: Hi Dmitry, The fix looks fine for me, but I'm still voting for adding this case to java/awt/Window/Grab/GrabTest.java Thanks, Anton. On 04.03.2014 17:26, dmitry markov wrote: Hello, Could you review the updated fix, please? webrev: http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.01/ - Fixed problem in changeFocusedWindow() - Added description for getOwnerFrameDialog() Thanks, Dmitry On 03/03/2014 20:19, Sergey Bylokhov wrote: On 3/3/14 6:15 PM, Anton V. Tarasov wrote: Hi all, The fix looks fine for me. The usage of getOwnerFrameDialog() in the mentioned cases indeed seems incorrect. I've looked at the rest of the code and found yet another incorrect usage, in LWWindowPeer.changeFocusedWindow line 1265. Please, fix it the same way. All the other use cases of the method should be fine as they relate to an activation (a simple window can't be an active window). Then it would be good to add appropriate javadoc to getOwnerFrameDialog to mention that it returns owner which can be activated(Frame or D ialog and not a WIndow). Also, I'd recommend to add a new testcase to java/awt/Window/Grab/GrabTest.java Thanks, Anton. On 03.03.2014 16:49, Sergey Bylokhov wrote: On 3/3/14 2:22 PM, dmitry markov wrote: If I get it right, getOwnerFrameDialog() is designed for another purpose. Also it is NOT only used in notifyMouseEvent() and notifyNCMouseDown(). Probably other places don't work also? I see that these usages are related to the focus staff. Anton do you know why LWWindowPeer.getOwnerFrameDialog() checks Frame and Dialog only? So I think we should stay this method as is to avoid any problems in future. It is really necessary to add new method isOneOfOwnersOf(). BTW this approach is already used on Windows platform, see awt_Window.cpp for details. Thanks, Dmitry On 03/03/2014 13:54, Petr Pchelko wrote: Hello, Dmitry. I've investigated a similar issue a while ago (https://bugs.openjdk.java.net/browse/JDK-8029686), could you please check if this issue is also resolved by your fix? In other words the current implementation assumes that the grabbingWindow must be an instance of Frame or Dialog and does not handle the case when the grabbingWindow is JWindow. When I was investigating this I did not understand why that was done that way. Do you know the reason? May be it's better not in introduce a new function but replace the getOwnerFrameDialog with your new implementation? Thank you. With best regards. Petr. On 03.03.2014, at 13:45, dmitry markov dmitry.mar...@oracle.com wrote: Hi Sergey, The current implementation of LWWindowPeer.getOwnerFrameDialog() may return an instance of Frame or Dialog. The returned value used to check whether the grabbingWindow is owner of mouse event target or not. If JComboBox is added to JFrame, the grabbingWindow is JFrame and getOwnerFrameDialog() returns the same JFrame object, (i.e. the check passes). If JCombobox is added to JWindow which is owned by JFrame, the grabbingWindow is JWindow but getOwnerFrameDialog() returns the JFrame, (i.e. the check fails). In other words the current implementation assumes that the grabbingWindow must be an instance of Frame or Dialog and does not handle the case when the grabbingWindow is JWindow. Thanks, Dmitry On 03/03/2014 13:01, Sergey Bylokhov wrote: Hi, Dmitry. Why the problem is reproduced in JWindow? Why it works in JFrame? On 3/3/14 10:40 AM, dmitry markov wrote: Hello, Could you review the fix for jdk9, please? bug: https://bugs.openjdk.java.net/browse/JDK-8032872 webrev: http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.00/ Problem description: It is impossible to make a selection in JComboBox added to JWindow via the mouse. The problem is caused by incorrect mouse events handling in LWWindowPeer class. When LWWindowPeer receives a mouse event intended for a popup window, it checks whether the current grabbingWindow is owner of the popup using getOwnerFrameDialog() method. This approach always fails for the JComboBox added to JWindow. As a result an UngrabEvent is send to the popup window
Re: AWT Dev [9] Review Request: JDK-8032872: [macosx] Cannot select from JComboBox in a JWindow
Hi Dmitry, The fix looks fine for me, but I'm still voting for adding this case to java/awt/Window/Grab/GrabTest.java Thanks, Anton. On 04.03.2014 17:26, dmitry markov wrote: Hello, Could you review the updated fix, please? webrev: http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.01/ - Fixed problem in changeFocusedWindow() - Added description for getOwnerFrameDialog() Thanks, Dmitry On 03/03/2014 20:19, Sergey Bylokhov wrote: On 3/3/14 6:15 PM, Anton V. Tarasov wrote: Hi all, The fix looks fine for me. The usage of getOwnerFrameDialog() in the mentioned cases indeed seems incorrect. I've looked at the rest of the code and found yet another incorrect usage, in LWWindowPeer.changeFocusedWindow line 1265. Please, fix it the same way. All the other use cases of the method should be fine as they relate to an activation (a simple window can't be an active window). Then it would be good to add appropriate javadoc to getOwnerFrameDialog to mention that it returns owner which can be activated(Frame or D ialog and not a WIndow). Also, I'd recommend to add a new testcase to java/awt/Window/Grab/GrabTest.java Thanks, Anton. On 03.03.2014 16:49, Sergey Bylokhov wrote: On 3/3/14 2:22 PM, dmitry markov wrote: If I get it right, getOwnerFrameDialog() is designed for another purpose. Also it is NOT only used in notifyMouseEvent() and notifyNCMouseDown(). Probably other places don't work also? I see that these usages are related to the focus staff. Anton do you know why LWWindowPeer.getOwnerFrameDialog() checks Frame and Dialog only? So I think we should stay this method as is to avoid any problems in future. It is really necessary to add new method isOneOfOwnersOf(). BTW this approach is already used on Windows platform, see awt_Window.cpp for details. Thanks, Dmitry On 03/03/2014 13:54, Petr Pchelko wrote: Hello, Dmitry. I've investigated a similar issue a while ago (https://bugs.openjdk.java.net/browse/JDK-8029686), could you please check if this issue is also resolved by your fix? In other words the current implementation assumes that the grabbingWindow must be an instance of Frame or Dialog and does not handle the case when the grabbingWindow is JWindow. When I was investigating this I did not understand why that was done that way. Do you know the reason? May be it's better not in introduce a new function but replace the getOwnerFrameDialog with your new implementation? Thank you. With best regards. Petr. On 03.03.2014, at 13:45, dmitry markov dmitry.mar...@oracle.com wrote: Hi Sergey, The current implementation of LWWindowPeer.getOwnerFrameDialog() may return an instance of Frame or Dialog. The returned value used to check whether the grabbingWindow is owner of mouse event target or not. If JComboBox is added to JFrame, the grabbingWindow is JFrame and getOwnerFrameDialog() returns the same JFrame object, (i.e. the check passes). If JCombobox is added to JWindow which is owned by JFrame, the grabbingWindow is JWindow but getOwnerFrameDialog() returns the JFrame, (i.e. the check fails). In other words the current implementation assumes that the grabbingWindow must be an instance of Frame or Dialog and does not handle the case when the grabbingWindow is JWindow. Thanks, Dmitry On 03/03/2014 13:01, Sergey Bylokhov wrote: Hi, Dmitry. Why the problem is reproduced in JWindow? Why it works in JFrame? On 3/3/14 10:40 AM, dmitry markov wrote: Hello, Could you review the fix for jdk9, please? bug: https://bugs.openjdk.java.net/browse/JDK-8032872 webrev: http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.00/ Problem description: It is impossible to make a selection in JComboBox added to JWindow via the mouse. The problem is caused by incorrect mouse events handling in LWWindowPeer class. When LWWindowPeer receives a mouse event intended for a popup window, it checks whether the current grabbingWindow is owner of the popup using getOwnerFrameDialog() method. This approach always fails for the JComboBox added to JWindow. As a result an UngrabEvent is send to the popup window. Fix: Introduce new private method LWWindowPeer.isOneOfOwnersOf(LWWindowPeer peer). The method will be invoked on grabbingWindow object to test whether it is owner of current mouse event target or not. The usage of getOwnerFrameDialog() should be replaced by isOneOfOwnersOf() in LWWindowPeer.notifyMouseEvent() and LWWindowPeer.NotifyNCMouseDown(). Thanks, Dmitry
Re: AWT Dev [9] Review Request: JDK-8032872: [macosx] Cannot select from JComboBox in a JWindow
Hi all, The fix looks fine for me. The usage of getOwnerFrameDialog() in the mentioned cases indeed seems incorrect. I've looked at the rest of the code and found yet another incorrect usage, in LWWindowPeer.changeFocusedWindow line 1265. Please, fix it the same way. All the other use cases of the method should be fine as they relate to an activation (a simple window can't be an active window). Also, I'd recommend to add a new testcase to java/awt/Window/Grab/GrabTest.java Thanks, Anton. On 03.03.2014 16:49, Sergey Bylokhov wrote: On 3/3/14 2:22 PM, dmitry markov wrote: If I get it right, getOwnerFrameDialog() is designed for another purpose. Also it is NOT only used in notifyMouseEvent() and notifyNCMouseDown(). Probably other places don't work also? I see that these usages are related to the focus staff. Anton do you know why LWWindowPeer.getOwnerFrameDialog() checks Frame and Dialog only? So I think we should stay this method as is to avoid any problems in future. It is really necessary to add new method isOneOfOwnersOf(). BTW this approach is already used on Windows platform, see awt_Window.cpp for details. Thanks, Dmitry On 03/03/2014 13:54, Petr Pchelko wrote: Hello, Dmitry. I've investigated a similar issue a while ago (https://bugs.openjdk.java.net/browse/JDK-8029686), could you please check if this issue is also resolved by your fix? In other words the current implementation assumes that the grabbingWindow must be an instance of Frame or Dialog and does not handle the case when the grabbingWindow is JWindow. When I was investigating this I did not understand why that was done that way. Do you know the reason? May be it's better not in introduce a new function but replace the getOwnerFrameDialog with your new implementation? Thank you. With best regards. Petr. On 03.03.2014, at 13:45, dmitry markov dmitry.mar...@oracle.com wrote: Hi Sergey, The current implementation of LWWindowPeer.getOwnerFrameDialog() may return an instance of Frame or Dialog. The returned value used to check whether the grabbingWindow is owner of mouse event target or not. If JComboBox is added to JFrame, the grabbingWindow is JFrame and getOwnerFrameDialog() returns the same JFrame object, (i.e. the check passes). If JCombobox is added to JWindow which is owned by JFrame, the grabbingWindow is JWindow but getOwnerFrameDialog() returns the JFrame, (i.e. the check fails). In other words the current implementation assumes that the grabbingWindow must be an instance of Frame or Dialog and does not handle the case when the grabbingWindow is JWindow. Thanks, Dmitry On 03/03/2014 13:01, Sergey Bylokhov wrote: Hi, Dmitry. Why the problem is reproduced in JWindow? Why it works in JFrame? On 3/3/14 10:40 AM, dmitry markov wrote: Hello, Could you review the fix for jdk9, please? bug: https://bugs.openjdk.java.net/browse/JDK-8032872 webrev: http://cr.openjdk.java.net/~dmarkov/8032872/jdk9/webrev.00/ Problem description: It is impossible to make a selection in JComboBox added to JWindow via the mouse. The problem is caused by incorrect mouse events handling in LWWindowPeer class. When LWWindowPeer receives a mouse event intended for a popup window, it checks whether the current grabbingWindow is owner of the popup using getOwnerFrameDialog() method. This approach always fails for the JComboBox added to JWindow. As a result an UngrabEvent is send to the popup window. Fix: Introduce new private method LWWindowPeer.isOneOfOwnersOf(LWWindowPeer peer). The method will be invoked on grabbingWindow object to test whether it is owner of current mouse event target or not. The usage of getOwnerFrameDialog() should be replaced by isOneOfOwnersOf() in LWWindowPeer.notifyMouseEvent() and LWWindowPeer.NotifyNCMouseDown(). Thanks, Dmitry
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Jim, so this is ready for a push then.
Thanks!
Anton.
On 15.02.2014 5:01, Jim Graham wrote:
I don't need to see an update for that. I didn't read the entire webrev, but I looked at this one
piece of code and if that was the only thing changed then I think that dealt with the outstanding
issues...
...jim
On 2/13/14 11:12 PM, Anton V. Tarasov wrote:
On 14.02.2014 2:52, Jim Graham wrote:
On 2/13/14 5:03 AM, Anton V. Tarasov wrote:
Hi Jim,
Please, look at the update:
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.5
Here I'm correcting the rect after the transform in SG2D:
2123 // In case of negative scale transform, reflect the rect
coords.
2124 if (w 0) {
2125 w *= -1;
2126 x -= w;
2127 }
2128 if (h 0) {
2129 h *= -1;
2130 y -= h;
2131 }
The blit direction (dx, dy) remains transformed. Is this the right
behavior from your perspective?
Yes, that looks good. I wonder if the w *= -1 results in a multiply
byte code whereas w = -w would avoid the multiply?
...jim
Jim,
Yes, this indeed results in different byte code instructions (imult
ineg). Just for curiosity I did some measuring which showed negatioation
worked 10% faster :)
Well, I'll fix it but let me please not send an update...
Thanks!
Anton.
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Jim,
Please, look at the update:
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.5
Here I'm correcting the rect after the transform in SG2D:
2123 // In case of negative scale transform, reflect the rect coords.
2124 if (w 0) {
2125 w *= -1;
2126 x -= w;
2127 }
2128 if (h 0) {
2129 h *= -1;
2130 y -= h;
2131 }
The blit direction (dx, dy) remains transformed. Is this the right behavior
from your perspective?
On 11.02.2014 23:45, Jim Graham wrote:
Hi Anton,
These comments are about future public API, but this current patch is about getting the mechanism
working behind the scenes. I'm OK with proceeding with the current patch as it is (modulo the
review feedback I gave) to get the mechanism working for the basic back buffer behind the scenes,
but we will eventually want the applications to be able to create their own HiDPI intermediate
buffers in addition to the back buffer that we manage for them - and these comments below are
about how we eventually expose this mechanism to them in a future stage...
Thanks for the clarification. (Please, see below.)
...jim
On 2/11/14 10:10 AM, Anton V. Tarasov wrote:
Hi Jim,
On 2/11/14 4:12 AM, Jim Graham wrote:
Just out of curiosity, on a Mac there is support for @2x images where
they get loaded and used (at half scale to preserve layout size)
automatically for you. In that respect, the added resolution is
hidden from the regular APIs and the developer doesn't really have to
deal with the meaning of size as it relates to HiDPI.
But, when you buy into HiDPI for your rendering, it looks like their
system requires you to ask them to calculate the proper extents for
the back buffer to render it and you are supposed to render it into
that rectangle (FX is calling convertRectToBacking and then using the
bounds to control the eventual blit of the back buffers).
If that is the case, then it looks like we have some precedence there
to have them buy into HiDPI backing stores or compatible images where
the images report their pixel sizes and they need to manage the
display size directly (i.e. by using drawImage(x,y,w,h) as we do
internally). I think we could make it a little more friendly than
their convertRectToBacking system, but it would mean we wouldn't
have to pollute the getWidth/Height APIs with conditional return values.
For example, if we added getLayoutWH() or getScaleFactor() to image or
bimg, then the normal ways of constructing those objects would simply
return objects where the answers were unscaled and unsurprising. If
they went out of their way to request one that was scaled, then those
new APIs (available on all images, but not very interesting except on
the specially constructed DPI-aware versions) would have new values to
help them manage the scaled image. Unaware code would simply see
these as overly large images, but it would be up to the developer to
manage hiding any HiDPI images from any code that they had not
converted to be DPI aware (just as we are doing here with our internal
Swing buffer).
So, HiDPI BI won't be backward compatible (unlike VolatileImage's) in terms of
behavior?
Thanks,
Anton.
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
On 14.02.2014 2:52, Jim Graham wrote:
On 2/13/14 5:03 AM, Anton V. Tarasov wrote:
Hi Jim,
Please, look at the update:
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.5
Here I'm correcting the rect after the transform in SG2D:
2123 // In case of negative scale transform, reflect the rect
coords.
2124 if (w 0) {
2125 w *= -1;
2126 x -= w;
2127 }
2128 if (h 0) {
2129 h *= -1;
2130 y -= h;
2131 }
The blit direction (dx, dy) remains transformed. Is this the right
behavior from your perspective?
Yes, that looks good. I wonder if the w *= -1 results in a multiply byte code whereas w = -w
would avoid the multiply?
...jim
Jim,
Yes, this indeed results in different byte code instructions (imult ineg). Just for curiosity I
did some measuring which showed negatioation worked 10% faster :)
Well, I'll fix it but let me please not send an update...
Thanks!
Anton.
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Jim, On 2/11/14 4:12 AM, Jim Graham wrote: Just out of curiosity, on a Mac there is support for @2x images where they get loaded and used (at half scale to preserve layout size) automatically for you. In that respect, the added resolution is hidden from the regular APIs and the developer doesn't really have to deal with the meaning of size as it relates to HiDPI. But, when you buy into HiDPI for your rendering, it looks like their system requires you to ask them to calculate the proper extents for the back buffer to render it and you are supposed to render it into that rectangle (FX is calling convertRectToBacking and then using the bounds to control the eventual blit of the back buffers). If that is the case, then it looks like we have some precedence there to have them buy into HiDPI backing stores or compatible images where the images report their pixel sizes and they need to manage the display size directly (i.e. by using drawImage(x,y,w,h) as we do internally). I think we could make it a little more friendly than their convertRectToBacking system, but it would mean we wouldn't have to pollute the getWidth/Height APIs with conditional return values. For example, if we added getLayoutWH() or getScaleFactor() to image or bimg, then the normal ways of constructing those objects would simply return objects where the answers were unscaled and unsurprising. If they went out of their way to request one that was scaled, then those new APIs (available on all images, but not very interesting except on the specially constructed DPI-aware versions) would have new values to help them manage the scaled image. Unaware code would simply see these as overly large images, but it would be up to the developer to manage hiding any HiDPI images from any code that they had not converted to be DPI aware (just as we are doing here with our internal Swing buffer). Ok, you're still against those conditional return values :) I already tried to convey my thoughts describing the reason why I couldn't simply throw them away. But Ok, let's do it eventually, then look at the new version and judge it... One thing to keep in mind, though, is that Windows appears to allow non-integer scales so I think we should not assume integer scale factors in whatever new API we create... I just sticked to the type of the scale factor returned by SurfaceData.getDefaultScale() which was int. Thanks, Anton. ...jim On 2/10/14 3:37 PM, Jim Graham wrote: On 2/10/14 6:11 AM, Anton V. Tarasov wrote: On 2/3/14 6:36 AM, Anton V. Tarasov wrote: In SG2D, the drawHiDPIImage, for instance, makes a call to op.filter(img, null); where the img is expected to return its layout size. That's why I still prefer to use the setReturnLayoutSize switcher, in order not to go deep into the 2D rendering code, casting here and there to OffscreenImage and calling getLayoutWidth/Height. Would we expect one of these images to have the BufferedImageOp version of drawImage be used on it? (It could happen if we ever leak the object into developers hands, but I'm not sure if that can happen or not - I'm pretty sure we don't use those Ops internally ourselves, do we?) We don't use it internally. Originally, I had an option in the fix with which a developer could create a HiDPI BufferedImage. Then, I implemented the last SG2D.drawImage method which didn't have hidpi support, and created a 2D test for it. In the test I drew some 2D primitives into a HiDPI image, using a BufferedImageOp transform. So, I just tested the ability to use it externally. In the current version of the fix there's no option to get a HiDPI image from the outside, so this code is not really used. I can eliminate it if we think we don't need it in the nearest future. It might make sense to leave it in for now. I'm not happy with that design conceptually in the long term, but I don't think there is a 100% pure/simple/obvious solution to the issues we are facing and it's not really hurting anything in the short term... ...jim
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Jim! On 08.02.2014 4:56, Jim Graham wrote: Hi Anton, In CPlatformLWWindow.java, why does it have to search for the right device when it was created with/from a Window object that should already know the right device? As I wrote before, JLF doesn't have any platform window behind (it's a trully lightweight frame). That's why we can't simply get an associated device and need some heuristic... SG2D, line 2114 - I think TRANSFORM_SCALE allows negative scale factors so I think you need a little more protection from the transform call reversing the rectangle. Ok, I'll elaborate on it. Otherwise, I didn't spot any other issues... Glad to hear that :) On 2/3/14 6:36 AM, Anton V. Tarasov wrote: In SG2D, the drawHiDPIImage, for instance, makes a call to op.filter(img, null); where the img is expected to return its layout size. That's why I still prefer to use the setReturnLayoutSize switcher, in order not to go deep into the 2D rendering code, casting here and there to OffscreenImage and calling getLayoutWidth/Height. Would we expect one of these images to have the BufferedImageOp version of drawImage be used on it? (It could happen if we ever leak the object into developers hands, but I'm not sure if that can happen or not - I'm pretty sure we don't use those Ops internally ourselves, do we?) We don't use it internally. Originally, I had an option in the fix with which a developer could create a HiDPI BufferedImage. Then, I implemented the last SG2D.drawImage method which didn't have hidpi support, and created a 2D test for it. In the test I drew some 2D primitives into a HiDPI image, using a BufferedImageOp transform. So, I just tested the ability to use it externally. In the current version of the fix there's no option to get a HiDPI image from the outside, so this code is not really used. I can eliminate it if we think we don't need it in the nearest future. Thanks, Anton. ...jim
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Thanks for the review, Anthony! Regards, Anton. On 04.02.2014 22:55, Anthony Petrov wrote: Hi Anton, I skimmed through the code that I'm familiar with, and the changes look good to me. Someone from Swing and 2D should review their parts, too. -- best regards, Anthony On 2/3/2014 6:36 PM, Anton V. Tarasov wrote: Hi Jim, Please look at the updated version: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.4 On 01.02.2014 5:35, Jim Graham wrote: Hi Anton, On 1/31/14 6:37 AM, Anton V. Tarasov wrote: My understanding is that, unless the fix is absolutely irrelevant (whic I hope it isn't), we should integrate it into 9/8u20 to support SwingNode in its current implementation on Retina displays. What do you think? I agree with this sentiment. My suggestion for reducing its footprint aside (which it looks like you are investigating), the main thing I will be looking for is whether or not we return an object to a developer (i.e. it isn't just managed internally to our own code) where they can do instanceof BufferedImage and then see something odd coming from its width/height. It looks like if we simply use getLayoutWH() internally then they would never ever see anything odd from getWidthHeight() anyway. The only additional gotcha would be if they grab the image and render it directly and it comes out an unexpected size to them (because, for instance, they didn't check the layout dims like we do internally). That's a pretty minor issue, though, and I think we could live with it. I've refactored the fix with this concern in mind. Here is what I've done: - eliminated the new OffscreenHiDPIImage class (as you suggested) and put all of its scale logic into the OffscreenImage. - made the scaled OffscreenImage return the physical size (like an ordinary BufferredImage does) by default . - renamed the set/isHiDPIEnabled method to set/isReturnLayoutSize. - made the setReturnLayoutSize(true) be called internally (where we don't leak the OffscreenImage). In SG2D, the drawHiDPIImage, for instance, makes a call to op.filter(img, null); where the img is expected to return its layout size. That's why I still prefer to use the setReturnLayoutSize switcher, in order not to go deep into the 2D rendering code, casting here and there to OffscreenImage and calling getLayoutWidth/Height. For 9.0 perhaps we could add the LayoutWH() as new API on BufferedImage and then most of this would be public? I'd have to mull over if that makes sense and I'm not entirely sure of the naming yet... That's a good idea, however we need a 8u working solution as well... As to the naming, I'm ready for any suggestions. Thanks, Anton. ...jim
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Jim, On 31.01.2014 2:32, Jim Graham wrote: Hi Anton, I think I'm getting a little lost in all of the details that I'm only on the periphery of, but it sounds like if that had been the original performance we had seen then we might not have gone down the path of using/forcing SW/BufferedImage? This is a good point. However Sergey mentioned an issue in Netbeans, where switching (for some reason) a JViewport to a backingstore (a buffered image back buffer) mode leads to a drastic performance drop on OSX. I didn't yet have chances to get into the details, but this looks just like the issue in question. Anyway, in the 2D code there's an obvious bottleneck - reading a texture into SW surface with glReadPixels by scanlines. To be clear, this is embedding a Swing hierarchy into an FX scene? Right. I forget, is FX forced into SW mode when this is used? Or are we reading out the pixels in Swing only to put them back into a texture when they get to FX? It isn't forced. Yes, we put back SW pixels to a texture on the fx side. This is the original, probably straightforward simple, design of the swing/fx interop. I think I mentioned the RSL (Resource Sharing Layer) that Dmitri and Chris had created which allowed external libraries to get at the texture IDs used by Java2D hw pipes, and at one time we were using that to run FX hw acceleration, but I think once we went with our own toolkit (Glass), we had to own our own contexts and then I think RSL broke. I wonder if we can reintroduce something similar here, or are we using completely different (and incompatible) contexts now between FX and J2D? Unfortunately, I can't answer this question now. As I already wrote, we have in mind the unified rendering project, which is aimed at exactly this - exchanging pixels on the gl/d3d level. But so far we have the sw-based interop. And I'd like to undestand what I'm doing with the sw-based Retina support. The fix is ready. It has a couple of concerns, 1) your last suggestion to migrate the scale logic from OffscreenHiDPIImage to OffscreenImage (I'm working on it) 2) a correction to the device detection logic is required. My understanding is that, unless the fix is absolutely irrelevant (whic I hope it isn't), we should integrate it into 9/8u20 to support SwingNode in its current implementation on Retina displays. What do you think? Regards, Anton. ...jim On 1/28/14 7:35 AM, Anton V. Tarasov wrote: Hi Jim, What I have so far. I've implemented flipping of the image on the native side. I didn't find any means by wich GL allows me to flip an image once it has been rendered to a texture, unless I set up a transform matrix prior to the render (just like in J2D). So I did that with an SW image right after I fetched it from the texture. At first, this works just fine in terms of correctness of the picture I eventually see on the screen. At second, the perceived performance is quite not bad. So, the native flip appeared to work pretty fast. I tried to get some scores. In average, with volatile images it worked from 3 to 20 times slower than with buffered images. The worst result is with scrolling and this is perceptibly (when I scroll really fast up and down, I can see some delay, but it's subtle). All the other scenarios I tried (text rendering, 2D animations) performed visually really similar to the buffered case. At least, the results are far from what it was before, when it worked close to freeze. So, I would say it now looks acceptable from the first glance. What is interesting is that blitting a GL surface to an SW surface spends a good time around glReadPixels, up to 3 times greater. Probably, we can do something with it as well. Additionally, I ran the bouncing balls app. It works 20% faster with buffered images, but consumes 30% more CPU. So, in total, the volatile version wins here. And also. The issue with JViewPort (which is forced to a buffered image) still exists (however, I have a straightforward solution and looking for a better one). As well as the issue with Nimbus which creates buffers based on the topmost BufferedImage. (May I use a volatile image as the topmost buffer? Didn't try yet). My conclusion is that, with the improvemtn, at least, the user may consider the volatile mode as an alternative choice. What are your thoughts? Thanks, Anton. On 25.01.2014 5:50, Jim Graham wrote: Hi Anton, I think the main question is how fast is it compared to forcing a software buffer? It may be slower than a straight read, but is that slow enough that we need to use a sw buffer instead? ...jim On 1/24/14 6:46 AM, Anton V. Tarasov wrote: Hi Jim, As I wrote in RT-30035, I tried that on the java side (actually, I tried to emulate the set of operations needed to turn the image over). This increased the perf by ~10 times, however this was still ~10 slower than a simple read. But, I think I can try the following as well: 1) to do the turn
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
On 25.01.2014 5:48, Jim Graham wrote: Another idea is to simply have a layout size parameter (and scale maybe) on the OSI and by default it is the same as the Raster size, but then the cases where we know internally that we are going to be using the object for HiDPI we then set the layout size appropriately. I think there may be a lot less boolean-testing code overall with that scheme, and I'm not sure you need a new subclass with the appropriate defaults - you can just use the base OSI with new attributes...? Ok, I'll see how it looks like. Thanks, Anton. ...jim On 1/24/14 6:47 AM, Anton V. Tarasov wrote: Hi Anthony, I see your concern. I'll think of a better name. Thanks, Anton. On 24.01.2014 15:47, Anthony Petrov wrote: Hi Anton, I suggest to rename the OffscreenHiDPIImage.hidpiEnabled and its corresponding setter/getter to something like reportLayoutSize and add a good javadoc for it so that it's clear what this boolean flag does just by looking at its name. -- best regards, Anthony On 1/21/2014 5:29 PM, Anton V. Tarasov wrote: Hi all, Let me please resume the review process. With the new webrev: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.3 I'm addressing the last concern which was about leaking the internal OffscreenHiDPIImage to the public via RepaintManager.getOffscreenBuffer. The explanation will follow, but before that I'd like to share the info related to the volatile buffer performance issue (which I was talking about before). I did some investigations of the native side and figured out the real source of the performance drop. It's the code where glReadPixels is called to read _every_ scanline of the image. This is so because of the nature of the OGL coordinate space which is upside down comparing to the j2d space. Please find more details here: https://javafx-jira.kenai.com/browse/RT-30035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=380146. If I'm not mistaken, we can do nothing about it. So, Swing/Interop can't use a volatile image as a back buffer, and it should use a buffered image or no back buffer at all. (Otherwise, performance is not acceptable). Now, to the fix. What I did is I added a hidpiEnabled property to the OffscreenHiDPIImage class. When it's true (by default) the image returns its size in layout space (just like in the previous version), when it's false the image returns its size in physical space (just like an ordinary BufferedImage). In RepaintManager I set the image hidpi disabled, thus hiding its layout size from the developer . The property is taken into account in SunGraphics2D.isHiDPIImage(). Because an OffscreenHiDPIImage with hidpiEnabled==false is drawn as an ordinary image, I draw it via drawImage(img, x, y, width, height) in RepaintManager. Why I still use the OffscreenHiDPIImage class instead of a BufferedImage is because otherwise I'd have to do pretty the same coding, but it would be scattered. The class basically incapsulates the following logic: - Keeps layout size of the image. (Used in drawImage.) - Keeps the scale factor. (Used by SurfaceData/VolatileImage/GraphicsConfig.) - Overrides SurfaceData.getDefaultScale. The point is that I can't simply call Graphics2D.scale(s, s) as this won't work when Swing draws into the image. (SunGraphics2D asks SurfaceData for the scale). - Overrides VolatileImage to make it return a scaled BI as its backup. (Used by Nimbus.) - Overrides GraphicsConfiguration to let it access the BI and its scale factor. (Used by Nimbus. This could be implemented otherwise, but not much better). No more changes in the current version. I know there're some concerns related to detecting a device change, but let me address it after we come to agreement about the approach discussed above. Thanks, Anton. On 18.12.2013 5:03, Jim Graham wrote: Hi Anton, javax.swing.RepaintManager.getOffscreenBuffer is a public method that can now return one of the new HiDPI offscreen images which is a subclass of BufferedImage. This was what I was worried about in terms of one of these internal double buffers leaking to developer code. If they test the image using instanceof they will see that it is a BufferdImage and they may try to dig out the raster and get confused... ...jim On 12/17/13 10:21 AM, Anton V. Tarasov wrote: Hi all, Please look at the new version: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.2 It contains the following changes: - All the scale related stuff is moved to the new class: OffScreenHiDPIImage.java - JViewport and RepaintManager no longer cache buffers. - JLightweightFrame has new method: createHiDPIImage(w, h). - JViewport, RepaintManager and AbstractRegionPainter goes the new path to create a HiDPI buffered image. - A new internal property is added: swing.jlf.hidpiImageEnabled. False by default. It makes JLF.createImage(w, h) forward the call to JLF.createHiDPIImage(w, h). This can be used by a third party
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Jim, What I have so far. I've implemented flipping of the image on the native side. I didn't find any means by wich GL allows me to flip an image once it has been rendered to a texture, unless I set up a transform matrix prior to the render (just like in J2D). So I did that with an SW image right after I fetched it from the texture. At first, this works just fine in terms of correctness of the picture I eventually see on the screen. At second, the perceived performance is quite not bad. So, the native flip appeared to work pretty fast. I tried to get some scores. In average, with volatile images it worked from 3 to 20 times slower than with buffered images. The worst result is with scrolling and this is perceptibly (when I scroll really fast up and down, I can see some delay, but it's subtle). All the other scenarios I tried (text rendering, 2D animations) performed visually really similar to the buffered case. At least, the results are far from what it was before, when it worked close to freeze. So, I would say it now looks acceptable from the first glance. What is interesting is that blitting a GL surface to an SW surface spends a good time around glReadPixels, up to 3 times greater. Probably, we can do something with it as well. Additionally, I ran the bouncing balls app. It works 20% faster with buffered images, but consumes 30% more CPU. So, in total, the volatile version wins here. And also. The issue with JViewPort (which is forced to a buffered image) still exists (however, I have a straightforward solution and looking for a better one). As well as the issue with Nimbus which creates buffers based on the topmost BufferedImage. (May I use a volatile image as the topmost buffer? Didn't try yet). My conclusion is that, with the improvemtn, at least, the user may consider the volatile mode as an alternative choice. What are your thoughts? Thanks, Anton. On 25.01.2014 5:50, Jim Graham wrote: Hi Anton, I think the main question is how fast is it compared to forcing a software buffer? It may be slower than a straight read, but is that slow enough that we need to use a sw buffer instead? ...jim On 1/24/14 6:46 AM, Anton V. Tarasov wrote: Hi Jim, As I wrote in RT-30035, I tried that on the java side (actually, I tried to emulate the set of operations needed to turn the image over). This increased the perf by ~10 times, however this was still ~10 slower than a simple read. But, I think I can try the following as well: 1) to do the turn natively (not sure if it's much faster) 2) to look if OGL is able do the turn in vram. Thanks, Anton. On 24.01.2014 2:08, Jim Graham wrote: Hi Anton, Could the upside-down nature of the pixel readback be solved by reading the entire frame in a single operation and then swapping the pixels around in our own code? The memory movement may be faster than the overhead of H calls to glReadPixels. (In the long run, we might want to teach the rest of our code how to deal with upside-down image data as well and then we don't have to swap it...) ...jim On 1/21/14 5:29 AM, Anton V. Tarasov wrote: Hi all, Let me please resume the review process. With the new webrev: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.3 I'm addressing the last concern which was about leaking the internal OffscreenHiDPIImage to the public via RepaintManager.getOffscreenBuffer. The explanation will follow, but before that I'd like to share the info related to the volatile buffer performance issue (which I was talking about before). I did some investigations of the native side and figured out the real source of the performance drop. It's the code where glReadPixels is called to read _every_ scanline of the image. This is so because of the nature of the OGL coordinate space which is upside down comparing to the j2d space. Please find more details here: https://javafx-jira.kenai.com/browse/RT-30035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=380146. If I'm not mistaken, we can do nothing about it. So, Swing/Interop can't use a volatile image as a back buffer, and it should use a buffered image or no back buffer at all. (Otherwise, performance is not acceptable). Now, to the fix. What I did is I added a hidpiEnabled property to the OffscreenHiDPIImage class. When it's true (by default) the image returns its size in layout space (just like in the previous version), when it's false the image returns its size in physical space (just like an ordinary BufferedImage). In RepaintManager I set the image hidpi disabled, thus hiding its layout size from the developer . The property is taken into account in SunGraphics2D.isHiDPIImage(). Because an OffscreenHiDPIImage with hidpiEnabled==false is drawn as an ordinary image, I draw it via drawImage(img, x, y, width, height) in RepaintManager. Why I still use the OffscreenHiDPIImage class instead of a BufferedImage is because otherwise I'd have
Re: AWT Dev [OpenJDK 2D-Dev] [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Jim, As I wrote in RT-30035, I tried that on the java side (actually, I tried to emulate the set of operations needed to turn the image over). This increased the perf by ~10 times, however this was still ~10 slower than a simple read. But, I think I can try the following as well: 1) to do the turn natively (not sure if it's much faster) 2) to look if OGL is able do the turn in vram. Thanks, Anton. On 24.01.2014 2:08, Jim Graham wrote: Hi Anton, Could the upside-down nature of the pixel readback be solved by reading the entire frame in a single operation and then swapping the pixels around in our own code? The memory movement may be faster than the overhead of H calls to glReadPixels. (In the long run, we might want to teach the rest of our code how to deal with upside-down image data as well and then we don't have to swap it...) ...jim On 1/21/14 5:29 AM, Anton V. Tarasov wrote: Hi all, Let me please resume the review process. With the new webrev: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.3 I'm addressing the last concern which was about leaking the internal OffscreenHiDPIImage to the public via RepaintManager.getOffscreenBuffer. The explanation will follow, but before that I'd like to share the info related to the volatile buffer performance issue (which I was talking about before). I did some investigations of the native side and figured out the real source of the performance drop. It's the code where glReadPixels is called to read _every_ scanline of the image. This is so because of the nature of the OGL coordinate space which is upside down comparing to the j2d space. Please find more details here: https://javafx-jira.kenai.com/browse/RT-30035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=380146. If I'm not mistaken, we can do nothing about it. So, Swing/Interop can't use a volatile image as a back buffer, and it should use a buffered image or no back buffer at all. (Otherwise, performance is not acceptable). Now, to the fix. What I did is I added a hidpiEnabled property to the OffscreenHiDPIImage class. When it's true (by default) the image returns its size in layout space (just like in the previous version), when it's false the image returns its size in physical space (just like an ordinary BufferedImage). In RepaintManager I set the image hidpi disabled, thus hiding its layout size from the developer . The property is taken into account in SunGraphics2D.isHiDPIImage(). Because an OffscreenHiDPIImage with hidpiEnabled==false is drawn as an ordinary image, I draw it via drawImage(img, x, y, width, height) in RepaintManager. Why I still use the OffscreenHiDPIImage class instead of a BufferedImage is because otherwise I'd have to do pretty the same coding, but it would be scattered. The class basically incapsulates the following logic: - Keeps layout size of the image. (Used in drawImage.) - Keeps the scale factor. (Used by SurfaceData/VolatileImage/GraphicsConfig.) - Overrides SurfaceData.getDefaultScale. The point is that I can't simply call Graphics2D.scale(s, s) as this won't work when Swing draws into the image. (SunGraphics2D asks SurfaceData for the scale). - Overrides VolatileImage to make it return a scaled BI as its backup. (Used by Nimbus.) - Overrides GraphicsConfiguration to let it access the BI and its scale factor. (Used by Nimbus. This could be implemented otherwise, but not much better). No more changes in the current version. I know there're some concerns related to detecting a device change, but let me address it after we come to agreement about the approach discussed above. Thanks, Anton. On 18.12.2013 5:03, Jim Graham wrote: Hi Anton, javax.swing.RepaintManager.getOffscreenBuffer is a public method that can now return one of the new HiDPI offscreen images which is a subclass of BufferedImage. This was what I was worried about in terms of one of these internal double buffers leaking to developer code. If they test the image using instanceof they will see that it is a BufferdImage and they may try to dig out the raster and get confused... ...jim On 12/17/13 10:21 AM, Anton V. Tarasov wrote: Hi all, Please look at the new version: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.2 It contains the following changes: - All the scale related stuff is moved to the new class: OffScreenHiDPIImage.java - JViewport and RepaintManager no longer cache buffers. - JLightweightFrame has new method: createHiDPIImage(w, h). - JViewport, RepaintManager and AbstractRegionPainter goes the new path to create a HiDPI buffered image. - A new internal property is added: swing.jlf.hidpiImageEnabled. False by default. It makes JLF.createImage(w, h) forward the call to JLF.createHiDPIImage(w, h). This can be used by a third party code in case it creates a buffered image via Component.createImage(w, h) and uses the image so that it can benefit from being a HiDPI image on a Retina
Re: AWT Dev [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Anthony, I see your concern. I'll think of a better name. Thanks, Anton. On 24.01.2014 15:47, Anthony Petrov wrote: Hi Anton, I suggest to rename the OffscreenHiDPIImage.hidpiEnabled and its corresponding setter/getter to something like reportLayoutSize and add a good javadoc for it so that it's clear what this boolean flag does just by looking at its name. -- best regards, Anthony On 1/21/2014 5:29 PM, Anton V. Tarasov wrote: Hi all, Let me please resume the review process. With the new webrev: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.3 I'm addressing the last concern which was about leaking the internal OffscreenHiDPIImage to the public via RepaintManager.getOffscreenBuffer. The explanation will follow, but before that I'd like to share the info related to the volatile buffer performance issue (which I was talking about before). I did some investigations of the native side and figured out the real source of the performance drop. It's the code where glReadPixels is called to read _every_ scanline of the image. This is so because of the nature of the OGL coordinate space which is upside down comparing to the j2d space. Please find more details here: https://javafx-jira.kenai.com/browse/RT-30035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=380146. If I'm not mistaken, we can do nothing about it. So, Swing/Interop can't use a volatile image as a back buffer, and it should use a buffered image or no back buffer at all. (Otherwise, performance is not acceptable). Now, to the fix. What I did is I added a hidpiEnabled property to the OffscreenHiDPIImage class. When it's true (by default) the image returns its size in layout space (just like in the previous version), when it's false the image returns its size in physical space (just like an ordinary BufferedImage). In RepaintManager I set the image hidpi disabled, thus hiding its layout size from the developer . The property is taken into account in SunGraphics2D.isHiDPIImage(). Because an OffscreenHiDPIImage with hidpiEnabled==false is drawn as an ordinary image, I draw it via drawImage(img, x, y, width, height) in RepaintManager. Why I still use the OffscreenHiDPIImage class instead of a BufferedImage is because otherwise I'd have to do pretty the same coding, but it would be scattered. The class basically incapsulates the following logic: - Keeps layout size of the image. (Used in drawImage.) - Keeps the scale factor. (Used by SurfaceData/VolatileImage/GraphicsConfig.) - Overrides SurfaceData.getDefaultScale. The point is that I can't simply call Graphics2D.scale(s, s) as this won't work when Swing draws into the image. (SunGraphics2D asks SurfaceData for the scale). - Overrides VolatileImage to make it return a scaled BI as its backup. (Used by Nimbus.) - Overrides GraphicsConfiguration to let it access the BI and its scale factor. (Used by Nimbus. This could be implemented otherwise, but not much better). No more changes in the current version. I know there're some concerns related to detecting a device change, but let me address it after we come to agreement about the approach discussed above. Thanks, Anton. On 18.12.2013 5:03, Jim Graham wrote: Hi Anton, javax.swing.RepaintManager.getOffscreenBuffer is a public method that can now return one of the new HiDPI offscreen images which is a subclass of BufferedImage. This was what I was worried about in terms of one of these internal double buffers leaking to developer code. If they test the image using instanceof they will see that it is a BufferdImage and they may try to dig out the raster and get confused... ...jim On 12/17/13 10:21 AM, Anton V. Tarasov wrote: Hi all, Please look at the new version: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.2 It contains the following changes: - All the scale related stuff is moved to the new class: OffScreenHiDPIImage.java - JViewport and RepaintManager no longer cache buffers. - JLightweightFrame has new method: createHiDPIImage(w, h). - JViewport, RepaintManager and AbstractRegionPainter goes the new path to create a HiDPI buffered image. - A new internal property is added: swing.jlf.hidpiImageEnabled. False by default. It makes JLF.createImage(w, h) forward the call to JLF.createHiDPIImage(w, h). This can be used by a third party code in case it creates a buffered image via Component.createImage(w, h) and uses the image so that it can benefit from being a HiDPI image on a Retina display. For instance, SwingSet2 has an animating Bezier curve demo. Switching the property on makes the curve auto scale smoothly. Please, look at the screenshots: -- http://cr.openjdk.java.net/~ant/JDK-8029455/RoughtCurve.png -- http://cr.openjdk.java.net/~ant/JDK-8029455/SmoothCurve.png - SunGraphics2D now draws a HiDPI buffered image the same way it draws a VolatileImage. - I've removed the copyArea() method from the BufImgSurfaceData, and modified the original version
Re: AWT Dev [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi all, Let me please resume the review process. With the new webrev: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.3 I'm addressing the last concern which was about leaking the internal OffscreenHiDPIImage to the public via RepaintManager.getOffscreenBuffer. The explanation will follow, but before that I'd like to share the info related to the volatile buffer performance issue (which I was talking about before). I did some investigations of the native side and figured out the real source of the performance drop. It's the code where glReadPixels is called to read _every_ scanline of the image. This is so because of the nature of the OGL coordinate space which is upside down comparing to the j2d space. Please find more details here: https://javafx-jira.kenai.com/browse/RT-30035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=380146. If I'm not mistaken, we can do nothing about it. So, Swing/Interop can't use a volatile image as a back buffer, and it should use a buffered image or no back buffer at all. (Otherwise, performance is not acceptable). Now, to the fix. What I did is I added a hidpiEnabled property to the OffscreenHiDPIImage class. When it's true (by default) the image returns its size in layout space (just like in the previous version), when it's false the image returns its size in physical space (just like an ordinary BufferedImage). In RepaintManager I set the image hidpi disabled, thus hiding its layout size from the developer . The property is taken into account in SunGraphics2D.isHiDPIImage(). Because an OffscreenHiDPIImage with hidpiEnabled==false is drawn as an ordinary image, I draw it via drawImage(img, x, y, width, height) in RepaintManager. Why I still use the OffscreenHiDPIImage class instead of a BufferedImage is because otherwise I'd have to do pretty the same coding, but it would be scattered. The class basically incapsulates the following logic: - Keeps layout size of the image. (Used in drawImage.) - Keeps the scale factor. (Used by SurfaceData/VolatileImage/GraphicsConfig.) - Overrides SurfaceData.getDefaultScale. The point is that I can't simply call Graphics2D.scale(s, s) as this won't work when Swing draws into the image. (SunGraphics2D asks SurfaceData for the scale). - Overrides VolatileImage to make it return a scaled BI as its backup. (Used by Nimbus.) - Overrides GraphicsConfiguration to let it access the BI and its scale factor. (Used by Nimbus. This could be implemented otherwise, but not much better). No more changes in the current version. I know there're some concerns related to detecting a device change, but let me address it after we come to agreement about the approach discussed above. Thanks, Anton. On 18.12.2013 5:03, Jim Graham wrote: Hi Anton, javax.swing.RepaintManager.getOffscreenBuffer is a public method that can now return one of the new HiDPI offscreen images which is a subclass of BufferedImage. This was what I was worried about in terms of one of these internal double buffers leaking to developer code. If they test the image using instanceof they will see that it is a BufferdImage and they may try to dig out the raster and get confused... ...jim On 12/17/13 10:21 AM, Anton V. Tarasov wrote: Hi all, Please look at the new version: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.2 It contains the following changes: - All the scale related stuff is moved to the new class: OffScreenHiDPIImage.java - JViewport and RepaintManager no longer cache buffers. - JLightweightFrame has new method: createHiDPIImage(w, h). - JViewport, RepaintManager and AbstractRegionPainter goes the new path to create a HiDPI buffered image. - A new internal property is added: swing.jlf.hidpiImageEnabled. False by default. It makes JLF.createImage(w, h) forward the call to JLF.createHiDPIImage(w, h). This can be used by a third party code in case it creates a buffered image via Component.createImage(w, h) and uses the image so that it can benefit from being a HiDPI image on a Retina display. For instance, SwingSet2 has an animating Bezier curve demo. Switching the property on makes the curve auto scale smoothly. Please, look at the screenshots: -- http://cr.openjdk.java.net/~ant/JDK-8029455/RoughtCurve.png -- http://cr.openjdk.java.net/~ant/JDK-8029455/SmoothCurve.png - SunGraphics2D now draws a HiDPI buffered image the same way it draws a VolatileImage. - I've removed the copyArea() method from the BufImgSurfaceData, and modified the original version. The only question I have is: do I need to check for instanceof OffScreenHiDPIImage.SurfaceData in case when transformState == TRANSFORM_TRANSLATESCALE? If this method is invoked with some other SD, and the transform is SCALE, will it do the job with the coordinates conversion done? - I've left the new methods in FramePeer default... May yet we implement them in other peers when we really need
Re: AWT Dev [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
On 20.12.2013 3:24, Jim Graham wrote: I'm starting to get the full picture now. Certainly, if we could share bits via the FX/Swing bridge then we could avoid the BufferedImage in the middle. There used to be support in Scenario for the resource sharing layer that Dmitri and Chris put into Java2D to grab texture IDs directly from Java2D. Could that be re-leveraged more easily than fixing this bug? This will require rewriting the whole fx/swing interface which is currently based on exchanging an int array of pixels (see the sun.swing.LightweightContent interface). Along with the texture sharing implementation (which I can't estimate as I don't have any good experience in OpenGL/D3D stuff), this is not going to be easier than the current fix. I'm afraid this is much more complicated task... One question about your non-double-buffered experiment below. You said that it didn't have any noticeable affect on performance. Is that the same as using a VI and a VI-BI copy? Or is it the same as using a BI the whole way? I didn't understand which other scenario you were comparing its performance to. I compared two scenarious: 1) Swing uses a BufferedImage as its double buffer (volatile off). 2) Swing doesn't use a double buffer at all, in which case all the drawings happen directly to the root Graphics (except that Nimbus uses some intermediate BI buffers to draw ui elements). If we still find some value in using VI on the Swing side, then how does the performance of rendering VI-BI differ from a direct pixel readback on the VI? I would hope that they were the same, but perhaps we are being stupid in the VI-BI copy loops. If readback is faster, could we modify the bridge to do a more direct readback rather than a drawImage operation? Have you traced the code to see where the performance is going when we use the current VI-BI copies? Maybe we are just missing a more direct copy loop in our list of loops? I debugged the java side of the drawImage call. What I saw is that it went into OGLSurfaceToSwBlit.Blit method with src == CGLOffScreenSurfaceData, dst == BufImgSurfaceData. It then created an OGLRenderQueue, prepared an op buffer, and eventually flushed the queue. The flush procedure took that long. Thanks, Anton. ...jim On 12/19/13 4:52 AM, Anton V. Tarasov wrote: Hi Jim and all, Thinking of getting rid of the need to use BuffedImage at all for JLF, I'm facing the following obstacles: 1. We have to forse developers to use a buffered image as a double buffer (currently by means of setting the corresponding property), because of the performance issue I'd mentioned (https://javafx-jira.kenai.com/browse/RT-30035). I investigated it a little. My assumption was that the problem is in copying from a volatile image to a buffered image (which Swing performs to flush its double buffer to Graphics which is tight to the root BufferedImage, containing the pixels of the JLF'c content). And that's the case. Measurement showed that copying b/w volatile and buffered works up to 300 times slower than copying b/w buffered and buffered. (This is what I see on my Mac, where AWT uses OpenGL). It dropps performance drastically. As an alternative, we can switch off double buffering at all. I checked how it affects the performance (theoretically, it should increase it). I didn't see any noticable difference... (perhaps, that's because there're so much buffers b/w fx swing, that one buffer less doesn't change the picture radically). But switching off double buffering changes the default behavior. Theoretically it can surprise the code that relies on it. 2. JViewport, in the interop mode, is forsedly switched to BACKINGSTORE (that is a BufferedImage). (By the way, here we also change the default behavior). This probably will be resolved in the future. 3. nimbus.AbstractRegionPainter takes a GraphicsConfig from the Graphics (which is tight to the BufferedImage, the roor buffer for JLF) and creates an Image based on it. As the Graphics is derived from BI, the result is BI as well. So far, I don't know if we can change this behavior for the AbstractRegionPainter. We could originally create a VolatileImage (the root image for JLF), however with a volatile buffer we will face the same performance issue when we extract pixels from it. The unified rendering (exchanging bits on GPU) is cool idea, but no one started it yet. On 19.12.2013 2:02, Jim Graham wrote: Hi Anton, I don't know enough about the overall architecture yet to be too specific about possible solutions at this point. Here are some questions that I still don't know the answer to... - I'm assuming that Swing gets its back buffer from the getOffscreenBuffer call because that was what you modified to return a HiDPI image. When Swing calls it internally, does it ever leak that instance? Could it use a different API to get that back buffer so that the public API doesn't change? I'm afraid it can't. It calls
Re: AWT Dev [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
= backbuffer.getGraphics(); g.scale(scale, scale); comp.paint(g); // ... // this call forces the logical size of the image without any special // processing or instance recognition in SG2D screengraphics.drawImage(backbuffer, x, y, w, h, null); then we don't need any fancy wrappers or anything. This doesn't solve any manual double buffering that a developer would do, though, but it evades return values from public methods that have mismatched BufferedImage objects... This is possible for RepaintManager and JViewport, but looks not direct for nimbus.AbstractRegionPainter where the moment of image creation is quite distant from its actual painting (the logic is spread across multiple calls)... But I can look at it deeper. Thanks, Anton. ...jim On 12/18/13 1:25 AM, Anton V. Tarasov wrote: Hi Jim, Thanks for noticing (sorry, but I simply forgot to check we don't export the buffer...) What can we do about that? I have the following thoughts: 1) We can warn developers to be ready for a HiDPI raster when they call that method under the following conditions: 1) the interop mode, 2) MacOSX 3) a Retina display. 2) In case the method is called, we can create a shadow buffer and start to sync it with the main buffer. The main buffer will be scaled to the shadow buffer on every repaint cycle. 3) We can introduce an internal property which will switch on/off the 2nd scenario. For instance, a developer may ask for the buffer and don't bother about its hidpi raster. Yes, I understand the solutions are far from perfect, but please take into account, the interop is a special mode where we (and developers) should be ready for compromises... What do you think? Do you have any better solutions in mind? Thanks, Anton. On 18.12.2013 5:03, Jim Graham wrote: Hi Anton, javax.swing.RepaintManager.getOffscreenBuffer is a public method that can now return one of the new HiDPI offscreen images which is a subclass of BufferedImage. This was what I was worried about in terms of one of these internal double buffers leaking to developer code. If they test the image using instanceof they will see that it is a BufferdImage and they may try to dig out the raster and get confused... ...jim On 12/17/13 10:21 AM, Anton V. Tarasov wrote: Hi all, Please look at the new version: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.2 It contains the following changes: - All the scale related stuff is moved to the new class: OffScreenHiDPIImage.java - JViewport and RepaintManager no longer cache buffers. - JLightweightFrame has new method: createHiDPIImage(w, h). - JViewport, RepaintManager and AbstractRegionPainter goes the new path to create a HiDPI buffered image. - A new internal property is added: swing.jlf.hidpiImageEnabled. False by default. It makes JLF.createImage(w, h) forward the call to JLF.createHiDPIImage(w, h). This can be used by a third party code in case it creates a buffered image via Component.createImage(w, h) and uses the image so that it can benefit from being a HiDPI image on a Retina display. For instance, SwingSet2 has an animating Bezier curve demo. Switching the property on makes the curve auto scale smoothly. Please, look at the screenshots: -- http://cr.openjdk.java.net/~ant/JDK-8029455/RoughtCurve.png -- http://cr.openjdk.java.net/~ant/JDK-8029455/SmoothCurve.png - SunGraphics2D now draws a HiDPI buffered image the same way it draws a VolatileImage. - I've removed the copyArea() method from the BufImgSurfaceData, and modified the original version. The only question I have is: do I need to check for instanceof OffScreenHiDPIImage.SurfaceData in case when transformState == TRANSFORM_TRANSLATESCALE? If this method is invoked with some other SD, and the transform is SCALE, will it do the job with the coordinates conversion done? - I've left the new methods in FramePeer default... May yet we implement them in other peers when we really need it? - CPlatformLWWindow.getGraphicsDevice() checks for an intersection + scale. This heuristic actually may fail when a Window is moved b/w three or four displays so that it intersects them all at some time. JFX will set a new scale factor in between and AWT may pick up a wrong device. I don't know any simple solution for that. For two monitors this will work. Thanks, Anton.
Re: AWT Dev [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Sergey, On 17.12.2013 23:38, Sergey Bylokhov wrote: Hi, Anton. Since OffScreenHiDPIImage looks similar to VolatileImage. Why we cannot use VolatileImage inside Swing everywhere? How Swing can use a VolatileImage when swing.volatileImageBufferEnabled=false is set? Could you please clarify your question? Why there's the need to support that option is because of the SwingNode issue: https://javafx-jira.kenai.com/browse/RT-30035. What happens if the graphicsConfig for the particular offscreen image will be changed/remoed/disposed? I suppose Volatile should became invalid in this case. If you are asking about the OffScreenHiDPIImage.VolatileImage, then as I can see, we can rely on the VolatileSurfaceManager.validate(..) method. At the worst case, it will call the getBackupImage() which is overriden to return a new HiDPI buffer. Also, I can't see any difference b/w how it behaved before, except for the getBackupImage call... CPlatformLWWindow : Why did you check scale when you try to find a necessary GraphicsDevice? When a Window is crossing the borders of two screens, FX switches to a new screen somewhere between and reports the scale factor change. At that moment the getGraphicsDevice is called and it finds the Window intersecting both the screens. However, as we know the scale factor has changed (otherwise, the method wouldn't be called) then we know the two devices have different scales, so we pick up the right device by additionally comparing the scale. Why not use the one correct device where the peer is located? Probably this code can be moved to the PlatformWindow interface? There's no a platform window for JLF, as I said. In the future, when (and if) we solve the problem with modality/z-order, JLF will be able to get the host window ID. But now it can't... Also, why I think current approach is acceptable is because of the following: - the configuration with three or four displays, when they are connected so that a Window can cross all of them at a time is a rare case - it's still possible to move a Window to either of the screens only involving two of them when a border is crossed (as a workaround) - even if a wrong device is picked up, it will work as JLF only needs its scale factor (so, the impact of the wrong behaviour is zero) FramePeer: Do we realy need notifyScaleFactorChanged? Probably notification about replacing GC is better? In this case it should notify all listeners that GC was changed(as a result recreated all buffers). Volatile handle this automatically? But this still doesn't solve the problem I've described above... Until we have a platform Window ID. When (and if) we have it, the existing code can be easily adopted to it. The notifyScaleFactorChanged call will tell JLF that it should 1) just scale the buffer appropriately, or 2) ask for the the host window ID and match it to the device, thus pleasing the AWT machinary. The 2dn is just an implementation detail, which I think should not be exposed on the API level (via bringing there screen or device notions). I suppose you disable volatile buffer in repaint manager. Why? If you mean the property: swing.volatileImageBufferEnabled=false, then yes. The reason is the issue I've referred above: https://javafx-jira.kenai.com/browse/RT-30035. Note that we have a bug on Swing+retina+scroll, when we use volatiles as a buffer, I am not sure what we will get in case of BI. https://bugs.openjdk.java.net/browse/JDK-8029253 Does switching off volatile makes any difference? I suppose SwingNode will experience the same slowness with large text scrolling. However, currently with volatile on, it performs badly even with a simple scroll (and not only with scroll, but with text typing as well). With buffered images, the perceived performance is quite close to a standalone Swing. Thanks, Anton. On 17.12.2013 22:21, Anton V. Tarasov wrote: Hi all, Please look at the new version: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.2 It contains the following changes: - All the scale related stuff is moved to the new class: OffScreenHiDPIImage.java - JViewport and RepaintManager no longer cache buffers. - JLightweightFrame has new method: createHiDPIImage(w, h). - JViewport, RepaintManager and AbstractRegionPainter goes the new path to create a HiDPI buffered image. - A new internal property is added: swing.jlf.hidpiImageEnabled. False by default. It makes JLF.createImage(w, h) forward the call to JLF.createHiDPIImage(w, h). This can be used by a third party code in case it creates a buffered image via Component.createImage(w, h) and uses the image so that it can benefit from being a HiDPI image on a Retina display. For instance, SwingSet2 has an animating Bezier curve demo. Switching the property on makes the curve auto scale smoothly. Please, look at the screenshots: -- http://cr.openjdk.java.net/~ant/JDK-8029455/RoughtCurve.png -- http://cr.openjdk.java.net
Re: AWT Dev [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Hi Jim, Thanks for noticing (sorry, but I simply forgot to check we don't export the buffer...) What can we do about that? I have the following thoughts: 1) We can warn developers to be ready for a HiDPI raster when they call that method under the following conditions: 1) the interop mode, 2) MacOSX 3) a Retina display. 2) In case the method is called, we can create a shadow buffer and start to sync it with the main buffer. The main buffer will be scaled to the shadow buffer on every repaint cycle. 3) We can introduce an internal property which will switch on/off the 2nd scenario. For instance, a developer may ask for the buffer and don't bother about its hidpi raster. Yes, I understand the solutions are far from perfect, but please take into account, the interop is a special mode where we (and developers) should be ready for compromises... What do you think? Do you have any better solutions in mind? Thanks, Anton. On 18.12.2013 5:03, Jim Graham wrote: Hi Anton, javax.swing.RepaintManager.getOffscreenBuffer is a public method that can now return one of the new HiDPI offscreen images which is a subclass of BufferedImage. This was what I was worried about in terms of one of these internal double buffers leaking to developer code. If they test the image using instanceof they will see that it is a BufferdImage and they may try to dig out the raster and get confused... ...jim On 12/17/13 10:21 AM, Anton V. Tarasov wrote: Hi all, Please look at the new version: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.2 It contains the following changes: - All the scale related stuff is moved to the new class: OffScreenHiDPIImage.java - JViewport and RepaintManager no longer cache buffers. - JLightweightFrame has new method: createHiDPIImage(w, h). - JViewport, RepaintManager and AbstractRegionPainter goes the new path to create a HiDPI buffered image. - A new internal property is added: swing.jlf.hidpiImageEnabled. False by default. It makes JLF.createImage(w, h) forward the call to JLF.createHiDPIImage(w, h). This can be used by a third party code in case it creates a buffered image via Component.createImage(w, h) and uses the image so that it can benefit from being a HiDPI image on a Retina display. For instance, SwingSet2 has an animating Bezier curve demo. Switching the property on makes the curve auto scale smoothly. Please, look at the screenshots: -- http://cr.openjdk.java.net/~ant/JDK-8029455/RoughtCurve.png -- http://cr.openjdk.java.net/~ant/JDK-8029455/SmoothCurve.png - SunGraphics2D now draws a HiDPI buffered image the same way it draws a VolatileImage. - I've removed the copyArea() method from the BufImgSurfaceData, and modified the original version. The only question I have is: do I need to check for instanceof OffScreenHiDPIImage.SurfaceData in case when transformState == TRANSFORM_TRANSLATESCALE? If this method is invoked with some other SD, and the transform is SCALE, will it do the job with the coordinates conversion done? - I've left the new methods in FramePeer default... May yet we implement them in other peers when we really need it? - CPlatformLWWindow.getGraphicsDevice() checks for an intersection + scale. This heuristic actually may fail when a Window is moved b/w three or four displays so that it intersects them all at some time. JFX will set a new scale factor in between and AWT may pick up a wrong device. I don't know any simple solution for that. For two monitors this will work. Thanks, Anton.
Re: AWT Dev [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
On 18.12.2013 13:25, Anton V. Tarasov wrote: Hi Jim, Thanks for noticing (sorry, but I simply forgot to check we don't export the buffer...) What can we do about that? I have the following thoughts: 1) We can warn developers to be ready for a HiDPI raster when they call that method under the following conditions: 1) the interop mode, 2) MacOSX 3) a Retina display. 2) In case the method is called, we can create a shadow buffer and start to sync it with the main buffer. The main buffer will be scaled to the shadow buffer on every repaint cycle. Just wanted to add, that SunVolatileImage.getSnapshot does the same (it scales the volatile image to a buffer), however it doesn't need to keep it synchronised... Thanks, Anton. 3) We can introduce an internal property which will switch on/off the 2nd scenario. For instance, a developer may ask for the buffer and don't bother about its hidpi raster. Yes, I understand the solutions are far from perfect, but please take into account, the interop is a special mode where we (and developers) should be ready for compromises... What do you think? Do you have any better solutions in mind? Thanks, Anton. On 18.12.2013 5:03, Jim Graham wrote: Hi Anton, javax.swing.RepaintManager.getOffscreenBuffer is a public method that can now return one of the new HiDPI offscreen images which is a subclass of BufferedImage. This was what I was worried about in terms of one of these internal double buffers leaking to developer code. If they test the image using instanceof they will see that it is a BufferdImage and they may try to dig out the raster and get confused... ...jim On 12/17/13 10:21 AM, Anton V. Tarasov wrote: Hi all, Please look at the new version: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.2 It contains the following changes: - All the scale related stuff is moved to the new class: OffScreenHiDPIImage.java - JViewport and RepaintManager no longer cache buffers. - JLightweightFrame has new method: createHiDPIImage(w, h). - JViewport, RepaintManager and AbstractRegionPainter goes the new path to create a HiDPI buffered image. - A new internal property is added: swing.jlf.hidpiImageEnabled. False by default. It makes JLF.createImage(w, h) forward the call to JLF.createHiDPIImage(w, h). This can be used by a third party code in case it creates a buffered image via Component.createImage(w, h) and uses the image so that it can benefit from being a HiDPI image on a Retina display. For instance, SwingSet2 has an animating Bezier curve demo. Switching the property on makes the curve auto scale smoothly. Please, look at the screenshots: -- http://cr.openjdk.java.net/~ant/JDK-8029455/RoughtCurve.png -- http://cr.openjdk.java.net/~ant/JDK-8029455/SmoothCurve.png - SunGraphics2D now draws a HiDPI buffered image the same way it draws a VolatileImage. - I've removed the copyArea() method from the BufImgSurfaceData, and modified the original version. The only question I have is: do I need to check for instanceof OffScreenHiDPIImage.SurfaceData in case when transformState == TRANSFORM_TRANSLATESCALE? If this method is invoked with some other SD, and the transform is SCALE, will it do the job with the coordinates conversion done? - I've left the new methods in FramePeer default... May yet we implement them in other peers when we really need it? - CPlatformLWWindow.getGraphicsDevice() checks for an intersection + scale. This heuristic actually may fail when a Window is moved b/w three or four displays so that it intersects them all at some time. JFX will set a new scale factor in between and AWT may pick up a wrong device. I don't know any simple solution for that. For two monitors this will work. Thanks, Anton.
Re: AWT Dev [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
The 4th option, previously suggested by Sergey, is to switch off double buffering at all. I'm investigating it. Thanks, Anton. On 12/18/13 1:25 PM, Anton V. Tarasov wrote: Hi Jim, Thanks for noticing (sorry, but I simply forgot to check we don't export the buffer...) What can we do about that? I have the following thoughts: 1) We can warn developers to be ready for a HiDPI raster when they call that method under the following conditions: 1) the interop mode, 2) MacOSX 3) a Retina display. 2) In case the method is called, we can create a shadow buffer and start to sync it with the main buffer. The main buffer will be scaled to the shadow buffer on every repaint cycle. 3) We can introduce an internal property which will switch on/off the 2nd scenario. For instance, a developer may ask for the buffer and don't bother about its hidpi raster. Yes, I understand the solutions are far from perfect, but please take into account, the interop is a special mode where we (and developers) should be ready for compromises... What do you think? Do you have any better solutions in mind? Thanks, Anton. On 18.12.2013 5:03, Jim Graham wrote: Hi Anton, javax.swing.RepaintManager.getOffscreenBuffer is a public method that can now return one of the new HiDPI offscreen images which is a subclass of BufferedImage. This was what I was worried about in terms of one of these internal double buffers leaking to developer code. If they test the image using instanceof they will see that it is a BufferdImage and they may try to dig out the raster and get confused... ...jim On 12/17/13 10:21 AM, Anton V. Tarasov wrote: Hi all, Please look at the new version: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.2 It contains the following changes: - All the scale related stuff is moved to the new class: OffScreenHiDPIImage.java - JViewport and RepaintManager no longer cache buffers. - JLightweightFrame has new method: createHiDPIImage(w, h). - JViewport, RepaintManager and AbstractRegionPainter goes the new path to create a HiDPI buffered image. - A new internal property is added: swing.jlf.hidpiImageEnabled. False by default. It makes JLF.createImage(w, h) forward the call to JLF.createHiDPIImage(w, h). This can be used by a third party code in case it creates a buffered image via Component.createImage(w, h) and uses the image so that it can benefit from being a HiDPI image on a Retina display. For instance, SwingSet2 has an animating Bezier curve demo. Switching the property on makes the curve auto scale smoothly. Please, look at the screenshots: -- http://cr.openjdk.java.net/~ant/JDK-8029455/RoughtCurve.png -- http://cr.openjdk.java.net/~ant/JDK-8029455/SmoothCurve.png - SunGraphics2D now draws a HiDPI buffered image the same way it draws a VolatileImage. - I've removed the copyArea() method from the BufImgSurfaceData, and modified the original version. The only question I have is: do I need to check for instanceof OffScreenHiDPIImage.SurfaceData in case when transformState == TRANSFORM_TRANSLATESCALE? If this method is invoked with some other SD, and the transform is SCALE, will it do the job with the coordinates conversion done? - I've left the new methods in FramePeer default... May yet we implement them in other peers when we really need it? - CPlatformLWWindow.getGraphicsDevice() checks for an intersection + scale. This heuristic actually may fail when a Window is moved b/w three or four displays so that it intersects them all at some time. JFX will set a new scale factor in between and AWT may pick up a wrong device. I don't know any simple solution for that. For two monitors this will work. Thanks, Anton.
Re: AWT Dev [9] Review Request: JDK-8023148 [macosx] java.util.NoSuchElementException at java.util.LinkedList.getFirst
Looks fine for me. Thanks, Anton. On 16.12.2013 13:56, Petr Pchelko wrote: Hello, AWT Team. Please review the fi for the issue: https://bugs.openjdk.java.net/browse/JDK-8023148 The fix is available at: http://cr.openjdk.java.net/~pchelko/9/8023148/webrev/ The problem is simple: LinkedList throws an exception on getFirst if the list is empty. The fix is trivial, so no reg test. With best regards. Petr.
Re: AWT Dev [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Summarizing your comments. We can't export a scaled version of a BufferedImage in the bounds of the current API without violating the spec. Unless a scaled BufferedImage is used internally, in which case we are less constrained. Ok, let's try this approach. I searched the code for all the cases of calling those factory methods I mentioned which create (or may create) a BufferedImage: Component.createImage(..), GraphicsConfiguration.createCompatibleImage(..), GraphicsConfiguration.createCompatibleVolatileImage(..). I found 19 cases, but only 3 of them matters (the cases in which JLF differs from standalone Swing). These are double buffers creation in JViewport and RepaintManager, and AbstractRegionPainter.getImage(). Plus 1 case when we create a root BufferedImage directly from JLightweightFrame. It's possible to replace them with internal versions of the factory methods which will return a scaled BufferedImage. Then, the suggestion to return layout bounds from a scaled BufferedImage, and physical bounds from its BufImgSurfaceData (we don't bother about getRGB for now) eliminates the need to translate to layout bounds in SG2D and unifies the code. So, I'm going this way... Thanks, Anton. On 12/13/13 2:54 AM, Jim Graham wrote: On 12/12/13 2:33 PM, Sergey Bylokhov wrote: On 12/12/13 11:27 PM, Jim Graham wrote: The only real difference here is that BufferedImages have multiple definitions of width and height. For VolatileImage objects there is no view of the pixels so the dimensions are the layout size and the expected renderable area and the returned graphics is pre-scaled. For BufferedImage we can do all of that, but the dimensions have the added implication that they define the valid range of values for getRGB(x, y) and grabbing the rasters/data buffers/arrays and digging out pixels manually. If it were just getRGB(x, y) we could simply do a 2x2 average of underlying pixels to return an RGB value, but I don't think there is any amount of workaround that we can apply to make the digging out of the rasters and storage arrays work for those who manually access pixels... :( But I am talking about OffScreenImage(or we can add new one), which is not public so we can try to block/change operations in our code. Not sure that our backbuffers leaked to the users. OffscreenImage is a subclass of BufferedImage so if a developer ever gets their hands on it then they may get confused by our use of the getWidth/Height. But, if there is no way for them to get a reference to it, then we can play games internally. This will not help us with the return value of getCompatibleImage(), though, which is specified to return a BufferedImage so we are somewhat restricted in any use of logical dimensions there. Also, if we are entirely managing the buffer internally, then we have the option to just use a regular BufferedImage. We don't need any extra magic if we render it with drawImage(x, y, w, h) since the logical or real dimensions of the image have no impact on the results there. If it is double-sized then those pixels will fit into the appropriate space on the destination without any need to special case them in SG2D... ...jim
Re: AWT Dev [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
On 12/13/13 2:05 AM, Anthony Petrov wrote: On 12/12/2013 07:18 PM, Anton V. Tarasov wrote: [cc'ing to j2d alias] On 11.12.2013 21:29, Sergey Bylokhov wrote: On 11.12.2013 20:23, Anton V. Tarasov wrote: - CGraphicsDevice This setter is only called from CPlatformLWView.getGraphicsDevice(). I've explained it in my previous message. It's needed to change the scale factor of the default device when no device in the list fits. The case is impossible with the current implementation of SwingNode (which only passes JLF a scale factor matching one of a real display), however, as JLF provides a generic lw embedding API, I should cover that case as well. Not sure that matching fx to awt devices via scale is not a good idea. Note that it is expected that fields in the CGraphicsDevice chenges only if the screen is changed/added/removed. Probably Instead of notifyScaleFactorChanged you can notify about screens changes? JLightweightFrame is a toplevel that paints its content to an off-screen buffer, so it is conceptually not associated with any screen. The host (SwingNode) application communicates with JLF on an API level. Introducing a notion of a screen to the API doesn't correlate with the JLF's concept, imho. Why? IMO, this would simplify the code significantly as Swing is already HiDPI-aware. It only needs to be able to determine the scale factor of a screen device its top-level is on. Of course, the code in the JLF implementation needs to know that too, so that it's able to create a BI of appropriate dimensions. So making JLF tracking its current GraphicsConfiguration looks like a reasonable idea to me. As Sergey suggested, this could be done easily by checking intersections between JLF's bounds in screen coordinates and the bounds of all the available GraphicsDevices. I'm not against the idea of using the right device internally (I just don't like the idea to add a notifyScreenChanged method). If this really can be implemented by means of the comparing the coordinates, then I'll do that. Thanks, Anton. -- best regards, Anthony Why I'm still picking the device is because this seemed to me an acceptable approach that integrates with LWAWT smoothly. But I agree with you that matching the device via a scale factor is not a good idea. Theoretically I can pickup wrong device, but even then it won't change anything for me. I just need a device with the requested scale. What do you think then if we always use default device, for which we will change the scale? - OffScreenImage I've put a BufferedImage accessor there, nothing else. I didn't find a better place... (I'd appreciate showing it). - JViewport, RepaintManager These classes create a double buffer. In case the buffer is backed by a BufferedImage, it will be created with the current scale factor set. The buffer won't be changed when a user moves the host window across multiple screens with different scales. I see two options. 1) Drop the double buffer reference every time the scale changes (in that case, the buffer will be recreated every time, I cross a screen) 2) Create a map which will cache the buffers (say, for 1 and 2 scale factors for double screen env). I think the second approach is better. Probably it will be better to disable doublebuffering and SwingPaintEventDispatcher completely(see swing.showFromDoubleBuffer)? Why? If we can manage it for JLF/SwingNode, why should we downgrade performance? You have 1 buffere on fx side, buffer in SwingNode, buffer in jviewport, and swing itself use double buffering. Ok, this is a good point. But still I can't simply switch off double buffering w/o doing any benchmarking. SwingNode perf analisys improvement is in plans... It would be good to know results of the benchmarks. Ok, but as this is a separate task I'd like to know what we're fighting for. Is the goal to avoid creating BufferedImage's at all? So far, unless it requires lots of coding (but it doesn't) I'd prefer to keep that option working. Actually I still do not understand why JViewport works in the standalone application. Could you please clarify, I don't understand this question... I see that JViewport use Offscreen image as a double buffer, is that true that it use it in the standalone swing application? If yes why it works. JViewport.paint() is not called with its default blit mode, and so it doesn't actually use an OffscreenBuffer. For JLF, the mode is set to backing store. If I set the backing store mode in standalone swing, JViewport stops scaling on Retina. So, it didn't work before. Is this related to the JDK-8023966? Right. Thanks, Anton. Thanks, Anton. Thanks for the review! Anton. On 10.12.2013 18:22, Anton V. Tarasov wrote: Hi Jim, Sergey and All, Please review the fix that adds support of Retina displays to JLightweightFrame (which javafx SwingNode is based on). webrev: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1 jira: https://bugs.openjdk.java.net
