> 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.
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? - Marco ----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
