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 all 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
@@ -72,31 +72,35 @@ static void negativeTests() {

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

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);

if (icon == null) {
throw new RuntimeException("icon is null!!!");
throw new RuntimeException(file.getAbsolutePath() + " icon is null!!!");
}

if (implComplete && icon.getIconWidth() != size) {
throw new RuntimeException("Wrong icon size " +
icon.getIconWidth() + " when requested " + size);
icon.getIconWidth() + " when requested " + size +
" for file " + file.getAbsolutePath());
}

if (icon.getImage() instanceof MultiResolutionImage) {
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");
}
} else {
if (implComplete) {
throw new RuntimeException("icon is supposed to be multi-resolution but it is not");
+ size + " in the multi resolution icon " + file.getAbsolutePath());
}
}
}