8282526: Default icon is not painted properly #7805
Conversation
Detect the situation when icon image painting requires interpolation and set the interpolation to bicubic to preserve the icon details as much as possible.
|
👋 Welcome back kizune! A progress list of the required criteria for merging this PR into |
|
@azuev-java The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
The icon returned by the system is 32x32 icon that looks like this: |
Webrevs
|
| g.drawImage(image, x, y, observer); | ||
| if (hintChanged) { | ||
| ((Graphics2D) g).setRenderingHint(RenderingHints.KEY_INTERPOLATION, | ||
| oldHint == null ? RenderingHints.VALUE_INTERPOLATION_NEAREST_NEIGHBOR : oldHint); |
There was a problem hiding this comment.
This might be a rookie question:
When it comes to modifying and restoring a Graphics, I've seen two common patterns:
A. Call newG = g.create() and later newG.dispose()
B. Call g.setX(newValue) and then call g.setX(oldValue) later
Is there guidance on when to use which? Or is one always preferred?
There was a problem hiding this comment.
This might be a rookie question:
When it comes to modifying and restoring a Graphics, I've seen two common patterns: A. Call
newG = g.create()and laternewG.dispose()B. Callg.setX(newValue)and then callg.setX(oldValue)laterIs there guidance on when to use which? Or is one always preferred?
Usually i follow the rule of minimal action. When i need to set a lot of hints that needs to be later unrolled such as changing graphics coordinates and such - i'm creating a new Graphics. When i only need to manipulate a single hint and it can easily be unrolled - i am using the same object. In this case i'm changing just a single hint.
There was a problem hiding this comment.
I am not sure that this is the right place to change the hints.
If we want to always scale the icon using the bicubic interpolation then it is better to do it where we call the paintIcon() method in some jcomponent(this probably may be configured by some option - similar to the text antialisaing). But it will affect performance since the bicubic is much slower.
But before discussing this shared code change could you please provide some info on why it worked fine before? Why we try to downscale the image and do not get an exact size from the native.
There was a problem hiding this comment.
The reason for the issue to manifest is that due to the high resolution icon fix on Windows we are returning a multiresolution image now and the new code that fetches the icons for some icons returns 32x32 icon no matter what resolution is requested so we ended up with an 16x16 multiresolution icon with only variant available 32x32 so icon painter is doing the scaling. One of the possible solutions i considered is to interpolate the icon in the process of creation but that will only hide the problem because you see - in my test i was able to reproduce the scaling problem without the Windows code, simply by wrapping the larger icon into smaller MultiResolutionIcon. And i do not think that changing hints in every component is a good idea - because it will require these components to be much more aware of the icon's internal mechanic which is not a great idea. And taking into consuderation that we only enable bicubic interpolation for icons that really require it - when icon does not have a resolution variant that fits the current icon's size - it should not be a serious performance impact. In majority of the cases interpolation is not required.
There was a problem hiding this comment.
I suggest to recheck the issues described above, updating the shared implementation of ImageIcon.java seems wrong
| public Object getProperty(String name, ImageObserver observer) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
This test reads a little strange to me. (I'm not an official openjdk reviewer, though, so feel free to disregard.)
If I run this test with the existing ImageIcon class it creates an image that resembles:
If I run this test with the revised code it creates an image that resembles:
Based on the changes in ImageIcon: I only expected to see changes related to rendering hints. (That is: I expected to see differences in antialiasing and scaling artifacts.)
Would you mind if I proposed a different test?
There was a problem hiding this comment.
Well, the difference in the images generated is the demonstration of how significant parts of the original image gets lost during the rendering. Not sure why you think this test is not valid.
There was a problem hiding this comment.
Ah, I figured out part of my confusion:
In my case (my primary monitor was set to 100% resolution): the ImageIcon we get back uses a BufferedImage that is 16x16. So when it is displayed on a 150% monitor it has to scale up.
Depending on your monitor resolution: you might start at a 32x32 BufferedImage that is scaling down. Or you might not have a BufferedImage at all: you might have their custom MultiResolutionIconImage (which is backed by a BufferedImage).
So all that is to say: I misunderstood the original code's design. And now it sounds like there a lot of separate convos here, so I'll make this my last comment unless someone wants to follow-up with anything.
Additional context/feedback:
The image in question is coming from Win32ShellFolder2#makeIcon(..)
<init>:315, BufferedImage (java.awt.image)
makeIcon:1020, Win32ShellFolder2 (sun.awt.shell)
call:1066, Win32ShellFolder2$15 (sun.awt.shell)
call:1039, Win32ShellFolder2$15 (sun.awt.shell)
run$$$capture:264, FutureTask (java.util.concurrent)
run:-1, FutureTask (java.util.concurrent)
- Async stack trace
<init>:132, FutureTask (java.util.concurrent)
newTaskFor:108, AbstractExecutorService (java.util.concurrent)
submit:139, AbstractExecutorService (java.util.concurrent)
invoke:621, Win32ShellFolderManager2$ComInvoker (sun.awt.shell)
invoke:519, ShellFolder (sun.awt.shell)
invoke:505, ShellFolder (sun.awt.shell)
getIcon:1039, Win32ShellFolder2 (sun.awt.shell)
getSystemIcon:249, FileSystemView (javax.swing.filechooser)
main:19, SwingDemo
If we wanted to apply a resolution somewhere other than the ImageIcon class:
We could instead change makeIcon() to return a custom MultiResolutionImage that always applies interpolation. Here's a variation of the original bug's app that models this idea:
https://docs.google.com/document/d/1fcXJ9g6uTTZZlIt1BfzNFg2-Hs05CCpNRjyG1FnXKns/edit?usp=sharing
I could spin this up as a PR or discuss this further if requested. I'm not saying it's necessarily better; I'm just brainstorming other approaches.
| ((Graphics2D) g).setRenderingHint(RenderingHints.KEY_INTERPOLATION, | ||
| oldHint == null ? RenderingHints.VALUE_INTERPOLATION_NEAREST_NEIGHBOR : oldHint); |
There was a problem hiding this comment.
Isn't the state of the Graphics object modified? Previously, the KEY_INTERPOLATION hint was set to null but it is set to VALUE_INTERPOLATION_NEAREST_NEIGHBOR on exit. Why can't it be set to null? Always to oldHint?
|
The problem is attempt to set it back to null causes NPE. null value means it is not set and there is no method to remove the hint. |
If the default value for |
Yes, that code depends on the understanding that if no hint is set than default behavior is as if it set to VALUE_INTERPOLATION_NEAREST_NEIGHBOR. The alternative would be to create a new Graphics, set the hint there and discard that graphics immediately after painting. |
| g.drawImage(image, x, y, observer); | ||
| if (hintChanged) { | ||
| ((Graphics2D) g).setRenderingHint(RenderingHints.KEY_INTERPOLATION, | ||
| oldHint == null ? RenderingHints.VALUE_INTERPOLATION_NEAREST_NEIGHBOR : oldHint); |
There was a problem hiding this comment.
So this is one of only two hints which do not have an "_DEFAULT" value. I don't know why it doesn't.
Not authoritative but the only place where we currently set this hint and then restore it is here
share/classes/javax/swing/plaf/nimbus/AbstractRegionPainter.java
Object oldScaleingHints = g.getRenderingHint(RenderingHints.KEY_INTERPOLATION);
g.setRenderingHint(RenderingHints.KEY_INTERPOLATION,RenderingHints.VALUE_INTERPOLATION_BILINEAR);
ImageScalingHelper.paint(g, 0, 0, w, h, img, insets, dstInsets,
ImageScalingHelper.PaintType.PAINT9_STRETCH, ImageScalingHelper.PAINT_ALL);
g.setRenderingHint(RenderingHints.KEY_INTERPOLATION,
oldScaleingHints!=null?oldScaleingHints:RenderingHints.VALUE_INTERPOLATION_NEAREST_NEIGHBOR);
if the get() returns null and you aren't happy with that then creating a new Graphics is the only alternative I see.
But it seems to me there are bigger problems to resolve first
Image variant = ((MultiResolutionImage) image).getResolutionVariant(this.width, this.height);
So getResolutionVariant wants image pixels, but I expect ImageIcon is sized in user-space, isn't it ?
Don't you need to apply the scale from the Graphics ?
Next, if we have an exact match for what we want, why do we need the hint ?
Next, as Sergey mentions, BICUBIC is expensive.
Probably how that should be used is to redraw the image into a cached image that
is dw= iwsx and dh= iwhsy where sx and sy are the graphics scale
and that is the graphics on which you apply the hint and the graphics is disposed and the restore issue vanishes
then you can just blit that resulting image so long as the cached dw x dy isn't changed.
The MR is still useful for finding the best starting image
There was a problem hiding this comment.
If I understand correctly, the problem occurs only when the image is scaled down. The test case renders 32×32 icon into 16×16 image.
So is the usage of bicubic interpolation need only when the image is scaled down?
Create a new Graphics2D object instead of juggle the rendering hints;
|
Ok, i have modified PR to use the scale factor to determine if we will need the scaling operation at all and i'm creating a new Graphics2D object so i do not need to swap the rendering hints. Saying that BICUBIC is expensive - i have found any noticeable performance impact when using this code. And it is not always the scale down that needs this hint - even scaling up of the icons when scaling factor is set to 125 or 175% gives much better result for icons that do not have the exact match in size for the said scaling. |
Yes, the issue manifests itself only on Windows - but i was able to reproduce it using generic MultiResolutionIcon in the test case so while it CAN be fixed in the Windows specific code i still think addressing it in the shared code is a right thing to do. |
Fixing it in the ImageIcon is similar to fixing it by changing internal default state of the rendering pipeline in any other places like images/graphics/fonts, while we have to change that state "externally". We have to set BICUBIC interpolation in the place where we draw that image/icon. But before discussing that, it would be good to understand:
|
We are setting the BICUBIC only in place where we draw the image part of ImageIcon. There is no benefit in pushing this change down the pipeline.
It is not a regression, it is a side effect of the JDK-8182043 combined with the Windows 10 update that changed the default windows file icon to the blank white sheet with 1 pixel wide dark gray border.
Because we can not read this icon from the native, when we ask Windows API to provide this icon it ignores the icon size and gives the same 32x32 icon instead. We knew this upfront and the consensus was that we will provide the icon in the MultiResolutionImage and scaling during painting will be good enough to scale it properly. But it is not. The default scale algorithm is terrible and when new icons appeared it became obvious. Aside of that default scaling on fractional magnification factors such as 5/8 or 7/8 looks just bad - a lot of scaling artifacts and missing details. That has to be addressed and that's what i'm trying to do. |
We don't need to push change down to the pipeline, we should push it up to the place where we paint this ImageIcon, eventually where we call ImageIcon.paintIcon()
Then why, as described in the JBS, it worked before? Did we request different image, or size, or did we skip scaling? |
Why? As i shown in the test case the problem affects any ImageIcon based on the MultiResolutionImage. I'm not trying to solve the singular issue that in the Windows LaF - i am trying to eliminate the reason this problem can show up.
Because before that change we were using different binary API to retrieve icons. This API only allows gathering 8x8 and 16x16 icons. The new API can be used for requesting icons of any size but for some files it ignores the requested size and only returns 32x32 icon. In this case we creating the multi resolution image with that icon inside and allow icon painting routine do the scaling. As i shown in my test for this bug the scaling works poorly and here i'm trying to enhance it. Can it be done in WinLAF? Yes, absolutely, but it will not solve the more generic issue. |
Default interpolation for all rendering in java2d is NN, if the user wants he may change it to any other modes. This is consistent for all types of images, and MultiResolutionImage is not any special. In this particular case the code where we call ImageIcon.paintIcon() may set different interpolation when some system property is set, similar to the antialiasing. But it is not so important since the root cause below.
So this is the actual root cause of the problem: the change in behavior of the FileSystemView.getSystemIcon() caused by using a different API that ignores the passed size. Since the size 16x16 is the default size for getSystemIcon() it would be good to restore the old behavior(mix the new and old) and request the best resolution from the system. It will save some memory and provide the best performance. |
|
It could be that the new API that we use on Windows ignores the passed in size and returns the icon of the default size. If possible, we should explore options to make this work correctly. Is a test case available for that? Do I simply start JFileChooser on standard screen which uses 100% scale and look for files without a defined type? On the other hand, @azuev-java demonstrates the problem exists in general: if an icon is scaled down, it may lose relevant details and become white square — it doesn't look good, does it? It's possible to give user/programmer control by adding a property: if it's defined, ImageIcon paints as before without switching to bicubic interpolation. |
Yes, but if we downscale then this is applicable for any other types of icons, or images. If the user render it directly then he can turn on the bicubic option, if we render it ourselves (like in this case) we can provide a solution I referenced above, similar to the text anti-aliasing, when the user may set a special client_property. But these are two different issues, one of them - specific to MRI and the previous fix where we changed the behavior, and another one - an addition customization of how the Swing renders the images. But before we provide such customization we need to check how slow it will be, in some cases it could be x10(or x1000? I do not remember) times slower. |
get the 16x16 icon which is better for scaling purposes.
|
I have fixed the 16x16 icon generation on Windows, but this will not help the generic problem that i demonstrated with the test case, on the magnification factors like 125% or 175% without the proposed fix for the ImageIcon the result looks trashy - jagged lines, missing details. I have done testing with the modified regression test case to see how my change affects performance. Notice that this is the worst case scenario, when all of the paintings trigger the fix (which is only triggered for MultiResolutionImage based icons and only when this MRI does not have the exact match for the requested resolution and scaling is required - this is like 0.01% of overall cases because for majority of image icons they are either single image based or MRI has the variant requested ready for display. And even in this worst case scenario on 1000 iterations test does not show any difference. On 10000 iterations the difference is measurable in tens of milliseconds between test runs. On 100000 iterations test execution with BICUBIC approximation is noticeable (like test run takes more than a second longer). We can limit the maximum size of icon we process by some reasonable number, like do not process icons bigger than 256 pixel wide or high so we do not cause any noticeable slowdown, but i still think this needs to be done - otherwise on these odd magnifications we look nothing alike the native applications (windows looks like using some sort of bicubic interpolation for their icons) and just plain bad. |
| if (size < 24) { | ||
| size = 16; | ||
| } | ||
| hres = pIcon->Extract(szBuf, index, &hIcon, NULL, size); | ||
| hres = pIcon->Extract(szBuf, index, &hIcon, &hIconSmall, size); |
There was a problem hiding this comment.
Looking into this change I assume that the reason why the windows "ignores" the requested size of 16x16 was that we request the size 16x16 but after that used the large icon only which is 32x32 by default, is it correct?
The last parameter of this method:
nIconSize
The desired size of the icon, in pixels. The low word contains the size of the large icon, and the high word contains the size of the small icon. The size specified can be the width or height. The width of an icon always equals its height.
Do we need to update the size value and take into account the low/hi sizes?
There was a problem hiding this comment.
Do we need to update the size value and take into account the low/hi sizes?
Not exactly, the issue is that for some files (but just for some of them) this call ignores the high icon size parameter and always returns the set of small and large icons as 16x16 plus 32x32. For the rest of the files it returns 16x16 and the requested size. If low size is not set it is assumed to be 16x16 so we can omit setting it.
There was a problem hiding this comment.
Do we need to update the size value and take into account the low/hi sizes?
Not exactly, the issue is that for some files (but just for some of them) this call ignores the high icon size parameter and always returns the set of small and large icons as 16x16 plus 32x32. For the rest of the files it returns 16x16 and the requested size. If low size is not set it is assumed to be 16x16 so we can omit setting it.
I can't see anything about that in the documentation for IExtractIconW::Extract which says:
nIconSize - The desired size of the icon, in pixels. The low word contains the size of the large icon, and the high word contains the size of the small icon.
For the return value, the documentation says, “Returns S_OK if the function extracted the icon, or S_FALSE if the calling application should extract the icon.” We don't even check the return value.
| hIcon = hIconSmall; | ||
| } else { | ||
| fn_DestroyIcon((HICON)hIconSmall); | ||
| } |
There was a problem hiding this comment.
Does it mean that we will extract all images except on the sides twice? for 16x16 we will extract 16x16 and 32x32 on the next iteration for 32x32 we will extract 32x32 and 64x64?
Can we try to do that via different API: https://devblogs.microsoft.com/oldnewthing/20140501-00/?p=1103 probably it will work better?
There was a problem hiding this comment.
Does it mean that we will extract all images except on the sides twice? for 16x16 we will extract 16x16 and 32x32 on the next iteration for 32x32 we will extract 32x32 and 64x64?
No, on the next iteration we will extract 16x16 and 64x64. So on each iteration we are going to extract additionally 16x16 icon and if size is 24 or more than we will ignore it. Since all the icons requested are cached that will not cause any performance degradation.
There was a problem hiding this comment.
Can we try to do that via different API: https://devblogs.microsoft.com/oldnewthing/20140501-00/?p=1103 probably it will work better?
No, during initial implementation i tried it and it worked even worse and less stable.
There was a problem hiding this comment.
But can we use it as a backup if the system does not return the expected size on the initial/current call?
There was a problem hiding this comment.
Does it mean that we will extract all images except on the sides twice? for 16x16 we will extract 16x16 and 32x32 on the next iteration for 32x32 we will extract 32x32 and 64x64?
No, on the next iteration we will extract 16x16 and 64x64. So on each iteration we are going to extract additionally 16x16 icon and if size is 24 or more than we will ignore it. Since all the icons requested are cached that will not cause any performance degradation.
Where it is specified that the size of the small icon will be the same and the size of the large will grow? Probably it is related to the size property where we did not set the size of the small icon?
I still do not see a reason why we cannot request the exact size of the image from the windows, so if the small icon and the screen scale is 1.25 we can try to extract 20x20
There was a problem hiding this comment.
But can we use it as a backup if the system does not return the expected size on the initial/current call?
No, because in case when current implementation returns bad results this function also returns incorrect results so it is hardly a backup.
There was a problem hiding this comment.
Because for some files - and we can not predict which files these are - function will always return set of 16x16 and 32x32 icons - no matter what we request. For the icons that return proper sizes we do request exact sizes and use them.
How do we request the proper size for the example above when the scale is 1.25 and the "small" image? The requested size should be 20x20 but we will request 16x16 or 32x32 and then upscale/downscale, no?
There was a problem hiding this comment.
No, because in case when current implementation returns bad results this function also returns incorrect results so it is hardly a backup.
Are you sure? That article said the opposite and suggest to use SHDefExtractIcon if the simple extract fails. Probably we should recheck why it does not work for your old testing it seems this should be even faster since it read only one image instead of small+large.
There was a problem hiding this comment.
Can we try to do that via different API: https://devblogs.microsoft.com/oldnewthing/20140501-00/?p=1103 probably it will work better?
No, during initial implementation i tried it and it worked even worse and less stable.
I can't remember SHDefExtractIcon was even tried. According to the documentation for IExtractIconW::Extract and to the article, another method is to be used when IExtractIconW::Extract returns S_FALSE.
Previously, we discussed Raymond Chen's post The format of icon resources and LookupIconIdFromDirectoryEx function.
No, on the next iteration we will extract 16x16 and 64x64. So on each iteration we are going to extract additionally 16x16 icon and if size is 24 or more than we will ignore it. Since all the icons requested are cached that will not cause any performance degradation.
I must admit I still don't like this approach much, we extract 16×16 icon even when we don't need it.
Is it better to request small icon only when the size is less than 32?
I think it makes sense to submit a bug to explore using LookupIconIdFromDirectoryEx to extract icons so that we have the set of icons which is provided provided in the resources and avoid getting scaled icons. There should be another bug to add fallback to using SHDefExtractIcon for the case where IExtractIconW::Extract returns S_FALSE.
These are separate issues from the icon rendering, even though icon extraction issues revealed the problem with rendering, scaling up/down the icons to be specific.
There was a problem hiding this comment.
Because for some files - and we can not predict which files these are - function will always return set of 16x16 and 32x32 icons - no matter what we request. For the icons that return proper sizes we do request exact sizes and use them.
How do we request the proper size for the example above when the scale is 1.25 and the "small" image? The requested size should be 20x20 but we will request 16x16 or 32x32 and then upscale/downscale, no?
We don't request proper size, we always request a list of size. If 16×16 icon is requested, then 20×20 up to 16*2 = 32.
jdk/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java
Lines 1150 to 1151 in 7f52c50
Such MRI handles scales from 1.0 to 2.0. Maybe we should increase the list to 3.0, some Windows laptop default to 300%.
Not an ideal benchmark but it shows that the bicubic is ~200 times slower. Time: 21313400 |
It depends on the native accelerated rendering pipeline implementation, which does not support bicubic and do it in on software loops.
The same question could be asked about all other the icons, why we should upscale/downscale by using the bicubic only the MRI images. The same issue may occur for other icons. For example the FileSystemView.getSystemIcon() may return the icon stored in the "FileView.directoryIcon" UI property. Or the user may create its own image, etc. The reason was about performance. The problem right now is that the user cannot select the "good" appearance if he wants, unlike the text antialiasing. My suggestion is to add a UI property which we discussed above. |
- Removed the old test - Added a new windows-specific test - Fixed the small icon retrieval - If there is no exact match for requested variant create a new one from the closest match by scaling it with bicubic interploation - The new variand is cached so no real performance hit there
|
Here, in the name of resolving this issue finally i have reverted the ImageIcon changes, instead i added some code that creates the properly scaled version of icon when requested variant is not available. Plus i have fixed the small icon retrieval so it is not an issue anymore. I am not going trough all the experiments with the SHDefExtractIcon again - at least not in this pull request. I checked and there is no performance impact at all from the latest changes on the 100% scaled screens due to the extracting an additional icon - looks like Windows extracts it anyways even if we specify null as a icon information reference - it just does not pass it down. |
This looks good!
I absolutely agree, not in this pull request. However, I'm for submitting a bug or two to explore other possibilities.
It could be the case. The shell may keep and likely keeps a cache of file icons for small and large icons. Windows used to have only two sizes of icons: |
| if (size < 24) { | ||
| size = 16; | ||
| } | ||
| hres = pIcon->Extract(szBuf, index, &hIcon, NULL, size); | ||
| hres = pIcon->Extract(szBuf, index, &hIcon, &hIconSmall, size); |
There was a problem hiding this comment.
Do we need to update the size value and take into account the low/hi sizes?
Not exactly, the issue is that for some files (but just for some of them) this call ignores the high icon size parameter and always returns the set of small and large icons as 16x16 plus 32x32. For the rest of the files it returns 16x16 and the requested size. If low size is not set it is assumed to be 16x16 so we can omit setting it.
I can't see anything about that in the documentation for IExtractIconW::Extract which says:
nIconSize - The desired size of the icon, in pixels. The low word contains the size of the large icon, and the high word contains the size of the small icon.
For the return value, the documentation says, “Returns S_OK if the function extracted the icon, or S_FALSE if the calling application should extract the icon.” We don't even check the return value.
| hIcon = hIconSmall; | ||
| } else { | ||
| fn_DestroyIcon((HICON)hIconSmall); | ||
| } |
There was a problem hiding this comment.
Can we try to do that via different API: https://devblogs.microsoft.com/oldnewthing/20140501-00/?p=1103 probably it will work better?
No, during initial implementation i tried it and it worked even worse and less stable.
I can't remember SHDefExtractIcon was even tried. According to the documentation for IExtractIconW::Extract and to the article, another method is to be used when IExtractIconW::Extract returns S_FALSE.
Previously, we discussed Raymond Chen's post The format of icon resources and LookupIconIdFromDirectoryEx function.
No, on the next iteration we will extract 16x16 and 64x64. So on each iteration we are going to extract additionally 16x16 icon and if size is 24 or more than we will ignore it. Since all the icons requested are cached that will not cause any performance degradation.
I must admit I still don't like this approach much, we extract 16×16 icon even when we don't need it.
Is it better to request small icon only when the size is less than 32?
I think it makes sense to submit a bug to explore using LookupIconIdFromDirectoryEx to extract icons so that we have the set of icons which is provided provided in the resources and avoid getting scaled icons. There should be another bug to add fallback to using SHDefExtractIcon for the case where IExtractIconW::Extract returns S_FALSE.
These are separate issues from the icon rendering, even though icon extraction issues revealed the problem with rendering, scaling up/down the icons to be specific.
|
Does the generic problem still exist? The one that you demonstrated with a sample image? If the user loads an icon which has lines, the lines may disappear if the icon is scaled down. The problem could be seen with regular The solution is to apply BICUBIC rendering hint, which could be not as easy: the icon is likely used in a Swing component rather than rendered directly. |
Better yet is to create the file dynamically in the test code? |
| @@ -1435,6 +1437,15 @@ public Image getResolutionVariant(double width, double height) { | |||
| } | |||
| } | |||
| } | |||
| if (retVal.getWidth(null) != w) { | |||
| BufferedImage newVariant = new BufferedImage(w, w, BufferedImage.TYPE_INT_ARGB); | |||
There was a problem hiding this comment.
This version look good, just one more thought. Can we extend this approach and do not request all sizes if the user wanted only one image? So if the user will request all images we request that list from the OS, otherwise only one image will be requested and cached, what do you think?
There was a problem hiding this comment.
This version look good, just one more thought. Can we extend this approach and do not request all sizes if the user wanted only one image? So if the user will request all images we request that list from the OS, otherwise only one image will be requested and cached, what do you think?
How would you implement this?
I mean the user requests the icon in user's space (right?). On a High DPI system which may change in runtime, a larger icon version is required for rendering. To get a larger icon from native, we have to keep the file name or PIDL and call shell interfaces on COM thread to fetch the icon.
Overall, I like the idea… We may have discussed it. Keeping only sizes that are needed will reduce the memory usage. All in all, it's definitely out of scope for this fix.
There was a problem hiding this comment.
How would you implement this?
I mean the user requests the icon in user's space (right?). On a High DPI system which may change in runtime, a larger icon version is required for rendering. To get a larger icon from native, we have to keep the file name or PIDL and call shell interfaces on COM thread to fetch the icon.
Overall, I like the idea… We may have discussed it. Keeping only sizes that are needed will reduce the memory usage. All in all, it's definitely out of scope for this fix.
That was the whole purpose of the initial 8182043 issue, to provide a HiDPI(or various DPI) icons for the user. If the user asks for the 16x16 we will provide them the MRI where the base image is 16x16, but that MRI could also provide large images by request. If that 16x16 image renders on scale=1.25 then the MRI should generate the image which looks good on that screen. This current proposal generates it on the fly using java code, but I think it should request that information from the native system, this was the purpose of the new getSystemIcon(file,w,w) method.
We do not need to always request the icon via COM thread, we can initially load the low dpi image, or the image for the main screen scale, then if the scale changed we can request to load the HiDPI image, and use it when ready.
The wondering why it looks bad on some DPI is actually proves that the previous fix has some limitations.
There was a problem hiding this comment.
That was the whole purpose of the initial 8182043 issue, to provide a HiDPI(or various DPI) icons for the user. If the user asks for the 16x16 we will provide them the MRI where the base image is 16x16, but that MRI could also provide large images by request. If that 16x16 image renders on scale=1.25 then the MRI should generate the image which looks good on that screen.
JDK-8182043 has enhanced support for High DPI, which can be confirmed by displaying a JFileChooser (in SwingSet2) and moving it from one monitor to another or changing the scale of a monitor where it's displayed.
Before JDK-8182043, the icon was either 16×16 or 32×32 and didn't change if the scale was changed. Now the icon gets updated after the scale changes, it's noticeable on folder icons in user's home because a set of icons is fetched.
There are still two different ways where icons are fetched from the Windows shell: Java_sun_awt_shell_Win32ShellFolder2_getIcon and Java_sun_awt_shell_Win32ShellFolder2_extractIcon. As @azuev-java notes, at times the returned size doesn't match the requested size.
This current proposal generates it on the fly using java code, but I think it should request that information from the native system, this was the purpose of the new getSystemIcon(file,w,w) method.
This current proposal handles the situation where the returned icon doesn't have the requested size.
So, there are drawbacks and limitations in the current implementation, and there are opportunities to improve it, some of them are listed in my comment above. I shall submit them so that we don't forget about them, if no one else has already done it.
We do not need to always request the icon via COM thread, we can initially load the low dpi image, or the image for the main screen scale, then if the scale changed we can request to load the HiDPI image, and use it when ready.
Why not? To request the icon of a larger size, the object which stores the icon should keep the reference to the file or folder, folders could have custom icons, and Documents, Downloads, Pictures are the examples of such folders. This must be done on COM thread because the icons are requested from the Windows shell.
I agree that we should not get all possible resolutions of an icon when only one is needed. But it's out of scope of this particular issue.
There was a problem hiding this comment.
That was the whole purpose of the initial 8182043 issue, to provide a HiDPI(or various DPI) icons for the user. If the user asks for the 16x16 we will provide them the MRI where the base image is 16x16, but that MRI could also provide large images by request. If that 16x16 image renders on scale=1.25 then the MRI should generate the image which looks good on that screen.
Before JDK-8182043, the icon was either 16×16 or 32×32 and didn't change if the scale was changed. Now the icon gets updated after the scale changes, it's noticeable on folder icons in user's home because a set of icons is fetched.
Before the JDK-8182043 we also used the MRI so it should be changed if the scale was changed from low to hidpi. It adds more variants which could be used by some other DPI, I think one important new variant is the 64x64 because it used as a large icon on HiDPI monitor. But if the dpi is 125% or 150% it will not work even if the user will try to request such image.
There are still two different ways where icons are fetched from the Windows shell:
Java_sun_awt_shell_Win32ShellFolder2_getIconandJava_sun_awt_shell_Win32ShellFolder2_extractIcon. As @azuev-java notes, at times the returned size doesn't match the requested size.
We had similar "issue" before the latest version of this change, we still have an issue related to the size value initialization. So it might be other our bug caused that the size is ignored.
This current proposal handles the situation where the returned icon doesn't have the requested size.
Take a look to the description of 8182043, the user wanted to get some large Icon, now imaging he would like to get a 79x79 icon for some file, did we provide a way to do that? I assume we even do not try to request that size from the operation system so it actually cannot ignore our request?
So, there are drawbacks and limitations in the current implementation, and there are opportunities to improve it, some of them are listed in my comment above. I shall submit them so that we don't forget about them, if no one else has already done it.
+1
Why not? To request the icon of a larger size, the object which stores the icon should keep the reference to the file or folder, folders could have custom icons, and Documents, Downloads, Pictures are the examples of such folders. This must be done on COM thread because the icons are requested from the Windows shell.
Because selection of the proper icon/image will be done during rendering of the component, and waiting of the results on the com thread may slowdown it. Instead we can use default image which we initially loaded, and later use the better one when it will be loaded on the com thread.
I agree that we should not get all possible resolutions of an icon when only one is needed. But it's out of scope of this particular issue.
Yes, please take a look to these change.
There was a problem hiding this comment.
Before the JDK-8182043 we also used the MRI so it should be changed if the scale was changed from low to hidpi. It adds more variants which could be used by some other DPI, I think one important new variant is the 64x64 because it used as a large icon on HiDPI monitor.
MRI was used only to accommodate for the fact that shell may return icon of higher resolution according the system settings.
jdk/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java
Lines 1032 to 1034 in 978bed6
If the main monitor scale was set to 200% and 16×16 icon was requested, the system returned 32×32.
In no way it handled different scale settings.
But if the dpi is 125% or 150% it will not work even if the user will try to request such image.
The current code by @azuev-java handles this situation because the icon returned will contain five variants for the range of 100%, 125%, 150%, 175%, 200%.
I don't clearly understand your concern here.
Take a look to the description of 8182043, the user wanted to get some large Icon, now imaging he would like to get a 79x79 icon for some file, did we provide a way to do that? I assume we even do not try to request that size from the operation system so it actually cannot ignore our request?
No, we do not request 79×79 because such an icon never exists in Windows because Windows shell does not use such a size to display a file. Well, it may exist if a developer for some reason included it in the app resources but it would rather be an exceptional case.
So if a Java app developer wants 79×79 icon, it will be scaled down from 96×96 or 128×128.
There was a problem hiding this comment.
Why not? To request the icon of a larger size, the object which stores the icon should keep the reference to the file or folder, folders could have custom icons, and Documents, Downloads, Pictures are the examples of such folders. This must be done on COM thread because the icons are requested from the Windows shell.
Because selection of the proper icon/image will be done during rendering of the component, and waiting of the results on the com thread may slowdown it. Instead we can use default image which we initially loaded, and later use the better one when it will be loaded on the com thread.
This sounds reasonable… until you take into account the fact the Icon or ImageIcon has no reference to a component. When the icon is requested, the scale at which will be rendered is unknown.
When an icon is rendered and the required variant does not exist in the MRI, there's no way to fetch it asynchronously: the icon can't make the component repaint because there's no reference to the component.
The only optimisation that I see is to request only the size which correspond to the current monitor scales. If a different size is needed, a synchronous call to COM thread is required; and for this the icon must keep the path to the file.
There was a problem hiding this comment.
This sounds reasonable… until you take into account the fact the
IconorImageIconhas no reference to a component. When the icon is requested, the scale at which will be rendered is unknown.
Correct.
When an icon is rendered and the required variant does not exist in the MRI, there's no way to fetch it asynchronously: the icon can't make the component repaint because there's no reference to the component.
The drawImage method takes the observer, which can be used to trigger repaint when the new image is reloaded, this is how the animated gifs are rendered in our components.
The only optimisation that I see is to request only the size which correspond to the current monitor scales. If a different size is needed, a synchronous call to COM thread is required; and for this the icon must keep the path to the file.
You missed the point that the goal of that fix was to provide the icons for the file(eventually be a kind of wrapper for "Extract"), the possibility to render them in HiDPI is a side effect. So we should be able to do both:
- If the user request the list of variants we should provide the list of icons we are able to load from the file, but the API is flaky to get the actual list of
- If the Icon is rendered to the image we may load the icon of the different DPI.
I submitted a list of issues to record all the comments that were raised here as well as in previous reviews.
|
the test folder. Verified on automatic run.
|
I have updated the test so test file is created and deleted during the testing so name is irrelevant as long as the extension is not registered and we get the default Windows icon. |
|
@azuev-java This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 1009 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
| @@ -1435,6 +1437,15 @@ public Image getResolutionVariant(double width, double height) { | |||
| } | |||
| } | |||
| } | |||
| if (retVal.getWidth(null) != w) { | |||
There was a problem hiding this comment.
It isn't in this webrev but i jyst noticed
// We only care about width since we don't support non-rectangular icons
I think that meant non-square ???
There was a problem hiding this comment.
It isn't in this webrev but i jyst noticed
// We only care about width since we don't support non-rectangular icons
I think that meant non-square ???
Yes. Now it seems to be apparent that English is not my native language...
|
/integrate |
|
Going to push as commit 6c8d0e6.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@azuev-java Pushed as commit 6c8d0e6. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
I came across this PR while researching the same issue using Windows11 w/ JDK8. If my comment is of merit, I can provide the exact JDK8 build. As I read the history of this PR, I can't help but feel that maybe it's gone a little off course? I agree with the reasoning for the addition of: However, I'm not sure I agree that forcing the legacy method I hope I've expressed my reasoning well. Just thought some input from "the field" might be of some value. p.s. I also realize this PR is closed but it still seemed like the best place to leave the feedback since several other issues were spawned from this one. |
Detect the situation where we do need to perform interpolation during ImageIcon
painting and set a hint to the rendering to perform bicubic approximation so
image details are preserved during transition.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/7805/head:pull/7805$ git checkout pull/7805Update a local copy of the PR:
$ git checkout pull/7805$ git pull https://git.openjdk.org/jdk pull/7805/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7805View PR using the GUI difftool:
$ git pr show -t 7805Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/7805.diff