-
Notifications
You must be signed in to change notification settings - Fork 863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DirectoryChooserUI: reduce reflective win ShellFolder API usage #8110
Conversation
- some internal methods are now public in FileSystemView and can be used instead of using reflection - remove calls to ShellFolder.getShellFolder(file) entirely this hopefully fixes the mysterious NPEs on windows caused by broken java.io.File instances with null paths.
File sf = useShellFolder? getShellFolderForFile(canonical) : canonical; | ||
File f = sf; | ||
File f = canonical; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this didn't seem to make a difference, removing this call allows us to remove the ShellFolder.getShellFolder(file)
call site entirely, which has no public API equivalent.
There is a chance that this is the place which injected the broken java.io.File
instances. If the public FileSystemView
methods would have caused this, other projects would likely have noticed it too by now.
also see #8040 (comment) and #8040 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. BTW, what this useShellFolder
does on Windows? I see a lot of custom code around it here and in the DelegatingChooserUI
as well.
Is that still relevant/useful feature to have?
@lkishalmi I think its a special windows specific file-link, can be "Desktop", "recent files", a drive or network folder (potentially a custom file explorer shortcut). The JDK makes it a special |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look OK; I assume you were testing it on Windows.
I don't have a Windows machine around at the moment, but I tested it on MacOS and at least confirmed it didn't immediately cause any problems there.
on win 10. Also on linux, but none of the methods return anything interesting on linux still #8040 (comment).
awesome, going to merge it soon |
FileSystemView
and can be used instead of using reflectionShellFolder.getShellFolder(file)
entirely since there is no public equivalent stillthis hopefully fixes the mysterious NPEs on windows caused by broken
java.io.File
instances with null paths.fixes #7067
fixes #8040