Extend existence check in URLImageDescriptor when running without OSGi #3106
+43
−20
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This e.g. breaks the scaled image data retrieval in DPIUtil: |
ptziegler
force-pushed
the
url-image-path
branch
2 times, most recently
from
5c2261f to
33196c4
Compare
laeubi
reviewed
Jul 14, 2025
| if (FILE_PROTOCOL.equalsIgnoreCase(url.getProtocol())) | ||
| return IPath.fromOSString(url.getFile()).toOSString(); | ||
| if (FILE_PROTOCOL.equalsIgnoreCase(url.getProtocol())) { | ||
| String filePath = IPath.fromOSString(url.getFile()).toOSString(); |
There was a problem hiding this comment.
can't we use Path.of(...) directly.
There was a problem hiding this comment.
I think Path.of(url.toURI()).toString() is even less readable than IPath.fromOSString(url.getFile()).toOSString(), in addition to also having to deal with the URISyntaxException.
There was a problem hiding this comment.
IPath.fromOSString(url.getFile()).toOSString()
to be honest this looks really bogus, why not simply
Suggested change
| String filePath = IPath.fromOSString(url.getFile()).toOSString(); | |
| String filePath = url.getFile(); |
but I more was about that below you are using a path anyways so
p = Path.of(url.toURI())
if (Files.exists(Path.of(p))) {
return p.toString();
}
There was a problem hiding this comment.
why not simply
As mentioned, you now have to also worry about the potential URISyntaxException and are now depending on something ambiguous as a toString() method...
But as long as the method behaves consistently, I don't really mind either way.
This extends the check added in cff785d to also consider the case when OSGi isn't running. Otherwise a path to a non-existent file is returned, thus breaking the contract specified in the JavaDoc.
ptziegler
force-pushed
the
url-image-path
branch
from
33196c4 to
abd678c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This extends the check added in cff785d to also consider the case when OSGi isn't running. Otherwise a path to a non-existent file is returned, thus breaking the contract specified in the JavaDoc.