Modify

Opened 10 months ago

Closed 6 months ago

Last modified 3 months ago

#18694 closed defect (fixed)

[PATCH] Wrongly rendered cursors on HiDPI screen

Reported by: johsin18 Owned by: team
Priority: normal Milestone: 20.05
Component: Core Version: tested
Keywords: template_report hidpi mouse cursor Cc:

Description (last modified by Don-vip)

What steps will reproduce the problem?

  1. Start JOSM on a HiDPI screen (200% Windows scaling)
  2. New Layer
  3. Hover mouse over data layer and press Shift key (showing mouse cursor for "add to selection")

What is the expected result?

Mouse cursor with a dashed rectangle close to it, containing a little plus sign. Mouse arrow and rectangle have approximately the same size.

correct lo DPI cursor

What happens instead?

Mouse cursor with a dashed rectangle quite distant to it, containing a very tiny plus sign (very hard to see). The mouse arrow is larger than the rectangle.

faulty hi DPI cursor

Please provide any additional information below. Attach a screenshot if possible.

See attached screenshots for both cases (imagine the hi DPI cursor scaled by half). The behavior for other cursors with overlay is similar.
Disregard the few faulty pixel on the left in the high DPI mouse arrow. They are caused by the Windows-built-in Magnifier tool somehow.

I would like to fix this myself. However, I would need some explanations on the existing code.
Deep down in the cursor construction I get via

GuiSizesHelper.getSizeDpiAdjusted()

to

GuiSizesHelper.getScreenDPI()

which claims to return the "screen resolution in DPI".
However, unless for the unlikely case that the user sets "gui.scale" in the settings manually, it will always return 96, regardless of the actual screen resolution (and/or scaling), will it not?
Is getScreenDPI() buggy or do I misinterpret its function?

Revision:15592
Is-Local-Build:true
Build-Date:2020-02-09 18:52:27

Identification: JOSM/1.5 (15592 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1909 (18363)
Memory Usage: 290 MB / 2028 MB (127 MB allocated, but free)
Java version: 15-ea+6-123, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: \Display0 3200x1800
Maximum Screen Size: 3200x1800
VM arguments: [-Djosm.home=<josm.pref>, -Didea.launcher.port=57345, -Didea.launcher.bin.path=C:\Program Files\JetBrains\IntelliJ IDEA Community Edition 2019.3\bin, -Dfile.encoding=UTF-8]
Program arguments: [--reset-preferences]
Dataset consistency test: No problems found

Last errors/warnings:
- W: Replacing existing preference file '<josm.pref>\preferences.xml' with default preference file.
- W: extended font config - overriding 'filename.Myanmar_Text=mmrtext.ttf' with 'MMRTEXT.TTF'
- W: extended font config - overriding 'filename.Mongolian_Baiti=monbaiti.ttf' with 'MONBAITI.TTF'
- W: No configuration settings found.  Using hardcoded default values for all pools.

Attachments (4)

CorrectLoDpiCursor.png (380 bytes) - added by johsin18 10 months ago.
correct lo DPI cursor
FaultyHiDpiCursor.png (288 bytes) - added by johsin18 10 months ago.
faulty hi DPI cursor
CorrectHiDpiCursor.png (591 bytes) - added by johsin18 8 months ago.
HiDPI cursor after fix.
hidpi-cursors-overlay.patch (26.2 KB) - added by johsin18 7 months ago.
complete patchset

Download all attachments as: .zip

Change History (20)

Changed 10 months ago by johsin18

Attachment: CorrectLoDpiCursor.png added

correct lo DPI cursor

Changed 10 months ago by johsin18

Attachment: FaultyHiDpiCursor.png added

faulty hi DPI cursor

comment:1 Changed 10 months ago by johsin18

Description: modified (diff)

comment:2 Changed 10 months ago by simon04

Keywords: hidpi added

comment:3 Changed 10 months ago by simon04

Keywords: mouse cursor added

comment:4 Changed 10 months ago by Don-vip

Description: modified (diff)

Changed 8 months ago by johsin18

Attachment: CorrectHiDpiCursor.png added

HiDPI cursor after fix.

comment:5 Changed 8 months ago by johsin18

I finally found some time to finish a patch, which also fixes #13173.

As IMHO the changes are better reviewed commit by commit, I pushed it to a git repo:
https://github.com/johsin18/josm/pull/1
Feel free to comment there.
I can also provide a monolithic patch if desired.

The cursor overlays are correctly rendered now, as tested on JRE 11 and JRE 15 EA on Windows.
HiDPI cursor after fix.
I haven't tested on Linux or MacOS.
The problem that the cursor is too large on the low-res screen still exists, that can only be fixed by the JRE (filed to the JDK people).

comment:6 Changed 8 months ago by johsin18

Summary: Wrongly rendered cursors on HiDPI screen[PATCH] Wrongly rendered cursors on HiDPI screen

comment:7 Changed 7 months ago by simon04

Milestone: 20.06

Some remarks on patch. Haven't tested the HiDPI functionality (have to find a test environment first).

  1. Please remove the public modifier of the following methods. Having multiple boolean flags as parameters is often considered as bad practice. For internal use (in ImageReource and ImageProvider) it's less an issue and we can refactor more easily (without impacting various plugins).
  • org.openstreetmap.josm.tools.ImageResource#getImageIcon(java.awt.Dimension, boolean)
  • org.openstreetmap.josm.tools.ImageResource#getImageIconAlreadyScaled
  • org.openstreetmap.josm.tools.ImageOverlay#process(BufferedImage, boolean) (keep the original as public function, also implements ImageProcessor)
  1. HiDPISupport.getResolutionVariants(overlay.getImage()).get(1) – please check that the list really has ⩾2 elements before accessing get(1)
  1. How does the code behave once the mentioned Java bugs (JDK-8238734, JDK-8240568) are resolved?

comment:8 Changed 7 months ago by johsin18

About testing: You don't need a real HiDPI screen. Just set the scaling to 200% and check that everything consistently is twice as large. The patch will only be effective with Java > 8, of course.

  1. done, I kept the Javadoc
  2. done
  3. Once the mentioned bugs are fixed, the proposed code will still work, but could be simplified a bit again. Fixing JDK-8240568 will also solve the problem of the cursor having twice of half the intended size under certain circumstances (mixed scaling), which we cannot fix ourselves.

Patch adapted, update commits squashed, see
https://github.com/johsin18/josm/pull/2

comment:9 in reply to:  8 ; Changed 7 months ago by simon04

Replying to johsin18:

Just set the scaling to 200% and check that everything consistently is twice as large.

Easier said than done – https://wiki.archlinux.org/index.php/HiDPI ;-)

Running java -Dsun.java2d.uiScale=2 josm-custom.jar makes everything huge except for the mouse cursor. But it isn't broken either. :-)


  1. The automated unit tests via Jenkins run in headless mode (which is available via -Djava.awt.headless=true). This causes two ImageProviderTest to fail:
java.awt.HeadlessException
	at org.openstreetmap.josm.tools.ImageProvider.getCursorImage(ImageProvider.java:1355)
	at org.openstreetmap.josm.tools.ImageProviderTest.testGetCursorImageWithOverlay(ImageProviderTest.java:213)
	at org.openstreetmap.josm.testutils.JOSMTestRules$TimeoutThread.run(JOSMTestRules.java:731)


java.awt.HeadlessException
	at org.openstreetmap.josm.tools.ImageProvider.getCursorImage(ImageProvider.java:1355)
	at org.openstreetmap.josm.tools.ImageProviderTest.testGetCursorImageForCrosshair(ImageProviderTest.java:203)
	at org.openstreetmap.josm.testutils.JOSMTestRules$TimeoutThread.run(JOSMTestRules.java:731)

Maybe you can mock java.awt.Toolkit similar to org.openstreetmap.josm.testutils.mockers.WindowMocker for the unit tests. Otherwise exclude them if java.awt.GraphicsEnvironment.isHeadless()

  1. A tiny thing: Javadoc for org.openstreetmap.josm.tools.GuiSizesHelper#setPixelDensity is lacking @param pixelDensity

Changed 7 months ago by johsin18

Attachment: hidpi-cursors-overlay.patch added

complete patchset

comment:10 in reply to:  9 Changed 7 months ago by johsin18

Replying to simon04:

Replying to johsin18:

Just set the scaling to 200% and check that everything consistently is twice as large.

Easier said than done – https://wiki.archlinux.org/index.php/HiDPI ;-)

Sorry, I was talking about Windows 10. But can't you configure it on KDE, Gnome, or XFCE level?

  1. great idea to use JMockit. Done. Hope the tests work now in Jenkins as well. Can I test my branch myself on Jenkins somehow?
  1. done. Could have been solved by JMockit as well, but I did not bother. At best, GuiSizesHelper, would disappear completely (#19277)

If you find other small problems in the patch, feel free to fix them yourself.

comment:11 Changed 6 months ago by simon04

Can I test my branch myself on Jenkins somehow?

Sorry, this is not possible. (For a long time. we've been talking about modernizing our infrastructure, see #16871.)

Using sun.awt.HeadlessToolkit causes the following Java warnings :

    [javac] /josm/test/unit/org/openstreetmap/josm/testutils/mockers/HeadlessToolkitMocker.java:10: warning: HeadlessToolkit is internal proprietary API and may be removed in a future release
    [javac] import sun.awt.HeadlessToolkit;
    [javac]               ^
    [javac] /josm/test/unit/org/openstreetmap/josm/testutils/mockers/HeadlessToolkitMocker.java:15: warning: HeadlessToolkit is internal proprietary API and may be removed in a future release
    [javac] public class HeadlessToolkitMocker extends MockUp<HeadlessToolkit> {
$ java -version
openjdk version "11.0.6" 2020-01-14
OpenJDK Runtime Environment (build 11.0.6+10)
OpenJDK 64-Bit Server VM (build 11.0.6+10, mixed mode)

I'm not sure how to solve this. Chaning to Toolkit for HeadlessToolkitMocker extends MockUp<Toolkit> doesn't seem to work.

comment:12 Changed 6 months ago by simon04

Resolution: fixed
Status: newclosed

In 16486/josm:

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

comment:13 Changed 6 months ago by simon04

Milestone: 20.0620.05

I've committed your patch without the HeadlessToolkitMocker. Instead, I've added two TODO mock Toolkit.getDefaultToolkit().getBestCursorSize()

Thanks for your work on HiDPI! And welcome! :-)

comment:14 Changed 6 months ago by simon04

Ticket #13173 has been marked as a duplicate of this ticket.

comment:15 Changed 3 months ago by simon04

In 16983/josm:

see #18694 - Extract ImageProviderTestManual, ImageProviderTestIT

comment:16 Changed 3 months ago by simon04

In 16984/josm:

see #18694 - Migrate ImageProviderTest to JUnit 5, mock bestCursorSize, compare cursors with reference images

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.