Re: [OpenJDK 2D-Dev] RFR: 8273102: Delete deprecated for removal the empty finalize() in java.desktop module

[Alexey Ivanov]

On Sun, 29 Aug 2021 01:09:36 GMT, Sergey Bylokhov  wrote:

> The "java.desktop" module has a few implementations of the finalize() which 
> do nothing, deprecated since jdk9, and are marked "forRemoval = true" since 
> jdk16. 
> 
> This is a request to delete such empty methods. 
> 
> CSR: https://bugs.openjdk.java.net/browse/JDK-8273103

Marked as reviewed by aivanov (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5292

Re: [OpenJDK 2D-Dev] RFR: 8272756: Remove unnecessary explicit initialization of volatile variables in java.desktop

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8272756: Remove unnecessary explicit initialization of volatile variables in java.desktop

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v4]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module [v2]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8269637: javax/swing/JFileChooser/FileSystemView/SystemIconTest.java fails on windows

[Alexey Ivanov]

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

Marked as reviewed by aivanov (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4777

Re: [OpenJDK 2D-Dev] RFR: 8269637: javax/swing/JFileChooser/FileSystemView/SystemIconTest.java fails on windows

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v14]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v15]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v14]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v7]

[Alexey Ivanov]

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 
>> ![image](https://user-images.githubusercontent.com/69642324/119092517-62a9f980-b9c3-11eb-9d51-b7745cab564c.png)
>
> 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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v7]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v10]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v9]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v7]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v6]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8264428: Cleanup usages of StringBuffer in java.desktop [v4]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8264923: PNGImageWriter.write_zTXt throws Exception with a typo

[Alexey Ivanov]

On Thu, 8 Apr 2021 13:00:49 GMT, Aleksey Shipilev  wrote:

> Noticed this when backporting JDK-8242557: there is a trivial copy-paste 
> error in exception message. See:
>  https://hg.openjdk.java.net/jdk/jdk/rev/645c71334acd#l1.58

Marked as reviewed by aivanov (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3397

[OpenJDK 2D-Dev] Integrated: 8263984: Invalidate printServices when there are no printers

[Alexey Ivanov]

On Tue, 23 Mar 2021 13:45:33 GMT, Alexey Ivanov  wrote:

> When `getAllPrinterNames()` returns null, the list of `printServices` is 
> assigned a new empty array without invalidating old services which were in 
> the array before.
> 
> The old print services should be invalidated.

This pull request has now been integrated.

Changeset: 9d650397
Author:    Alexey Ivanov 
URL:   https://git.openjdk.java.net/jdk/commit/9d650397
Stats: 8 lines in 1 file changed: 7 ins; 1 del; 0 mod

8263984: Invalidate printServices when there are no printers

Reviewed-by: serb, jdv

-

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8264428: Cleanup usages of StringBuffer in java.desktop [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]

[Alexey Ivanov]

On Wed, 7 Apr 2021 04:12:55 GMT, Prasanta Sadhukhan  
wrote:

>> [Code Conventions for 
>> Java](https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html#248)
>>  say, “Line wrapping for `if` statements should generally use the 8-space 
>> rule, since conventional (4 space) indentation makes seeing the body 
>> difficult.” (It's the second to last block on the page.)
>
> If we are adding a new line, then I think we should need to add at l129, l138 
> otherwise it will look odd doing it at one place only.

I haven't touched that code at all.
Not that odd because it's isolated to the new function now.

What is your suggestion? Refactor all if statements in these two functions? 
Submit a separate bug for refactor all if statements the entire file? Revert 
back to no new line, leaving this particular if untouched as well?

-

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]

[Alexey Ivanov]

On Tue, 6 Apr 2021 20:35:09 GMT, Alexey Ivanov  wrote:

>> BTW another solution is to move { to the next line if the previous statement 
>> was split across few lines.
>
> I added a blank line between the condition and the body.
> Does it look good? @mrserb, @jayathirthrao

[Code Conventions for 
Java](https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html#248)
 say, “Line wrapping for `if` statements should generally use the 8-space rule, 
since conventional (4 space) indentation makes seeing the body difficult.” 
(It's the second to last block on the page.)

-

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]

[Alexey Ivanov]

On Fri, 2 Apr 2021 01:56:37 GMT, Sergey Bylokhov  wrote:

>>> To make it clear, you're for keeping the indentation as it was in the 
>>> original PR to visually separate condition from the statement in the if 
>>> block. Do I get it right, @mrserb?
>> 
>> I think it looks better.
>
> BTW another solution is to move { to the next line if the previous statement 
> was split across few lines.

I added a blank line between the condition and the body.
Does it look good? @mrserb, @jayathirthrao

-

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]

[Alexey Ivanov]

> When `getAllPrinterNames()` returns null, the list of `printServices` is 
> assigned a new empty array without invalidating old services which were in 
> the array before.
> 
> The old print services should be invalidated.

Alexey Ivanov has updated the pull request incrementally with one additional 
commit since the last revision:

  Add a blank line to separate condition from body

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3151/files
  - new: https://git.openjdk.java.net/jdk/pull/3151/files/edc8ab06..774e4074

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3151=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3151=01-02

  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3151.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3151/head:pull/3151

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons

[Alexey Ivanov]

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

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]

[Alexey Ivanov]

On Thu, 1 Apr 2021 17:29:08 GMT, Sergey Bylokhov  wrote:

>>> I would prefer if you revert this line or if we want to put emphasis on 
>>> readability moving '{' to new line also seems fine.
>> 
>> Done.
>
> I do not suggest that the change should be moved forth and back, but I think 
> that the second conditions should always be shifted, and if this causes 80 
> chars overflow then some other line split/rename/etc should be done to 
> prevent that.
> 
> Recent example of such style...:
> if ((getColorSpaceType (p) == ColorSpace.TYPE_RGB) &&
> (getData (p, icSigMediaWhitePointTag) != null) &&
> (getData (p, icSigRedColorantTag) != null) &&
> (getData (p, icSigGreenColorantTag) != null) &&
> (getData (p, icSigBlueColorantTag) != null) &&
> (getData (p, icSigRedTRCTag) != null) &&
> (getData (p, icSigGreenTRCTag) != null) &&
> (getData (p, icSigBlueTRCTag) != null)) {
> thisProfile = new ICC_ProfileRGB (p);
> 
> Is that really looks fine?

This looks even worse than the case in this PR which has only two conditions.

If it'd been the new code, I wouldn't have agreed to revert indentation.

Let's wait for another opinion… 

To make it clear, you're for keeping the indentation as it was in the original 
PR to visually separate condition from the statement in the if block. Do I get 
it right, @mrserb?

-

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]

[Alexey Ivanov]

On Thu, 1 Apr 2021 13:19:07 GMT, Alexey Ivanov  wrote:

>> I would prefer if you revert this line or if we want to put emphasis on 
>> readability moving '{' to new line also seems fine.
>
>> By mapping i mean same indentation for all conditions in if statement…
> 
> Okay, it's the first time I've come across to such a use of `map`.
> 
>> I have not come across code in java.desktop where we add indentation at each 
>> continuation line of 'if' condition.
> 
> Take a look at 
> [DefaultTableCellRenderer.getTableCellRendererComponent](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/table/DefaultTableCellRenderer.java#L201).
> 
>> I understand difficulty to scan without indentation but then in cases where 
>> we have multiple lines on continuation line in if statement we will easily 
>> hit 80 characters limit.
> 
> What is more important code readability or the strict limit of 80 columns?
> 
> All in all, all these styles are used throughout `java.desktop` module.
> I chose to opt for readability in this particular case and the line fits into 
> 80 column limit.
> 
> Do I revert the change to this line?
> Any other suggestions? What is your preference?

> I would prefer if you revert this line or if we want to put emphasis on 
> readability moving '{' to new line also seems fine.

Done.

-

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]

[Alexey Ivanov]

> When `getAllPrinterNames()` returns null, the list of `printServices` is 
> assigned a new empty array without invalidating old services which were in 
> the array before.
> 
> The old print services should be invalidated.

Alexey Ivanov has updated the pull request incrementally with one additional 
commit since the last revision:

  Revert indentation of if condition continuation line

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3151/files
  - new: https://git.openjdk.java.net/jdk/pull/3151/files/98129416..edc8ab06

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3151=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3151=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3151.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3151/head:pull/3151

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Alexey Ivanov]

On Thu, 1 Apr 2021 13:10:40 GMT, Jayathirth D V  wrote:

>> By mapping i mean same indentation for all conditions in if statement 
>> without adding additional indentation for each continuation line 
>> like(Basically line without your change of indentation)
>> if ((condition1) &&
>>  (condition2)) {
>> }
>> 
>> or 
>> 
>> if ((condition1)
>>  &&(condition2)) {
>> }
>> 
>> I have not come across code in java.desktop where we add indentation at each 
>> continuation line of 'if' condition.
>> I understand difficulty to scan without indentation but then in cases where 
>> we have multiple lines on continuation line in if statement we will easily 
>> hit 80 characters limit.
>> 
>> If we want to differentiate between if conditions and actual statement 
>> execution to improve readability, we can move the statement block to new 
>> line like 
>> if ((condition1) &&
>>  (condition2))
>> {
>> }
>
> I would prefer if you revert this line or if we want to put emphasis on 
> readability moving '{' to new line also seems fine.

> By mapping i mean same indentation for all conditions in if statement…

Okay, it's the first time I've come across to such a use of `map`.

> I have not come across code in java.desktop where we add indentation at each 
> continuation line of 'if' condition.

Take a look at 
[DefaultTableCellRenderer.getTableCellRendererComponent](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/table/DefaultTableCellRenderer.java#L201).

> I understand difficulty to scan without indentation but then in cases where 
> we have multiple lines on continuation line in if statement we will easily 
> hit 80 characters limit.

What is more important code readability or the strict limit of 80 columns?

All in all, all these styles are used throughout `java.desktop` module.
I chose to opt for readability in this particular case and the line fits into 
80 column limit.

Do I revert the change to this line?
Any other suggestions? What is your preference?

-

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Alexey Ivanov]

On Thu, 1 Apr 2021 06:04:32 GMT, Jayathirth D V  wrote:

>> When `getAllPrinterNames()` returns null, the list of `printServices` is 
>> assigned a new empty array without invalidating old services which were in 
>> the array before.
>> 
>> The old print services should be invalidated.
>
> src/java.desktop/windows/classes/sun/print/PrintServiceLookupProvider.java 
> line 159:
> 
>> 157: for (int j=0; j < printServices.length; j++) {
>> 158: if ((printServices[j] instanceof Win32PrintService) &&
>> 159: 
>> (!printServices[j].equals(defaultPrintService))) {
> 
> Is this indentation right? I thought we always map newline additional 
> conditions and not add extra indentation.

Is it not right?
I admit I don't understand what you mean by _map_ here.

When the code is written like it was:
if ((printServices[j] instanceof Win32PrintService) &&
(!printServices[j].equals(defaultPrintService))) {
((Win32PrintService)printServices[j]).invalidateService();
}
it's hard to scan: it's not clear what is part of the condition and what is the 
statement inside the if block.

I'd prefer to write it like this:
if ((printServices[j] instanceof Win32PrintService)
&& (!printServices[j].equals(defaultPrintService))) {

((Win32PrintService)printServices[j]).invalidateService();
}
That is moving the operator to the continuation line which makes it obvious it 
is a continuation line of the condition and adding a blank line before the 
statement in the code. It's still not perfect, however; and it changes the 
author in `blame` output.

I indented the continuation line by additional 8 spaces, which is also a common 
practice in Java, to visually separate the condition and the statement. In 
fact, it's IDE that updated the formatting, I decided to keep it because it 
makes the code clearer.

I can revert the change to this line if you like.
Or I can just add a blank line between the condition and the statement.

What is your preference?

-

PR: https://git.openjdk.java.net/jdk/pull/3151

[OpenJDK 2D-Dev] Withdrawn: 8263893: getPrinterNames() leaks nameArray if Java String allocation fails

[Alexey Ivanov]

On Fri, 19 Mar 2021 20:36:57 GMT, Alexey Ivanov  wrote:

> If `JNU_NewStringPlatform` fails to allocate new Java String object for 
> printer name, `std::bad_alloc` is thrown. The handler for the exception does 
> not release the local reference to the `nameArray`, thus it will be leaked.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk/pull/3096

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Alexey Ivanov]

On Wed, 24 Mar 2021 13:07:21 GMT, Prasanta Sadhukhan  
wrote:

> Since this is windows specific code, I am not sure if system will not have 
> any printers. I guess by default, Microsoft XPS Document Writer, Microsoft 
> Print-to-PDF, Fax are present in printers list.

Yes, that's correct. But you can remove them. You can also configure the image 
so that these are not installed by default.

> That is the reason we do not have getPrintService() return 0 printer although 
> there may not be any real printer present in windows system...reason for some 
> failure in jtreg test which caused us to use `@key printer `tag in those 
> tests to make real printers are configured.

I agree that getting zero printers in Windows is unlikely. But it's still 
possible.

Consider the following scenario: Let's assume there are 10 printers in the 
system. The user removes 5 of them. In this case, `invalidateService()` is 
called on the instances of `Win32PrintService` which were removed from the 
system.

Then the user removes the remaining 5 printers. In this case, 
`invalidateService()` is not called at all. If an application has references to 
any of these instances, they will continue to appear operational. However, the 
flag `isInvalid` in `Win32PrintService` is used in two methods only: 
`getPrinterState` and `getPrinterStateReasons`.

This fix is minor, probably this situation never occurs in the real life.

The difference in handling the deleted services caught my attention. If 
everyone agrees it's not problem, I'll withdraw the PR and close the bug as 
_Not an Issue_.

-

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8263893: getPrinterNames() leaks nameArray if Java String allocation fails

[Alexey Ivanov]

On Fri, 19 Mar 2021 22:31:48 GMT, Sergey Bylokhov  wrote:

>> If `JNU_NewStringPlatform` fails to allocate new Java String object for 
>> printer name, `std::bad_alloc` is thrown. The handler for the exception does 
>> not release the local reference to the `nameArray`, thus it will be leaked.
>
> src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp line 178:
> 
>> 176: } catch (std::bad_alloc&) {
>> 177: delete [] pPrinterEnum;
>> 178: if (nameArray != NULL) {
> 
> Not sure that we usually clean the local refs in the native JNI methods, that 
> only required in the native loop which are never returned to the java(I have 
> check that by the grep of env->NewObjectArray)

You're right. It's not required: all local refs are cleared when the JNI method 
exits.
My reasoning was that `env->DeleteLocalRef(utf_str)` is called in the loop. 
Strictly, it's not required either. Yet if there's a large number of elements 
in the array, it could cause a problem.

I'm going to withdraw the PR as the fix is not necessary.

-

PR: https://git.openjdk.java.net/jdk/pull/3096

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Alexey Ivanov]

On Wed, 24 Mar 2021 09:16:33 GMT, Prasanta Sadhukhan  
wrote:

>> When `getAllPrinterNames()` returns null, the list of `printServices` is 
>> assigned a new empty array without invalidating old services which were in 
>> the array before.
>> 
>> The old print services should be invalidated.
>
> src/java.desktop/windows/classes/sun/print/PrintServiceLookupProvider.java 
> line 120:
> 
>> 118: // don't get the default.
>> 119: invalidateServices();
>> 120: printServices = new PrintService[0];
> 
> I am not sure this call to invalidateServices() is needed as we are creating 
> a new `printServices` object of 0 length so making the old printServices 
> invalid is redundant as it is same global variable.

That's exactly the problem I am addressing.

In the regular case, the existing services left in `printServices` are 
invalidated before `newServices` is assigned to it. Yet no existing services 
are invalidated if all the printers were removed from the system.

Why is it needed to invalidate deleted services if the list of printers is 
updated and if at least one printer is left in the system? Why is it not needed 
to invalidate deleted services if all the printers are removed from the system?

-

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Alexey Ivanov]

On Tue, 23 Mar 2021 13:45:33 GMT, Alexey Ivanov  wrote:

> When `getAllPrinterNames()` returns null, the list of `printServices` is 
> assigned a new empty array without invalidating old services which were in 
> the array before.
> 
> The old print services should be invalidated.

src/java.desktop/windows/classes/sun/print/PrintServiceLookupProvider.java line 
119:

> 117: // In Windows it is safe to assume no default if printers == 
> null so we
> 118: // don't get the default.
> 119: invalidateServices();

The default printer service, `defaultPrintService`, is usually in 
`printServices` array. It's possible that it's not in the array; in this case 
getDefaultPrintService() will reset it to `null` when accessed.

-

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Alexey Ivanov]

On Tue, 23 Mar 2021 13:45:33 GMT, Alexey Ivanov  wrote:

> When `getAllPrinterNames()` returns null, the list of `printServices` is 
> assigned a new empty array without invalidating old services which were in 
> the array before.
> 
> The old print services should be invalidated.

src/java.desktop/windows/classes/sun/print/PrintServiceLookupProvider.java line 
116:

> 114: private synchronized void refreshServices() {
> 115: String[] printers = getAllPrinterNames();
> 116: if (printers == null) {

`getAllPrinterNames` returns null when an error occurs or when there are no 
printers in the system.

Up for discussion:
If an error occurs, we may leave the old services without invalidating them. In 
this case, it is necessary to distinguish between the error and the lack of 
printers in the system. Does this make any sense?

-

PR: https://git.openjdk.java.net/jdk/pull/3151

[OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Alexey Ivanov]

When `getAllPrinterNames()` returns null, the list of `printServices` is 
assigned a new empty array without invalidating old services which were in the 
array before.

The old print services should be invalidated.

-

Commit messages:
 - 8263984: Invalidate printServices when there are no printers

Changes: https://git.openjdk.java.net/jdk/pull/3151/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3151=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263984
  Stats: 8 lines in 1 file changed: 6 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3151.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3151/head:pull/3151

PR: https://git.openjdk.java.net/jdk/pull/3151

Re: [OpenJDK 2D-Dev] Clarification regarding PageFormat and Paper getImageable* functions orientation consideration

[Alexey Ivanov]

Hi Rajat,

I agree with Phil.

Both PageFormat [1] and Paper [2] have getImageable*() methods. The spec 
for each of these methods in PageFormat states, “This method takes into 
account the orientation of the page.” The implementation confirms this 
statement.


Yet the spec for Paper does not have this statement.

The test case in JDK-8203395 [3] gets the margins from Paper object and 
it gets the same results, irrespective of the page orientation. This is 
*expected* because Paper does not take into account the orientation.


As you noticed, getting the margins from PageFormat object directly 
produces the expected result: the margins are different in portrait 
(default) and landscape orientation.


Thus both classes — PageFormat and Paper — behave exactly as specified.


As for the question:
My question is that should Paper getImageable* functions also need 
take into account orientation as per spec ?, or is the current 
behavior of them not considering orientation correct and why ?
The Paper object doesn't have orientation. So I believe the current 
behaviour is correct.


--
Regards,
Alexey

[1] 
https://docs.oracle.com/javase/8/docs/api/java/awt/print/PageFormat.html#getImageableX--

https://docs.oracle.com/en/java/javase/16/docs/api/java.desktop/java/awt/print/PageFormat.html#getImageableX()
[2] 
https://docs.oracle.com/javase/8/docs/api/java/awt/print/Paper.html#getImageableX--

https://docs.oracle.com/en/java/javase/16/docs/api/java.desktop/java/awt/print/Paper.html#getImageableX()
[3]  https://bugs.openjdk.java.net/browse/JDK-8203395


On 22/03/2021 21:54, Philip Race wrote:

There's no bug here from what I am being shown.

>"PageFormat showing wrong printer margins in LANDSCAPE orientation” .

Really ? But I don't see anywhere the PageFormat is queried for this.
Instead the test case digs inside the PageFormat and retrieves the 
Paper and asks for *its* margins


I see a comment in the bug :
> All getImageable methodes javadoc say "This method takes into 
account the orientation of the page" but It's not true before printing.


The methods that say that, are the ones on PageFormat, not the ones on 
Paper. So wrong.



-phil

On 3/22/21 1:55 PM, Rajat Mahajan wrote:


Hi all,

*_Issue:_*

*__*

I am working on https://bugs.openjdk.java.net/browse/JDK-8203395 
  and this bug is 
about *“PageFormat showing wrong printer margins in LANDSCAPE 
orientation” .*


**

*Application code it trying to print in Landscape and Portrait but 
both show same margins:*


Margins default : 12,12,17,17

Margins OrientationRequested.LANDSCAPE : 12,12,17,17

**

*The code of the application uses Paper object for getting margins :*

PageFormat pf = printerJob.getPageFormat(aset);

   Paper paper = pf.getPaper();

   long paperTopMargin = Math.round(paper.getImageableY());
   long paperLeftMargin = Math.round(paper.getImageableX());
   long paperRightMargin = Math.round(paper.getWidth() - 
paper.getImageableWidth() - paperLeftMargin);
   long paperBottomMargin = Math.round(paper.getHeight() - 
paper.getImageableHeight() - paperTopMargin);


When I looked at the latest JDK code, PageFormat getImageable 
functions are taking into account Orientation and Paper getImageable 
functions are not , and hence we see the same output.


Using Page format object instead of Paper shows the correct output:

Margins default : 12,12,17,17

Margins OrientationRequested.LANDSCAPE : 17,17,12,12

*_Question:_*

My question is that should Paper getImageable* functions also need 
take into account orientation as per spec ?, or is the current 
behavior of them not considering orientation correct and why ?

[OpenJDK 2D-Dev] Integrated: 8263894: Convert defaultPrinter and printers fields to local variables

[Alexey Ivanov]

On Fri, 19 Mar 2021 20:35:03 GMT, Alexey Ivanov  wrote:

> `PrintServiceLookupProvider` has `defaultPrinter` and `printers` fields but 
> they are used only in `getDefaultPrintService()` and `refreshServices()` 
> correspondingly. Thus these two fields can be converted to local variables in 
> the corresponding methods.

This pull request has now been integrated.

Changeset: 840ab7bf
Author:Alexey Ivanov 
URL:   https://git.openjdk.java.net/jdk/commit/840ab7bf
Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod

8263894: Convert defaultPrinter and printers fields to local variables

Reviewed-by: prr, azvegint, kizune

-

PR: https://git.openjdk.java.net/jdk/pull/3095

Re: [OpenJDK 2D-Dev] RFR: 8263894: Convert defaultPrinter and printers fields to local variables [v2]

[Alexey Ivanov]

On Fri, 19 Mar 2021 21:05:57 GMT, Phil Race  wrote:

> Why did you delete the comments ?

The comments highlighted the difference between what is stored in `printers` 
and in `printServices`: _excludes_ vs. _includes_ the default printer. However, 
according to the code, `printers` _included_ the default printer too.

Now there's only `printServices` which contains all print services. I decided 
the comment didn't have much value now. Yet, upon thinking about it, the 
comment for `printServices` still provides additional insight: the array also 
contains the `defaultPrintService`.

-

PR: https://git.openjdk.java.net/jdk/pull/3095

Re: [OpenJDK 2D-Dev] RFR: 8263894: Convert defaultPrinter and printers fields to local variables [v2]

[Alexey Ivanov]

> `PrintServiceLookupProvider` has `defaultPrinter` and `printers` fields but 
> they are used only in `getDefaultPrintService()` and `refreshServices()` 
> correspondingly. Thus these two fields can be converted to local variables in 
> the corresponding methods.

Alexey Ivanov has updated the pull request incrementally with one additional 
commit since the last revision:

  Bring back the comment for printServices

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3095/files
  - new: https://git.openjdk.java.net/jdk/pull/3095/files/cfe460c2..2b98fec2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3095=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3095=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3095.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3095/head:pull/3095

PR: https://git.openjdk.java.net/jdk/pull/3095

[OpenJDK 2D-Dev] RFR: 8263893: getPrinterNames() leaks nameArray if Java String allocation fails

[Alexey Ivanov]

If `JNU_NewStringPlatform` fails to allocate new Java String object for printer 
name, `std::bad_alloc` is thrown. The handler for the exception does not 
release the local reference to the `nameArray`, thus it will be leaked.

-

Commit messages:
 - 8263893: getPrinterNames() leaks nameArray if Java String allocation fails

Changes: https://git.openjdk.java.net/jdk/pull/3096/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3096=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263893
  Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3096.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3096/head:pull/3096

PR: https://git.openjdk.java.net/jdk/pull/3096

[OpenJDK 2D-Dev] RFR: 8263894: Convert defaultPrinter and printers fields to local variables

[Alexey Ivanov]

`PrintServiceLookupProvider` has `defaultPrinter` and `printers` fields but 
they are used only in `getDefaultPrintService()` and `refreshServices()` 
correspondingly. Thus these two fields can be converted to local variables in 
the corresponding methods.

-

Commit messages:
 - 8263894: Convert defaultPrinter and printers fields to local variables

Changes: https://git.openjdk.java.net/jdk/pull/3095/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3095=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263894
  Stats: 5 lines in 1 file changed: 0 ins; 2 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3095.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3095/head:pull/3095

PR: https://git.openjdk.java.net/jdk/pull/3095

[OpenJDK 2D-Dev] Integrated: 8263311: Watch registry changes for remote printers update instead of polling

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8263311: Watch registry changes for remote printers update instead of polling

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8263311: Watch registry changes for remote printers update instead of polling

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8263311: Watch registry changes for remote printers update instead of polling

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8263311: Watch registry changes for remote printers update instead of polling

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8263311: Watch registry changes for remote printers update instead of polling

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8263311: Watch registry changes for remote printers update instead of polling

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons

[Alexey Ivanov]

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: 

[OpenJDK 2D-Dev] RFR: 8263311: Watch registry changes for remote printers update instead of polling

[Alexey Ivanov]

[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: [OpenJDK 2D-Dev] RFR: 8263138: Initialization of sun.font.SunFontManager.platformFontMap is not thread safe

[Alexey Ivanov]

On Sat, 27 Feb 2021 18:33:52 GMT, Andrey Turbanov 
 wrote:

> 8263138: Initialization of sun.font.SunFontManager.platformFontMap is not 
> thread safe

Marked as reviewed by aivanov (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/2762

[OpenJDK 2D-Dev] Integrated: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

[Alexey Ivanov]

On Thu, 4 Mar 2021 21:37:33 GMT, Alexey Ivanov  wrote:

> **Root cause:**
> `getPrinterNames()` has two calls to 
> [`::EnumPrinters`](https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters).
>  The first call is to get the required buffer size to contain the structures 
> and any strings. The second call is to get the list of printers.
> 
> Yet the list of printers or the names of printers can change between the two 
> calls. If it happens, the second call to `EnumPrinters` fails but it is not 
> checked.
> 
> I couldn't reproduce the crash myself. However, I reproduced the failure in 
> the second call to `::EnumPrinters` by adding `::Sleep(500)` and by renaming 
> the printers so that the data doesn't fit into the allocated buffer:
> 
> 1: bResult: 0, cbNeeded: 504, cReturned: 0
> 2: bResult: 0, cbNeeded: 512, cReturned: 0
> !!! error
> 
> During my testing, `cReturned` has always been zero whenever `EnumPrinters` 
> fails.
> 
> The crash dumps show that `cReturned` is non-zero but the `pPrinterEnum` 
> buffer doesn't contain valid data. Reading `info4->pPrinterName` at the line
> utf_str = JNU_NewStringPlatform(env, info4->pPrinterName);
> raises Access Violation exception.
> 
> **The fix:**
> Check the return value of `EnumPrinters` and allow for 5 retries. If the 
> function still returns failure, make sure `cReturned` is set to zero.
> 
> I'm going to close 
> [JDK-6996051](https://bugs.openjdk.java.net/browse/JDK-6996051) and 
> [JDK-8182683](https://bugs.openjdk.java.net/browse/JDK-8182683) reported 
> previously about the same crash as duplicate of the current 
> [JDK-8262829](https://bugs.openjdk.java.net/browse/JDK-8262829).
> 
> **Testing:**
> I used 
> [`RemotePrinterStatusRefresh.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/RemotePrinterStatusRefresh/RemotePrinterStatusRefresh.java)
>  for testing, it shows the list of currently available printers. In the 
> background I ran `PrinterRename.ps1` PowerShell script which remains a 
> printer, the script is attached to the JBS issue.
> 
> Without the fix, the list of available printers was empty occasionally 
> because `EnumPrinters` returned failure and set `cReturned` to zero. With the 
> fix, the list of printers is always filled.

This pull request has now been integrated.

Changeset: a6e34b3d
Author:Alexey Ivanov 
URL:   https://git.openjdk.java.net/jdk/commit/a6e34b3d
Stats: 20 lines in 1 file changed: 13 ins; 0 del; 7 mod

8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Reviewed-by: prr, psadhukhan, serb

-

PR: https://git.openjdk.java.net/jdk/pull/2836

Re: [OpenJDK 2D-Dev] RFR: 8262446: DragAndDrop hangs on Windows [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8262446: DragAndDrop hangs on Windows [v2]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

[Alexey Ivanov]

On Thu, 4 Mar 2021 22:09:55 GMT, Phil Race  wrote:

> I guess this is OK and yes we should have been checking this.
> Not sure we really got to the bottom of the real world problem because I'd 
> expect the 2nd call just milliseconds after the first. But it could be that 
> this happens during some system updates and we get one printer reconfigured 
> message followed by another ..

Most of the time, the crash happens when a client reconnects to their active 
session where a JVM is already running. On reconnect, the list of printers is 
synced with the local printers on the client. In such a scenario, other 
software could also update its list of printers as well as repaint its UI. If 
the system is under high load, it's not impossible to have a long enough pause 
between the calls.

-

PR: https://git.openjdk.java.net/jdk/pull/2836

Re: [OpenJDK 2D-Dev] RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

[Alexey Ivanov]

On Fri, 5 Mar 2021 03:49:22 GMT, Prasanta Sadhukhan  
wrote:

>> I guess since we are changing this method anyway, can we use 
>> PRINTER_ENUM_CONNECTIONS flag instead of hardcoded 4 so that it is more 
>> informative!!
>
> ok, it is for flags and not for level. please ignore

> 
> 
> Don't we need to check that this method call succeeds? Probably it is a crash 
> reason when the EnumPrinters fails but we use cbNeeded anyway for array 
> allocation?

`EnumPrinters` always returns failure here because zero-sized buffer is too 
small to contain any data.

-

PR: https://git.openjdk.java.net/jdk/pull/2836

[OpenJDK 2D-Dev] RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

[Alexey Ivanov]

**Root cause:**
`getPrinterNames()` has two calls to 
[`::EnumPrinters`](https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters).
 The first call is to get the required buffer size to contain the structures 
and any strings. The second call is to get the list of printers.

Yet the list of printers or the names of printers can change between the two 
calls. If it happens, the second call to `EnumPrinters` fails but it is not 
checked.

I couldn't reproduce the crash myself. However, I reproduced the failure in the 
second call to `::EnumPrinters` by adding `::Sleep(500)` and by renaming the 
printers so that the data doesn't fit into the allocated buffer:

1: bResult: 0, cbNeeded: 504, cReturned: 0
2: bResult: 0, cbNeeded: 512, cReturned: 0
!!! error

During my testing, `cReturned` has always been zero whenever `EnumPrinters` 
fails.

The crash dumps show that `cReturned` is non-zero but the `pPrinterEnum` buffer 
doesn't contain valid data. Reading `info4->pPrinterName` at the line
utf_str = JNU_NewStringPlatform(env, info4->pPrinterName);
raises Access Violation exception.

**The fix:**
Check the return value of `EnumPrinters` and allow for 5 retries. If the 
function still returns failure, make sure `cReturned` is set to zero.

I'm going to close 
[JDK-6996051](https://bugs.openjdk.java.net/browse/JDK-6996051) and 
[JDK-8182683](https://bugs.openjdk.java.net/browse/JDK-8182683) reported 
previously about the same crash as duplicate of the current 
[JDK-8262829](https://bugs.openjdk.java.net/browse/JDK-8262829).

**Testing:**
I used 
[`RemotePrinterStatusRefresh.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/RemotePrinterStatusRefresh/RemotePrinterStatusRefresh.java)
 for testing, it shows the list of currently available printers. In the 
background I ran `PrinterRename.ps1` PowerShell script which remains a printer, 
the script is attached to the JBS issue.

Without the fix, the list of available printers was empty occasionally because 
`EnumPrinters` returned failure and set `cReturned` to zero. With the fix, the 
list of printers is always filled.

-

Commit messages:
 - 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Changes: https://git.openjdk.java.net/jdk/pull/2836/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2836=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262829
  Stats: 20 lines in 1 file changed: 13 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2836.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2836/head:pull/2836

PR: https://git.openjdk.java.net/jdk/pull/2836

Re: [OpenJDK 2D-Dev] RFR: 8262446: DragAndDrop hangs on Windows

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8262915: java.awt.color.ColorSpace.getName() is not thread-safe

[Alexey Ivanov]

On Wed, 3 Mar 2021 03:46:59 GMT, Sergey Bylokhov  wrote:

> The java.awt.color.ColorSpace.getName()  method does not use any kind of 
> synchronization to properly initialize and use the static cache for the color 
> components names.

Marked as reviewed by aivanov (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/2801

Re: [OpenJDK 2D-Dev] RFR: 8257500: Drawing MultiResolutionImage with ImageObserver "leaks" memory

[Alexey Ivanov]

On Wed, 24 Feb 2021 18:57:25 GMT, Sergey Bylokhov  wrote:

> This bug was reported as a leak of the ImageObserver when the user draws a 
> multiresolution image.
> 
> The multiresolution image is an image that contains a few images inside, when 
> the app uses the observer to get notifications about image loading we create 
> a special internal observer and use it to track loading resolution-variants. 
> This internal observer forwards notification from the resolution variant to 
> the application observer.
> 
> The mapping from the application observer to the internal observer is done 
> via "SoftCache" which is a kind of HashMap that uses soft references to the 
> key and value.
> 
> Here the bug comes, the soft references are cleared only under memory 
> pressure, and unused objects may sit hours in memory before being cleaned. 
> Moreover, the internal observer was implemented using a strong reference to 
> the application observer(it is not obvious since the lambda is used). So the 
> key object refers to the application's observer cannot be clear fast. This 
> causes an even longer delay of the memory cleanup, which was considered by 
> the use as a "leak".
> 
> The fix changes the usage of SoftCache to the WeakHashMap, so the key(the 
> application observer) will be cleared when the application lost the reference 
> to it.

Can `MultiResolutionToolkitImageTest.java` not be run on other platforms?

-

Marked as reviewed by aivanov (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2711

Re: [OpenJDK 2D-Dev] RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v10]

[Alexey Ivanov]

On Wed, 17 Feb 2021 15:54:22 GMT, Prasanta Sadhukhan  
wrote:

>> The API doc for Graphics2D.clip(shape s) claims that passing a null argument 
>> would actually clear the existing clipping area, which is incorrect.
>> This statement is applicable only to G2D.setClip() and not for the clip() 
>> method. G2D.clip() would throw a NullPointerException when it encounters a 
>> null argument. 
>> Updated spec to rectify this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   javadoc change

Marked as reviewed by aivanov (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/2476

Re: [OpenJDK 2D-Dev] RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v9]

[Alexey Ivanov]

On Wed, 17 Feb 2021 15:48:12 GMT, Prasanta Sadhukhan  
wrote:

>> The API doc for Graphics2D.clip(shape s) claims that passing a null argument 
>> would actually clear the existing clipping area, which is incorrect.
>> This statement is applicable only to G2D.setClip() and not for the clip() 
>> method. G2D.clip() would throw a NullPointerException when it encounters a 
>> null argument. 
>> Updated spec to rectify this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   javadoc change

src/java.desktop/share/classes/java/awt/Graphics2D.java line 1207:

> 1205:  * with the current clip, it will throw {@code NullPointerException}
> 1206:  * for a null shape unless the user clip is also {@code null}.
> 1207:  * So calling this method with a {@code null} argument is not 
> recommended.

I wonder why not “{@code null} shape” at line 1206?

-

PR: https://git.openjdk.java.net/jdk/pull/2476

Re: [OpenJDK 2D-Dev] RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v8]

[Alexey Ivanov]

On Wed, 17 Feb 2021 15:38:10 GMT, Prasanta Sadhukhan  
wrote:

>> The API doc for Graphics2D.clip(shape s) claims that passing a null argument 
>> would actually clear the existing clipping area, which is incorrect.
>> This statement is applicable only to G2D.setClip() and not for the clip() 
>> method. G2D.clip() would throw a NullPointerException when it encounters a 
>> null argument. 
>> Updated spec to rectify this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   javadoc change

src/java.desktop/share/classes/java/awt/Graphics2D.java line 1207:

> 1205:  * with the current clip, it will throw {@code NullPointerException}
> 1206:  * for a null shape unless the user clip is also {@code null}.
> 1207:  * So calling this method with a null argument is not recommended.

This suggestion still applies too.
Suggestion:

 * for a {@code null} shape unless the user clip is also {@code null}.
 * So calling this method with a {@code null} argument is not recommended.

-

PR: https://git.openjdk.java.net/jdk/pull/2476

Re: [OpenJDK 2D-Dev] RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v7]

[Alexey Ivanov]

On Wed, 17 Feb 2021 15:08:58 GMT, Prasanta Sadhukhan  
wrote:

>> The API doc for Graphics2D.clip(shape s) claims that passing a null argument 
>> would actually clear the existing clipping area, which is incorrect.
>> This statement is applicable only to G2D.setClip() and not for the clip() 
>> method. G2D.clip() would throw a NullPointerException when it encounters a 
>> null argument. 
>> Updated spec to rectify this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   javadoc change

src/java.desktop/share/classes/java/awt/Graphics2D.java line 1206:

> 1204:  * with the current clip, it will throw {@code NullPointerException}
> 1205:  * for a null shape unless the user clip is also {@code null}.
> 1206:  * So calling this method with a null argument is not recommended.

Suggestion:

 * for a {@code null} shape unless the user clip is also {@code null}.
 * So calling this method with a {@code null} argument is not recommended.

src/java.desktop/share/classes/java/awt/Graphics2D.java line 1206:

> 1204:  * with the current clip, it will throw {@code NullPointerException}
> 1205:  * for a null shape unless the user clip is also {@code null}.
> 1206:  * So calling this method with a null argument is not recommended.

Since you're editing the javadoc for this method, wouldn't adding a couple 
`` break up the description to make it clearer?

1198 * The user clip modified by this method is independent of the
1203 * user clip.
 * Since this method intersects the specified shape
The rendered javadoc will have clear separation between different paragraphs 
and will facilitate scanning the method description.

Does it make any sense?

-

PR: https://git.openjdk.java.net/jdk/pull/2476

Re: [OpenJDK 2D-Dev] RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v6]

[Alexey Ivanov]

On Mon, 15 Feb 2021 12:48:01 GMT, Prasanta Sadhukhan  
wrote:

>> The API doc for Graphics2D.clip(shape s) claims that passing a null argument 
>> would actually clear the existing clipping area, which is incorrect.
>> This statement is applicable only to G2D.setClip() and not for the clip() 
>> method. G2D.clip() would throw a NullPointerException when it encounters a 
>> null argument. 
>> Updated spec to rectify this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   javdoc tag for NPE

src/java.desktop/share/classes/java/awt/Graphics2D.java line 1205:

> 1203:  * user clip. Since this method intersects the specified shape
> 1204:  * with the current clip, it will throw {@code 
> NullPointerException} for a null shape
> 1205:  * unless the user clip is also null.

Shall the text be re-wrapped?

I'm not sure if null should always be {@code null}, it's not used consistently. 
At the same time, two lines above it's “…with a {@code null} argument…”; it 
makes sense to make Javadoc consistent in this regard at least for one method.

-

PR: https://git.openjdk.java.net/jdk/pull/2476

Re: [OpenJDK 2D-Dev] RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method [v5]

[Alexey Ivanov]

On Thu, 11 Feb 2021 15:44:57 GMT, Prasanta Sadhukhan  
wrote:

>> The API doc for Graphics2D.clip(shape s) claims that passing a null argument 
>> would actually clear the existing clipping area, which is incorrect.
>> This statement is applicable only to G2D.setClip() and not for the clip() 
>> method. G2D.clip() would throw a NullPointerException when it encounters a 
>> null argument. 
>> Updated spec to rectify this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Review comment fix. Test updated

src/java.desktop/share/classes/java/awt/Graphics2D.java line 1204:

> 1202:  * argument, the specified {@code Shape} becomes the new
> 1203:  * user clip. Since this method intersects the specified shape
> 1204:  * with the current clip, it will throw NPE for a null shape

Shall NPE be spelled out as `{@code NullPointerException}`?

Also should it rather be, “…it will throws NPE…”?

-

PR: https://git.openjdk.java.net/jdk/pull/2476

Re: [OpenJDK 2D-Dev] RFR: 8260695: The java.awt.color.ICC_Profile#getData/getData(int) are not thread safe [v3]

[Alexey Ivanov]

On Thu, 11 Feb 2021 20:00:01 GMT, Sergey Bylokhov  wrote:

>> Both methods are implemented in a similar way.
>>  1. Requests the size of the profile/tag data
>>  2. Creates an array of the correct size
>>  3. Requests the data and copy it to the array
>> 
>> If the data will be changed concurrently between steps 2. and 3. then we 
>> will get a mismatch between the array and copied data. 
>> 
>> In the fix, all steps above are merged to just one step - return the data 
>> when requested.
>
> 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 eight additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8260695
>  - Merge branch 'master' into JDK-8260695
>  - cleanup
>  - Merge branch 'JDK-8260695' of https://github.com/mrserb/jdk into 
> JDK-8260695
>  - Update LCMSProfile.java
>  - Update LCMSProfile.java
>  - Create MTGetData.java
>  - Initial fix

Marked as reviewed by aivanov (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/2330

Re: [OpenJDK 2D-Dev] RFR: 6211257: BasicStroke.createStrokedShape(Shape): NPE is not specified [v3]

[Alexey Ivanov]

On Sun, 7 Feb 2021 09:18:04 GMT, Prasanta Sadhukhan  
wrote:

>> Method createStrokedShape(Shape) for java.awt.BasicStroke class throws 
>> NullPointerException when passed a null object reference for a input 
>> parameter but it's not specified in the spec.
>> Updated spec to illustrate this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Address review comments
>  - Revert "Address review comments"
>
>This reverts commit 3fff74d7563a6141d67cb18fd7c3dda731a4c752.
>  - Address review comments

Marked as reviewed by aivanov (Reviewer).

test/jdk/java/awt/BasicStroke/TestNullShape.java line 42:

> 40: } catch (NullPointerException ne) {
> 41: System.out.println("result (npe): true");
> 42: return;

Return is from catch is redundant now.
Suggestion:

-

PR: https://git.openjdk.java.net/jdk/pull/2377

Re: [OpenJDK 2D-Dev] RFR: 6211257: BasicStroke.createStrokedShape(Shape): NPE is not specified [v2]

[Alexey Ivanov]

On Thu, 4 Feb 2021 04:15:03 GMT, Prasanta Sadhukhan  
wrote:

>> Method createStrokedShape(Shape) for java.awt.BasicStroke class throws 
>> NullPointerException when passed a null object reference for a input 
>> parameter but it's not specified in the spec.
>> Updated spec to illustrate this.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Doc change to @throws

test/jdk/java/awt/BasicStroke/TestNullShape.java line 44:

> 42: return;
> 43: }
> 44: throw new RuntimeException("NPE is expected");

I would suggest moving this into the try-block as it would make the intention 
clearer: if NPE is not thrown, it's a failure.

Also catch should be on the same line as the closing brace of try.

-

PR: https://git.openjdk.java.net/jdk/pull/2377

Re: [OpenJDK 2D-Dev] RFR: 8197825: [Test] Intermittent timeout with javax/swing JColorChooser Test [v4]

[Alexey Ivanov]

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

Re: [OpenJDK 2D-Dev] RFR: 8197825: [Test] Intermittent timeout with javax/swing JColorChooser Test [v4]

[Alexey Ivanov]

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

Re: [OpenJDK 2D-Dev] RFR: 8197825: [Test] Intermiitent timeout with javax/swing JColorChooser Test [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8197825: [Test] Intermiitent timeout with javax/swing JColorChooser Test [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8197825: [Test] Intermiitent timeout with javax/swing JColorChooser Test [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8197825: [Test] Intermiitent timeout with javax/swing JColorChooser Test [v3]

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8197825: [Test] Intermiitent timeout with javax/swing JColorChooser Test [v2]

[Alexey Ivanov]

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

[OpenJDK 2D-Dev] Integrated: 8260314: Replace border="1" on tables with CSS

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8260314: Replace border="1" on tables with CSS

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8260314: Replace border="1" on tables with CSS

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8260314: Replace border="1" on tables with CSS [v3]

[Alexey Ivanov]

> 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: [OpenJDK 2D-Dev] RFR: 8260314: Replace border="1" on tables with CSS [v2]

[Alexey Ivanov]

> 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: [OpenJDK 2D-Dev] RFR: 8260314: Replace border="1" on tables with CSS

[Alexey Ivanov]

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: [OpenJDK 2D-Dev] RFR: 8260314: Replace border="1" on tables with CSS

[Alexey Ivanov]

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

[OpenJDK 2D-Dev] RFR: 8260314: Replace border="1" on tables with CSS

[Alexey Ivanov]

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 

[OpenJDK 2D-Dev] Integrated: 8240247: No longer need to wrap files with contentContainer

[Alexey Ivanov]

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