Re: RFR: 8182043: Access to Windows Large Icons [v3]

2021-04-30 Thread 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: RFR: 8182043: Access to Windows Large Icons [v3]

2021-04-30 Thread 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: RFR: 8182043: Access to Windows Large Icons [v3]

2021-04-30 Thread 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: RFR: 8182043: Access to Windows Large Icons [v3]

2021-04-30 Thread 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: RFR: 8182043: Access to Windows Large Icons [v3]

2021-04-30 Thread 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: RFR: 8264125: Specification of Taskbar::getIconImage doesn't mention that the returned image might not be equal to the Taskbar::setIconImage one. (eg on Mac OS)

2021-04-30 Thread Alexander Zvegintsev 
On Mon, 29 Mar 2021 18:57:55 GMT, Alexander Zvegintsev  
wrote:

> This fix is to explicitly specify that `Taskbar::getIconImage` may return an 
> object different from passed to `Taskbar::setIconImage`.
> 
> Actually it is always returns a different object on macOS(the only OS which 
> supports this feature), but I want to save some options if we decide to 
> rework this.

CSR added.

-

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

Re: RFR: 8182043: Access to Windows Large Icons [v3]

2021-04-30 Thread Alexander Zuev 
On Wed, 10 Mar 2021 20:44:19 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/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.

Fixed.

-

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

Re: RFR: 8182043: Access to Windows Large Icons [v3]

2021-04-30 Thread Alexander Zuev 
> 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>

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2875/files
  - new: https://git.openjdk.java.net/jdk/pull/2875/files/6607b61f..4a360621

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=01-02

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

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

Integrated: 8265761: Font with missed font family name is not properly printed on Windows

2021-04-30 Thread Alexander Scherbatiy 
On Thu, 22 Apr 2021 15:13:45 GMT, Alexander Scherbatiy  
wrote:

> PDFBox 1.8 uses 
> [Graphics2D.drawGlyphVector()](https://github.com/apache/pdfbox/blob/41ae21bd4c3f304373d3b05f63af5325df248019/pdfbox/src/main/java/org/apache/pdfbox/pdmodel/font/PDSimpleFont.java#L352)
>   method with scaled glyphs to print a text and PDF 2.0 uses 
> [Graphics2D.fill()](https://github.com/apache/pdfbox/blob/4f14dee47ff821e44d9e2ff11532959d95e94d5b/pdfbox/src/main/java/org/apache/pdfbox/rendering/PageDrawer.java#L512)
>  to print glyphs path. Both methods finally calls 
> WPathGraphics.convertToWPath(...) in jdk on Windows which call GDI FillPath.
> 
> Using a custom PageDrawer to draw a text in PDFBox with 
> Graphics2D.drawString() method reveals the fact that some pdf documents which 
> are properly printed by PDFBox 1.8 and PDF 2.0 on Linux and Windows have 
> issues with printing them on Windows.
> 
> The reason is that such docs use fonts which have empty font family name.
> The awt_PrintJob.jFontToWFontA(...) method is not able to select the required 
> font when the passed font family name is empty.
> https://github.com/openjdk/jdk/blob/7c37c022a1664437bf8d2c8d76ad039521f3ffa7/src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp#L2264
>  
> 
> The proposed solution returns false from WPrinterJob.setFont(...) method when 
> the font family is empty so the text printing falls back to printing a text 
> by GDI FillPath method:
> https://github.com/openjdk/jdk/blob/6d49cc3b655433d00e967fdcec3f3759412cd925/src/java.desktop/windows/classes/sun/awt/windows/WPrinterJob.java#L1157
> 
> To reproduce the issue I created a simple 
> [SampleBowMissedFamilyName.ttf](https://bugs.openjdk.java.net/secure/attachment/94344/SampleBowMissedFamilyName.ttf)
>  font which contains only capital letters "ABCDEF" and saved it with empty 
> font family name.
> 
> Here is a simple 
> [PrintFontSample.java](https://bugs.openjdk.java.net/secure/attachment/94343/PrintFontSample.java)
>  program that helps to reproduce the issue using the 
> SampleBowMissedFamilyName.ttf font.
> 
> The PrintFontSample program draws a text using three methods:
> - Graphics2D.drawString(...)
> - Graphics2D.drawGlyphVector(...)
> - Graphics2D.drawGlyphVector(...) using transformed glyphs 
> 
> Running the program with jdk 16 on Windows (without the fix)
>>  java PrintFontSample SampleBowMissedFamilyName.ttf
> 
> shows that the first and the second lines are not properly printed: 
> [sample-doc-without-fix.pdf](https://bugs.openjdk.java.net/secure/attachment/94345/sample-doc-without-fix.pdf)
> Running the program with the fix properly prints all three lines: 
> [sample-doc-with-fix.pdf](https://bugs.openjdk.java.net/secure/attachment/94349/sample-doc-with-fix.pdf)
> 
> The provided manual test uses the created SampleBowMissedFamilyName.ttf font 
> with empty font family name.

This pull request has now been integrated.

Changeset: e9370a13
Author:Alexander Scherbatiy 
URL:   
https://git.openjdk.java.net/jdk/commit/e9370a13b6f3f99d223ef5966f9e218b94d954b4
Stats: 293 lines in 3 files changed: 293 ins; 0 del; 0 mod

8265761: Font with missed font family name is not properly printed on Windows

Reviewed-by: serb, prr

-

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