Bug 338308 - Icons too large, wrong dpi calculation
Summary: Icons too large, wrong dpi calculation
Status: RESOLVED FIXED
Alias: None
Product: libplasma
Classification: Frameworks and Libraries
Component: libplasma (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Marco Martin
URL:
Keywords:
: 335510 338183 341145 341858 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-08-15 20:11 UTC by Adi Stadi
Modified: 2017-08-21 15:05 UTC (History)
8 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Xorg.0.log with fixed display dimension (197.79 KB, text/x-log)
2014-08-21 20:19 UTC, Adi Stadi
Details
Output of xrandr -q (627 bytes, text/plain)
2014-08-22 22:12 UTC, enoopt.adams
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Adi Stadi 2014-08-15 20:11:32 UTC
Tested with live iso neon5-201408080914

Cause: Invalid edid information from samsung syncmaster using hdmi (160x90)

No effect using
xorg.conf monitor section with DisplaySize 520 290
or
xsession.d with xrandr -fbmm 520x290;xrandr --dpi 94x94

session-errors: http://paste.kde.org/pgu7yzwi7
Xorg.0.log: http://paste.kde.org/pmg17uyxb
Comment 1 David Edmundson 2014-08-21 16:51:55 UTC
*** Bug 338183 has been marked as a duplicate of this bug. ***
Comment 2 Adi Stadi 2014-08-21 20:19:37 UTC
Created attachment 88358 [details]
Xorg.0.log with fixed display dimension
Comment 3 Adi Stadi 2014-08-21 20:30:55 UTC
xrandr output:
Screen 0: minimum 320 x 200, current 1920 x 1080, maximum 8192 x 8192
VGA-0 disconnected (normal left inverted right x axis y axis)
HDMI-0 connected primary 1920x1080+0+0 (normal left inverted right x axis y axis) 160mm x 90mm
   1920x1080      60.0*+   50.0     59.9  
   1920x1080i     60.1     50.0     60.0  
   1600x1200      60.0  
   1680x1050      59.9  
   1280x1024      60.0  
   1440x900       59.9  
   1280x960       60.0  
   1280x800       59.9  
   1280x720       60.0     50.0     59.9  
   1024x768       60.0  
   800x600        60.3     56.2  
   720x576        50.0  
   720x480        60.0     59.9  
   640x480        60.0     59.9
Comment 4 enoopt.adams 2014-08-22 22:12:09 UTC
Created attachment 88377 [details]
Output of xrandr -q
Comment 5 enoopt.adams 2014-08-22 22:14:38 UTC
I'm having this problem as well as other incorect scaling of plasma elements (like the screen corner glow size (https://bugs.kde.org/show_bug.cgi?id=337712)). Icon is the correct size in project neon on a virtualpc, but is in error on my laptop running arch. Still having this problem on a build from source a few days old.
Comment 6 David Edmundson 2014-08-23 18:54:15 UTC
*** Bug 335510 has been marked as a duplicate of this bug. ***
Comment 7 David Edmundson 2014-08-23 19:10:01 UTC
Assigning to Plasma Frameworks as it's the Units that's "wrong".

Marco, can we just use the user configured Icon sizes (KIconLoader::IconSize)  rather than scaling numbers by DPI ourselves. It clearly isn't working that well currently as this problem has had 3 reports already.
Comment 8 Sebastian Kügler 2014-08-25 09:37:29 UTC

Using the settings from the user can still be done, and will take effect, but the DPI scaling will be intact, so it's not breaking that feature. Ignoring DPI will mean that we lose all the HiDPI goodness we introduced with exactly this scaling algorithm -- a huge step back for many users.

I really do not want to lose this feature, but I'm open to think about different ways. With the DPI features in Qt 5.4, we can perhaps do better? Just removing it is not an option to me, sorry, as it introduces regressions for everybody on a slightly better than normal display, and it will simply make many things unusable for those with real high DPI displays.
Comment 9 Marco Martin 2014-08-25 09:55:13 UTC
"With the DPI features in Qt 5.4, we can perhaps do better?"
iirc the highdpi branch didn't make it for 5.4, so is still a future problem.
but yes, our dpi calculations should remain, at least until qt will have that too.
(on the other hand, imo the sizes categories from kiconlaoder like desktop,panel,toolbar etc, should be dprecated as in conceptually obsolete, or at least new more meaningful categories should be introduced)
Comment 10 David Edmundson 2014-08-25 10:36:37 UTC
If the user doesn't change their icon settings all apps will look broken anyway, so it must be safe to assume they're correct... and if it's not we need to fix it anyway?
Comment 11 Sebastian Kügler 2014-08-25 10:41:14 UTC
Thought a bit more about it ... we could use the same ratios as for units.gridUnit, since that is almost guaranteed to match with the surrounding UI (and the user can influence it together with font settings).

The bugreports I looked at should be fine with such a solution. Thoughts?
Comment 12 David Edmundson 2014-08-25 10:47:57 UTC
So scale according to how much the font differs from the default font size (12px?) 

I'd still prefer using the user set values, but I'd be happy enough with this approach, it would fix all of these bugs.
Comment 13 Sebastian Kügler 2014-08-25 11:02:33 UTC
No, scaling with the pixelsize of the fonts as the user configured them. That takes DPI and user settings into account and makes it the same as gridUnit (which forms the basis of all the other ui sizing).
Comment 14 Sebastian Kügler 2014-08-28 23:04:03 UTC
Proposed patch: https://git.reviewboard.kde.org/r/119983/
Comment 15 David Edmundson 2014-11-29 20:09:47 UTC
*** Bug 341145 has been marked as a duplicate of this bug. ***
Comment 16 David Edmundson 2014-12-19 12:40:45 UTC
*** Bug 341858 has been marked as a duplicate of this bug. ***
Comment 17 David Edmundson 2015-03-04 10:26:05 UTC
Git commit 1ce575cf5a3b2e37d54a47340396b2b9fd8cfa41 by David Edmundson.
Committed on 04/03/2015 at 09:14.
Pushed by davidedmundson into branch 'master'.

Simple fix for huge icon sizes

Currently units is based on font metrics except icon size and some SVG
parts which use DPI directly.

Instead of taking the physical DPI we take the logical DPI which is used
for working out font point -> pixel sizes.

We can rely on this being correct as otherwise every other app would
appear broken.

REVIEW: 122799

Change-Id: I22f668ccea7d6d15ff475e1368c82964bdde1e60

M  +1    -1    src/declarativeimports/core/units.cpp

http://commits.kde.org/plasma-framework/1ce575cf5a3b2e37d54a47340396b2b9fd8cfa41