Re: RFR: 8272756: Remove unnecessary explicit initialization of volatile variables in java.desktop
On Mon, 23 Aug 2021 17:57:52 GMT, Sergey Bylokhov wrote: >> Maybe it's kind of a protection from a race. Yet it's possible either way: >> another thread could see `essentialTags == null` before this one initialises >> the field. > > I think we can drop it completely. Agree. - PR: https://git.openjdk.java.net/jdk/pull/5197
Re: RFR: 8272756: Remove unnecessary explicit initialization of volatile variables in java.desktop
On Mon, 23 Aug 2021 06:35:34 GMT, Сергей Цыпанов
wrote:
>> src/java.desktop/share/classes/com/sun/imageio/plugins/tiff/TIFFIFD.java
>> line 64:
>>
>>> 62: Set tags = essentialTags;
>>> 63: if (tags == null) {
>>> 64: essentialTags = Set.of(
>>
>> What is the purpose of the local tags here?
>
> Looks like there's no purpose, `tags` is not used after assignment
Maybe it's kind of a protection from a race. Yet it's possible either way:
another thread could see `essentialTags == null` before this one initialises
the field.
-
PR: https://git.openjdk.java.net/jdk/pull/5197
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v7]
On Fri, 20 Aug 2021 15:10:41 GMT, lawrence.andrews wrote: >> 1) Automated the manual test case. >> 2) Removed html dependent file >> 3) Removed javax.swing.JApplet dependent. >> 4) Test case can be executed independently as well with jtreg framework. >> 5) Added methods to know that JFrame and Other component is visible before >> starting the user action via Robot. >> >> @shurymury > > lawrence.andrews has updated the pull request incrementally with one > additional commit since the last revision: > > Fixed the right word Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v6]
On Fri, 20 Aug 2021 14:50:51 GMT, lawrence.andrews
wrote:
>> 1) Automated the manual test case.
>> 2) Removed html dependent file
>> 3) Removed javax.swing.JApplet dependent.
>> 4) Test case can be executed independently as well with jtreg framework.
>> 5) Added methods to know that JFrame and Other component is visible before
>> starting the user action via Robot.
>>
>> @shurymury
>
> lawrence.andrews has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fixed review comments
test/jdk/java/awt/im/4959409/bug4959409.java line 87:
> 85: keyPressedEventLatch.countDown();
> 86: jLabel.setText("keyPressed receive for Shift+1");
> 87: System.out.println("keyPressed receive for
> Shift+1");
Here, it was correct: “received”.
-
PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v5]
On Thu, 19 Aug 2021 18:54:48 GMT, lawrence.andrews
wrote:
>> 1) Automated the manual test case.
>> 2) Removed html dependent file
>> 3) Removed javax.swing.JApplet dependent.
>> 4) Test case can be executed independently as well with jtreg framework.
>> 5) Added methods to know that JFrame and Other component is visible before
>> starting the user action via Robot.
>>
>> @shurymury
>
> lawrence.andrews has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Added WindowListener to check Frame is opened and simplified the testcase
Marked as reviewed by aivanov (Reviewer).
test/jdk/java/awt/im/4959409/bug4959409.java line 59:
> 57: private static JLabel jLabel;
> 58:
> 59: public static void createUIAndTest() throws InterruptedException,
> InvocationTargetException, AWTException {
`throws Exception` would be enough and shorter:
Suggestion:
public static void createUIAndTest() throws Exception {
It's a test code, any exception means the test fails, so we don't care much
about which specific exceptions can be thrown.
test/jdk/java/awt/im/4959409/bug4959409.java line 80:
> 78:
> 79: jTextField.addKeyListener(new KeyAdapter() {
> 80:
I'd remove this blank line for consistency with the anonymous class above.
test/jdk/java/awt/im/4959409/bug4959409.java line 92:
> 90: } else {
> 91: jLabel.setText("Did not received keyPressed for
> Shift+1");
> 92: System.out.println("Did not received keyPressed
> for Shift+1");
The grammar:
Suggestion:
jLabel.setText("Did not receive keyPressed for
Shift+1");
System.out.println("Did not receive keyPressed for
Shift+1");
test/jdk/java/awt/im/4959409/bug4959409.java line 120:
> 118: });
> 119:
> 120: clickTextField(robot, points[0].x + rect[0].width / 2,
> points[0].y + rect[0].height / 2);
I'd probably wrap this line, it doesn't fit even in 120 columns.
test/jdk/java/awt/im/4959409/bug4959409.java line 131:
> 129:
> 130: if (!keyPressedEventLatch.await(TIMEOUT, TimeUnit.SECONDS)) {
> 131: throw new RuntimeException("Did not received keyPressed for
> Shift + 1 , test failed");
The grammar:
Suggestion:
throw new RuntimeException("Did not receive keyPressed for Shift +
1 , test failed");
test/jdk/java/awt/im/4959409/bug4959409.java line 147:
> 145:
> 146: public static void main(String[] args) throws InterruptedException,
> InvocationTargetException,
> 147: AWTException {
Suggestion:
public static void main(String[] args) throws Exception {
Shorter.
-
PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v4]
On Tue, 17 Aug 2021 15:50:56 GMT, lawrence.andrews
wrote:
>> 1) Automated the manual test case.
>> 2) Removed html dependent file
>> 3) Removed javax.swing.JApplet dependent.
>> 4) Test case can be executed independently as well with jtreg framework.
>> 5) Added methods to know that JFrame and Other component is visible before
>> starting the user action via Robot.
>>
>> @shurymury
>
> lawrence.andrews has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fixed review comments
Hi Lawrence,
I played a bit with the test and it looks it can be simplified.
First of all, `jTextField.isVisible` is always `true`. So it makes no sense to
check its value.
As for the frame, you have `ComponentListener` which decreases the value of
`frameVisibleLatch`. Alternatively, you could use `WindowListener` and its
`windowOpened`. Once the frame is displayed, the text field is also displayed.
So it looks like the code could be simplified to the following:
SwingUtilities.invokeAndWait( /* create the frame and components */);
robot.waitForIdle();
if (!frameVisibleLatch.await(TIMEOUT, TimeUnit.SECONDS)) {
new RuntimeException("Frame is not visible after " + TIMEOUT + "
seconds");
}
robot.waitForIdle();
performMouseAction();
// And so on
What do you think?
I suggest renaming `performMouseAction` to something more specific and
descriptive, like `clickTextField` — this way the purpose of the action /
method is obvious without opening its code.
-
PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v4]
On Tue, 17 Aug 2021 15:50:56 GMT, lawrence.andrews
wrote:
>> 1) Automated the manual test case.
>> 2) Removed html dependent file
>> 3) Removed javax.swing.JApplet dependent.
>> 4) Test case can be executed independently as well with jtreg framework.
>> 5) Added methods to know that JFrame and Other component is visible before
>> starting the user action via Robot.
>>
>> @shurymury
>
> lawrence.andrews has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fixed review comments
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/im/4959409/bug4959409.java line 24:
> 22: */
> 23:
> 24: /**
I suggest to remove the second `*` to avoid the warning from the IDE about the
dangling Javadoc comment.
test/jdk/java/awt/im/4959409/bug4959409.java line 65:
> 63: final CountDownLatch keyPressedEventLatch = new CountDownLatch(1);
> 64: final Point points[] = new Point[1];
> 65: final Rectangle rect[] = new Rectangle[1];
Suggestion:
final Point[] points = new Point[1];
final Rectangle[] rect = new Rectangle[1];
The IDE issues a warning about C-style array declaration.
test/jdk/java/awt/im/4959409/bug4959409.java line 103:
> 101: } else {
> 102: jLabel.setText("Did not received KEY PRESS for
> Shift+1");
> 103: System.out.println("Did not received KEY PRESS
> for Shift+1");
Suggestion:
jLabel.setText("Did not receive KEY PRESS for Shift+1");
System.out.println("Did not receive KEY PRESS for
Shift+1");
Probably it makes sense to spell “KEYPRESS”, without the space, for consistency.
test/jdk/java/awt/im/4959409/bug4959409.java line 147:
> 145:
> 146: if (!keyPressedEventLatch.await(TIMEOUT, TimeUnit.SECONDS)) {
> 147: throw new RuntimeException("Did not received KEY PRESS for
> Shift + 1 , test failed");
Suggestion:
throw new RuntimeException("Did not receive KEY PRESS for Shift + 1
, test failed");
And the same for “KEY PRESS”.
test/jdk/java/awt/im/4959409/bug4959409.java line 162:
> 160: }
> 161:
> 162: public static void checkSwingTopLevelVisible(javax.swing.JFrame
> jFrame, CountDownLatch topLevelVisibleLatch) throws InterruptedException,
> InvocationTargetException {
Is there a reason why you use the fully qualified `javax.swing.JFrame` when
it's imported?
I suggest wrapping `throws` declaration to next line; this line is longer than
100.
Both comments apply to the method below.
test/jdk/java/awt/im/4959409/bug4959409.java line 170:
> 168: }
> 169:
> 170: public static boolean isTopLevelVisible(javax.swing.JFrame jFrame,
> CountDownLatch topLevelVisibleLatch) throws Exception {
The logic in this method polls `jFrame.isVisible` instead of waiting for it to
become visible. Nothing wrong with this approach yet the last call
`topLevelVisibleLatch.await(TIMEOUT, TimeUnit.SECONDS)` doesn't make sense in
this case as the latch can't reach 0 if it's not zero.
So, if something goes wrong, the test still waits for `TIMEOUT` (30) seconds
for the condition that can never be satisfied. I suggest returning
`topLevelVisibleLatch.getCount() == 0` and save 30 seconds.
Probably it's enough to check whether the text field is visible. The text field
can become visible only if frame is visible, both likely become visible at the
same moment anyway.
test/jdk/java/awt/im/4959409/bug4959409.java line 177:
> 175: TimeUnit.SECONDS.sleep(1);
> 176: checkSwingTopLevelVisible(jFrame, topLevelVisibleLatch);
> 177: if (topLevelVisibleLatch.getCount() == 0) {
Maybe this condition could be in the while loop?
while (count <= 5 && topLevelVisibleLatch.getCount() != 0)
Then there's no need for explicit premature break from the loop.
test/jdk/java/awt/im/4959409/bug4959409.java line 186:
> 184: }
> 185:
> 186: public static void checkJComponentVisible(javax.swing.JComponent
> jComponent, CountDownLatch componentVisibleLatch) throws
> InterruptedException, InvocationTargetException {
You're using fully qualified `javax.swing.JComponent` here: `JComponent` isn't
imported. Why not import it?
Please also wrap `throws` declaration to the next line.
test/jdk/java/awt/im/4959409/bug4959409.java line 216:
> 214: if (frame != null) {
> 215: SwingUtilities.invokeAndWait(frame::dispose);
> 216: }
This has inconsistency where `frame != null` is run on main thread without
synchronisation. The null check should also be run on the EDT as I originally
suggested.
-
PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8267161 : Write automated test case for JDK-4479161 [v3]
On Mon, 16 Aug 2021 20:05:58 GMT, lawrence.andrews
wrote:
>> 1) Automated the manual test case.
>> 2) Removed html dependent file
>> 3) Removed javax.swing.JApplet dependent.
>> 4) Test case can be executed independently as well with jtreg framework.
>> 5) Added methods to know that JFrame and Other component is visible before
>> starting the user action via Robot.
>>
>> @shurymury
>
> lawrence.andrews has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Fixed a single space issue
> - Add frame.setLocationRelativeTo to get the frame to center of the screen
test/jdk/java/awt/im/4959409/bug4959409.java line 47:
> 45: import java.awt.Robot;
> 46: import java.util.concurrent.CountDownLatch;
> 47: import java.util.concurrent.TimeUnit;
Usually in JDK, `javax.*` imports go after `java.*`.
test/jdk/java/awt/im/4959409/bug4959409.java line 124:
> 122: }
> 123:
> 124: Robot robot = new Robot();
Won't using `robot.setAutoDelay(DELAY)` give the same effect without the need
to interleave key presses and mouse moves with explicit delay?
test/jdk/java/awt/im/4959409/bug4959409.java line 144:
> 142: robot.delay(DELAY);
> 143: robot.keyRelease(KeyEvent.VK_1);
> 144: robot.delay(DELAY);
Shall we not release '1' first and then Shift?
Isn't it the case mentioned in #5079 ?
test/jdk/java/awt/im/4959409/bug4959409.java line 173:
> 171: while (count <= 5) {
> 172: TimeUnit.SECONDS.sleep(1);
> 173: if (component.isVisible()) {
Should `component.isVisible()` also be called on EDT?
test/jdk/java/awt/im/4959409/bug4959409.java line 198:
> 196: });
> 197: }
> 198: }
Probably this could be simplified to:
Suggestion:
try {
createUIAndTest();
} finally {
SwingUtilities.invokeAndWait(()-> {
if (frame != null) {
frame.dispose();
}
});
}
With the assumption `createUIAndTest()` is made static.
-
PR: https://git.openjdk.java.net/jdk/pull/5058
Re: RFR: 8272342: [TEST_BUG] java/awt/print/PrinterJob/PageDialogMarginTest.java catches all exceptions [v4]
On Mon, 16 Aug 2021 20:29:51 GMT, rajat mahajan wrote: >> Removed try catch block so the test can function as expected and fail >> when it ought to, rather than passing all the time because of catch >> block catching all exceptions and Passing test even there is a failure. > > rajat mahajan has updated the pull request incrementally with one additional > commit since the last revision: > > remove unneccessary imports. Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5115
Re: RFR: 8272342: [TEST_BUG] java/awt/print/PrinterJob/PageDialogMarginTest.java catches all exceptions [v2]
On Sat, 14 Aug 2021 01:15:45 GMT, rajat mahajan wrote: >> Removed try catch block so the test can function as expected and fail >> when it ought to, rather than passing all the time because of catch >> block catching all exceptions and Passing test even there is a failure. > > rajat mahajan has updated the pull request incrementally with one additional > commit since the last revision: > > Fix IDE warnings, added Default Locale as US, fixed Copyright to include > current year. Changes requested by aivanov (Reviewer). test/jdk/java/awt/print/PrinterJob/PageDialogMarginTest.java line 2: > 1: /* > 2: * Copyright (c) 2016, 2021, Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2016, 2021, Oracle and/or its affiliates. All rights reserved. Two many spaces, there should be one space after the comma. test/jdk/java/awt/print/PrinterJob/PageDialogMarginTest.java line 40: > 38: import javax.swing.JOptionPane; > 39: import javax.swing.SwingUtilities; > 40: import java.text.MessageFormat; I wonder where do DialogTypeSelection and MessageFormat come from? The imports should be sorted by default, usually your IDE handles it perfectly on its own. - PR: https://git.openjdk.java.net/jdk/pull/5115
Re: RFR: 8272342: [TEST_BUG] java/awt/print/PrinterJob/PageDialogMarginTest.java catches all exceptions
On Fri, 13 Aug 2021 19:23:21 GMT, rajat mahajan wrote: > Removed try catch block so the test can function as expected and fail > when it ought to, rather than passing all the time because of catch > block catching all exceptions and Passing test even there is a failure. Please add Locale.setDefault(Locale.US); as the first line in `main()`, otherwise the Page Format dialog uses millimetres instead of inches for non-US locales. Please resolve IDE warnings: 1. Use Java style array declaration in main: `String[] args`. 2. Line 51: Casting 'null' to 'Component' is redundant. 3. Line 50: Statement lambda can be replaced with expression lambda. Please update the copyright year. test/jdk/java/awt/print/PrinterJob/PageDialogMarginTest.java line 58: > 56: HashPrintRequestAttributeSet aset = new > HashPrintRequestAttributeSet(); > 57: PageFormat pf; > 58: pf = pj.pageDialog(aset); I suggest merging these two lines: Suggestion: PageFormat pf = pj.pageDialog(aset); - Changes requested by aivanov (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5115
Re: RFR: 8270116: Expand ButtonGroupLayoutTraversalTest.java to run in all LaFs, including Aqua on macOS [v2]
On Wed, 4 Aug 2021 21:43:53 GMT, rajat mahajan wrote: >> Summary: Expanded ButtonGroupLayoutTraversalTest.java to run in all LAFs on >> all OS. Added synchronization for focusCnt. > > rajat mahajan has updated the pull request incrementally with one additional > commit since the last revision: > > make variables with static final modifier CAPS, as per coding standard. Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4937
Re: RFR: 8270116: Expand ButtonGroupLayoutTraversalTest.java to run in all LaFs, including Aqua on macOS
On Wed, 4 Aug 2021 05:06:13 GMT, Prasanta Sadhukhan wrote: >> @prsadhuk I did what you asked, do you have any more questions or comments >> ?, if not could you please approve this PR, thanks. > > I still think nx, ny should be made CAPS...It seems to be the case for static > final constant variables in java/awt test folder...I don't think it will > increase noise as it will impact only in l57 It will affect more lines; these constants are used in for-loops to move focus between buttons as well as to validate the results. Let it be. - PR: https://git.openjdk.java.net/jdk/pull/4937
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v4]
On Tue, 3 Aug 2021 23:42:55 GMT, Sergey Bylokhov wrote:
>> This is a request to clean up a desktop module as was done in JDK-8233884
>> for "java.base" module.
>>
>> In many places standard charsets are looked up via their names, for example:
>> absolutePath.getBytes("UTF-8");
>>
>> This could be done more efficiently(x20 time faster) with use of
>> java.nio.charset.StandardCharsets:
>> absolutePath.getBytes(StandardCharsets.UTF_8);
>>
>> The later variant also makes the code cleaner, as it is known not to throw
>> UnsupportedEncodingException in contrary to the former variant.
>>
>> Tested by the desktop headless/headful tests on linux/windows.
>
> Sergey Bylokhov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update IPPPrintService.java
I admit I prefer static imports in this case: the code is shorter and is as
clear. It's pretty obvious where the encoding comes from.
-
Marked as reviewed by aivanov (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 22:03:54 GMT, Sergey Bylokhov wrote: > > I am fine to do that, if there are no objections I can change the whole fix. Modifying the entire changeset seems like an overkill. Using static imports in only one file is _inconsistent_, yet it makes the places where the encodings are used clearer. - PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 21:39:14 GMT, Sergey Bylokhov wrote: >> src/java.desktop/windows/classes/sun/awt/windows/WDataTransferer.java line >> 270: >> >>> 268: charset = new String((byte[])localeTransferable. >>> 269: getTransferData(javaTextEncodingFlavor), >>> 270: StandardCharsets.UTF_8); >> >> Suggestion: >> >> getTransferData(javaTextEncodingFlavor), >> StandardCharsets.UTF_8); >> >> The parameter on the second line should probably be aligned with the first >> parameter as it's done in the snippet above. > > it is aligned already, the StandardCharsets.UTF_8 is parameter of "new > String()", not the getTransferData. Ah, right! But it's confusing: it looks as if `StandardCharsets.UTF_8` is a parameter to `getTransferData`. Maybe avoid breaking the line and leave `UTF_8` on the same line? If you import `UTF_8` and `UTF_16LE` statically, line break is unnecessary. - PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]
On Tue, 3 Aug 2021 19:37:04 GMT, Sergey Bylokhov wrote:
>> This is a request to clean up a desktop module as was done in JDK-8233884
>> for "java.base" module.
>>
>> In many places standard charsets are looked up via their names, for example:
>> absolutePath.getBytes("UTF-8");
>>
>> This could be done more efficiently(x20 time faster) with use of
>> java.nio.charset.StandardCharsets:
>> absolutePath.getBytes(StandardCharsets.UTF_8);
>>
>> The later variant also makes the code cleaner, as it is known not to throw
>> UnsupportedEncodingException in contrary to the former variant.
>>
>> Tested by the desktop headless/headful tests on linux/windows.
>
> Sergey Bylokhov has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains two additional
> commits since the last revision:
>
> - Merge branch 'master' into JDK-8271456
> - Initial fix for JDK-8271456
Marked as reviewed by aivanov (Reviewer).
src/java.desktop/windows/classes/sun/awt/windows/WDataTransferer.java line 270:
> 268: charset = new String((byte[])localeTransferable.
> 269: getTransferData(javaTextEncodingFlavor),
> 270: StandardCharsets.UTF_8);
Suggestion:
getTransferData(javaTextEncodingFlavor),
StandardCharsets.UTF_8);
The parameter on the second line should probably be aligned with the first
parameter as it's done in the snippet above.
-
PR: https://git.openjdk.java.net/jdk/pull/4951
Re: RFR: 8270116: Expand ButtonGroupLayoutTraversalTest.java to run in all LaFs, including Aqua on macOS
On Thu, 29 Jul 2021 20:29:31 GMT, rajat mahajan wrote: > Summary: Expanded ButtonGroupLayoutTraversalTest.java to run in all LAFs on > all OS. Added synchronization for focusCnt. Looks good to me. - Marked as reviewed by aivanov (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4937
Re: RFR: 8269637: javax/swing/JFileChooser/FileSystemView/SystemIconTest.java fails on windows
On Wed, 14 Jul 2021 11:19:53 GMT, Alexander Zuev wrote: > Make fallback code for inaccessible file to return multiresolution icon > Remove test from ProblemList So, essentially all the icons returned are MultiResolutionIcon even if there's only one icon, right? - PR: https://git.openjdk.java.net/jdk/pull/4777
Re: RFR: 8182043: Access to Windows Large Icons [v14]
On Wed, 26 May 2021 17:40:44 GMT, Phil Race wrote:
>> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
>> line 300:
>>
>>> 298:
>>> 299: if(!f.exists()) {
>>> 300: return null;
>>
>> Shall it throw `FileNotFoundException` or `IllegalArgumentException` if the
>> file doesn't exist?
>> It could more convenient to return `null` rather than catch an exception.
>>
>> The space is missing between if and the opening parenthesis.
>
> It definitely should not be IAE. But FNFE is a reasonable idea.
> However it changes the usage since it is a checked exception.
> I'm on the fence and could go either way.
The older similar method, `getSystemIcon(File f)`, returns `null` if the file
doesn't exist. It could make sense to preserve the behaviour from this point of
view and not to make the user of the API handle FNFE; thus the new method could
easily be used instead.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 16:52:38 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with two additional > commits since the last revision: > > - Fixed small errors >Adjustments in the test > - Grammar fix in method documentation Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v14]
On Tue, 25 May 2021 23:36:43 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> null file now properly causes IllegalArgumentException
> Small fixed in JavaDoc
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
296:
> 294:
> 295: if (f == null){
> 296: throw new IllegalArgumentException("File reference should be
> specified");
Shall the exception message be more specific: "The file reference should not be
null"?
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
300:
> 298:
> 299: if(!f.exists()) {
> 300: return null;
Shall it throw `FileNotFoundException` or `IllegalArgumentException` if the
file doesn't exist?
It could more convenient to return `null` rather than catch an exception.
The space is missing between if and the opening parenthesis.
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 58:
> 56:
> 57: static void negativeTests() {
> 58: ImageIcon icon;
Can it be icon? This test doesn't use the fact that the returned object is
`ImageIcon`.
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 67:
> 65: if (!gotException) {
> 66: throw new RuntimeException("Negative size icon should throw
> invalid argument exception");
> 67: }
A suggestion: throw the exception inside try-block and ignore the expected
exception, then `gotException` can be dropped.
Suggestion:
try {
fsv.getSystemIcon(new File("."), -1, 16);
throw new RuntimeException("Negative size icon should throw invalid
argument exception");
} catch (IllegalArgumentException iae) {
// expected
}
Shall the test also exercise 0 as an invalid parameter? Shall the test also
pass an invalid height?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 18:16:36 GMT, Sergey Bylokhov wrote: >>> It is accessible from the "JFileChooser" drop-down menu. On my system, the >>> Libraries at that drop down menu contain "GIT", "Documents", "Music". >>> https://docs.microsoft.com/en-us/windows/client-management/windows-libraries >>> >>> There are also "known folders" such as "Network" or "My computer" which do >>> not have a real path but can be navigated in the JFileChooser and has an >>> icon. >> >> Ok, got it. Well, since they can be translated into the file system paths - >> even if these paths do not belong to physical filesystem - they are >> supported by the new API. Not for all of them i am able to receive the high >> resolution icons - like "Recent Items" for some reason only provides 32x32 >> and 16x16 no matter what size i am asking for. Others such as Documents, My >> Computer or 3D Objects do provide all resolutions available. For example >> here's the screenshot of all the available icons for 3D Objects >>  > > But how you got them via this method? I am not sure what parameters should be > passed to it. Didn't you answer your question already? If `FileSystemView.getRoots()` returns Desktop in Windows shell namespace. Otherwise, I don't know a way to get a reference to a *virtual* folder in Windows shell namespace which doesn't reference a file system object. Alex's example uses "3D Objects" folder which is one of the known folders and has its own icon instead of the standard folder icon. It's possible that I don't understand the question clearly. Alex's fix affects WindowsPlacesBar on the left of JFileChooser in Windows LaF, the icons there look crispier in High DPI environment. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Thu, 20 May 2021 17:37:20 GMT, Sergey Bylokhov wrote: >>> Are we sure that all possible paths can be pointed by the file object? >> >> We specify that we return the icon for a file. If path can not be resolved >> in the file object we can not return the icon for it. > > So the libraries are not supported? I doubt since JFileChooser should support > them, and supports of it is one of the reasons why we use shell folder to > iterate the folders and do not use the javaio. No, libraries are not supported. I see no contradiction here: `JFileChooser` uses Windows Shell API to enumerate objects and navigate the shell namespace. But it does not return non-file objects, does it? The new method accepts `File` object which implies it's a file system object rather than an arbitrary object in Windows Shell namespace which cannot be represented in Java. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 08:26:07 GMT, Alexander Zuev wrote: >> SurfaceData is not a public class, do we use this term somewhere in the >> spec? If not then it will be better to use size/points in the user space >> coordinate system, it is used already in the java2d. > > The CSR is already approved and changing specification at this point will > require to roll it back to draft and then going trough the approval process > again which in turn risks this change not to make it in the upcoming LTS > release. I would prefer to integrate it and create a follow-up bug to clarify > the wording where we can discuss the matter and - if it is worth it - to > submit a new CSR to amend the specification. If *user coordinate system* is used in similar context in other methods, we should change the wording to match it. I don't mind using a new bug to clarify *virtual pixels*. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v9]
On Tue, 18 May 2021 00:29:21 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed documentation based on CSR review feedback Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 14 May 2021 19:46:03 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Slight change of wording in javadoc > Fixed Win32ShellFolder2.getSystemIcon scaling issue Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 21:34:59 GMT, Alexander Zuev wrote:
>> No, it isn't wrapped: if `getShell32Icon` is called in `getIcon(final
>> boolean getLargeIcon)`, the returned value is immediately returned to the
>> caller, see lines 1157–1163 in the updated code:
>>
>> if (hIcon <= 0) {
>> if (isDirectory()) {
>> return getShell32Icon(FOLDER_ICON_ID, size);
>> } else {
>> return getShell32Icon(FILE_ICON_ID, size);
>> }
>> }
>>
>> It's not wrapped into multi-resolution icon when called from
>> `Win32ShellFolder2.get()` for keys `shell32Icon *` and `shell32LargeIcon *`
>> (lines 411/413–414).
>>
>> Neither is the returned value of `Win32ShellFolder2.getSystemIcon` wrapped;
>> it's also called from `Win32ShellFolder2.get()` when getting icons for
>> `optionPaneIcon *` (lines 405/407). These icons are supposed to be large
>> 32×32 icons, thus if the size of the icon in `getSystemIcon(SystemIcon
>> iconType)` differs from 32, it should be wrapped. Otherwise, it could cause
>> regression for
>> [JDK-8151385](https://bugs.openjdk.java.net/browse/JDK-8151385): _[hidpi]
>> JOptionPane-Icons only partially visible when using Windows 10 L_.
>
> I see - but still it has to be solved in the getShell32Icon method - which
> i'm going to do in a moment.
`Win32ShellFolder2.getSystemIcon` is still affected, the return value should be
wrapped into MR-icon if its size is not equal to `LARGE_ICON_SIZE`.
static Image getSystemIcon(SystemIcon iconType) {
long hIcon = getSystemIcon(iconType.getIconID());
Image icon = makeIcon(hIcon);
if (LARGE_ICON_SIZE != icon.getWidth(null)) {
icon = new MultiResolutionIconImage(size, icon);
}
disposeIcon(hIcon);
return icon;
}
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v6]
On Tue, 11 May 2021 21:41:15 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Make getShell32Icon method return multi-resolition image in case of
> requested icon size and actual icon size mismatch.
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
266:
> 264: * {@code ShellFolder} class. Whenever possible, the icon
> 265: * returned will be a multi-resolution icon image,
> 266: * which will allow better support for High DPI environments
I'm not sure but probably “…which allows better…” is more grammatical, as well
as in the line above: “the icon returned is a multi-resolution”.
Phil or Joe could correct this.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 19:36:12 GMT, Alexey Ivanov wrote: >> I will create a separate bug to track this - i do not know if library >> loading gets cached on some level and what impact on performance will we >> have if we release it every call. But i will check it out separately. > > Sure! I just wanted to bring it up. I could've submitted the bug myself… yet > I decided to ask at first. > As far as I can see `JDK_LoadSystemLibrary` has no caching, it just loads the > library. If any icons are accessed `shell32.dll` will already be loaded. > Probably, we never fully release it from COM. So in this case, loading and > freeing will just increase and decrease the counters. Even if it's never > really unloaded, we should release the resources that's not needed any more. For documentation purposes, [JDK-8266948](https://bugs.openjdk.java.net/browse/JDK-8266948): ShellFolder2::getIconResource does not release the library loaded. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 18:48:31 GMT, Alexander Zuev wrote:
>> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
>> 1196:
>>
>>> 1194: Image icon = makeIcon(hIcon);
>>> 1195: disposeIcon(hIcon);
>>> 1196: return icon;
>>
>> Shall it not be wrapped into multi-resolution image if the `size` is
>> different from the actual size of the icon?
>> Previously, it was inside `makeIcon`.
>
> Wherever it is necessary down the line we are wrapping the result in
> multi-resolution and if we wrap it here too we will have multi-resolution
> icon that contains multi-resolution images as a resolution variant.
> Unfortunately AWT can't handle painting of such abomination.
No, it isn't wrapped: if `getShell32Icon` is called in `getIcon(final boolean
getLargeIcon)`, the returned value is immediately returned to the caller, see
lines 1157–1163 in the updated code:
if (hIcon <= 0) {
if (isDirectory()) {
return getShell32Icon(FOLDER_ICON_ID, size);
} else {
return getShell32Icon(FILE_ICON_ID, size);
}
}
It's not wrapped into multi-resolution icon when called from
`Win32ShellFolder2.get()` for keys `shell32Icon *` and `shell32LargeIcon *`
(lines 411/413–414).
Neither is the returned value of `Win32ShellFolder2.getSystemIcon` wrapped;
it's also called from `Win32ShellFolder2.get()` when getting icons for
`optionPaneIcon *` (lines 405/407). These icons are supposed to be large 32×32
icons, thus if the size of the icon in `getSystemIcon(SystemIcon iconType)`
differs from 32, it should be wrapped. Otherwise, it could cause regression for
[JDK-8151385](https://bugs.openjdk.java.net/browse/JDK-8151385): _[hidpi]
JOptionPane-Icons only partially visible when using Windows 10 L_.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 18:50:13 GMT, Alexander Zuev wrote:
>> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
>> 1192:
>>
>>> 1190: */
>>> 1191: static Image getShell32Icon(int iconID, int size) {
>>> 1192: long hIcon = getIconResource("shell32.dll", iconID, size,
>>> size);
>>
>> It's outside the scope for this code review but still, should
>> `getIconResource` free the loaded library? With each call, the library gets
>> loaded but it's never freed and thus it's never unloaded even if it's not
>> needed any more.
>
> I will create a separate bug to track this - i do not know if library loading
> gets cached on some level and what impact on performance will we have if we
> release it every call. But i will check it out separately.
Sure! I just wanted to bring it up. I could've submitted the bug myself… yet I
decided to ask at first.
As far as I can see `JDK_LoadSystemLibrary` has no caching, it just loads the
library. If any icons are accessed `shell32.dll` will already be loaded.
Probably, we never fully release it from COM. So in this case, loading and
freeing will just increase and decrease the counters. Even if it's never really
unloaded, we should release the resources that's not needed any more.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Sat, 8 May 2021 00:19:21 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Small fixes
>Added testing for MultiResolutionImage
> - Merge remote-tracking branch 'origin/master' into JDK-8182043
> - Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp
>
>Select one icon at a time.
>
>Co-authored-by: Alexey Ivanov
> <70774172+aivanov-...@users.noreply.github.com>
> - Small fixes
>Remived size parameter from makeIcon
>Removed useVGAColors usage as irrelevant since it is ignored on all
>supported platforms now
> - 8182043: Access to Windows Large Icons
>Fix updated based on the previous review.
Changes requested by aivanov (Reviewer).
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1098:
> 1096: imageCache = getLargeIcon ?
> smallSystemImages : largeSystemImages;
> 1097: }
> 1098: newIcon2 =
> imageCache.get(Integer.valueOf(index));
Probably, explicit boxing is unnecessary.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1192:
> 1190: */
> 1191: static Image getShell32Icon(int iconID, int size) {
> 1192: long hIcon = getIconResource("shell32.dll", iconID, size, size);
It's outside the scope for this code review but still, should `getIconResource`
free the loaded library? With each call, the library gets loaded but it's never
freed and thus it's never unloaded even if it's not needed any more.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1196:
> 1194: Image icon = makeIcon(hIcon);
> 1195: disposeIcon(hIcon);
> 1196: return icon;
Shall it not be wrapped into multi-resolution image if the `size` is different
from the actual size of the icon?
Previously, it was inside `makeIcon`.
src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp line 929:
> 927: }
> 928: HRESULT hres;
> 929: IExtractIconW* pIcon;
`pIcon->Release()` must be called before returning as done in `extractIcon`.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v3]
On Fri, 7 May 2021 17:30:15 GMT, Alexander Zuev wrote: >> src/java.desktop/share/classes/sun/awt/shell/ShellFolder.java line 213: >> >>> 211: * Returns the icon of the specified size used to display this >>> shell folder. >>> 212: * >>> 213: * @param size size of the icon > 0 (Valid range: 1 to 256) >> >> I'm unsure the valid range of 1 to 256 makes sense provided the icon smaller >> than 16×16 is never returned (on Windows at least). > > Well, user still can request 1x1 icon - we will return the multiresolution > image with minimal (1x1) icon inside that will be scaled every time the > actual painting occurs. The size will define the layout of the component with > the icon and can be auto-generated from the user code so i do not see why we > should limit the lowest requested size - especially in the shared instance > that not only specific for Windows platform. Even though 1×1 icon doesn't make much sense, you're right imposing a limitation is no good either. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Sat, 8 May 2021 00:19:21 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Small fixes >Added testing for MultiResolutionImage > - Merge remote-tracking branch 'origin/master' into JDK-8182043 > - Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp > >Select one icon at a time. > >Co-authored-by: Alexey Ivanov > <70774172+aivanov-...@users.noreply.github.com> > - Small fixes >Remived size parameter from makeIcon >Removed useVGAColors usage as irrelevant since it is ignored on all >supported platforms now > - 8182043: Access to Windows Large Icons >Fix updated based on the previous review. src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 267: > 265: * returned will be a multi-resolution icon image, > 266: * which will allow better scaling with different > 267: * scaling factors. “…which will allow better support for High DPI environments with different scaling factors.” — does it look better? “scaling with … scaling factors” doesn't sound good to me. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v3]
On Wed, 10 Mar 2021 20:53:43 GMT, Alexey Ivanov wrote: >> Alexander Zuev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp >> >> Select one icon at a time. >> >> Co-authored-by: Alexey Ivanov >> <70774172+aivanov-...@users.noreply.github.com> > > src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line > 1146: > >> 1144: } >> 1145: Map multiResolutionIcon = new HashMap<>(); >> 1146: int start = size > MAX_QUALITY_ICON ? >> ICON_RESOLUTIONS.length - 1 : 0; > > Does it make sense to always start at zero? > The icons of smaller size will never be used, will they? > Thus it's safe to start at the index which corresponds to the requested size > if `size` matches, or the index such as `ICON_RESOLUTIONS[index] < size && > ICON_RESOLUTIONS[index + 1] > size`. This comment is also about the case of not fetching icons of sizes smaller than requested size. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v3]
On Wed, 10 Mar 2021 20:40:40 GMT, Alexey Ivanov wrote:
>> Alexander Zuev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp
>>
>> Select one icon at a time.
>>
>> Co-authored-by: Alexey Ivanov
>> <70774172+aivanov-...@users.noreply.github.com>
>
> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 64:
>
>> 62: }
>> 63:
>> 64: if (icon.getIconWidth() != size) {
>
> Does it make sense to check that the icon is a multi-resolution image with
> the expected sizes?
> We know for sure `explorer.exe` contains the entire set of icons.
Since the test always expects a multi-resolution image, does it make sense to
assert `icon instanceof MultiResolutionImage` at the very least?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v3]
On Thu, 29 Apr 2021 17:04:17 GMT, Alexander Zuev wrote: >> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line >> 1114: >> >>> 1112: bothIcons.put(getLargeIcon? >>> SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2); >>> 1113: newIcon = new >>> MultiResolutionIconImage(getLargeIcon ? LARGE_ICON_SIZE >>> 1114: : SMALL_ICON_SIZE, >>> bothIcons); >> >> I still propose to refactor this code to make it clearer. The code always >> returns two icons: _small + large_ or _large + small_ — combined in >> `MultiResolutionIconImage`. >> >> You can get `smallIcon` and `largeIcon` and then wrap them into >> `MultiResolutionIconImage` in the correct order and store each icon in the >> cache. >> >> My opinion hasn't changed here. >> >> I still think there's not much value in getting smaller icon size in >> addition to larger one. Or does it provide an alternative image for the case >> where the system scaling is 200% and the window is moved to a monitor with >> scaling of 100%? > > Getting smaller icon is relevant in the case of the scaling. I do not think > refactoring image caches from icons to multiresolution images will make code > much cleaner - at the end we will have to extract images from the > multiresolution image to repack them into different multiresolution image > because the base size of the image will not be the same and it does matter in > how they will be scaled on the different screens. And we still need to > extract both images because sometimes small resolution image looks not like > the large resolution one and for a reason - so small resolution image is not > blurred by the small detail on the large icon when scaling on the low > resolution screen. No, I can't see how it's relevant. If the scale factor is 100% and the requested icon size is 16, then 16×16 is used; if the window is moved to a monitor with scale factor 200%, then 32×32 icon is used for rendering which is already fetched and available in the multi-resolution image — perfect, there's the benefit. But when is it possible that the scale factor is less than 100%? If `JFileChooser` requests a large icon that is 32×32, then it will be used for rendering when the scale factor is 100%. When the scale factor is 200%, the icon of 64×64 is required but it's not available, instead there's 16×16 icon. For this icon to be used, the scale factor needs to be 50%. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v3]
On Fri, 30 Apr 2021 12:27:19 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp
>
> Select one icon at a time.
>
> Co-authored-by: Alexey Ivanov
> <70774172+aivanov-...@users.noreply.github.com>
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
267:
> 265: * returned will be a multi-resolution icon image,
> 266: * which will allow better scaling with different
> 267: * magnification factors.
I'm not sure what are the standard terms to refer to High DPI environments and
the scale factors associated with High DPI.
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
275:
> 273: *
> 274: *
> 275: * @param f a File object
Suggestion:
* @param f a {@code File} object
src/java.desktop/share/classes/sun/awt/shell/ShellFolder.java line 213:
> 211: * Returns the icon of the specified size used to display this shell
> folder.
> 212: *
> 213: * @param size size of the icon > 0 (Valid range: 1 to 256)
I'm unsure the valid range of 1 to 256 makes sense provided the icon smaller
than 16×16 is never returned (on Windows at least).
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1193:
> 1191: static Image getShell32Icon(int iconID, int size) {
> 1192: Toolkit toolkit = Toolkit.getDefaultToolkit();
> 1193: String shellIconBPP =
> (String)toolkit.getDesktopProperty("win.icon.shellIconBPP");
Looks like these lines aren't used any more and should be removed too.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v3]
On Thu, 29 Apr 2021 18:47:27 GMT, Alexander Zuev wrote: >> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line >> 1044: >> >>> 1042: new BufferedImage(iconSize, iconSize, >>> BufferedImage.TYPE_INT_ARGB); >>> 1043: img.setRGB(0, 0, iconSize, iconSize, iconBits, 0, >>> iconSize); >>> 1044: return img; >> >> There are cases where the size of the buffered image is different from the >> requested size. It could affect the layout because it breaks the assumption >> that the returned image has the requested size but it may be larger. (Or is >> it no longer possible?) I think it should be wrapped into >> `MultiResolutionIconImage` in this case. > > Actually in makeImage we do not use requested size, we return the bits that > system returns to us. How the generated image is treated depends on the > implementation of the public methods - where it matters they are wrapped in > the corresponding multi resolution images so it behaves correctly in UI. Okay, if it *always* wrapped in a multi-resolution image where it *matters*. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8198619: java/awt/Focus/FocusTraversalPolicy/ButtonGroupLayoutTraversal/ButtonGroupLayoutTraversalTest.java fails on mac [v3]
On Mon, 26 Apr 2021 07:44:37 GMT, Prasanta Sadhukhan wrote: >> This test was failing in CI nightly testing due to samevm mode issue. >> Modified test to use waitForIdle() judiciously, dispose frame in finally >> block and move the frame to center of screen. >> Several iterations of the test pass in all platforms. Link in JBS. > > Prasanta Sadhukhan has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains four commits: > > - Merge master > - Fix > - Fix > - 8198619: > java/awt/Focus/FocusTraversalPolicy/ButtonGroupLayoutTraversal/ButtonGroupLayoutTraversalTest.java > fails on mac Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3618
Re: RFR: 8197800: Test java/awt/Focus/NonFocusableWindowTest/NoEventsTest.java fails on Windows
On Thu, 22 Apr 2021 11:52:27 GMT, Prasanta Sadhukhan wrote: > This test rootcause which is > > sun.awt.SunToolkit$InfiniteLoop > at sun.awt.SunToolkit.realSync(SunToolkit.java:1498) > at sun.awt.SunToolkit.realSync(SunToolkit.java:1426) > at java.awt.Robot.waitForIdle(Robot.java:574) > at NoEventsTest.pause(NoEventsTest.java:50) > at NoEventsTest.performFocusClick(NoEventsTest.java:161) > at NoEventsTest.main(NoEventsTest.java:109) > > is found out to be same as with this bug JDK-8000171 via this comment > https://bugs.openjdk.java.net/browse/JDK-8000171?focusedCommentId=13825066=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13825066 > which is dup of 8196100: > javax/swing/text/JTextComponent/5074573/bug5074573.java fails and and fixed > there. > > So, we can remove the test from problemlist Looks good provided it's stable now. Could you please add the root cause into JBS? Otherwise it will look as if the test was removed from the problem list without any fix. [JDK-8196100](https://bugs.openjdk.java.net/browse/JDK-8196100) could be added as duplicate link in JBS and as relates to [JDK-8000171](https://bugs.openjdk.java.net/browse/JDK-8000171) - PR: https://git.openjdk.java.net/jdk/pull/3623
Re: RFR: 8197800: Test java/awt/Focus/NonFocusableWindowTest/NoEventsTest.java fails on Windows
On Thu, 22 Apr 2021 11:52:27 GMT, Prasanta Sadhukhan wrote: > This test rootcause which is > > sun.awt.SunToolkit$InfiniteLoop > at sun.awt.SunToolkit.realSync(SunToolkit.java:1498) > at sun.awt.SunToolkit.realSync(SunToolkit.java:1426) > at java.awt.Robot.waitForIdle(Robot.java:574) > at NoEventsTest.pause(NoEventsTest.java:50) > at NoEventsTest.performFocusClick(NoEventsTest.java:161) > at NoEventsTest.main(NoEventsTest.java:109) > > is found out to be same as with this bug JDK-8000171 via this comment > https://bugs.openjdk.java.net/browse/JDK-8000171?focusedCommentId=13825066=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13825066 > which is dup of 8196100: > javax/swing/text/JTextComponent/5074573/bug5074573.java fails and and fixed > there. > > So, we can remove the test from problemlist Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3623
Re: RFR: 8039261: [TEST_BUG]: There is not a minimal security level in Java Preferences and the TestApplet.html is blocked. [v2]
On Tue, 13 Apr 2021 01:34:17 GMT, Alexander Zvegintsev wrote: >> This fix simply removes tests mentioned in the bug with for unsupported >> scenarios(applets in browser). > > Alexander Zvegintsev has updated the pull request incrementally with one > additional commit since the last revision: > > TestApplet.java removed Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3441
Re: RFR: 8264428: Cleanup usages of StringBuffer in java.desktop [v4]
On Thu, 8 Apr 2021 14:43:50 GMT, Andrey Turbanov wrote: >> There are few possible cleanups in java.desktop related to legacy >> StringBuffer usages: >> 1. In few places StringBuffer can be replaced with plain String >> concatenation. >> 2. StringBuffer can be replaced with StringBuilder. StringBuilder has better >> performance as it is not thread-safe. >> 3. There are few places where result of string concatenation is passed to >> StringBuffer.append method. Using separate `.append` calls is more clear. >> 4. In few places primitives are unnecessary converted to String before >> `.append` call. They can be replaced with specialized methods (like >> `.append(int)` calls. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8264484: Replace uses of StringBuffer with StringBuilder in > jdk.hotspot.agent > > place each 'append' call on its own line Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3251
Re: RFR: 8264428: Cleanup usages of StringBuffer in java.desktop [v3]
On Wed, 7 Apr 2021 06:39:48 GMT, Andrey Turbanov
wrote:
>> There are few possible cleanups in java.desktop related to legacy
>> StringBuffer usages:
>> 1. In few places StringBuffer can be replaced with plain String
>> concatenation.
>> 2. StringBuffer can be replaced with StringBuilder. StringBuilder has better
>> performance as it is not thread-safe.
>> 3. There are few places where result of string concatenation is passed to
>> StringBuffer.append method. Using separate `.append` calls is more clear.
>> 4. In few places primitives are unnecessary converted to String before
>> `.append` call. They can be replaced with specialized methods (like
>> `.append(int)` calls.
>
> Andrey Turbanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> [PATCH] Replace uses of StringBuffer with StringBuilder in java.desktop
>
> fix copyright year
Marked as reviewed by aivanov (Reviewer).
src/java.desktop/unix/classes/sun/awt/X11/XCreateWindowParams.java line 88:
> 86: while (eIter.hasNext()) {
> 87: Map.Entry entry = eIter.next();
> 88: buf.append(entry.getKey()).append(":
> ").append(entry.getValue()).append("\n");
Would it be clearer if each append was on its own line?
Suggestion:
buf.append(entry.getKey())
.append(": ")
.append(entry.getValue())
.append("\n");
-
PR: https://git.openjdk.java.net/jdk/pull/3251
Re: RFR: 8182043: Access to Windows Large Icons
On Mon, 8 Mar 2021 13:22:07 GMT, Alexander Zuev wrote: > Fix updated after first round of review. src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 260: > 258: > 259:/** > 260: * Icon for a file, directory, or folder as it would be displayed in *Returns an icon* for a file… src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 276: > 274: * > 275: * @param f a File object > 276: * @param size width and height of the icon in pixels Pixels could be ambiguous now. Usually, Swing deals with user's space. That is 16×16 icon should actually be 32×32 if the scale factor of the current display is 200%. Yes, this issue is somewhat irrelevant because the method returns multi-resolution icon. However, the terminology used must be unambiguous and clear; for consistency with other Swing API, it should be in terms of user's space coordinates, *virtual pixels*. - PR: https://git.openjdk.java.net/jdk/pull/2875
Integrated: 8263311: Watch registry changes for remote printers update instead of polling
On Wed, 10 Mar 2021 15:38:27 GMT, Alexey Ivanov wrote: > [JDK-8153732](https://bugs.openjdk.java.net/browse/JDK-8153732) implemented > polling for remote printers. > That bug description also mentions watching the registry for changes and > links to the article which describes the method yet it does so in terms of > WMI. Using WMI is not necessary to watch for the registry updates. > > It is possible to replace polling mechanism with registry change > notifications. If the registry at `HKCU\Printers\Connections` is updated, > refresh the list of print services. > > It works perfectly well in my own testing with sharing a Generic / Text Only > printer from another laptop. The notification comes as soon as the printer is > installed, it results in a new key created under `Connections`. If a remote > printer is removed, the notification is also triggered as the key > corresponding to that printer is removed from the registry. > > I updated the steps in the manual test: `RemotePrinterStatusRefresh.java`. This pull request has now been integrated. Changeset: a85dc557 Author:Alexey Ivanov URL: https://git.openjdk.java.net/jdk/commit/a85dc557 Stats: 207 lines in 3 files changed: 31 ins; 158 del; 18 mod 8263311: Watch registry changes for remote printers update instead of polling Reviewed-by: psadhukhan, serb - PR: https://git.openjdk.java.net/jdk/pull/2915
Re: RFR: 8263311: Watch registry changes for remote printers update instead of polling
On Sat, 13 Mar 2021 00:18:25 GMT, Sergey Bylokhov wrote: > Any idea why RegistryValueChange was rejected as a solution? I've no idea. I read that comment, it's exactly the comment that was in the code above `RemotePrinterChangeListener` class in `PrintServiceLookupProvider.java`. > RegistryValueChange cannot be used in combination with WMI to get registry > value change notification because of an error that may be generated because > the scope of the query would be too big to handle(at times). It's very vague. I don't know what it really means. Neither do I know if a prototype was even tried. If fact, this comment cites portions of the article [How to listen for Printer Connections?](https://docs.microsoft.com/en-gb/archive/blogs/hmahrt/how-to-listen-for-printer-connections). The link to this article is in the description of [JDK-8153732](https://bugs.openjdk.java.net/browse/JDK-8153732). The article discusses the subject with WMI and WQL. This is what it says: > There is no equivalent to `FindFirstPrinterChangeNotification`, which listens > for new/changed Printer Connections – and a polling mechanism was not an > option. However, there is another way, using WMI. Note: _polling mechanism was not an option_. Yet it is what was implemented in Java. Then the articles describes the steps needed to monitor for printer changes. Close to the end, there's the quoted sentence: > Unfortunately, we cannot use the `RegistryValueChangeEvent` (because the > scope of the query would be too big (we’d receive an error message), so we > can only know **when** something changed below the `Printers\Connections` > key, but not **what**. This is why we have to rely on **`EnumPrinters`**. This statement is a bit confusing. It says they cannot use `RegistryValueChangeEvent` (it's not a Windows API function, it's another WMI class) but the WQL query above _uses it_. I interpret the following statement this way: Registry notifications do not provide information on **what** changed under `Printers\Connections`, only that a change occurred. So `EnumPrinters` is used to enumerate the remote printers and update the stored list of printers after the change to the registry occurs. I used this article as the inspiration and the implementation I'm proposing in this PR is purely based on this article. Yet I'm using Windows API function [RegNotifyChangeKeyValue](https://docs.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regnotifychangekeyvalue) to watch for registry updates and to get notified as soon as a change under `HKCU\Printers\Connections` occurs. Then the list of printers is updated by the `refreshServices` upcall into Java which refreshes both _local_ and _remote_ printers. - PR: https://git.openjdk.java.net/jdk/pull/2915
Re: RFR: 8263311: Watch registry changes for remote printers update instead of polling
On Fri, 12 Mar 2021 11:38:08 GMT, Prasanta Sadhukhan wrote: >>> Is this only about addition/removal? What about printer name change? >> >> You cannot change the name of a remote printer. >> >> Opening *Printer Properties* dialog from the printer context menu on the >> local host and editing its name changes the name of the printer on the >> remote host which shares it. Windows warns, “This is a shared printer. If >> you rename a shared printer, existing connections to this printer from other >> computers will break and will have to be created again.” >> >> Nothing has been updated on the local system after renaming the printer. As >> the warning said, the printer does not work any more because it refers to a >> printer that does not exist. >> >> To fix this, one has to add a new remote printer which refers to the new >> name of the printer on the remote host. >> >>> Shouldn't we get notified in that case as trying to print on printer with >>> old name will not find the printer!! >> >> I'm afraid there's nothing Java can do to mitigate renaming the printer. >> >>> If yes, in that regard I guess `REG_NOTIFY_CHANGE_LAST_SET` is the one to >>> go for as it states >>> _"Notify the caller of changes to a value of the key. This can include >>> adding or deleting a value, or changing an existing value. "_ >> >> Since no values are created, removed, or changed when a remote printer is >> added or removed under `Connections` key, adding >> `REG_NOTIFY_CHANGE_LAST_SET` to `dwNotifyFilter` is not required. There are >> some values stored in the printer key but Java does not use them directly; >> if any value is changed there, getting notifications about such an event >> only creates noise. >> >> So, as far as Java is concerned, getting notifications about new keys being >> created or removed under `Connections` is enough. This is what >> `REG_NOTIFY_CHANGE_NAME` filter does. > > I can understand that as a user, you cannot /shouldn't change the name of a > remote network printer but a network admin can change the name, so shouldn't > we get notification on that name change when this method gets called after > the name change? or would it be notified as a new printer in that case? Then > what will happen to the old stale printer in the registry? You can't. Try it yourself. The printer connection is per-user, after all it's stored under HKCU. Windows displays the name of remote printers as “Generic / Text Only on 192.168.1.18”: _the name of the printer_ and _the remote host_. If you open the *Printer Properties* dialog of a remote printer and edit its name, you change the name of the printer on the remote host which shares it. It changes *absolutely nothing* on the local system. > shouldn't we get notification on that name change when this method gets > called after the name change? Yes, we should. But we cannot. For Windows, nothing has changed on the local system, it's the remote system that has been changed. > or would it be notified as a new printer in that case? No, it wouldn't _because nothing has changed on the local system_. > Then what will happen to the old stale printer in the registry? Nothing, again. It just stays there but Windows reports an error if you try to open its Printer Properties, Windows reports an error if you try to print to it. I tried printing with Java and Notepad: the behaviour is the same. The difference is that Notepad displays the error message whereas Java throws an exception (it depends on the Java app, I guess). The behaviour aligns to the one seen when the printer is unreachable because of, let's say, network issues. - PR: https://git.openjdk.java.net/jdk/pull/2915
Re: RFR: 8263311: Watch registry changes for remote printers update instead of polling
On Fri, 12 Mar 2021 04:55:45 GMT, Prasanta Sadhukhan wrote: > Is this only about addition/removal? What about printer name change? You cannot change the name of a remote printer. Opening *Printer Properties* dialog from the printer context menu on the local host and editing its name changes the name of the printer on the remote host which shares it. Windows warns, “This is a shared printer. If you rename a shared printer, existing connections to this printer from other computers will break and will have to be created again.” Nothing has been updated on the local system after renaming the printer. As the warning said, the printer does not work any more because it refers to a printer that does not exist. To fix this, one has to add a new remote printer which refers to the new name of the printer on the remote host. > Shouldn't we get notified in that case as trying to print on printer with old > name will not find the printer!! I'm afraid there's nothing Java can do to mitigate renaming the printer. > If yes, in that regard I guess `REG_NOTIFY_CHANGE_LAST_SET` is the one to go > for as it states > _"Notify the caller of changes to a value of the key. This can include adding > or deleting a value, or changing an existing value. "_ Since no values are created, removed, or changed when a remote printer is added or removed under `Connections` key, adding `REG_NOTIFY_CHANGE_LAST_SET` to `dwNotifyFilter` is not required. There are some values stored in the printer key but Java does not use them directly; if any value is changed there, getting notifications about such an event only creates noise. So, as far as Java is concerned, getting notifications about new keys being created or removed under `Connections` is enough. This is what `REG_NOTIFY_CHANGE_NAME` filter does. - PR: https://git.openjdk.java.net/jdk/pull/2915
Re: RFR: 8263311: Watch registry changes for remote printers update instead of polling
On Wed, 10 Mar 2021 15:38:27 GMT, Alexey Ivanov wrote: > [JDK-8153732](https://bugs.openjdk.java.net/browse/JDK-8153732) implemented > polling for remote printers. > That bug description also mentions watching the registry for changes and > links to the article which describes the method yet it does so in terms of > WMI. Using WMI is not necessary to watch for the registry updates. > > It is possible to replace polling mechanism with registry change > notifications. If the registry at `HKCU\Printers\Connections` is updated, > refresh the list of print services. > > It works perfectly well in my own testing with sharing a Generic / Text Only > printer from another laptop. The notification comes as soon as the printer is > installed, it results in a new key created under `Connections`. If a remote > printer is removed, the notification is also triggered as the key > corresponding to that printer is removed from the registry. > > I updated the steps in the manual test: `RemotePrinterStatusRefresh.java`. src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp line 258: > 256: > REG_NOTIFY_CHANGE_NAME, > 257: NULL, > 258: FALSE); [`RegNotifyChangeKeyValue`](https://docs.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regnotifychangekeyvalue) notifies the caller about changes to the attributes or contents of a specified registry key. • `hKey`: A handle to `HKEY_CURRENT_USER\Printers\Connections` key which is opened above. • `bWatchSubtree = TRUE`: The function reports changes in the specified key and its subkeys. • `dwNotifyFilter = REG_NOTIFY_CHANGE_NAME`: Notify the caller if a subkey is added or deleted. • `hEvent = NULL`: If `fAsynchronous` is FALSE, `hEvent` is ignored. • `fAsynchronous = FALSE`: The function does not return until a change has occurred. When a new remote printer is added, a new key is created under `HKCU\Printers\Connections`; when an existing remote printer is removed, the key below `Connections` is removed; no values are added or removed in `Connections` key, thus `REG_NOTIFY_CHANGE_LAST_SET` filter is not needed. - PR: https://git.openjdk.java.net/jdk/pull/2915
Re: RFR: 8263311: Watch registry changes for remote printers update instead of polling
On Thu, 11 Mar 2021 11:30:52 GMT, Prasanta Sadhukhan wrote: >>> I guess having "FALSE" as fAsynchronous value mean the function does not >>> return until a change has occurred so do we still need this do-while >>> monitoring loop? >> >> You're right, `FALSE` for `fAsynchronous` means the function doesn't return >> until a change occurred. >> >> If a change occurs, we refresh the list of print services and then start to >> wait again. If we exit the loop, we'll not catch other changes that may >> occur. >> >>> And if the function fails once, should we stop monitoring? >> I followed Sergey's approach in `notifyLocalPrinterChange`, namely if >> `FindNextPrinterChangeNotification` returns an error, we quit the loop. >> >> I can't see how we can fix the error if it occurs. Will it succeed the next >> time? Probably not. Thus I decided to quit the loop in case of an error. > > I also am not sure on this. But I think since this is for remote printer, > sometimes network availability issue might be there so it may fail more > compared to local printer so I guess we should give this method a fair > chance, more than that of local printer change, and not bail out on one > failure.maybe try out after some duration...or 5 times spaced out...as > you did for the other EnumPrinter fix.. No, network connectivity cannot affect this. The function watches for registry changes, specifically keys created or removed under `HKCU\Printers\Connections`. If network is down, you won't be able to add a new printer. Yet you can still remove an existing printer. The case with `EnumPrinters` is very different. A printer may be renamed or a new printer may be added therefore the allocated buffer becomes to small to fit the updated data. Thus retrying with larger buffer makes perfect sense. In this case, `RegNotifyChangeKeyValue` could fail only because of invalid parameters. If it does, it will fail on the retry because the parameters haven't changed. In that sense, it's the same as with local printers. If the wait/notification function fails the first time, it will likely fail the second time and so on… - PR: https://git.openjdk.java.net/jdk/pull/2915
Re: RFR: 8263311: Watch registry changes for remote printers update instead of polling
On Thu, 11 Mar 2021 10:39:56 GMT, Prasanta Sadhukhan wrote: > I guess having "FALSE" as fAsynchronous value mean the function does not > return until a change has occurred so do we still need this do-while > monitoring loop? You're right, `FALSE` for `fAsynchronous` means the function doesn't return until a change occurred. If a change occurs, we refresh the list of print services and then start to wait again. If we exit the loop, we'll not catch other changes that may occur. > And if the function fails once, should we stop monitoring? I followed Sergey's approach in `notifyLocalPrinterChange`, namely if `FindNextPrinterChangeNotification` returns an error, we quit the loop. I can't see how we can fix the error if it occurs. Will it succeed the next time? Probably not. Thus I decided to quit the loop in case of an error. - PR: https://git.openjdk.java.net/jdk/pull/2915
Re: RFR: 8182043: Access to Windows Large Icons
On Mon, 8 Mar 2021 13:22:07 GMT, Alexander Zuev wrote:
> Fix updated after first round of review.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1044:
> 1042: new BufferedImage(iconSize, iconSize,
> BufferedImage.TYPE_INT_ARGB);
> 1043: img.setRGB(0, 0, iconSize, iconSize, iconBits, 0,
> iconSize);
> 1044: return img;
There are cases where the size of the buffered image is different from the
requested size. It could affect the layout because it breaks the assumption
that the returned image has the requested size but it may be larger. (Or is it
no longer possible?) I think it should be wrapped into
`MultiResolutionIconImage` in this case.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1114:
> 1112: bothIcons.put(getLargeIcon?
> SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2);
> 1113: newIcon = new
> MultiResolutionIconImage(getLargeIcon ? LARGE_ICON_SIZE
> 1114: : SMALL_ICON_SIZE,
> bothIcons);
I still propose to refactor this code to make it clearer. The code always
returns two icons: _small + large_ or _large + small_ — combined in
`MultiResolutionIconImage`.
You can get `smallIcon` and `largeIcon` and then wrap them into
`MultiResolutionIconImage` in the correct order and store each icon in the
cache.
My opinion hasn't changed here.
I still think there's not much value in getting smaller icon size in addition
to larger one. Or does it provide an alternative image for the case where the
system scaling is 200% and the window is moved to a monitor with scaling of
100%?
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1195:
> 1193: */
> 1194: static Image getShell32Icon(int iconID, int size) {
> 1195: boolean useVGAColors = true; // Will be ignored on XP and later
I cannot see where `useVGAColors` is used. Since the supported OS are later
than XP, it can be removed, can't it?
src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp line 983:
> 981: size = 16;
> 982: }
> 983: hres = pIcon->Extract(szBuf, index, , NULL, (16 << 16)
> + size);
Suggestion:
hres = pIcon->Extract(szBuf, index, , NULL, size);
Now you request only one icon.
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 64:
> 62: }
> 63:
> 64: if (icon.getIconWidth() != size) {
Does it make sense to check that the icon is a multi-resolution image with the
expected sizes?
We know for sure `explorer.exe` contains the entire set of icons.
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
264:
> 262: *
> 263: * The default implementation gets information from the
> 264: * ShellFolder class. Whenever possible, the icon
Do we continue using `` elements? I thought that {@code} is the preferred
way to markup class names.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1112:
> 1110: Map bothIcons = new
> HashMap<>(2);
> : bothIcons.put(getLargeIcon?
> LARGE_ICON_SIZE : SMALL_ICON_SIZE, newIcon);
> 1112: bothIcons.put(getLargeIcon?
> SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2);
Suggestion:
bothIcons.put(getLargeIcon ?
LARGE_ICON_SIZE : SMALL_ICON_SIZE, newIcon);
bothIcons.put(getLargeIcon ?
SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2);
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1146:
> 1144: }
> 1145: Map multiResolutionIcon = new HashMap<>();
> 1146: int start = size > MAX_QUALITY_ICON ?
> ICON_RESOLUTIONS.length - 1 : 0;
Does it make sense to always start at zero?
The icons of smaller size will never be used, will they?
Thus it's safe to start at the index which corresponds to the requested size if
`size` matches, or the index such as `ICON_RESOLUTIONS[index] < size &&
ICON_RESOLUTIONS[index + 1] > size`.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1149:
> 1147: int increment = size > MAX_QUALITY_ICON ? -1 : 1;
> 1148: int end = size > MAX_QUALITY_ICON ? -1 :
> ICON_RESOLUTIONS.length;
> 1149: for (int i = start; i != end; i+=increment) {
Suggestion:
for (int i = start; i != end; i += increment) {
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1422:
> 1420: int w = (int) width;
> 1421: int retindex = 0;
> 1422: for (Integer i: resolutionVariants.keySet()) {
Suggestion:
for (Integer i : resolutionVariants.keySet()) {
-
PR:
RFR: 8263311: Watch registry changes for remote printers update instead of polling
[JDK-8153732](https://bugs.openjdk.java.net/browse/JDK-8153732) implemented polling for remote printers. That bug description also mentions watching the registry for changes and links to the article which describes the method yet it does so in terms of WMI. Using WMI is not necessary to watch for the registry updates. It is possible to replace polling mechanism with registry change notifications. If the registry at `HKCU\Printers\Connections` is updated, refresh the list of print services. It works perfectly well in my own testing with sharing a Generic / Text Only printer from another laptop. The notification comes as soon as the printer is installed, it results in a new key created under `Connections`. If a remote printer is removed, the notification is also triggered as the key corresponding to that printer is removed from the registry. I updated the steps in the manual test: `RemotePrinterStatusRefresh.java`. - Commit messages: - 8263311: Watch registry changes for remote printers update instead of polling Changes: https://git.openjdk.java.net/jdk/pull/2915/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2915=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263311 Stats: 207 lines in 3 files changed: 31 ins; 158 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/2915.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2915/head:pull/2915 PR: https://git.openjdk.java.net/jdk/pull/2915
Re: RFR: 8263142: Delete unused entry points in libawt/libawt_xawt/libawt_headless
On Sun, 7 Mar 2021 23:16:18 GMT, Sergey Bylokhov wrote: > During the review of: >8254024: Enhance native libs for AWT and Swing to work with GraalVM Native > Image > > I have found that some of the entry points in our libraries are never used, > and can be removed, we do not need to update the code to make it work in > static_build. Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2865
Re: RFR: 8262446: DragAndDrop hangs on Windows [v3]
On Fri, 5 Mar 2021 17:21:36 GMT, Dmitry Markov wrote: >> The IME functions and the DND operation must be executed on the toolkit >> thread. If the DND operation is in progress, the IME API is invoked via >> SendMessage() call inside InvokeInputMethodFunction() to avoid a hang. The >> flag isInDoDragDropLoop indicates whether the DND takes place or not. The >> flag works properly if the DND is performed between two Java windows. >> However if anything is dragged from native app, (e.g. Windows FileExplorer) >> to Java the flag is NOT set. That’s the root cause of the hang. >> >> Fix: >> Introduce a new flag to indicate DND operation between Java and native app. >> >> Testing: >> mach5 green > > Dmitry Markov has updated the pull request incrementally with one additional > commit since the last revision: > > Remove flag setting from DragOver Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2825
Re: RFR: 8262446: DragAndDrop hangs on Windows [v2]
On Fri, 5 Mar 2021 16:06:04 GMT, Dmitry Markov wrote:
>> The IME functions and the DND operation must be executed on the toolkit
>> thread. If the DND operation is in progress, the IME API is invoked via
>> SendMessage() call inside InvokeInputMethodFunction() to avoid a hang. The
>> flag isInDoDragDropLoop indicates whether the DND takes place or not. The
>> flag works properly if the DND is performed between two Java windows.
>> However if anything is dragged from native app, (e.g. Windows FileExplorer)
>> to Java the flag is NOT set. That’s the root cause of the hang.
>>
>> Fix:
>> Introduce a new flag to indicate DND operation between Java and native app.
>>
>> Testing:
>> mach5 green
>
> Dmitry Markov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> reuse isInDoDragDropLoop
Marked as reviewed by aivanov (Reviewer).
src/java.desktop/windows/native/libawt/windows/awt_DnDDT.cpp line 228:
> 226: HRESULT __stdcall AwtDropTarget::DragOver(DWORD grfKeyState, POINTL pt,
> DWORD __RPC_FAR *pdwEffect) {
> 227: TRY;
> 228: AwtToolkit::GetInstance().isInDoDragDropLoop = TRUE;
This is a new addition. Did you miss this function in previous iteration?
-
PR: https://git.openjdk.java.net/jdk/pull/2825
Re: RFR: 8262446: DragAndDrop hangs on Windows
On Thu, 4 Mar 2021 10:36:56 GMT, Dmitry Markov wrote: > The IME functions and the DND operation must be executed on the toolkit > thread. If the DND operation is in progress, the IME API is invoked via > SendMessage() call inside InvokeInputMethodFunction() to avoid a hang. The > flag isInDoDragDropLoop indicates whether the DND takes place or not. The > flag works properly if the DND is performed between two Java windows. However > if anything is dragged from native app, (e.g. Windows FileExplorer) to Java > the flag is NOT set. That’s the root cause of the hang. > > Fix: > Introduce a new flag to indicate DND operation between Java and native app. > > Testing: > mach5 green Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2825
Re: RFR: JDK-8262461: handle wcstombsdmp return value correctly in unix awt_InputMethod.c
On Fri, 26 Feb 2021 14:19:03 GMT, Matthias Baesken wrote: > The function wcstombsdmp is called at a few places in awt_InputMethod.c. > This function needs checking of a NULL return value and freeing of the memory > allocated by this function. However this is missing at one place in the file. Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2747
Re: RFR: 8197825: [Test] Intermittent timeout with javax/swing JColorChooser Test [v4]
On Sat, 30 Jan 2021 05:10:07 GMT, Prasanta Sadhukhan wrote: >>> Does this volatile modifier resolve the now-removed infinite loop in `while >>> (!tk.IsDisposed())` in `WToolkit_shutdown`? >> >> The loop should not be removed. > > Unfortunately, volatile modifier has no effect if infinite loop is > reinstated.. > > Does this volatile modifier resolve the now-removed infinite loop in `while > > (!tk.IsDisposed())` in `WToolkit_shutdown`? > > The loop should not be removed. No, it should not, as you noted previously. However, making `m_breakMessageLoop` volatile does not look right either. If `QuitMessageLoop` is called from `!IsMainThread()` thread, it is posted as a message to run on the correct thread. Thus `m_breakMessageLoop` should be accessed on a single thread only; if it's the case, volatile is unneeded. And @prsadhuk's latest test confirms it. There must be something else… - PR: https://git.openjdk.java.net/jdk/pull/2220
Integrated: 8260462: Missing in Modality.html
On Fri, 29 Jan 2021 13:36:40 GMT, Alexey Ivanov wrote: > The first row in The Standard Blocking Matrix table in The AWT Modality > (`Modality.html`) is a header row and it should be inside `` element. This pull request has now been integrated. Changeset: fcfe6478 Author:Alexey Ivanov URL: https://git.openjdk.java.net/jdk/commit/fcfe6478 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8260462: Missing in Modality.html Reviewed-by: serb, psadhukhan - PR: https://git.openjdk.java.net/jdk/pull/2314
Re: RFR: 8197825: [Test] Intermittent timeout with javax/swing JColorChooser Test [v4]
On Fri, 29 Jan 2021 18:05:07 GMT, Prasanta Sadhukhan wrote: >> This test was failing in our nightly mach5 testing. Appropriate stability >> code in form of waitForIdle and delay is added. >> mach5 job running for several iterations on all platforms is ok. Link in JBS. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Declare m_breakMessageLoop volatile src/java.desktop/windows/native/libawt/windows/awt_Toolkit.h line 478: > 476: BOOL m_breakOnError; > 477: > 478: volatile BOOL m_breakMessageLoop; Does this volatile modifier resolve the now-removed infinite loop in `while (!tk.IsDisposed())` in `WToolkit_shutdown`? - PR: https://git.openjdk.java.net/jdk/pull/2220
RFR: 8260462: Missing in Modality.html
The first row in The Standard Blocking Matrix table in The AWT Modality (`Modality.html`) is a header row and it should be inside `` element. - Commit messages: - 8260462: Missing in Modality.html Changes: https://git.openjdk.java.net/jdk/pull/2314/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2314=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260462 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2314.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2314/head:pull/2314 PR: https://git.openjdk.java.net/jdk/pull/2314
Re: RFR: 8197825: [Test] Intermiitent timeout with javax/swing JColorChooser Test [v3]
On Mon, 25 Jan 2021 15:09:35 GMT, Alexey Ivanov wrote: > Yet I suggest fixing the typo in the bug synopsis: Intermiitent → Intermittent I've edited the JBS synopsis. Please also update the PR subject. - PR: https://git.openjdk.java.net/jdk/pull/2220
Re: RFR: 8197825: [Test] Intermiitent timeout with javax/swing JColorChooser Test [v3]
On Fri, 29 Jan 2021 04:03:29 GMT, Prasanta Sadhukhan wrote: > It seems "m_breakMessageLoop" is never true for unsuccessful run even though > AwtToolkit::QuitMessageLoop finish execution (where m_breakMessageLoop is set > to true), > so AwtToolkit::MessageLoop never ends and shutdown hook gets called where > tk.isDisposed() loop start spinning. seems to be some timing issue. Then this does look like a synchronisation problem. One thread changes the value of `m_breakMessageLoop` but another doesn't see it's changed. Should `m_breakMessageLoop` be declared as `volatile`? - PR: https://git.openjdk.java.net/jdk/pull/2220
Re: RFR: 8197825: [Test] Intermiitent timeout with javax/swing JColorChooser Test [v3]
On Thu, 28 Jan 2021 11:57:06 GMT, Alexey Ivanov wrote: >> It seems in successful run, when the test finish >> - AwtToolkit::MessageLoop starts >> - DoQuitMessageLoop is called >> - AwtToolkit::QuitMessageLoop starts >> - AwtToolkit::QuitMessageLoop finishes >> - AwtToolkit::MessageLoop finish >> - Dispose() is called, m_isDisposed sets to true >> - shutdown hook isDisposed is true so no infinite loop >> >> During unsuccessful run, >> - AwtToolkit::MessageLoop starts >> - DoQuitMessageLoop is called >> - AwtToolkit::QuitMessageLoop starts >> - AwtToolkit::QuitMessageLoop finishes >> - AwtToolkit::MessageLoop NEVER ends so Dispose() is not called so >> m_isDisposed is not set to true so shutdown hook goes in infinite loop. > > I was looking at the code yesterday. Could it be because of synchronisation? > I mean, do we need to use native synchronisation to guarantee variable > changes are seen across the threads? > > Does MessageLoop not receive Quit / Null message? > > My point is that this is not a test bug, so the test should not be changed. The test never dispose of the frame. Why is it expected to shut down the toolkit? Shall the frame be disposed of when the main thread in the test finishes? - PR: https://git.openjdk.java.net/jdk/pull/2220
Re: RFR: 8197825: [Test] Intermiitent timeout with javax/swing JColorChooser Test [v3]
On Thu, 28 Jan 2021 09:59:22 GMT, Prasanta Sadhukhan wrote: >> Please take a look at the "AwtToolkit::Dispose()" method, on how much stuff >> should be done to properly shutdown the toolkit. This Dispose() method is >> executed immediately when we exit the message loop in the >> "Java_sun_awt_windows_WToolkit_eventLoop". So when the shutdown hook is >> executed we should have the message loop, then we call tk.QuitMessageLoop to >> stop it, and wait until all code in the Dispose() is executed. But since the >> IsDisposed() return false we for unknow reason hang, does it mean that the >> message loop still operates? Or we got some error during "QuitMessageLoop"? > > It seems in successful run, when the test finish > - AwtToolkit::MessageLoop starts > - DoQuitMessageLoop is called > - AwtToolkit::QuitMessageLoop starts > - AwtToolkit::QuitMessageLoop finishes > - AwtToolkit::MessageLoop finish > - Dispose() is called, m_isDisposed sets to true > - shutdown hook isDisposed is true so no infinite loop > > During unsuccessful run, > - AwtToolkit::MessageLoop starts > - DoQuitMessageLoop is called > - AwtToolkit::QuitMessageLoop starts > - AwtToolkit::QuitMessageLoop finishes > - AwtToolkit::MessageLoop NEVER ends so Dispose() is not called so > m_isDisposed is not set to true so shutdown hook goes in infinite loop. I was looking at the code yesterday. Could it be because of synchronisation? I mean, do we need to use native synchronisation to guarantee variable changes are seen across the threads? Does MessageLoop not receive Quit / Null message? - PR: https://git.openjdk.java.net/jdk/pull/2220
Re: RFR: 8197825: [Test] Intermiitent timeout with javax/swing JColorChooser Test [v2]
On Wed, 27 Jan 2021 12:39:01 GMT, Prasanta Sadhukhan
wrote:
>> This test was failing in our nightly mach5 testing. Appropriate stability
>> code in form of waitForIdle and delay is added.
>> mach5 job running for several iterations on all platforms is ok. Link in JBS.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Prevent infinite loop
test/jdk/javax/swing/JColorChooser/Test6827032.java line 78:
> 76: } finally {
> 77: if (frame != null) {
> 78: SwingUtilities.invokeAndWait(() -> frame.dispose());
Shall the frame be `volatile` too? It's accessed from main thread for the
null-check.
-
PR: https://git.openjdk.java.net/jdk/pull/2220
Re: RFR: JDK-8260426: awt debug_mem.c DMem_AllocateBlock might leak memory
On Wed, 27 Jan 2021 09:56:05 GMT, Matthias Baesken wrote: > It seems that the function DMem_AllocateBlock (debug_mem.c) has an early > return that might lead to memory leaks. > See also the sonar issue Potential leak of memory pointed to by 'header' : > https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8B7zBBG2CXpcngZM=false=BLOCKER=BUG Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2252
Integrated: 8260314: Replace border="1" on tables with CSS
On Sat, 23 Jan 2021 19:06:05 GMT, Alexey Ivanov wrote:
> Replace presentational attribute `border="1"` on `` element with CSS
> styles:
> table {border: outset 1px}
> th, td {border: inset 1px}
>
> These declarations produce the same rendering as the default one in Firefox
> and Edge.
>
> Another option is to use `solid` in both cases.
This pull request has now been integrated.
Changeset: 7ed591cc
Author:Alexey Ivanov
URL: https://git.openjdk.java.net/jdk/commit/7ed591cc
Stats: 36 lines in 6 files changed: 0 ins; 1 del; 35 mod
8260314: Replace border="1" on tables with CSS
Reviewed-by: serb
-
PR: https://git.openjdk.java.net/jdk/pull/2210
Re: RFR: 8260314: Replace border="1" on tables with CSS
On Tue, 26 Jan 2021 19:17:24 GMT, Jonathan Gibbons wrote: > In general, I recommend where possible using the styles provided in the > standard stylesheet, for overall visual consistency. I totally agree. I overlooked the standard styles for the tables. Thanks to @mrserb for pointing me in the right direction. Since most tables in java.desktop use striped tables, it makes sense to use this style in these files too. Although I had said I preferred `plain` class for `componentProperties.html`, I changed my opinion after looking at the updated file, it feels lighter when striped. The table in `NumericShaper` has confusing rendering with `striped` class, that's why it uses `plain`. > FYI, although it seems like the standard styles work for you in this case, if > you should ever need it, javadoc now supports package-specific and > module-specific stylesheets. Just put `*.css` files in the `doc-files` > subdirectory or a package or module, and javadoc should pick it up and use it. Thank you. It's good to know as it allows for keeping consistent look across files as opposed to applying style changes in individual files. - PR: https://git.openjdk.java.net/jdk/pull/2210
Re: RFR: 8260314: Replace border="1" on tables with CSS
On Tue, 26 Jan 2021 15:39:30 GMT, Alexey Ivanov wrote: >> Probably we can import the CSS used by the javadoc itself? > >> Probably we can import the CSS used by the javadoc itself? > > We do. For example, [AWT Modality in JDK > 15](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/java/awt/doc-files/Modality.html) > has borders because of `border="1"` attribute. If you remove it or change it > to `border="1"` in Developer Tools in browser, the borders around cells > disappear. > > However, I looked into it further: there are tables in Javadoc comments. One > example is [AWTKeyStroke > constructor](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/java/awt/AWTKeyStroke.html#%3Cinit%3E()) > which uses `class="striped"`. In most cases, classes in java.desktop module > use striped tables. > > There's also `plain` class. I've found one usage of `class=plain"` in > [NumericShaper > class](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/java/awt/font/NumericShaper.html). > This style has collapsed borders around each cell. It looks similar to what > we have now in “plain” HTML documents, yet by default the borders are not > collapsed, that is you see borders around each individual cell. > > The stylesheet also declares `borderless` class which has no borders. > > For the consistent look and feel of documentation, I think `striped` is the > most appropriate class. However, I prefer `plain` class for > `componentProperties.html` file. I updated all the tables with `class="striped"`. I've also uploaded the files with different styles for visual comparison: ### Current: JDK 15 - [DesktopProperties.html](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/java/awt/doc-files/DesktopProperties.html) - [Modality.html](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/java/awt/doc-files/Modality.html) - [gif_metadata.html](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/javax/imageio/metadata/doc-files/gif_metadata.html) - [tiff_metadata.html](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/javax/imageio/metadata/doc-files/tiff_metadata.html) - [componentProperties.html](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/javax/swing/plaf/synth/doc-files/componentProperties.html) - [synthFileFormat.html](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/javax/swing/plaf/synth/doc-files/synthFileFormat.html) ### Manual: as suggested initially - [DesktopProperties.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v1-manual/api/java.desktop/java/awt/doc-files/DesktopProperties.html) - [Modality.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v1-manual/api/java.desktop/java/awt/doc-files/Modality.html) - [gif_metadata.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v1-manual/api/java.desktop/javax/imageio/metadata/doc-files/gif_metadata.html) - [tiff_metadata.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v1-manual/api/java.desktop/javax/imageio/metadata/doc-files/tiff_metadata.html) - [componentProperties.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v1-manual/api/java.desktop/javax/swing/plaf/synth/doc-files/componentProperties.html) - [synthFileFormat.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v1-manual/api/java.desktop/javax/swing/plaf/synth/doc-files/synthFileFormat.html) ### Striped: `class="striped"` - [DesktopProperties.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v2-striped/api/java.desktop/java/awt/doc-files/DesktopProperties.html) - [Modality.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v2-striped/api/java.desktop/java/awt/doc-files/Modality.html) - [gif_metadata.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v2-striped/api/java.desktop/javax/imageio/metadata/doc-files/gif_metadata.html) - [tiff_metadata.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v2-striped/api/java.desktop/javax/imageio/metadata/doc-files/tiff_metadata.html) - [componentProperties.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v2-striped/api/java.desktop/javax/swing/plaf/synth/doc-files/componentProperties.html) - [synthFileFormat.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v2-striped/api/java.desktop/javax/swing/plaf/synth/doc-files/synthFileFormat.html) ### Plain: `class="plain"` - [DesktopProperties.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v3-plain/api/java.desktop/java/awt/doc-files/DesktopProperties.html) - [Modality.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v3-plain/api/java.desktop/java/awt/doc-files/Modality.html) - [componentProperties.html](http://cr.openjdk.java.net/~aivanov/8260314/borders/v3-plain/api/java.desktop/javax/swing/plaf/synth/doc-files/componentProperties.html) - PR: https://git.openjdk.java.net/jdk/pull/2210
Re: RFR: 8260314: Replace border="1" on tables with CSS [v3]
> Replace presentational attribute `border="1"` on `` element with CSS
> styles:
> table {border: outset 1px}
> th, td {border: inset 1px}
>
> These declarations produce the same rendering as the default one in Firefox
> and Edge.
>
> Another option is to use `solid` in both cases.
Alexey Ivanov has updated the pull request incrementally with one additional
commit since the last revision:
Revert changes to style block in componentProperties.html
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2210/files
- new: https://git.openjdk.java.net/jdk/pull/2210/files/b6f95ed8..9a7d1315
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2210=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2210=01-02
Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/2210.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2210/head:pull/2210
PR: https://git.openjdk.java.net/jdk/pull/2210
Re: RFR: 8260314: Replace border="1" on tables with CSS [v2]
> Replace presentational attribute `border="1"` on `` element with CSS
> styles:
> table {border: outset 1px}
> th, td {border: inset 1px}
>
> These declarations produce the same rendering as the default one in Firefox
> and Edge.
>
> Another option is to use `solid` in both cases.
Alexey Ivanov has updated the pull request incrementally with two additional
commits since the last revision:
- Remove redundant center-align for element
- Apply class="striped" to the tables
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2210/files
- new: https://git.openjdk.java.net/jdk/pull/2210/files/eb057764..b6f95ed8
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2210=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2210=00-01
Stats: 59 lines in 6 files changed: 0 ins; 24 del; 35 mod
Patch: https://git.openjdk.java.net/jdk/pull/2210.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2210/head:pull/2210
PR: https://git.openjdk.java.net/jdk/pull/2210
Re: RFR: 8260314: Replace border="1" on tables with CSS
On Tue, 26 Jan 2021 01:54:10 GMT, Sergey Bylokhov wrote: > Probably we can import the CSS used by the javadoc itself? We do. For example, [AWT Modality in JDK 15](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/java/awt/doc-files/Modality.html) has borders because of `border="1"` attribute. If you remove it or change it to `border="1"` in Developer Tools in browser, the borders around cells disappear. However, I looked into it further: there are tables in Javadoc comments. One example is [AWTKeyStroke constructor](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/java/awt/AWTKeyStroke.html#%3Cinit%3E()) which uses `class="striped"`. In most cases, classes in java.desktop module use striped tables. There's also `plain` class. I've found one usage of `class=plain"` in [NumericShaper class](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/java/awt/font/NumericShaper.html). This style has collapsed borders around each cell. It looks similar to what we have now in “plain” HTML documents, yet by default the borders are not collapsed, that is you see borders around each individual cell. The stylesheet also declares `borderless` class which has no borders. For the consistent look and feel of documentation, I think `striped` is the most appropriate class. However, I prefer `plain` class for `componentProperties.html` file. - PR: https://git.openjdk.java.net/jdk/pull/2210
Re: RFR: 8260314: Replace border="1" on tables with CSS
On Sat, 23 Jan 2021 19:06:05 GMT, Alexey Ivanov wrote:
> Replace presentational attribute `border="1"` on `` element with CSS
> styles:
> table {border: outset 1px}
> th, td {border: inset 1px}
>
> These declarations produce the same rendering as the default one in Firefox
> and Edge.
>
> Another option is to use `solid` in both cases.
@jonathan-gibbons, what do you think?
-
PR: https://git.openjdk.java.net/jdk/pull/2210
RFR: 8260314: Replace border="1" on tables with CSS
Replace presentational attribute `border="1"` on `` element with CSS
styles:
table {border: outset 1px}
th, td {border: inset 1px}
These declarations produce the same rendering as the default one in Firefox and
Edge.
Another option is to use `solid` in both cases.
-
Commit messages:
- Move table styles to common
Integrated: 8260315: Typo "focul" instead of "focus" in FocusSpec.html
On Sat, 23 Jan 2021 12:15:04 GMT, Alexey Ivanov wrote: > A trivial fix for the typo in AWT Focus Spec, it replaces “focul” with > “focus”. This pull request has now been integrated. Changeset: b53d5cac Author:Alexey Ivanov URL: https://git.openjdk.java.net/jdk/commit/b53d5cac Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8260315: Typo "focul" instead of "focus" in FocusSpec.html Reviewed-by: kizune, pbansal - PR: https://git.openjdk.java.net/jdk/pull/2209
RFR: 8260315: Typo "focul" instead of "focus" in FocusSpec.html
A trivial fix for the typo in AWT Focus Spec, it replaces “focul” with “focus”. - Commit messages: - 8260315: Typo "focul" instead of "focus" in FocusSpec.html Changes: https://git.openjdk.java.net/jdk/pull/2209/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2209=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260315 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2209.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2209/head:pull/2209 PR: https://git.openjdk.java.net/jdk/pull/2209
Integrated: 8240247: No longer need to wrap files with contentContainer
On Fri, 22 Jan 2021 19:50:17 GMT, Alexey Ivanov wrote: > [JDK-8231144](https://bugs.openjdk.java.net/browse/JDK-8231144) wrapped the > contents of plain HTML documents into `` to > apply the styles and add the margins around the content for a consistent look. > > This is no longer necessary after > [JDK-8239817](https://bugs.openjdk.java.net/browse/JDK-8239817) was fixed. > > These documents looks fine without being wrapped. So this fix basically > undoes changes under JDK-8231144 and removes the redundant `` wrapper > element. This pull request has now been integrated. Changeset: f624dba6 Author:Alexey Ivanov URL: https://git.openjdk.java.net/jdk/commit/f624dba6 Stats: 45 lines in 15 files changed: 0 ins; 30 del; 15 mod 8240247: No longer need to wrap files with contentContainer Reviewed-by: serb - PR: https://git.openjdk.java.net/jdk/pull/2203
RFR: 8240247: No longer need to wrap files with contentContainer
[JDK-8231144](https://bugs.openjdk.java.net/browse/JDK-8231144) wrapped the contents of plain HTML documents into `` to apply the styles and add the margins around the content for a consistent look. This is no longer necessary after [JDK-8239817](https://bugs.openjdk.java.net/browse/JDK-8239817) was fixed. These documents looks fine without being wrapped. So this fix basically undoes changes under JDK-8231144 and removes the redundant `` wrapper element. - Commit messages: - Remove contentContainer wrapper Changes: https://git.openjdk.java.net/jdk/pull/2203/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2203=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8240247 Stats: 45 lines in 15 files changed: 0 ins; 30 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/2203.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2203/head:pull/2203 PR: https://git.openjdk.java.net/jdk/pull/2203
Re: RFR: 8258805: Japanese characters not entered by mouse click on Windows 10 [v2]
On Thu, 21 Jan 2021 13:51:58 GMT, Dmitry Markov wrote: >> Problem description: >> The IME behaviour has changed starting from recent Windows 10 builds. In >> particular if the complex characters (Japanese, Chinese, etc.) are entered >> to some component and the focus is transferred to another component (which >> does not support the IM) the IM is disabled and the unconfirmed composition >> string gets discarded. >> On previous Windows versions in the same situation the composition string is >> not discarded. >> >> Fix: >> It is necessary to take care of unconfirmed composition string once the IME >> is going to be disabled. >> >> Testing: >> All our automated regression and JCK tests passed with the proposed change. > > Dmitry Markov has updated the pull request incrementally with one additional > commit since the last revision: > > Use endComposition() instead of endCompositionNative() Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2142
Re: RFR: 8258805: Japanese characters not entered by mouse click on Windows 10
On Thu, 21 Jan 2021 14:54:34 GMT, Dmitry Markov wrote: >>> Now we will commit the composition string if there is an active client. >>> That changes eliminates the issue described in JDK-8258805. Also the >>> behaviour of AWTTextTest1 is the same as it used to be on the previous >>> versions w/o the fix. >> >> Just to confirm: the updated behaviour is similar to what was happening in >> previous versions of Windows 10, that is if a compositing text isn't >> committed, it remains uncommitted in the text component when the focus >> changes instead of being committed as it was the case in the previous >> iteration. Is my understanding correct? > >> > Now we will commit the composition string if there is an active client. >> > That changes eliminates the issue described in JDK-8258805. Also the >> > behaviour of AWTTextTest1 is the same as it used to be on the previous >> > versions w/o the fix. >> >> Just to confirm: the updated behaviour is similar to what was happening in >> previous versions of Windows 10, that is if a compositing text isn't >> committed, it remains uncommitted in the text component when the focus >> changes instead of being committed as it was the case in the previous >> iteration. Is my understanding correct? > > I am sorry but you didn't get it right. The behaviour, which was introduced > in previous iteration, is still in place. It is required to get rid of the > problem described in JDK-8258805 > The purpose of of the second iteration is to eliminate negative side effect > (more details here > https://github.com/openjdk/jdk/pull/2142#issuecomment-763340186 ) introduced > by the first version of the fix. > > > Now we will commit the composition string if there is an active client. > > > That changes eliminates the issue described in JDK-8258805. Also the > > > behaviour of AWTTextTest1 is the same as it used to be on the previous > > > versions w/o the fix. > > > > > > Just to confirm: the updated behaviour is similar to what was happening in > > previous versions of Windows 10, that is if a compositing text isn't > > committed, it remains uncommitted in the text component when the focus > > changes instead of being committed as it was the case in the previous > > iteration. Is my understanding correct? > > I am sorry but you didn't get it right. The behaviour, which was introduced > in previous iteration, is still in place. It is required to get rid of the > problem described in JDK-8258805 > The purpose of of the second iteration is to eliminate negative side effect > (more details here [#2142 > (comment)](https://github.com/openjdk/jdk/pull/2142#issuecomment-763340186) ) > introduced by the first version of the fix. I admit I am even more confused now. To me, the description in the comment above is nearly the same as in [JBS comment](https://bugs.openjdk.java.net/browse/JDK-8258805?focusedCommentId=14391025=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14391025). Is the difference that the original test case disabled IME for the middle JTextField whereas in the test case above all JTextField support IME? In the updated version of the fix, we always commit the text on any focus change whether the newly focused component supports IME or not. - PR: https://git.openjdk.java.net/jdk/pull/2142
Re: RFR: 8258805: Japanese characters not entered by mouse click on Windows 10
On Thu, 21 Jan 2021 13:47:30 GMT, Dmitry Markov wrote: > Now we will commit the composition string if there is an active client. That > changes eliminates the issue described in JDK-8258805. Also the behaviour of > AWTTextTest1 is the same as it used to be on the previous versions w/o the > fix. Just to confirm: the updated behaviour is similar to what was happening in previous versions of Windows 10, that is if a compositing text isn't committed, it remains uncommitted in the text component when the focus changes instead of being committed as it was the case in the previous iteration. Is my understanding correct? - PR: https://git.openjdk.java.net/jdk/pull/2142
Re: RFR: 8258805: Japanese characters not entered by mouse click on Windows 10
On Tue, 19 Jan 2021 13:18:22 GMT, Dmitry Markov wrote: > > The fix commits the unconfirmed composition string. Committing is better > > than discarding. Is it possible to preserve the > > state and to leave the string uncommitted? > > The fix reverts the previous (correct) behaviour back. According to the bug description, it used to stay in uncommitted state. > It is unnecessary to store the state and keep the string. That activity may > be quite complicated and requires additional resources especially if there > are several components with active IME I definitely agree that committing the text is better behaviour than discarding uncommitted text. I am for accepting these changes unless we can keep the text uncommitted and allow the user to work with text when the focus moves back to the text component. - PR: https://git.openjdk.java.net/jdk/pull/2142
Re: RFR: 8258805: Japanese characters not entered by mouse click on Windows 10
On Tue, 19 Jan 2021 11:10:35 GMT, Dmitry Markov wrote: > Fix: > It is necessary to take care of unconfirmed composition string once the IME > is going to be disabled. The fix commits the unconfirmed composition string. Committing is better than discarding. Is it possible to preserve the state and to leave the string uncommitted? - PR: https://git.openjdk.java.net/jdk/pull/2142
Re: RFR: 8259522: Apply java.io.Serial annotations in java.desktop [v2]
On Tue, 12 Jan 2021 20:36:21 GMT, Sergey Bylokhov wrote: >> src/java.desktop/share/classes/com/sun/media/sound/InvalidDataException.java >> line 42: >> >>> 40: */ >>> 41: @Serial >>> 42: private static final long serialVersionUID = 1L; >> >> This is the standard wording, yet should it mention the serialization is >> between the same versions only because the value of serialVersionUID is not >> unique? > > I think it is "quite" unique, the "1L" is used from the beginning. It is just > different from the value which could be generated by the "serialver" but > still should work fine. Okay. There are several classes which extend this one, all of them use the same 1L value which makes it not "quite" unique. Changing the values at this time presents more risks than benefits. - PR: https://git.openjdk.java.net/jdk/pull/2020
Re: RFR: 8259519: The java.awt.datatransfer.DataFlavor#ioInputStreamClass field is redundant
On Sun, 10 Jan 2021 03:57:42 GMT, Sergey Bylokhov wrote: > A long time ago in the pre-1.0 era, this field was initialized via reflection > since the "InputStream" class was optional. This was changed since then and a > separate field to refer the "InputStream.class" is not needed. Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2016
Re: RFR: 8259522: Apply java.io.Serial annotations in java.desktop
On Mon, 11 Jan 2021 06:21:52 GMT, Sergey Bylokhov wrote: > Please review the application of @java.io.Serial annotation (JDK-8202385) to > types in the desktop module to enable stricter compile-time checking of > serialization-related declarations. > > This annotation can be applied to these methods in the module: > > private void writeObject(java.io.ObjectOutputStream stream) throws > IOException > private void readObject(java.io.ObjectInputStream stream) throws > IOException, ClassNotFoundException > private void readObjectNoData() throws ObjectStreamException > ANY-ACCESS-MODIFIER Object writeReplace() throws ObjectStreamException > ANY-ACCESS-MODIFIER Object readResolve() throws ObjectStreamException > private static final ObjectStreamField[] serialPersistentFields > private static final long serialVersionUID > > Notes: > - I have tried to update the comments for serialVersionUID as accurately as > possible, but mostly based on the source code history and bugs in JBS where > that field was added > - Some of the readObject/writeObject methods in the javax.swing package > does not have a spec, because this package and some others are excluded from > the serialization specification. > > A similar fix was implemented for java.base module as well: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-August/062046.html Marked as reviewed by aivanov (Reviewer). src/java.desktop/share/classes/com/sun/media/sound/InvalidDataException.java line 42: > 40: */ > 41: @Serial > 42: private static final long serialVersionUID = 1L; This is the standard wording, yet should it mention the serialization is between the same versions only because the value of serialVersionUID is not unique? src/java.desktop/share/classes/java/awt/image/RasterFormatException.java line 28: > 26: package java.awt.image; > 27: > 28: Suggestion: In the majority of classes, there's only one empty line between package declaration and the first import statement. src/java.desktop/share/classes/java/awt/image/ImagingOpException.java line 28: > 26: package java.awt.image; > 27: > 28: Suggestion: - PR: https://git.openjdk.java.net/jdk/pull/2020
Re: RFR: 7194219: java/awt/Component/UpdatingBootTime/UpdatingBootTime.html fails on Linux [v2]
On Thu, 7 Jan 2021 03:40:29 GMT, Sergey Bylokhov wrote: >> The test checks that the timestamp of the mouse event accurately represents >> the current time of the system, even if the time of the system is jumped to >> the past/future. >> >> On Unix in xawt we calculate the timstamp using this method: >> reset_time_utc = System.currentTimeMillis() - getCurrentServerTime(); >> timstamp = reset_time_utc + server_offset; >> >> (1.) Note that the "server_offset" - timstamp when the native event was >> generated, and we try to convert it to the UTC timestamp. Unfortunatly we >> callculate the "reset_time_utc" only once at the start of the application >> and then rarely update it. So if the time in the system is changed we still >> use the old one. >> >> The exactly the same bug described at (1.) was fixed on windows by the >> https://bugs.openjdk.java.net/browse/JDK-6461933 and for that bug the test >> in question was created. That bug was fixed by the "recalculation" system >> time more often. But it does not solve the general time issue and the code >> was reworked again by the https://bugs.openjdk.java.net/browse/JDK-8046495 >> >> I would like to fix the current bug in the same was as on windows, see link >> below: >> https://bugs.openjdk.java.net/browse/JDK-8046495?focusedCommentId=13517205=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13517205 >> >> After the fix we will use the same >> System.currentTimeMillis()/JVM_CurrentTimeMillis on all platforms. > > Sergey Bylokhov has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge branch 'master' into JDK-7194219 > - Initial fix Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/925
Re: RFR: 8259439: Apply java.io.Serial annotations in java.datatransfer
On Fri, 8 Jan 2021 04:51:50 GMT, Sergey Bylokhov wrote: > Please review the application of `@java.io.Serial` annotation (JDK-8202385) > to types in the datatransfer module to enable stricter compile-time checking > of serialization-related declarations. > > This annotation can be applied to these methods in the module: > * private void writeObject(java.io.ObjectOutputStream stream) throws > IOException > * private void readObject(java.io.ObjectInputStream stream) throws > IOException, ClassNotFoundException > * private void readObjectNoData() throws ObjectStreamException >* ANY-ACCESS-MODIFIER Object writeReplace() throws ObjectStreamException >* ANY-ACCESS-MODIFIER Object readResolve() throws ObjectStreamException >* private static final ObjectStreamField[] serialPersistentFields >* private static final long serialVersionUID > > But only the `serialVersionUID` is updated since only this field is used in > the datatransfer module. > > A similar fix was implemented for java.base module as well: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-August/062046.html Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1996
Re: RFR: 6278172: TextComponent.getSelectedText() throws StringIndexOutOfBoundsException [v2]
On Fri, 8 Jan 2021 00:32:32 GMT, Sergey Bylokhov wrote: > The new call to select() method should take care of that since the user as > well can call it passing wrong parameters. Thank you for clarification. Looks good. - PR: https://git.openjdk.java.net/jdk/pull/1104
Re: RFR: 6278172: TextComponent.getSelectedText() throws StringIndexOutOfBoundsException [v2]
On Thu, 7 Jan 2021 20:10:07 GMT, Alexey Ivanov wrote: >> Sergey Bylokhov has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains three additional >> commits since the last revision: >> >> - Merge branch 'master' into JDK-6278172 >> - Merge branch 'master' into JDK-6278172 >> - Initial fix > > Marked as reviewed by aivanov (Reviewer). I wonder how native text components behave: is the selection preserved when text is replaced? - PR: https://git.openjdk.java.net/jdk/pull/1104
Re: RFR: 6278172: TextComponent.getSelectedText() throws StringIndexOutOfBoundsException [v2]
On Thu, 7 Jan 2021 03:42:24 GMT, Sergey Bylokhov wrote: >> The text components are implements as wrappers over the "native" peers. Most >> of the functionality is provided by the peers not wrappers like >> TextArea/TextField. The current bug occurs in the TextArea/TextField when >> the native peer was created yet. >> >> The steps to reproduce: >> 1. Sets the long text to the component >> 2. Select all text in the component >> 3. Sets the short text to the component >> 4. Request the selected text -> BOOM >> >> The bug on step 4 occurred because we request a long substring of the short >> text. To eliminate an exception we have two choices: >> 1. Preserve the selection in the setText(), but limit it by the length of >> the current text. >> 2. Resets the selection. >> >> I tried both solutions, the second caused some TCK tests to fail, so I >> selected the first one. > > Sergey Bylokhov has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'master' into JDK-6278172 > - Merge branch 'master' into JDK-6278172 > - Initial fix Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1104
Re: RFR: 6508941: java.awt.Desktop.open causes VM to crash with video files sporadically
On Sat, 21 Nov 2020 21:59:28 GMT, Sergey Bylokhov wrote: > The report of this bug quite outdated so can be closed but I found that in > the documentation: > https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shellexecutea > >> Because ShellExecute can delegate execution to Shell extensions (data >> sources, context menu handlers, verb implementations) that are activated >> using Component Object Model (COM), COM should be initialized before >> ShellExecute is called. Some Shell extensions require the COM >> single-threaded apartment (STA) type. In that case, COM should be >> initialized as shown here: CoInitializeEx(NULL, COINIT_APARTMENTTHREADED | >> COINIT_DISABLE_OLE1DDE) > > But this CoInitializeEx is missed in the Desktop class. Absent of this > initialization caused some other crashes in past, see JDK-6950553 for example: > https://bugs.openjdk.java.net/browse/JDK-6950553 Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1369
Re: RFR: 8256373: [Windows/HiDPI] The Frame#setBounds does not work in a minimized state
On Mon, 16 Nov 2020 03:02:59 GMT, Sergey Bylokhov wrote: > On HIDPI Windows, the Frame#setBounds does not work in a minimized state. The > bug found during the development of JDK-8211999. When the frame is iconized > and we try to save new bounds for future use, we store the coordinates in the > user's space to the field which use device space: > https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp#L664 Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1216
Re: RFR: 8211999: Window positioning bugs due to overlapping GraphicsDevice bounds (Windows/HiDPI) [v4]
On Tue, 10 Nov 2020 18:40:45 GMT, Sergey Bylokhov wrote:
>> src/java.desktop/share/classes/sun/java2d/SunGraphicsEnvironment.java line
>> 357:
>>
>>> 355: * @param config the graphics configuration which bounds are
>>> requested
>>> 356: * @return the bounds of the area covered by this
>>> 357: * {@code GraphicsConfiguration} in device space(pixels).
>>
>> Suggestion:
>>
>> * {@code GraphicsConfiguration} in device space (pixels).
>> If changed here, all other similar places should be updated too to keep doc
>> comments consistent.
>
> I'll fix it and also merge the master, then update the PR.
In this case, also consider adding a space between the word and the opening
parenthesis in these _coordinates (x, y)_ and _size (w, h)_ and, probably, a
space after comma.
-
PR: https://git.openjdk.java.net/jdk/pull/375
Re: RFR: 8211999: Window positioning bugs due to overlapping GraphicsDevice bounds (Windows/HiDPI) [v4]
On Wed, 28 Oct 2020 23:46:55 GMT, Sergey Bylokhov wrote: >> Hello. >> Please review the fix for jdk. >> >> Old review request: >> https://mail.openjdk.java.net/pipermail/awt-dev/2020-July/015991.html >> >> >> (Note: the fix use API available since Windows 8.1: WM_DPICHANGED, but it >> should be fine for >> Windows 7, because it does not support different DPI for different monitors) >> >> >> Short description: >> In the multi-screen configurations when each screen have different DPI >> we calculate the bounds of each monitor by these formulas: >> >> userSpaceBounds = deviceX / scaleX, deviceY / scaleY, deviceW / >> scaleX, deviceH / scaleY >> devSpaceBounds = userX * scaleX, userY * scaleY, userW * scaleX, >> userH * scaleY >> >> This formula makes the next configuration completely broken: >> - The main screen on the left and 100% DPI >> - The second screen on the right and 200% DPI >> When we translate the bounds of the config from the device space to the >> user's space, >> the bounds of both screen overlap in the user's space, because we use >> bounds of >> the main screen as-is, and the X/Y of the second screen are divided by >> the scaleX/Y. >> >> Since the screens are overlapped we cannot be sure how to translate the >> user's space >>coordinates to device space in the overlapped zone. >> As a result => we use the wrong screen >>=> got wrong coordinates in the device space >> => show top-level windows/popups/tooltips/menus/etc on >> the wrong screen >> >>The proposed solution for this bug is to change the formulas to these: >> >> userSpaceBounds = deviceX, deviceY, deviceW / scaleX, deviceH / >> scaleY >> devSpaceBounds = userX, userY, userW * scaleX, userH * scaleY >> >>In other words, we should not transform the X and Y coordinates of the >> screen(the top/left corner). This will >>complicate the way of how we transform coordinates on the screen: user's >> <--> device spaces: >> >>Before the fix: >> >> userX = deviceX * scaleX; >> deviceX = userX / scaleX; >> >>After the fix(only the part appeared on this screen should be scaled) >> >> userX = screenX + (deviceX - screenX) * scaleX >> deviceX = screenX + (userX - screenX) / scaleX >> >>Note that these new formulas are applicable only for the coordinates on >> the screen such as X and Y. >>If we will need to calculate the "size" such as W and H then the old >> formula should be used. >> >>The main changes for the problem above are: >>- Skip transformation of X and Y of the screen bounds: >> >> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsConfig.cpp.sdiff.html >>- A number of utility methods in java and native: >> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp.sdiff.html >> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/share/classes/sun/java2d/SunGraphicsEnvironment.java.sdiff.html >> >> >> >> Long description: >> Unfortunately, the changes above are not enough to fix the problem when >> different monitors >> have different DPI, even if the bounds are *NOT* overlapped. >> >>- Currently, when we try to set the bounds of the window, we manually >> convert it in "java" to the >> expected device position based on the current GraphicsConfiguration of >> the peer, and then >> additionally, tweak it in native using the device scale stored in >> native code. Unfortunately >> this two scale might not be in sync:(after we use the >> GraphicsConfiguration scale in peer, >> the config might be changed and the tweak in native will use a >> different screen). >> >> As a fix I have moved all transformation from the peer to the native >> code, it will be executed >> on the toolkit thread: >> See the change in WWindowPeer.setBounds() and awt_Window.cpp >> AwtWindow::Reshape >> >> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/classes/sun/awt/windows/WWindowPeer.java.sdiff.html >> >> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp.sdiff.html >> I think at some point we should delete all transformation in java, and >> apply it in the native code only. >> >> >>- We had a special code that tracked the dragging of the window by the >> user from one screen to another, >> and change the size of the window only when the user stops the drag >> operation. I've proposed to use standard Windows >> machinery for that via WM_DPICHANGED: >>
