Extract remaining image specific meta data into additional AbstractImageProviderWrapper

[akoch-yatta]

As ImageDataProvider and ImageFileNameProvider details were refactored into internal sub classes already, attributes that are only used for the "standard case" remain in the Image class. Those should be extracted into a third AbstractImageProviderWrapper.
Some usages of package protected attributes in other parts of SWT must be evaluated and probably adapted in the same step to make this possible

[akoch-yatta]

Some results on the further analysis of the Image class. There are currently the following types of images:

1. Initialized with ImageFileNameProvider
2. Initialized with ImageDataProvider
3. Initialized with ImageDrawingProvider (Add ImageDrawingProvider interface for Images #143)
4. Initialized by concrete size
5. Initialized with ImageData
6. Initialized with InputStream
7. Initialized with file name
8. Initialized with other Image

I would group them up into:

Dynamic Images (1 & 3):

All OS handles with those images could be created only on demand -> reduce number of handles. As the bounds (in points) of the image is needed in some places, those values could be calculated on creation time with a temporary handle, that is disposed afterwards. Currently one persistent handle is created is retained.
These kinds of images would not need any kind of initial zoom value.

Static Images (4 -8):

For each Static image the OS handle is created in the constructor

Images with concrete size

Common use case of those images is using them together with a GC, like

Image image = new Image(device, 16, 16);
Gc gc = new GC(image);
// draw stuff with the GC

As the drawing will be done on the OS bitmap in the end, this bitmap must be initialized with a concrete size in pixels. The zoom to identify the scale factor from points to pixels is currently DPIUtil.getNativeDeviceZoom(). An additional constructor with the desired target zoom is necessary to have a support for multi-zoom scenario. #143 is designed so most of the usages of this constructor into Dynamic Images with ImageDrawingProvider.
There are some more complex examples like #138 where multiple Images and multiple GCs are involved and used for double buffering that will not be translatable to ImageDrawingProviders

Tasks regarding this kinds of images

• Finish Add ImageDrawingProvider interface for Images #143

Image with ImageData

There are two kinds of constructors: only with ImageData or with ImageData source + ImageData mask. These Images will intialize an OS handle with the passed data. As ImageData is always in pixels an assumption about how the ImageData was created must be done. The existing implementation assumes the passed ImageData is created for a zoom of 100. This data will be scaled to target zoom (again DPIUtil.getNativeDeviceZoom()) and then used for the initialization of the handle. This whole process be adaptable to a dynamic approach by wrapping it into an ImageDataProvider -> Assumption about ImageData being passed at zoom of 100 will still hold here. This will always lead to destructive scaling!

Tasks regarding this kinds of images

• Dynamize by wrapping cloned ImageData into an ImageDataProvider
• Re-evaluate existing usages of the constructors -> could this be solved in a better non-destructive way

Image with InputStream

The Image will intialize an OS handle with the passed stream as image data. As the passed data is always in pixels an assumption about how the data was created must be done. The existing implementation assumes the passed datais created for a zoom of 100. This data will be scaled to target zoom (again DPIUtil.getNativeDeviceZoom()) and then used for the initialization of the handle.
This looks like a legacy constructor that is still be used at some points. I would propose to deprecate this constructor and recommend to replace the calling places with a "Dynamic Image" alternative

Tasks regarding this kinds of images

• Deprecate the constructor and replace all usages with a "Dynamic Image" alternative

Image with file name

The Image will intialize an OS handle with the passed filename loaded as image data. As the passed data is always in pixels an assumption about how the data was created must be done. The existing implementation assumes the passed datais created for a zoom of 100. This data will be scaled to target zoom (again DPIUtil.getNativeDeviceZoom()) and then used for the initialization of the handle.
This looks like a legacy constructor that is still be used at some points, e.g. when a image file is chosen with a FileDialog and then used to e shown somewhere. The usages could be checked if they could be replaced, but I think the usages and the assumption as being on 100% data is fine.

Image with other Image

This will copy the specified Image or an adapted (e.g. greyed) variant of the Images and will inherit the attributes of the passed Image. I don't see any changes necessary here.

Conclusion

Best scenario would be to get rid of all Static Images if possible and provide simple ways to translate them into Dynamic Images

Steps to do

• Finish ticket: Add ImageDrawingProvider interface for Images #143
• Create ticket: calculate bounds for dynamic Image on creation time with a temporary handle, that is disposed afterwards
• Create ticket: Introduce additional constructor with zoom for 4. Images with concrete size
• Create ticket: Dynamize by wrapping cloned ImageData into an ImageDataProvider for 5. Image with ImageData
• Create ticket: Re-evaluate existing usages of the constructors for 5. Image with ImageData
• Create ticket: Deprecate the constructor and replace all usages with a Dynamic Image 6. Image with InputStream
• Create ticket: Extract logic for non Dynamic Images into AbstractImageProviderWrapper

[HeikoKlare]

The analysis appears overall sound to me. I completely agree with the conclusion that static images should, as far as possible, be replaced by dynamic ones.

This is also a good chance of getting rid of outdated constructors and the API up to only provide reasonable, dynamic way of creating images. When deprecating constructors, we should provide examples for how to replace their usage with proper, dynamic ways of creating the according image. So far, we completely agree.

Taking a look at the analysis and the image implementation again, I got a slightly extended / different understanding of the overall issue. I try to summarize the questions arising for me and a provide a concrete proposal (not deeply thought through) in the following.

Central Question(s)

But I have one central comprehension question: is the problem really about dynamic vs. static images?
Let's assume I create an image (no matter whether it's dynamic or static). The image I receive conforms to some zoom (currently the native zoom, either by retrieving the image in that size from a dynamic source or by scaling a static one to that size). So if I, for example, create a GC for that image, it will have a size according to that zoom (which is currently dependent on the native zoom either used for retrieving a dynamic image in that size or for scaling up a static one). This is where it is important for which zoom the image was originally created. This does not even depend on whether the image is static or dynamic, it is rather that you will usually not want to modify dynamic images.
For the case that I only want to draw an image, it will be retrieved in the proper zoom (via getImageData(zoom)) anyway. This is then either dynamically calculated or upscaled from the original image that was based on size, ImageData, InputStream or any other static data source.

So isn't the issue rather about the necessity to modify (or create) an image than about whether it is static or dynamic? Given that, I would expect most (if not all) images that will be modified to be created explicitly, i.e., via a constructor only taking a size, which is the scenario that #143 addresses, if I am not mistaken.
Note that every image will still have a zoom, which applies to the "default" data of the image, i.e., the one used when, for example, creating a GC on that image. When adding a constructor taking the image size and a zoom, that one will be the only not using nativeZoom as the default zoom, which also seems a bit inconsistent, but from my understanding addresses exactly the problem of modifying an image at a desired zoom.

So for me the first question to answer is: given an image, what zoom can or should I expect it to conform to? Should that be the same for every image? Should it be the nativeZoom like now (maybe then some initialNativeZoom that is static to the application and never changes)?

Proposal

As a completely new idea: what if we stick to the current behavior of initializing every image with some zoom and provide some method to change the zoom of that image. This would mean that a dynamic image would replace its "default" data with the one conforming to the zoom to change to (which could then be overwritten with a GC, even though that is not recommended) and a static image would be scaled (just like the constructor does now).
Then everyone requiring an image at a specific zoom (e.g., for modifying it) could first adapt it to that zoom. Probably, the only relevant use case is the creation of an image based on a size to be filled for a specific zoom afterwards.

We could then even say that with monitor-specific scaling enabled (where is nothing like a nativeDeviceZoom to be used as the initial zoom), we could just create everything at 100% and do not perform any scaling and only scale via the proposed new method on demand.

As an example:

Image selfPaintedImage = new Image(device, 16, 16); // will be 16x16, no matter what native zoom we currently have
selfPaintedImage.changeZoom(getZoom()); // adapts the image to the zoom of the current context, e.g., a control
GC gc = new GC(selfPaintedImage); 
// ... do some painting
gc.dispose();

Additional Questions

I have few questions:

• "Dynamic Images" also includes 2 (ImageDataProvider), doesn't it?
• I see that all calls to init() withint the constructors are made with getZoom(), i.e., the "corrected" zoom value in case of integer/integer200 scale mode. Is that correct or shouldn't that be nativeZoom? I am just wondering whether the scaling through all these methods currently provides proper results at all, but probably it does because in case of integer/integer200 only images for 100/200% are required and requested anyway

[HeikoKlare]

Additional note from today's discussion: we should log an error or at least a warning when creating a GC on any image that is not supposed to be used for modifying it. This includes at least:

• all dynamic images (retrieved via any of the existing providers)
• all images for which handles in other zoom values have already been created

[akoch-yatta]

I created sub-tickets out of the discussion here:

• Dynamize Image constructors with ImageData #198
• Evaluate calling places of Image constructors with ImageData #199
• Create handles for ImageDataProvider and ImageFilenameProvider on demand #200
• Replace all calls to the Image constructor with InputStream #202
• Dynamize Image constructors with Rectangle #203
• Log an error, when GC with Image in unsupported use cases #204

This ticket will be used for its original task to extract one or more additional AbstractImageProviderWrapper subclass for the non-dynamic image cases