Skip to content
This repository has been archived by the owner. It is now read-only.

8269269: [macos11] SystemIconTest fails with ClassCastException #178

Closed
wants to merge 3 commits into from
Closed
Changes from 2 commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -742,7 +742,6 @@ javax/swing/Popup/TaskbarPositionTest.java 8065097 macosx-all,linux-all
javax/swing/JEditorPane/6917744/bug6917744.java 8213124 macosx-all
javax/swing/JRootPane/4670486/bug4670486.java 8042381 macosx-all
javax/swing/JPopupMenu/4634626/bug4634626.java 8017175 macosx-all
javax/swing/JFileChooser/FileSystemView/SystemIconTest.java 8268280 windows-x64

sanity/client/SwingSet/src/ToolTipDemoTest.java 8225012 windows-all,macosx-all
sanity/client/SwingSet/src/ScrollPaneDemoTest.java 8225013 linux-all
@@ -24,6 +24,7 @@
/*
* @test
* @bug 8182043
* @key headful
* @summary Access to Windows Large Icons
* @run main SystemIconTest
*/
@@ -72,9 +73,20 @@ static void negativeTests() {

static void testSystemIcon(File file, boolean implComplete) {
int[] sizes = new int[] {16, 32, 48, 64, 128};
if (!file.exists() || !file.canRead()) {

This comment has been minimized.

@mrserb

mrserb Jun 29, 2021
Member Outdated

If this file is not accessible the "getSystemIcon" should return "null" you can check it here.

This comment has been minimized.

@azuev-java

azuev-java Jun 30, 2021
Author Member Outdated

If this file is not accessible the "getSystemIcon" should return "null" you can check it here.

If file is not accessible we still return default icon for file or folder - if we, for example, looking at the folder in file explorer with file not readable then we should display some icon for it. It will not be icon from the file itself - because we can not read it - but it will not be null.

This comment has been minimized.

@mrserb

mrserb Jun 30, 2021
Member Outdated

  1. If the file does not exist then the getSystemIcon() will always return null, but what happens if the file cannot be read? We will call the "sf = ShellFolder.getShellFolder" and then we call "sf.getIcon()" and it returns what? As far as I understand it always return MultiResolutionIconImage, no?

  2. If we return some icon when the file cannot be read then you can check it here as well.

This comment has been minimized.

@azuev-java

azuev-java Jun 30, 2021
Author Member Outdated

  • but what happens if the file cannot be read? We will call the "sf = ShellFolder.getShellFolder" and then we call "sf.getIcon()" and it returns what? As far as I understand it always return MultiResolutionIconImage, no?

No, the native code will fail to load the icon but since file does exist and we only return null for nonexistent file we will fall back to UIManager.getIcon which returns icon from the installed LAF resource bundle. This is correct behavior since otherwise we might end in file manager with file that has no icon at all. Right now that will be a single resolution icon for all the LAFs. Again, in the future that can be changed but right now it is a single resolution icon.

This comment has been minimized.

@mrserb

mrserb Jun 30, 2021
Member Outdated

No, the native code will fail to load the icon
So what call will fail? Is the "sf.getIcon()" return null? in what line of code?

This comment has been minimized.

@azuev-java

azuev-java Jun 30, 2021
Author Member Outdated

Yes, from my remote debugging it happens in sf.getIcon() but the circumstances are quite rare - file has to be inaccessible and the user's GUI session should not be initialized - that's why this failure only happens on headless testing. Another solution would be to mark this test as headful - that would solve issue as well.

This comment has been minimized.

@prrace

prrace Jun 30, 2021
Contributor Outdated

I am a bit surprised the tests for this are not already headful. I can imagine all sorts of platform reasons why making them headless is not testing the "real" use case making the tests less than useful.
Ever seen a hidpi headless system ??

This comment has been minimized.

@azuev-java

azuev-java Jun 30, 2021
Author Member Outdated

I am a bit surprised the tests for this are not already headful

Fixed.

This comment has been minimized.

@mrserb

mrserb Jul 1, 2021
Member Outdated

That test was not marked as headful because this API should work in the headless environment as well, if it does not work when we have a bug in the shell classes which should return the icon from the UIManager on any error. I think it worked as expected.

As far as I understand when we cannot access the image of some specific file we tried to get a generic image via shell32.dll, does it fail as well? Is it possible that when we try to create the failback icon here:
https://github.com/openjdk/jdk/blob/82bfc5d45c54fb37dc021bc91fa17efe34f77f44/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java#L1199
we get the icon of the correct size and skip the creation of "MultiResolutionIconImage"? As a result, the test fails because the icon is not multi-resolution.

This comment has been minimized.

@azuev-java

azuev-java Jul 1, 2021
Author Member Outdated

we get the icon of the correct size and skip the creation of "MultiResolutionIconImage"? As a result, the test fails because the icon is not multi-resolution.

Well, it might be this case sometimes but in the case when i was able to reproduce this issue it was null coming from native code so we were falling back to UIManager. I can enforce MRI creation even if size is correct - but again, the method signature does not promise the MRI in all cases so we might just modify the test case to not expect the MRI as granted even for the well-known files.

This comment has been minimized.

@mrserb

mrserb Jul 1, 2021
Member Outdated

When in what line of code we got a null from the native code so the UIManager was used?

This comment has been minimized.

@azuev-java

azuev-java Jul 1, 2021
Author Member Outdated

When in what line of code we got a null from the native code so the UIManager was used?

I haven't got into the exact native part which is failing. The problem is that i was able to reproduce it only twice in a couple of weeks - both time remotely, both time on headless machine. If i insert enough debug output to pinpoint the location it stops reproducing.

This comment has been minimized.

@azuev-java

azuev-java Jul 1, 2021
Author Member Outdated

That test was not marked as headful because this API should work in the headless environment as well

Ok, i removed the headful mark and also removed the testing for icon always be MRI on windows because it is not so under some circumstances and it is within spec to return single resolution icon so this assumption is incorrect.

This comment has been minimized.

@mrserb

mrserb Jul 2, 2021
Member Outdated

so under some circumstances and it is within spec to return single resolution icon

Under what circumstances? Above we found a code path that may incorrectly return the non-MRI images. Do we have some other similar places?

This comment has been minimized.

@azuev-java

azuev-java Jul 8, 2021
Author Member Outdated

Do we have some other similar places?

No, i do not see any other places where we return direct result of OS giving out an icon.

This comment has been minimized.

@mrserb

mrserb Jul 9, 2021
Member Outdated

Then we can just fix this one place, and check how it will work.

System.err.println("File " + file.getAbsolutePath() + " is inaccessible");
return;
}

for (int size : sizes) {
ImageIcon icon = (ImageIcon) fsv.getSystemIcon(file, size, size);

This comment has been minimized.

@mrserb

mrserb Jun 29, 2021
Member

Probably we can wrap the "UIManager.getIcon" in the "ImageIcon"?

This comment has been minimized.

@azuev-java

azuev-java Jun 30, 2021
Author Member

Probably we can wrap the "UIManager.getIcon" in the "ImageIcon"?

Why? In some LaFs that might be a procedural image that generates it content every time paint() is called on it - i do not think wrapping it in the ImageIcon will solve anything.

This comment has been minimized.

@mrserb

mrserb Jun 30, 2021
Member

If it is generated every time, then it will be wrapped every time as well. The difference is that it will not be needed to check the type of the icon. Even now, on Windows it is necessary to check is the returned object ImageIcon or not.

This comment has been minimized.

@azuev-java

azuev-java Jun 30, 2021
Author Member

Even now, on Windows it is necessary to check is the returned object ImageIcon or not.

Yes but since API states that returned type is an Icon it should be done anyways - because we might wrap up Icon in ImageIcon locally but we can't force any 3rd party Java implementation to do the same so there will be requirement to check the returned type - unless we change the API to always return ImageIcon or MultiResolutionImageIcon or something even more complicated.

Icon i = fsv.getSystemIcon(file, size, size);
if (!(i instanceof ImageIcon)) {
// Default UI resource icon returned - it is not covered
// by new implementation so we can not test it
continue;
}

ImageIcon icon = (ImageIcon) i;

This comment has been minimized.

@mrserb

mrserb Jun 29, 2021
Member

One more time would like to highlight that to get the data of the requested image the user need to do one "if" and two "instanceof". Still hope that we can improve this.

    Icon icon = fsv.getSystemIcon(file, width, height);
    if (icon.getIconWidth() != width && icon.getIconHeight() != height) {
        return scaleTheIconInTheSameWayAsBeforeTheFix(icon, width, height);
    } else if (icon instanceof ImageIcon) {
        ImageIcon imageIcon = (ImageIcon) icon;
        if (icon.getImage() instanceof MultiResolutionImage) {
            MultiResolutionImage mri = (MultiResolutionImage) icon.getImage();
            return  mri.getResolutionVariant(width, height);
        } else {
            return imageIcon;
        }
    } else {
        return icon;
    }

This comment has been minimized.

@azuev-java

azuev-java Jun 30, 2021
Author Member

One more time would like to highlight that to get the data of the requested image the user need to do one "if" and two "instanceof". Still hope that we can improve this.

We might when the implementation will be complete on all the relevant platforms, then we can be sure that we can provide specific type of an icon like ImageIcon or even MultiResolutionImage - but until then i would tend to use common denominator like simple Icon.

This comment has been minimized.

@mrserb

mrserb Jun 30, 2021
Member

But you cannot change the return type later, so all these instanceof+casts will be there forever.

This comment has been minimized.

@azuev-java

azuev-java Jun 30, 2021
Author Member

But you cannot change the return type later, so all these instanceof+casts will be there forever.

Well, if there is a reason the API can be amended in the future version of Java. It is not cast in stone. Yes, it will require some justification, but if justification is provided then API can be changed.

This comment has been minimized.

@mrserb

mrserb Jun 30, 2021
Member

Not in this case, you cannot change the public signature.

This comment has been minimized.

@azuev-java

azuev-java Jun 30, 2021
Author Member

Not in this case, you cannot change the public signature.

Well, it is a change in API and should be done within the same process as any API change. But it definitely can be done in the future major release.

This comment has been minimized.

@mrserb

mrserb Jul 1, 2021
Member

No, we cannot change the return type of the method in the public API even via CSR. that's an incompatible change.

//Enable below to see the icon
//JLabel label = new JLabel(icon);
//JOptionPane.showMessageDialog(null, label);
@@ -92,11 +104,12 @@ static void testSystemIcon(File file, boolean implComplete) {
MultiResolutionImage mri = (MultiResolutionImage) icon.getImage();
if (mri.getResolutionVariant(size, size) == null) {
throw new RuntimeException("There is no suitable variant for the size "
+ size + " in the multi resolution icon");
+ size + " in the multi resolution icon " + file.getAbsolutePath());
}
} else {
if (implComplete) {
throw new RuntimeException("icon is supposed to be multi-resolution but it is not");
throw new RuntimeException("icon for " +
file.getAbsolutePath() + " is supposed to be multi-resolution but it is not");
}
}
}