Fix cursor loading incorrect image size when ImageData is unavailable at requested zoom

[arunjose696]

Description

Before this change, the cursor used DPIUtil.validateAndGetImageDataAtZoom() to get image data. However, this method could return image data at a different zoom level if the exact match wasn't available from the ImageDataProvider. As a result, the cursor could be scaled incorrectly, since the returned image data was assumed to match the requested zoom.

With this change, a temporary Image is created, and Image.getImageData(zoom) is called to get properly scaled image data for the requested zoom level.

Steps to reproduce:

Run the below snippet at 150% monitor zoom. The ImageDataProvider in the snippet returns image data only for zoom levels 100 and 200.

Without this change
When run without any swt.autoScale VM argument, the cursor size was incorrectly scaled to 60×60.
When run with -Dswt.autoScale=quarter, the cursor is 30x30.

With this current fix applied
The cursor is now correctly sized (e.g., 45×45 at 150% zoom).

package org.eclipse.swt.snippets;

import org.eclipse.swt.graphics.*;
import org.eclipse.swt.internal.*;
import org.eclipse.swt.widgets.*;

public class Snippet118 {

   public static void main(String[] args) {

      final Display display = new Display();
      final Shell shell = new Shell(display);
      Rectangle bounds = shell.getBounds();

      shell.setLocation((int) (bounds.width * 0.9f), 100);
      shell.setText("Snippet 118");
      shell.setSize(1000, 1000);

      final Cursor[] cursor = new Cursor[1];
      final ImageData[] imageData = new ImageData[1];



      shell.addPaintListener(event -> {

         GC gc = event.gc;

         ImageData oneImageData = imageData[0];
         if (oneImageData != null) {

            gc.drawText("Cursor width %d  - height %d".formatted(
                  oneImageData.width,
                  oneImageData.height), 10, 50, true);
         }

         int i = 1;


         int monitorZoom = DPIUtil.getNativeDeviceZoom();
         int deviceZoom = DPIUtil.getDeviceZoom();
         float devScale = deviceZoom / 100f;

         gc.drawText("Monitor zoom = %d".formatted(monitorZoom), 10, 140);

         int yPos = 400;
         int tickHeight = 10;

         for (int tickIndex = 0; tickIndex < 11; tickIndex++) {

            int xPos = 100 + tickIndex * 10;
            int yOffset = 0;

            if (tickIndex % 3 == 1) {
               yOffset = 20;
            } else if (tickIndex % 3 == 2) {
               yOffset = 40;
            }

            int xPosUnscaled = (int) (xPos / devScale);
            int YPosUnscaled = (int) (yPos / devScale);

            gc.drawLine(xPosUnscaled, YPosUnscaled, xPosUnscaled, YPosUnscaled + tickHeight);
            gc.drawText(Integer.toString(tickIndex * 10), xPosUnscaled - 5, YPosUnscaled + 12 + yOffset);
         }
      });


      ImageDataProvider imageDataProvider = zoom -> {
  		if(zoom==100)
  			return new ImageData(30, 30, 1, new PaletteData(new RGB(255, 0, 0)));
  		else if(zoom==200)
  			return new ImageData(60, 60, 1, new PaletteData(new RGB(255, 0, 0)));
  		else
  			return null;
  	};


      cursor[0] = new Cursor(display, imageDataProvider, 0, 0);
      shell.setCursor(cursor[0]);


      shell.redraw();

      shell.open();
      while (!shell.isDisposed()) {
         if (!display.readAndDispatch())
            display.sleep();
      }

      if (cursor[0] != null)
         cursor[0].dispose();
      display.dispose();
   }
}

Note : This snippet is adapted from [issue #2057](#2057 (comment) to get the scales

[github-actions]

Test Results

   546 files  + 7     546 suites  +7   29m 15s ⏱️ - 4m 29s
 4 412 tests +54   4 395 ✅ +52   17 💤 +3  0 ❌  - 1 
16 718 runs  +54  16 591 ✅ +52  127 💤 +3  0 ❌  - 1 

Results for commit 8776994. ± Comparison against base commit 216f1ad.

♻️ This comment has been updated with latest results.

[fedejeanne]

This PR seems to work around an issue that comes from the wrong usage of DPIUtil::validateAndGetImageDataAtZoom by avoiding to call that method. Hence these questions:

1. Would it be possible/worth it to fix the error inside the method instead?
2. If the previous is not possible, shouldn't you also hide the method to avoid future errors like the one this PR tries to solve? You could simply move the method to the class where it is still used (there is only 1 left) and make it private.
3. There is also the method DPIUtil::validateAndGetImagePathAtZoom. Does it suffer from the same bug/issue (if yes, address it in a separate issue/PR)

[arunjose696]

1. Would it be possible/worth it to fix the error inside the method instead?

The method currently behaves as described in its Javadoc:
“If there is an image at the specified zoom level, it is returned. Otherwise, the next larger image at 150% and 200% is returned, if existing. If none of these is found, the 100% image is returned as a fallback.”

Given this, it seems the method is functioning as intended, no internal fix appears necessary for the method.

2. If the previous is not possible, shouldn't you also hide the method to avoid future errors like the one this PR tries to solve?

There are currently two usages of this method one in the GTK image class and another in the Windows image class. Moving the method to one of those classes and making it private would result in code duplication.
Additionally, this method internally calls getElementAtZoom, which is private to DPIUtil, so moving it would break that dependency unless we expose getElementAtZoom as well which might not be desirable.

3.There is also the method DPIUtil::validateAndGetImagePathAtZoom. Does it suffer from the same bug/issue (if yes, address it in a separate issue/PR)

Same responses as above for this method as well.

[fedejeanne]

Good points, it seems the method does what it is supposed to (however inconvenient) and GTK compensates for that by resizing the data if necessary:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Image.java

Lines 797 to 798 in 216f1ad

	ElementAtZoom<ImageData> data = DPIUtil.validateAndGetImageDataAtZoom (imageDataProvider, zoom);
	ImageData resizedData = DPIUtil.scaleImageData (device, data.element(), zoom, data.zoom());

Would that be an option too in this PR? It would save us from creating and disposing an image.

[HeikoKlare]

Good points, it seems the method does what it is supposed to (however inconvenient)

I think there is a misunderstanding here: the ImageDataProvider does exactly what it is supposed to do. There is nothing about convenience because that one is an interface to be implemented by consumers that want to provide image data in a specific way. Those consumers will have the data at hand in specific zooms (usually 100%, maybe also 200%). Everything to process this data (such as scaling it to different zooms) is exactly the task of an Image. Putting that in the data provider would not make any sense, as then every consumer would have to implement what is centrally implemented in the Image. Actually, the ImageDataProvider is only a configuration for creating an Image and nothing that is really supposed to be used standalone.

Would that be an option too in this PR? It would save us from creating and disposing an image.

You could do that, but why? The Image already does exactly what we need: it will load the image data at the most appropriate size using the ImageDataProvider and then resize it accordingly. And it does that resizing in the most maintained way, other than all consumers that reimplement that functionality and may use a non-standard, limited, non-to-date way of doing it.

[fedejeanne]

Thank you for the clarification.

I assumed that creating an image to immediately dispose it would be wasteful but I agree that it is better than to rely in outdated code to do the resizing. That is a trade-off I can live with 👍

[fedejeanne]

LGTM after the clarification. @amartya4256 / @HeikoKlare any final thoughts? Shall we merge?