Modify

Opened 5 years ago

Last modified 4 years ago

#19277 new enhancement

Class GuiSizesHelper should be refactored or removed

Reported by: johsin18 Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: refactoring Cc:

Description (last modified by johsin18)

Class org.openstreetmap.josm.tools.GuiSizesHelper should be refactored or removed, as the code IMHO has multiple deficiencies:

  • literal "96f" is repeated many times, sometimes with "f" suffix (float), sometimes without (int)
    • Mitigation: at least make it a named constant
    • better Mitigation: keep the pixel density as a member, remove 96f everywhere (also see below)
  • Toolkit.getDefaultToolkit().getScreenResolution() never returns anything else than 96, at least on Windows, even when scaling!=100% is set for screens
    • in case it returns 72 for MacOS, we are in even more trouble
    • Mitigation: getScreenResolution should never be called, just read the "gui.scale" preference
    • getScreenResolution() tries to achieve something similar to HiDPIsupport.getHiDPIScale(), but in a different, possibly conflicting way
  • GuiSizesHelper is not a very descriptive name
    • Mitigation: rename it to UserPreferredUiScaling or something alike, as the actual screen resolution does not play a role
  • method isHiDPI() is not used at all
    • Mitigation: remove method, together with getPixelDensity(), which is only used by isHiDPI()
  • methods check everywhere that passed dimensions are >0. However, for values 0, the computations should result in exactly the same, so the check is not needed. If negative values are passed, this is proabaly a sign for problems in the calling code.
    • Mitigation: remove the checks
  • Most GUI elements are scaled, but not the mouse cursors.
    • Mitigation: rather rely on Toolkit.getBestCursorSize()
  • There is no test coverage for its functionality at all
    • Mitigation: add tests
  • Adds an indirect dependency to Config for all using classes/tests

Bottom line: Before fixing this quite broken class, we should probably remove it together with its functionality, as already proposed in https://josm.openstreetmap.de/wiki/Help/HiDPISupport (at the very bottom), and rather fix remaning bugs in the official HiDPI support.

SCNR I volunteer to get this done.

Attachments (0)

Change History (3)

comment:1 by simon04, 4 years ago

The class has been written by long term JOSM core contributors, please be respectful.

Patches are welcome!

Note that apparently unused code might still be in use by one of the many Plugins. The same holds true for renaming (of classes/functions). Negative dimensions are in fact used, see org.openstreetmap.josm.tools.ImageResource#DEFAULT_DIMENSION.

comment:2 by simon04, 4 years ago

Keywords: refactoring added
Type: defectenhancement

comment:3 by johsin18, 4 years ago

Description: modified (diff)
Summary: Class GuiSizesHelper has tons of bad codeClass GuiSizesHelper should be refactored or removed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user.
Next status will be 'needinfo'. The owner will be changed from team to johsin18.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.