Replace and deprecate Image creating methods accepting Strings as file-system paths

[HannesWell]

Using Strings to represent file-system paths is usually a bad idea since using String as type lacks context (String is almost like a primitive) and requires that callers handle OS-specific file-separators properly.

This PR proposes to provide alternatives for existing Image creating methods that accept Strings to represent file-system paths and to deprecate the existing methods, but not for removal since they probably have a huge user-base.

The type File is used instead of the more modern java.nio.file.Path to avoid confusion with the Path class that already exists in the 'o.e.swt.graphics' package.

There are multiple callers left that have to be adapted but I want to reach consensus about the new API first.

[github-actions]

Test Results

   539 files   -  6     539 suites   - 6   30m 57s ⏱️ + 2m 58s
 4 332 tests  - 37   4 322 ✅  - 35    9 💤  - 3  1 ❌ +1 
16 587 runs   - 37  16 479 ✅  - 35  107 💤  - 3  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 6f0e9a0. ± Comparison against base commit 9a9efe1.

This pull request removes 37 tests.

AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testHtmlTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromCopiedImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageData
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageDataFromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testRtfTransfer
…

♻️ This comment has been updated with latest results.

[HannesWell]

There are multiple callers left that have to be adapted but I want to reach consensus about the new API first.

Furthermore with this the existing ImageFileNameProvider should be deprecated too and replaced with a ImageFileProvider returning a File too.

But with all the ongoing effort on improving high-DPI support I wonder if this should be part of a greater API rework? Not that I'm aware of one, but I would like to make sure that there aren't concurrent, maybe even conflicting efforts.
@HeikoKlare could you help here?

[laeubi]

The type File is used instead of the more modern java.nio.file.Path to avoid confusion with the Path class that already exists in the 'o.e.swt.graphics' package.

I think this is a bad choice, as Path is not really "modern" but much more flexible and while each File can be converted into a Path the reverse is not true. Also o.e.swt.graphics.Path usage is quite rare in SWT application and actually this is for what namespaces where made in java.

So adding any new API should also use new/modern/better types and it would allow to use alternative sources beside local files.

[HeikoKlare]

Yes, we plan to rework the Image class because of the HiDPI improvements. And actually, the proposed change is conflicting to what we need to do for that.

The reason is that we currently have a bunch of constructors that create "static" images. That means, these images are created once for a specific zoom value and can then only be rescaled to other zoom values via scaling the raster graphics (leading to blurry results). We are working on replacing all these "static" image initializations with "dynamic" ones, i.e., those that are able to create/render an image for whatever zoom is required (like already possible with ImageFileNameProvider/ImageDataProvider). That is the reason why we, for example, created a new constructor taking an "image drawer" that is capable of rendering an on-demand image in whatever zoom value is required as a replacement for just creating an image with fixed width/height and drawing into it: #1734

This is necessary as with the enhanced HiDPI support on Windows it must be possible to always retrieve an image according to the zoom of the monitor the containing shell is currently placed on. If you are interested, you find an analysis of static/dynamic image construction in our backlog: vi-eclipse/Eclipse-Platform#171 (comment)

I can have a further look at this PR and the neccessity of it to find a solution that is conforming with the enhanced HiDPI support. I just have to leave now for the rest of the day, so I at least wanted to give you the above comment to make you aware of that "conflict".

[HeikoKlare]

So having now taken another look at the proposal, I am not yet completely sure about the reason for these changes. I agree that the existing APIs are outdated and need to be reworked, but is there a specific trigger for doing it now and in this way? In particular, do we need a replacement for the deprecated constructor at all? We would actually propose to deprecate the Image constructor accepting a string (referring to a local file) without adding a new one as a replacement for it. Instead, the deprecation notice should give advise how to create an image in a "dynamic" way, i.e., with the ability to rescale according to requested zoom (if possible) based on the existing providers.

Taking a look at current usages of new Image(device, filename) in the Eclipse platform, I only see two that are not in tests or examples, which are inside FileImageDescriptor and URLImageDescriptor. I would try to replace those two usages with ImageFileNameProviders (or maybe some replacement of that based on File/Path instead of String) and then completely get rid of this constructor. I have not taken a look at other consumers like JDT to see whether there may be more usages of that method, but I did not find that many blurry-scaled images with the new monitor-specific scaling mode on Windows, but all images created with that constructor would become blurry when changing monitor scaling.

[HannesWell]

The type File is used instead of the more modern java.nio.file.Path to avoid confusion with the Path class that already exists in the 'o.e.swt.graphics' package.

I think this is a bad choice, as Path is not really "modern" but much more flexible and while each File can be converted into a Path the reverse is not true. Also o.e.swt.graphics.Path usage is quite rare in SWT application and actually this is for what namespaces where made in java.

Ok, if it's not widely used I'll change it to Path. Personally I cannot remember to have used SWT's path, but I'm also not developing that many UIs.

So having now taken another look at the proposal, I am not yet completely sure about the reason for these changes. I agree that the existing APIs are outdated and need to be reworked, but is there a specific trigger for doing it now and in this way?

Yes, the specific trigger is #1638 (comment)
With the current state of the PR we would add new constructors that use Strings for file-system paths and I would like to avoid that.

In particular, do we need a replacement for the deprecated constructor at all? We would actually propose to deprecate the Image constructor accepting a string (referring to a local file) without adding a new one as a replacement for it. Instead, the deprecation notice should give advise how to create an image in a "dynamic" way, i.e., with the ability to rescale according to requested zoom (if possible) based on the existing providers.

Not having a simple constructor that allows to directly create an image from a file would make the creation of images less simple, but 'forcing' users to use the more capable constructors is probably better on the long run.

On the other hand not all images are icons. Maybe one just wants to display an image image because that person has implemented a simple image-viewer. Of course one can always use zoom -> Path.of("myImage.png") instead of just Path.of("myImage.png").

Taking a look at current usages of new Image(device, filename) in the Eclipse platform, I only see two that are not in tests or examples, which are inside FileImageDescriptor and URLImageDescriptor. I would try to replace those two usages with ImageFileNameProviders (or maybe some replacement of that based on File/Path instead of String) and then completely get rid of this constructor. I have not taken a look at other consumers like JDT to see whether there may be more usages of that method, but I did not find that many blurry-scaled images with the new monitor-specific scaling mode on Windows, but all images created with that constructor would become blurry when changing monitor scaling.

Using ImageFileNameProvider respectively a new ImageFileProvider or maybe better ImagePathProvider sounds like an interesting idea. I'll check how that can be integrated with #1638, but in general this sounds suitable.

In general I think it's good we are talking about it. :)

[HeikoKlare]

On the other hand not all images are icons. Maybe one just wants to display an image image because that person has implemented a simple image-viewer. Of course one can always use zoom -> Path.of("myImage.png") instead of just Path.of("myImage.png").

I see, valid point. From a design perspective, it would of course make sense to separate these two use cases and distinguish between "dynamic" and "static" images already by type. But for the sake of API compatibility, that's of course not that easy. So probably we will need to provide separate constructors that clearly state what they are supposed to be used for, i.e., those being used for scalable graphics like icons need to be "provider-based" while plain images may be created directly via a file.

Yes, the specific trigger is #1638 (comment)
With the current state of the PR we would add new constructors that use Strings for file-system paths and I would like to avoid that.

Just to get this correctly: the PR does currently only require new ImageData constructors, but not necessary Image constructors, does it? So wouldn't it be possible to only extend ImageData and wrap the conversion String -> File inside the existing Image constructor? I am just asking for the sake of understanding, not that I propose to make this the final solution :-)

[HannesWell]

I have now reduced the scope of this PR to just cover ImageData and the introduction of new convenience factories as replacement for the existing convenience constructors (using Path instead of String) and the introduction of an ImageFileProvider as replacement for ImageFileNameProvider.
I added the latter as introducing and migrating to the former does not really work seamlessly without the latter.
But at least for me, both parts are clearly the way to go and I don't see alternatives (at the moment).
If you disagree, please let me know.

Furthermore this also inlines the ImageDataLoader helper class, that can be replaced with the new ImageData factories and whos purpose (nowadays) isn't clear anymore. The javadoc says:

/**
 * Internal class that separates ImageData from ImageLoader to allow removal of ImageLoader from the toolkit.
 */

I can only speculate what this means exactly and where (or in which distributions?) ImageLoader would have been removed, but I'm not aware of any relevance for today.

[HannesWell]

Instead of load() this method could also be named create(), but the former seems a bit more suitable for me.

[HannesWell]

Theoretically we could also just use a IntFunction<Path> instead, but this would make especially the documentation inconvenient in many places and we might loose some additional context, so I think a dedicated interface is fine for it.

On the other hand, this interface could at least extend IntFunction<Path> and default implement its apply() method.

Suggested change

	public interface ImageFileProvider {
	public interface ImageFileProvider implements IntFunction<Path> {

[HeikoKlare]

What would be the benefit of implementing that interface? It is not that expressive, so I like the semantically precise implementation that is proposed more.

[HannesWell]

The advantage is that it could be used directly in existing methods that accept an IntFunction, but I agree that we should have a dedicated interface for this purpose.
The remaining question for me is if it should extend IntFunction, so that such provider could for example be used directly with an IntStream. But I'm not sure if that that's really done that often and worth the side-effects this would have? OTOH, the side-effects are probably not that impactful, I assume?

[HeikoKlare]

From my point of view, the disadvantage is that it prevents you from declaring a method properly reflecting its semantics (like the current getImagePath(zoom)) and gives you a meaningless apply(value) instead. Functional interfaces make sense for defining lambas or other anonynous types, but not being implemented by an actual interface or class as usually they are too generic to actually use their methods in a proper OO way.

[HannesWell]

@HeikoKlare how should we proceed here respectively what do you think about the current (reduced compared to the initial) state of this PR? Since there is currently work happening at the same area, rebasing this again and again is a bit tedious. :)
Do you or anybody else think this is fine, should something be adapted or should it be postponed for longer?

[HeikoKlare]

I have to admit that I still have no strong opinion about this. I agree that it definitely improves API when using properly typed Path objects in favor of strings to identify files. On the other hand, I am still wondering what kind of problem will be resolved by this (the linked PR does not contain any related change anymore). This produces quite some effort to adapt to and basically the exchange will be that every ImageFileNameProvider will be replaced by an ImageFileProvider that just wraps the string in a Path.of(...) or the like. But I have taken a look into our product and found that we have no further ImageFileNameProvider implementation next to the two ones existing in JFace already.

On the long, we will of course have better APIs and usages of them. Still we need to be aware that we will probably not be able to remove the ImageFileNameProvider within the next years (even if we mark it for removal) since there may be so many consumers that we would eventually break.

Based on the above considerations (especially the finding that there do not seem to be that many implementations of that interface outside of Eclispe itself), I would be slightly in favor of doing the change. I have made few comments but we will probably have to make another review once this is rebased on latest master again.

[HeikoKlare]

What would be the benefit of implementing that interface? It is not that expressive, so I like the semantically precise implementation that is proposed more.

[HeikoKlare]

Why is this placed in DPIUtil`? I don't see any DPI-relation here. Shouldn't it be at some other place for some common Image-related implementations?

[HannesWell]

It's not related at all. I just didn't found a shared/common internal Util class that seems suitable, but I agree that it should be in something Image related.
I have now moved it as package-private method to ImageData, but ImageLoader would also be possible. I found didn't find one much more suitable than the other. What's your preference or do you have a much better other place?

[HeikoKlare]

Probably there is no really good place as the best would obviously be inside Image, but that class is not common so code would need to be replicated. Adding a utility class only for that method also seems to be a bit inappropriate even though it would probably be the "cleanest" we could do. So maybe ImageData is probably the least bad place :-)

One minor addition: with the recent rename of ImageFileProvider to ImagePathProvider this method should be renamed to asImagePathProvider accordingly.

[HannesWell]

I have to admit that I still have no strong opinion about this. I agree that it definitely improves API when using properly typed Path objects in favor of strings to identify files. On the other hand, I am still wondering what kind of problem will be resolved by this (the linked PR does not contain any related change anymore).

Modelling file-system paths as String is a major source of problems and is also inconvenient, therefore I would strongly discourage from doing that. And with that in mind I think it's bad that currently users are forced to use String to model paths.
So although this was triggered by the SVG work, I think this is still important, even if it's not necessary for that work.

This produces quite some effort to adapt to and basically the exchange will be that every ImageFileNameProvider will be replaced by an ImageFileProvider that just wraps the string in a Path.of(...) or the like.

That's possible, but maybe it's also the other way round and it will allow implementers to remove .toString() calls from the paths they want to provide.

On the long, we will of course have better APIs and usages of them. Still we need to be aware that we will probably not be able to remove the ImageFileNameProvider within the next years (even if we mark it for removal) since there may be so many consumers that we would eventually break.

The replaced methods are not marked for removal. They are just deprecated to make developers aware that these methods/constructors have better alternatives. If and when they are really removed, should be discussed later. But currently I would say there is no harm to keep them for longer, maybe even forever.

[HeikoKlare]

Thank you for the additional thoughts. It all sounds fine for me. As said, I am in favor of using Path of String and I am just a bit concerned that this will introduce complexity without being adopted in a way that gives benefit. Still I agree that we should aim for a more longer-term goal and thus should try to make things better by doing this kind of improvement. So feel free to merge this once you consider it ready.