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 )
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 , 4 years ago
comment:2 by , 4 years ago
Keywords: | refactoring added |
---|---|
Type: | defect → enhancement |
comment:3 by , 4 years ago
Description: | modified (diff) |
---|---|
Summary: | Class GuiSizesHelper has tons of bad code → Class GuiSizesHelper should be refactored or removed |
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
.