Merge FileImageDescriptor into URLImageDescriptor and clean it up #2899
Conversation
HannesWell
force-pushed
the
unify-ImageDescriptor-logic
branch
from
e6c5232 to
a1eada2
Compare
|
Honestly this now feels it become completely unmaintainable and hardly understandable, can we not simply use an abstract base-class to share code instead of all this inlined lamda and hype-generic method calls? |
I'm not so sure if that becomes much simpler, but I can have a look. |
|
While looking at both classes again I got the impression that actually the |
HannesWell
force-pushed
the
unify-ImageDescriptor-logic
branch
from
bd58803 to
1289fc9
Compare
HannesWell
changed the title
I have now taken this path further and didn't found a reason speaking against it. The tests should now pass as well. |
HannesWell
force-pushed
the
unify-ImageDescriptor-logic
branch
from
1289fc9 to
3d2506d
Compare
In it's core the FileImageDescriptor queries the given context class for a resource at the given path and then operates on the returned URL or directly fetches the resource's stream. By optaining the resource's URL immediately and using an URLImageDescriptor with it a lot of similar code and logic from the FileImageDescriptor can be saved. Additionally apply a few minor code clean-ups and remove a unused internal method.
HannesWell
force-pushed
the
unify-ImageDescriptor-logic
branch
from
3d2506d to
1f8f8eb
Compare
|
This contains now much less, i.e. just one additional lambda in the zoom-image computation and is, from my side, ready for submission. |
There was a problem hiding this comment.
I really like this simplification. I did not check in detail whether everything the FileImageDescriptor did is properly captured by the URLImageDescriptor as well. So far, I just found one place where I am not sure if behavior is properly preserved.
If you want me to do a more in-depth review, let me know, but I am not sure how soon I will find the time for it. So from my side, feel free to further process this PR if you are confident that everything works as expected.
| @@ -84,7 +87,11 @@ protected ImageDescriptor() { | |||
| * @return a new image descriptor | |||
| */ | |||
| public static ImageDescriptor createFromFile(Class<?> location, String filename) { | |||
| return new FileImageDescriptor(location, filename); | |||
| URL url = location != null ? location.getResource(filename) : null; | |||
There was a problem hiding this comment.
There was some more complex logic for retrieving the URL, in particular for the case that location was null. Is it intended to remove that?
| return null; | ||
| } | ||
|
|
||
| private static <R> R getZoomedImage(URL url, String urlString, int zoom, Function<URL, R> getImage) { |
There was a problem hiding this comment.
nit: this does actually not return an image (as indicated by the method name), but something depending on the type parameter, such as some image data or even just a path to the image. But I have to admit that I have no better generic name to propose.
In it's core the FileImageDescriptor queries the given context class for a resource at the given path and then operates on the returned URL or directly fetches the resource's stream. By optaining the resource's URL immediately and using an URLImageDescriptor with it a lot of similar code and logic from the FileImageDescriptor can be saved.
Additionally apply a few minor code clean-ups and remove a unused internal method.