Changeset 16486 in josm

Timestamp:

2020-05-23T21:19:14+02:00 (4 years ago)

Author:

simon04

Message:

fix #18694 - Wrongly rendered cursors on HiDPI screen (patch by johsin18)

Location:

trunk

Files:

• src/org/openstreetmap/josm/tools/GuiSizesHelper.java (modified) (1 diff)
• src/org/openstreetmap/josm/tools/HiDPISupport.java (modified) (6 diffs)
• src/org/openstreetmap/josm/tools/ImageOverlay.java (modified) (3 diffs)
• src/org/openstreetmap/josm/tools/ImageProvider.java (modified) (1 diff)
• src/org/openstreetmap/josm/tools/ImageResource.java (modified) (2 diffs)
• test/unit/org/openstreetmap/josm/tools/ImageProviderTest.java (modified) (4 diffs)

trunk/src/org/openstreetmap/josm/tools/GuiSizesHelper.java

@@ -55 +55 @@
     public static float getPixelDensity() {
         return getScreenDPI() / 96f;
+    }
+
+    /**
+     * Sets coefficient of monitor pixel density.
+     * @param pixelDensity coefficient of monitor pixel density to be set.
+     */
+    public static void setPixelDensity(float pixelDensity) {
+        screenDPI = pixelDensity * 96f;
     }
 

trunk/src/org/openstreetmap/josm/tools/HiDPISupport.java

@@ -33 +33 @@
     private static final Constructor<? extends Image> baseMultiResolutionImageConstructor;
     private static final Method resolutionVariantsMethod;
+    private static final Method resolutionVariantMethod;
 
     static {
@@ -38 +39 @@
         baseMultiResolutionImageConstructor = initBaseMultiResolutionImageConstructor();
         resolutionVariantsMethod = initResolutionVariantsMethod();
+        resolutionVariantMethod = initResolutionVariantMethod();
     }
 
@@ -57 +59 @@
         double uiScale = getHiDPIScale();
         if (uiScale != 1.0 && baseMultiResolutionImageConstructor != null) {
-            ImageIcon zoomed = ir.getImageIcon(new Dimension(
+            ImageIcon zoomed = ir.getImageIconAlreadyScaled(new Dimension(
                     (int) Math.round(base.getWidth(null) * uiScale),
-                    (int) Math.round(base.getHeight(null) * uiScale)), false);
+                    (int) Math.round(base.getHeight(null) * uiScale)), false, true);
             Image mrImg = getMultiResolutionImage(Arrays.asList(base, zoomed.getImage()));
             if (mrImg != null) return mrImg;
@@ -139 +141 @@
 
     /**
+     * Wrapper for method <code>java.awt.image.MultiResolutionImage#getResolutionVariant(double destImageWidth, double destImageHeight)</code>.
+     * <p>
+     * Will return the argument, in case it is not a multi-resolution image.
+     * @param img the image
+     * @return if <code>img</code> is a <code>java.awt.image.BaseMultiResolutionImage</code>,
+     * then the result of the method <code>#getResolutionVariant(destImageWidth, destImageHeight)</code>,
+     * otherwise the image itself
+     */
+    public static Image getResolutionVariant(Image img, double destImageWidth, double destImageHeight) {
+        if (baseMultiResolutionImageClass == null || resolutionVariantsMethod == null) {
+            return img;
+        }
+        if (baseMultiResolutionImageClass.isInstance(img)) {
+            try {
+                return (Image) resolutionVariantMethod.invoke(img, destImageWidth, destImageHeight);
+            } catch (IllegalAccessException | InvocationTargetException ex) {
+                Logging.error("Unexpected error while calling method: " + ex);
+            }
+        }
+        return img;
+    }
+
+    /**
      * Detect the GUI scale for HiDPI mode.
      * <p>
@@ -145 +170 @@
      * @return the GUI scale for HiDPI mode, a value of 1.0 means standard mode.
      */
-    private static double getHiDPIScale() {
+    static double getHiDPIScale() {
         if (GraphicsEnvironment.isHeadless())
             return 1.0;
@@ -237 +262 @@
         }
     }
+
+    private static Method initResolutionVariantMethod() {
+        try {
+            return baseMultiResolutionImageClass != null
+                    ? baseMultiResolutionImageClass.getMethod("getResolutionVariant", Double.TYPE, Double.TYPE)
+                    : null;
+        } catch (NoSuchMethodException ex) {
+            Logging.error("Cannot find expected method: " + ex);
+            return null;
+        }
+    }
 }

trunk/src/org/openstreetmap/josm/tools/ImageOverlay.java

@@ -3 +3 @@
 
 import java.awt.Dimension;
+import java.awt.Image;
 import java.awt.image.BufferedImage;
+import java.util.List;
 
 import javax.swing.ImageIcon;
@@ -67 +69 @@
     @Override
     public BufferedImage process(BufferedImage ground) {
+        return process(ground, false);
+    }
+
+    /**
+     * Handle overlay. The image passed as argument is modified!
+     *
+     * @param ground the base image for the overlay (gets modified!)
+     * @param highResolution whether the high resolution variant should be used to overlay
+     * @return the modified image (same as argument)
+     */
+    BufferedImage process(BufferedImage ground, boolean highResolution) {
         /* get base dimensions for calculation */
         int w = ground.getWidth();
@@ -78 +91 @@
             height = (int) (h*(offsetBottom-offsetTop));
         }
-        ImageIcon overlay;
         image = new ImageProvider(image).setMaxSize(new Dimension(width, height));
-        overlay = image.get();
+        ImageIcon overlay = image.get();
+        if (highResolution) {
+            List<Image> resolutionVariants = HiDPISupport.getResolutionVariants(overlay.getImage());
+            if (resolutionVariants.size() >= 2) {
+                overlay = new ImageIcon(resolutionVariants.get(1));
+            }
+        }
         int x, y;
         if (width == -1 && offsetLeft < 0) {

trunk/src/org/openstreetmap/josm/tools/ImageProvider.java

@@ -1318 +1318 @@
      */
     public static Cursor getCursor(String name, String overlay) {
-        ImageIcon img = get("cursor", name);
-        if (overlay != null) {
-            img = new ImageProvider("cursor", name).setMaxSize(ImageSizes.CURSOR)
-                .addOverlay(new ImageOverlay(new ImageProvider("cursor/modifier/" + overlay)
-                    .setMaxSize(ImageSizes.CURSOROVERLAY))).get();
-        }
         if (GraphicsEnvironment.isHeadless()) {
             Logging.debug("Cursors are not available in headless mode. Returning null for ''{0}''", name);
             return null;
         }
-        return Toolkit.getDefaultToolkit().createCustomCursor(img.getImage(),
-                "crosshair".equals(name) ? new Point(10, 10) : new Point(3, 2), "Cursor");
+
+        Point hotSpot = new Point();
+        Image image = getCursorImage(name, overlay, hotSpot);
+
+        return Toolkit.getDefaultToolkit().createCustomCursor(image, hotSpot, name);
+    }
+
+    /**
+     * Load a cursor image with a given file name, optionally decorated with an overlay image
+     *
+     * @param name the cursor image filename in "cursor" directory
+     * @param overlay optional overlay image
+     * @param hotSpot will be set to the properly scaled hotspot of the cursor
+     * @return cursor with a given file name, optionally decorated with an overlay image
+     */
+    static Image getCursorImage(String name, String overlay, /* out */ Point hotSpot) {
+        ImageProvider imageProvider = new ImageProvider("cursor", name);
+        if (overlay != null) {
+            imageProvider
+                .setMaxSize(ImageSizes.CURSOR)
+                .addOverlay(new ImageOverlay(new ImageProvider("cursor/modifier/" + overlay)
+                                                .setMaxSize(ImageSizes.CURSOROVERLAY)));
+        }
+        hotSpot.setLocation("crosshair".equals(name) ? new Point(10, 10) : new Point(3, 2));
+        ImageIcon imageIcon = imageProvider.get();
+        Image image = imageIcon.getImage();
+        int width = image.getWidth(null);
+        int height = image.getHeight(null);
+
+        // AWT will resize the cursor to bestCursorSize internally anyway, but miss to scale the hotspot as well
+        // (bug JDK-8238734).  So let's do this ourselves, and also scale the hotspot accordingly.
+        Dimension bestCursorSize = Toolkit.getDefaultToolkit().getBestCursorSize(width, height);
+        if (bestCursorSize.width != 0 && bestCursorSize.height != 0) {
+            // In principle, we could pass the MultiResolutionImage itself to AWT, but due to bug JDK-8240568,
+            // this results in bad alpha blending and thus jaggy edges.  So let's select the best variant ourselves.
+            image = HiDPISupport.getResolutionVariant(image, bestCursorSize.width, bestCursorSize.height);
+            if (bestCursorSize.width != image.getWidth(null) || bestCursorSize.height != image.getHeight(null)) {
+                image = image.getScaledInstance(bestCursorSize.width, bestCursorSize.height, Image.SCALE_DEFAULT);
+            }
+
+            hotSpot.x = hotSpot.x * bestCursorSize.width / width;
+            hotSpot.y = hotSpot.y * bestCursorSize.height / height;
+        }
+
+        return image;
     }
 

trunk/src/org/openstreetmap/josm/tools/ImageResource.java

@@ -154 +154 @@
      * @since 12722
      */
-    public ImageIcon getImageIcon(Dimension dim, boolean multiResolution) {
+    ImageIcon getImageIcon(Dimension dim, boolean multiResolution) {
+        return getImageIconAlreadyScaled(GuiSizesHelper.getDimensionDpiAdjusted(dim), multiResolution, false);
+    }
+
+    /**
+     * Get an ImageIcon object for the image of this resource. A potential UI scaling is assumed
+     * to be already taken care of, so dim is already scaled accordingly.
+     * @param  dim The requested dimensions. Use (-1,-1) for the original size and (width, -1)
+     *         to set the width, but otherwise scale the image proportionally.
+     * @param  multiResolution If true, return a multi-resolution image
+     * (java.awt.image.MultiResolutionImage in Java 9), otherwise a plain {@link BufferedImage}.
+     * When running Java 8, this flag has no effect and a plain image will be returned in any case.
+     * @param highResolution whether the high resolution variant should be used for overlays
+     * @return ImageIcon object for the image of this resource, scaled according to dim
+     */
+    ImageIcon getImageIconAlreadyScaled(Dimension dim, boolean multiResolution, boolean highResolution) {
         CheckParameterUtil.ensureThat((dim.width > 0 || dim.width == -1) && (dim.height > 0 || dim.height == -1),
                 () -> dim + " is invalid");
+
         BufferedImage img = imgCache.get(dim);
         if (img == null) {
             if (svg != null) {
-                Dimension realDim = GuiSizesHelper.getDimensionDpiAdjusted(dim);
-                img = ImageProvider.createImageFromSvg(svg, realDim);
+                img = ImageProvider.createImageFromSvg(svg, dim);
                 if (img == null) {
                     return null;
@@ -168 +183 @@
                 if (baseImage == null) throw new AssertionError();
 
-                int realWidth = GuiSizesHelper.getSizeDpiAdjusted(dim.width);
-                int realHeight = GuiSizesHelper.getSizeDpiAdjusted(dim.height);
                 ImageIcon icon = new ImageIcon(baseImage);
-                if (realWidth == -1 && realHeight == -1) {
-                    realWidth = GuiSizesHelper.getSizeDpiAdjusted(icon.getIconWidth());
-                    realHeight = GuiSizesHelper.getSizeDpiAdjusted(icon.getIconHeight());
-                } else if (realWidth == -1) {
-                    realWidth = Math.max(1, icon.getIconWidth() * realHeight / icon.getIconHeight());
-                } else if (realHeight == -1) {
-                    realHeight = Math.max(1, icon.getIconHeight() * realWidth / icon.getIconWidth());
+                if (dim.width == -1 && dim.height == -1) {
+                    dim.width = GuiSizesHelper.getSizeDpiAdjusted(icon.getIconWidth());
+                    dim.height = GuiSizesHelper.getSizeDpiAdjusted(icon.getIconHeight());
+                } else if (dim.width == -1) {
+                    dim.width = Math.max(1, icon.getIconWidth() * dim.height / icon.getIconHeight());
+                } else if (dim.height == -1) {
+                    dim.height = Math.max(1, icon.getIconHeight() * dim.width / icon.getIconWidth());
                 }
-                Image i = icon.getImage().getScaledInstance(realWidth, realHeight, Image.SCALE_SMOOTH);
-                img = new BufferedImage(realWidth, realHeight, BufferedImage.TYPE_INT_ARGB);
+                Image i = icon.getImage().getScaledInstance(dim.width, dim.height, Image.SCALE_SMOOTH);
+                img = new BufferedImage(dim.width, dim.height, BufferedImage.TYPE_INT_ARGB);
                 img.getGraphics().drawImage(i, 0, 0, null);
             }
             if (overlayInfo != null) {
                 for (ImageOverlay o : overlayInfo) {
-                    o.process(img);
+                    o.process(img, highResolution);
                 }
             }

trunk/test/unit/org/openstreetmap/josm/tools/ImageProviderTest.java

@@ -2 +2 @@
 package org.openstreetmap.josm.tools;
 
+import static java.awt.image.BufferedImage.TYPE_INT_ARGB;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
-
+import static org.junit.Assert.assertTrue;
+
+import java.awt.Color;
 import java.awt.Dimension;
+import java.awt.GraphicsEnvironment;
+import java.awt.GridLayout;
+import java.awt.Graphics;
+import java.awt.Image;
+import java.awt.Point;
+import java.awt.Toolkit;
 import java.awt.Transparency;
+import java.awt.event.MouseEvent;
+import java.awt.event.MouseListener;
 import java.awt.image.BufferedImage;
 import java.io.File;
 import java.io.IOException;
 import java.util.EnumSet;
+import java.util.List;
 import java.util.logging.Handler;
 import java.util.logging.LogRecord;
@@ -18 +30 @@
 
 import javax.swing.ImageIcon;
-
+import javax.swing.JFrame;
+import javax.swing.JPanel;
+
+import org.junit.Before;
 import org.junit.BeforeClass;
+import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
@@ -72 +88 @@
     }
 
+    @Before
+    public void resetPixelDensity() {
+        GuiSizesHelper.setPixelDensity(1.0f);
+    }
+
     /**
      * Non-regression test for ticket <a href="https://josm.openstreetmap.de/ticket/9984">#9984</a>
@@ -149 +170 @@
         assertNotNull(ImageProvider.getPadded(OsmUtils.createPrimitive("relation type=route route=railway"), iconSize, noDefault));
     }
+
+    /**
+     * Test getting a bounded icon given some UI scaling configured.
+     */
+    @Test
+    public void testGetImageIconBounded() {
+        int scale = 2;
+        GuiSizesHelper.setPixelDensity(scale);
+
+        ImageProvider imageProvider = new ImageProvider("open").setOptional(true);
+        ImageResource resource = imageProvider.getResource();
+        Dimension iconDimension = ImageProvider.ImageSizes.SMALLICON.getImageDimension();
+        ImageIcon icon = resource.getImageIconBounded(iconDimension);
+        Image image = icon.getImage();
+        List<Image> resolutionVariants = HiDPISupport.getResolutionVariants(image);
+        if (resolutionVariants.size() > 1) {
+            assertEquals(2, resolutionVariants.size());
+            int expectedVirtualWidth = ImageProvider.ImageSizes.SMALLICON.getVirtualWidth();
+            assertEquals(expectedVirtualWidth * scale, resolutionVariants.get(0).getWidth(null));
+            assertEquals((int) Math.round(expectedVirtualWidth * scale * HiDPISupport.getHiDPIScale()),
+                         resolutionVariants.get(1).getWidth(null));
+        }
+    }
+
+    public static final int ORIGINAL_CURSOR_SIZE = 32;
+
+    /**
+     * Test getting an image for a crosshair cursor.
+     */
+    @Test
+    public void testGetCursorImageForCrosshair() {
+        if (GraphicsEnvironment.isHeadless()) {
+            // TODO mock Toolkit.getDefaultToolkit().getBestCursorSize()
+            return;
+        }
+        Point hotSpot = new Point();
+        Image image = ImageProvider.getCursorImage("crosshair", null, hotSpot);
+        assertCursorDimensionsCorrect(new Point.Double(10.0, 10.0), image, hotSpot);
+    }
+
+    /**
+     * Test getting an image for a custom cursor with overlay.
+     */
+    @Test
+    public void testGetCursorImageWithOverlay() {
+        if (GraphicsEnvironment.isHeadless()) {
+            // TODO mock Toolkit.getDefaultToolkit().getBestCursorSize()
+            return;
+        }
+        Point hotSpot = new Point();
+        Image image = ImageProvider.getCursorImage("normal", "selection", hotSpot);
+        assertCursorDimensionsCorrect(new Point.Double(3.0, 2.0), image, hotSpot);
+        BufferedImage bufferedImage = new BufferedImage(image.getWidth(null), image.getWidth(null), TYPE_INT_ARGB);
+        bufferedImage.getGraphics().drawImage(image, 0, 0, null);
+
+        // check that the square of 1/4 size right lower to the center has some non-emtpy pixels
+        boolean nonEmptyPixelExistsRightLowerToCenter = false;
+        for (int x = image.getWidth(null) / 2; x < image.getWidth(null) * 3 / 4; ++x) {
+            for (int y = image.getHeight(null) / 2; y < image.getWidth(null) * 3 / 4; ++y) {
+                if (bufferedImage.getRGB(x, y) != 0)
+                    nonEmptyPixelExistsRightLowerToCenter = true;
+            }
+        }
+        assertTrue(nonEmptyPixelExistsRightLowerToCenter);
+    }
+
+    private void assertCursorDimensionsCorrect(Point.Double originalHotspot, Image image, Point hotSpot) {
+        Dimension bestCursorSize = Toolkit.getDefaultToolkit().getBestCursorSize(ORIGINAL_CURSOR_SIZE, ORIGINAL_CURSOR_SIZE);
+        Image bestCursorImage = HiDPISupport.getResolutionVariant(image, bestCursorSize.width, bestCursorSize.height);
+        int bestCursorImageWidth = bestCursorImage.getWidth(null);
+        assertEquals((int) Math.round(bestCursorSize.getWidth()), bestCursorImageWidth);
+        int bestCursorImageHeight = bestCursorImage.getHeight(null);
+        assertEquals((int) Math.round(bestCursorSize.getHeight()), bestCursorImageHeight);
+        assertEquals(originalHotspot.x / ORIGINAL_CURSOR_SIZE * bestCursorImageWidth, hotSpot.x, 1 /* at worst one pixel off */);
+        assertEquals(originalHotspot.y / ORIGINAL_CURSOR_SIZE * bestCursorImageHeight, hotSpot.y, 1 /* at worst one pixel off */);
+    }
+
+
+    /**
+     * Test getting a cursor
+     */
+    @Ignore("manual execution only, as the look of the cursor cannot be checked automatedly")
+    @Test
+    public void testGetCursor() throws InterruptedException {
+        JFrame frame = new JFrame();
+        frame.setSize(500, 500);
+        frame.setLayout(new GridLayout(2, 2));
+        JPanel leftUpperPanel = new JPanel(), rightUpperPanel = new JPanel(), leftLowerPanel = new JPanel(), rightLowerPanel = new JPanel();
+        leftUpperPanel.setBackground(Color.DARK_GRAY);
+        rightUpperPanel.setBackground(Color.DARK_GRAY);
+        leftLowerPanel.setBackground(Color.DARK_GRAY);
+        rightLowerPanel.setBackground(Color.DARK_GRAY);
+        frame.add(leftUpperPanel);
+        frame.add(rightUpperPanel);
+        frame.add(leftLowerPanel);
+        frame.add(rightLowerPanel);
+
+        leftUpperPanel.setCursor(ImageProvider.getCursor("normal", "select_add")); // contains diagonal sensitive to alpha blending
+        rightUpperPanel.setCursor(ImageProvider.getCursor("crosshair", "joinway")); // combination of overlay and hotspot not top left
+        leftLowerPanel.setCursor(ImageProvider.getCursor("hand", "parallel_remove")); // reasonably nice bitmap cursor
+        rightLowerPanel.setCursor(ImageProvider.getCursor("rotate", null)); // ugly bitmap cursor, cannot do much here
+
+        frame.setVisible(true);
+
+        // hover over the four quadrant to observe different cursors
+
+        // draw red dot at hotspot when clicking
+        frame.addMouseListener(new MouseListener() {
+            @Override
+            public void mouseClicked(MouseEvent e) {
+                Graphics graphics = frame.getGraphics();
+                graphics.setColor(Color.RED);
+                graphics.drawRect(e.getX(), e.getY(), 1, 1);
+            }
+
+            @Override
+            public void mousePressed(MouseEvent e) { }
+
+            @Override
+            public void mouseReleased(MouseEvent e) { }
+
+            @Override
+            public void mouseEntered(MouseEvent e) { }
+
+            @Override
+            public void mouseExited(MouseEvent e) { }
+        });
+        Thread.sleep(9000); // test would time out after 10s
+    }
 }