[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 125773: Fix units.gridSize and theme.mSize for some fonts
From: "David Rosca" <nowrep () gmail ! com>
Date: 2015-11-25 15:27:43
Message-ID: 20151125152743.23104.49085 () mimi ! kde ! org
[Download RAW message or body]
--===============8718297629096943449==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> On Nov. 2, 2015, 12:05 p.m., Sebastian Kügler wrote:
> > I don't like this change, as it introduces a magic constant for a value that we \
> > completely control our own. (Well, to the degree that we say "a gridUnit is \
> > roughly the height of a line of text". The 1.6 constant looks weird here, and I'm \
> > against adding font specific hacks in, especially since at that point in the \
> > code, we don't even know what the font is.
> > This is not a structural solution, so -1.
>
> David Rosca wrote:
> I understand, but what is a structural solution?
> The font metric is clearly wrong for Noto, Oxygen (and certainly some other fonts) \
> and this change is backwards compatible with current code.
> And this is not a font specific hack, this just scales the actual height of M to \
> the expected gridUnit value because gridUnit never was a height of M character (it \
> is like 1.6 * height of M = thus the magic number).
> Sebastian Kügler wrote:
> Well, problems arise as soon as you change the font -- the computation is not based \
> on the default font, but on the currently selected font. The funny thing here is \
> that the Oxygen font is actually the wrong one, as that includes more height than \
> the rendered character, but also some spacing above.
> What is the exact issue you see? I'm still not quite clear on that.
>
> The bug you're referring to should be fixed by not computing the font manually, but \
> use units.iconSizes.* instead. (I haven't tried, but that's the semantically \
> correct solution.)
> David Rosca wrote:
> > Well, problems arise as soon as you change the font
>
> In the current implementation, yes. Because QFontMetrics::boundingRect(QString = \
> "M").height() returns more than just height of M character, and this extra size is \
> different for various fonts - it will return different value for 2 fonts even when \
> the font size (eg. actual height of M character) is the same in both.
> > The funny thing here is that the Oxygen font is actually the wrong one, as that \
> > includes more height than the rendered character, but also some spacing above.
>
> Yes, but that spacing is there for every font, but different and that's exactly the \
> whole problem. For DejaVu Sans font it returns value that plays well with what we \
> use as gridUnit (roughly M height * 1.6), but for Noto and Oxygen it returns just \
> too much (~ * 2). Only QFontMetrics::boundingRect(QChar = 'M').height() returns the \
> actual M height without any spacing.
> > What is the exact issue you see? I'm still not quite clear on that.
>
> My issue is that units.gridUnit and theme.mSize is too big and that leads to too \
> big spacing relative to font size. You can see it on screenshots, it shows the \
> minimum size of system tray popup (it is calculated as x multiple of \
> units.gridUnit). You can see on the first screenshot that it is just too big, and \
> the same story is for every other plasma widget.
> Maybe I should have posted the whole screen, not just a snip. It looks much worse \
> when you see the whole screen and can compare the size to the rest of screen \
> (1920x1080).
> > The bug you're referring to should be fixed by not computing the font manually, \
> > but use units.iconSizes.* instead.
>
> Yes, that would fix the linked bug - size of systemtray icons. But it won't fix the \
> general issue mentioned above.
> Marco Martin wrote:
> also -1.
> the sizing of systray icons should depend in a range between iconsizes, so from \
> iconSizes.small to smallMedium (or medium, or whetever). for the grid unit, adding \
> that ratio to the m height, i get that you are trying to have pretty much the \
> height of a line of text? could QFontMetrics::lineSpacing() work?
> Martin Klapetek wrote:
> From me it's a +1, the patch is technically correct. The icons sizes are just a \
> byproduct of this patch, this is about the difference that
> boundingRect("M") and
> boundingRect('M') make.
>
> Using the QString overload is wrong because it is not just the M height, but also \
> the spacing above and that spacing is random, making things scale randomly with \
> different fonts. Using the ::lineSpacing wouldn't really make much difference I \
> think because it would still need some kind of multiplier to get back to the \
> original value that boundingRect("M") returns up till now, otherwise Plasma will \
> get really tiny for users with Oxygen font. Using boundingRect('M'), the QL1Char \
> overload, it counds *only* the M size, no other spacing included. This would result \
> in much much more consistent scaling with different fonts. Curretnly the difference \
> can be quite big.
> So, +1, this patch is correct from my point of view.
Perhaps could you give this a second thought?
If I wasn't myself clear then please read Martin's comment, he explained it very \
well. I'll just add that lineSpacing won't work as it has the same issue with random \
spacing. For me personally, I cannot use Noto without this patch as everything has \
big spacing (+ annoying big systray icons as side effect).
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125773/#review87864
-----------------------------------------------------------
On Oct. 29, 2015, 6:16 p.m., David Rosca wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125773/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2015, 6:16 p.m.)
>
>
> Review request for Plasma.
>
>
> Bugs: 343349
> http://bugs.kde.org/show_bug.cgi?id=343349
>
>
> Repository: plasma-framework
>
>
> Description
> -------
>
> For some fonts, QFontMetrics::boundingRect(QString) returns too high rect which \
> makes the gridSize too big. It now returns correctly the actual height of M \
> character. For backwards compatibility, the value is multiplied with 1.6.
> This affects eg. Noto Sans font that is now default for Plasma 5.5.
>
>
> Diffs
> -----
>
> src/declarativeimports/core/units.h fa2256e
> src/declarativeimports/core/units.cpp 4e2adae
> src/plasma/theme.cpp c49ad4c
>
> Diff: https://git.reviewboard.kde.org/r/125773/diff/
>
>
> Testing
> -------
>
> When switching to Noto Sans font, I noticed that icons in system tray grow to big \
> size so it switched to 1 column in vertical panel. Basically everything in Plasma \
> grow too much (even though the font is visually the same or even smaller than \
> DejaVu Sans that I was using before - same font size 9 was used) - too big spacing \
> in task manager, too big popups (application menu, system tray popups), etc ...
> This fixes the issue. This may also fix BUG 343349
>
>
> File Attachments
> ----------------
>
> systray + popup before
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/29/d409b391-e35e-451f-bb5b-aa42e7eb2bf3__before.png
> after
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/29/445009bc-a4a8-4998-8e82-996a0a4e33fb__after.png
>
>
> Thanks,
>
> David Rosca
>
>
--===============8718297629096943449==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/125773/">https://git.reviewboard.kde.org/r/125773/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On November 2nd, 2015, 12:05 p.m. UTC, \
<b>Sebastian Kügler</b> wrote:</p> <blockquote style="margin-left: 1em; \
border-left: 2px solid #d0d0d0; padding-left: 10px;"> <pre style="white-space: \
pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't like this \
change, as it introduces a magic constant for a value that we completely control our \
own. (Well, to the degree that we say "a gridUnit is roughly the height of a line of \
text". The 1.6 constant looks weird here, and I'm against adding font specific hacks \
in, especially since at that point in the code, we don't even know what the font \
is.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">This is not a structural solution, so -1.</p></pre> \
</blockquote>
<p>On November 2nd, 2015, 12:41 p.m. UTC, <b>David Rosca</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I \
understand, but what is a structural solution? The font metric is clearly wrong for \
Noto, Oxygen (and certainly some other fonts) and this change is backwards compatible \
with current code.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">And this is not a font specific hack, \
this just scales the actual height of M to the expected gridUnit value because \
gridUnit never was a height of M character (it is like 1.6 * height of M = thus the \
magic number).</p></pre> </blockquote>
<p>On November 2nd, 2015, 1:05 p.m. UTC, <b>Sebastian Kügler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Well, \
problems arise as soon as you change the font -- the computation is not based on the \
default font, but on the currently selected font. The funny thing here is that the \
Oxygen font is actually the wrong one, as that includes more height than the rendered \
character, but also some spacing above.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">What is the exact issue \
you see? I'm still not quite clear on that.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">The bug you're \
referring to should be fixed by not computing the font manually, but use \
units.iconSizes.* instead. (I haven't tried, but that's the semantically correct \
solution.)</p></pre> </blockquote>
<p>On November 2nd, 2015, 1:34 p.m. UTC, <b>David Rosca</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote \
style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid \
#bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;"> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Well, problems arise as soon as you change the font</p> </blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">In the current implementation, yes. Because \
QFontMetrics::boundingRect(QString = "M").height() returns more than just height of M \
character, and this extra size is different for various fonts - it will return \
different value for 2 fonts even when the font size (eg. actual height of M \
character) is the same in both.</p> <blockquote style="text-rendering: \
inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 \
0 0 0.5em;line-height: inherit;"> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">The funny thing here is \
that the Oxygen font is actually the wrong one, as that includes more height than the \
rendered character, but also some spacing above.</p> </blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Yes, but that spacing is there for every font, but \
different and that's exactly the whole problem. For DejaVu Sans font it returns value \
that plays well with what we use as gridUnit (roughly M height * 1.6), but for Noto \
and Oxygen it returns just too much (~ * 2). Only QFontMetrics::boundingRect(QChar = \
'M').height() returns the actual M height without any spacing.</p> <blockquote \
style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid \
#bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;"> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">What is the exact issue you see? I'm still not quite clear on that.</p> \
</blockquote> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">My issue is that units.gridUnit and theme.mSize is too \
big and that leads to too big spacing relative to font size. You can see it on \
screenshots, it shows the minimum size of system tray popup (it is calculated as x \
multiple of units.gridUnit). You can see on the first screenshot that it is just too \
big, and the same story is for every other plasma widget.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Maybe \
I should have posted the whole screen, not just a snip. It looks much worse when you \
see the whole screen and can compare the size to the rest of screen (1920x1080).</p> \
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid \
#bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;"> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">The bug you're referring to should be fixed by not computing the font \
manually, but use units.iconSizes.* instead.</p> </blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Yes, that would fix the linked bug - size of \
systemtray icons. But it won't fix the general issue mentioned above.</p></pre> \
</blockquote>
<p>On November 3rd, 2015, 9:52 a.m. UTC, <b>Marco Martin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">also \
-1. the sizing of systray icons should depend in a range between iconsizes, so from \
iconSizes.small to smallMedium (or medium, or whetever). for the grid unit, adding \
that ratio to the m height, i get that you are trying to have pretty much the height \
of a line of text? could QFontMetrics::lineSpacing() work?</p></pre> </blockquote>
<p>On November 3rd, 2015, 1:42 p.m. UTC, <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">From me it's a +1, \
the patch is technically correct. The icons sizes are just a byproduct of this patch, \
this is about the difference that
boundingRect("M") and
boundingRect('M') make.
Using the QString overload is wrong because it is not just the M height, but also the \
spacing above and that spacing is random, making things scale randomly with different \
fonts. Using the ::lineSpacing wouldn't really make much difference I think \
because it would still need some kind of multiplier to get back to the original value \
that boundingRect("M") returns up till now, otherwise Plasma will get \
really tiny for users with Oxygen font. Using boundingRect('M'), the QL1Char \
overload, it counds *only* the M size, no other spacing included. This would result \
in much much more consistent scaling with different fonts. Curretnly the difference \
can be quite big.
So, +1, this patch is correct from my point of view.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Perhaps could you give this a second thought?</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If I \
wasn't myself clear then please read Martin's comment, he explained it very well. \
I'll just add that lineSpacing won't work as it has the same issue with random \
spacing. For me personally, I cannot use Noto without this patch as everything has \
big spacing (+ annoying big systray icons as side effect).</p></pre> <br />
<p>- David</p>
<br />
<p>On October 29th, 2015, 6:16 p.m. UTC, David Rosca wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for Plasma.</div>
<div>By David Rosca.</div>
<p style="color: grey;"><i>Updated Oct. 29, 2015, 6:16 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=343349">343349</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-framework
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">For some fonts, QFontMetrics::boundingRect(QString) \
returns too high rect which makes the gridSize too big. It now returns correctly the \
actual height of M character. For backwards compatibility, the value is multiplied \
with 1.6.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">This affects eg. Noto Sans font that is now default \
for Plasma 5.5.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">When switching to Noto Sans font, I noticed that icons \
in system tray grow to big size so it switched to 1 column in vertical panel. \
Basically everything in Plasma grow too much (even though the font is visually the \
same or even smaller than DejaVu Sans that I was using before - same font size 9 was \
used) - too big spacing in task manager, too big popups (application menu, system \
tray popups), etc ...</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">This fixes the issue. This may also fix \
BUG 343349</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/declarativeimports/core/units.h <span style="color: \
grey">(fa2256e)</span></li>
<li>src/declarativeimports/core/units.cpp <span style="color: \
grey">(4e2adae)</span></li>
<li>src/plasma/theme.cpp <span style="color: grey">(c49ad4c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125773/diff/" style="margin-left: \
3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments \
</h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/10/29/d409b391-e35e-451f-bb5b-aa42e7eb2bf3__before.png">systray \
+ popup before</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/10/29/445009bc-a4a8-4998-8e82-996a0a4e33fb__after.png">after</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============8718297629096943449==--
[Attachment #3 (text/plain)]
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic