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

8269269: [macos11] SystemIconTest fails with ClassCastException #178

wants to merge 3 commits into from

Conversation

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Jun 29, 2021

Added check for the icon class type
Added check if file or folder we are testing against exists and
accessible
Removed test from problem list


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

  • JDK-8269269: [macos11] SystemIconTest fails with ClassCastException
  • JDK-8269637: javax/swing/JFileChooser/FileSystemView/SystemIconTest.java fails on windows

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/178/head:pull/178
$ git checkout pull/178

Update a local copy of the PR:
$ git checkout pull/178
$ git pull https://git.openjdk.java.net/jdk17 pull/178/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 178

View PR using the GUI difftool:
$ git pr show -t 178

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/178.diff

Added check for the icon class type
Added check if file or folder we are testing against exists and
accessible
Removed test from problem list
@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Jun 29, 2021

/issue add 8268280

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 29, 2021

👋 Welcome back kizune! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Jun 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 29, 2021

@azuev-java The issue 8268280 was not found in the JDK project - make sure you have entered it correctly.
As there were validation problems, no additional issues will be added to the list of solved issues.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 29, 2021

@azuev-java The following label will be automatically applied to this pull request:

  • swing

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.

Loading

@openjdk openjdk bot added the swing label Jun 29, 2021
@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Jun 29, 2021

/issue add JDK-8268280

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 29, 2021

@azuev-java The issue 8268280 was not found in the JDK project - make sure you have entered it correctly.
As there were validation problems, no additional issues will be added to the list of solved issues.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 29, 2021

Webrevs

Loading

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Jun 29, 2021

/issue add JDK-8269637

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 29, 2021

@azuev-java
Adding additional issue to issue list: 8269637: javax/swing/JFileChooser/FileSystemView/SystemIconTest.java fails on windows.

Loading

@victordyakov
Copy link
Contributor

@victordyakov victordyakov commented Jun 29, 2021

@prsadhuk @azvegint please review

Loading

@@ -72,9 +72,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()) {
Copy link
Member

@mrserb mrserb Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@azuev-java azuev-java Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@mrserb mrserb Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

Loading

Copy link
Member Author

@azuev-java azuev-java Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Loading

Copy link
Member

@mrserb mrserb Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

Copy link
Member Author

@azuev-java azuev-java Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@azuev-java azuev-java Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@mrserb mrserb Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

Copy link
Member Author

@azuev-java azuev-java Jul 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@mrserb mrserb Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

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

@mrserb mrserb Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@azuev-java azuev-java Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@mrserb mrserb Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@azuev-java azuev-java Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading


ImageIcon icon = (ImageIcon) i;
Copy link
Member

@mrserb mrserb Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@azuev-java azuev-java Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@mrserb mrserb Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@azuev-java azuev-java Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@mrserb mrserb Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@azuev-java azuev-java Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@mrserb mrserb Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

azuev-java added 2 commits Jun 30, 2021
Do not test assumption that icon on Windows is always a MultiResolution
the specification does not state it and under some circumstances it is
not.
@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Jul 14, 2021

Closing this review, the issue will be fixed in the mainline instead.

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants