Fix alignment of icons in Places panel and Compact view mode
ClosedPublic

Authored by sharvey on Mar 24 2018, 4:28 PM.

Details

Summary

Adjust calculation of icon pixmap Y value; now based off center of text label bounding rectangle. Previously, icons appeared top-aligned when text size was larger than icon size.

Centering is done by obtaining the center point of the text frame (m_textRect.center().y()) and setting the top Y point of the icon to one-half of the scaled icon height (m_scaled_PixmapSize.height()) Division is done by 2.0, to ensure calculations are done with floating-point math, in keeping with QPointF, which returns floating-point values.

Also includes an adjustment named midlineShift (contributed by @zzag), which takes into account the font's midline in respect to the center of the text frame. Certain fonts (i.e. Noto Sans) can have a slightly offset midline, resulting in imperfect alignment of the icon. This small adjustment resolves that potential issue.

See before and after screenshots.


Before, showing misalignment (with and without debugging wireframes)


After, showing correction

BUG: 390771

Test Plan
  • Compile Dolphin with patch
  • Increase system font size by several points (i.e. 15pt)
  • Check that Places panel icons remain centered with the text label
  • Select Compact View mode
  • Adjust icon size slider to minimum
  • Ensure that icons also remain centered in file listing
  • Check several combinations of icon size & font size to ensure centering is consistent

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sharvey created this revision.Mar 24 2018, 4:28 PM
Restricted Application added a subscriber: Dolphin. · View Herald TranscriptMar 24 2018, 4:28 PM
sharvey requested review of this revision.Mar 24 2018, 4:28 PM
sharvey added a reviewer: cfeck.
sharvey edited the summary of this revision. (Show Details)Mar 24 2018, 4:31 PM

A slightly different and more consistent Y-centering calculation. Now based off the centerline of the text label, rather than trying to calculate from the height of the icon pixmap.

Thanks to the original author or whomever added the debugging wireframe capability. Came in very handy when checking the alignment.

elvisangelaccio added inline comments.
src/kitemviews/kstandarditemlistwidget.cpp
1037

This comment probably becomes obsolete after this patch?

1039

Should be const

1044

What about padding? Is there a reason we are removing it?

sharvey updated this revision to Diff 30433.Mar 24 2018, 8:00 PM
  • Changed QPointF to const QPointF
sharvey marked an inline comment as done.Mar 24 2018, 8:16 PM

Thanks for the correction on const. It's updated.

src/kitemviews/kstandarditemlistwidget.cpp
1037

Don't quite understand. It's still centered, just using a different method.

1044

I tried multiple ways of centering the icon in Y before settling on this. I couldn't find a combination of icon size & font size where padding was necessary in the Y direction. The size of textRect is calculated with padding included, so textRect would appear to always be larger than the icon pixmap. Even when the icon is larger than the text, the text frame is always larger. I didn't come across a case where additional padding was necessary.

Definitely an improvement for most fonts and sizes, but I still see an a diminished version of the original issue with probably the most common way to trigger it: by increasing the size of the default font by 1, giving you 11pt Noto Sans. With this size and font, the text still looks bottom-aligned (though not as bad as it was before):

It looks fine for any other size of Noto Sans, FWIW.

The issue persists in HiDPI mode, seen here with a 1.5x scale factor:

At this point, it looks like the icon is properly positioned, but the label itself is a pixel or two too low.

@ngraham: I'm having to dig deeper into this. I can't find an ideal solution by tweaking the calculation I submitted. It's incredibly finicky. QPointF returns X and Y coordinates as floating-point values, so there's always going to be a rounding error (for lack of a better term) when it comes to painting individual physical pixels. Boosting QT_SCALE_FACTOR to emulate HiDPI mode adds another layer of complexity (but it sure looks pretty).

What I've got now, even with adding or subtracting fractions of padding and other pre-defined spacing values, isn't getting me a perfect solution. I'll keep trying different things and will post an update once I've found something workable. Thanks for testing.

sharvey updated this revision to Diff 30651.Mar 26 2018, 6:31 PM
  • Adjustment to centering calculation

Now using text center & scaled icon height


Spec sheet of various font sizes, font faces, and Qt scale factors.

I think this might do the trick, but please test.

Thanks.

Hmm, I still see a problem with 11pt Noto Sans. Take a look at this blown-up screenshot, and specifically, the Pictures item:

Also, looking at the raw diff in git rather than Phabricator, it looks like there's a newly-added unnecessary extra tab in the whole code block.

Nevertheless, if this is caused by an unavoidable rounding error, I'm willing to accept the patch (modulo the requested code clean-up) as it does represent a clear improvement over the status quo for the Noto Sans 11 case!

src/kitemviews/kstandarditemlistwidget.cpp
1043

Doesn't the Y axis adjust the vertical center, not the horizontal center?

1044

Since textRectCenter is only used once, we should probably access that data inline rather than defining a new variable. How about this?

m_pixmapPos.setY(m_textRect.center().y() - (m_scaledPixmapSize.height() / 2 ));

sharvey updated this revision to Diff 30706.Mar 27 2018, 10:24 AM
  • Eliminate duplicate variable, adjust indenting, correct "vertical"
sharvey marked an inline comment as done.Mar 27 2018, 10:28 AM

Redundant variable removed (originally done for my own clarity & readability). Code block indentation adjusted to match others in the file. Changed comment to "vertical" instead of horizontal (did I really do that?)


Extreme closeup, with Krita's pixel grid turned on (Noto Sans 11). As you say, it's not pixel-perfect, but it's a definite improvement over what we started with. I think this might be as close as we can get, without attempting more drastic changes to the code - something I'd rather avoid. This code also drives the Compact Mode listing in the main file window, and that's something we don't want to disrupt.

zzag added a subscriber: zzag.EditedMar 27 2018, 1:08 PM


Extreme closeup, with Krita's pixel grid turned on (Noto Sans 11).

Try to use qRound explicitly. I think it may fix the issue.

Update
Oh, never mind.

zzag added a comment.EditedMar 27 2018, 3:28 PM

Hmm, I still see a problem with 11pt Noto Sans. Take a look at this blown-up screenshot, and specifically, the Pictures item:

No, it's an issue specific to Noto Sans.. It seems like Noto Sans has a "lowered baseline". Maybe, it can be fixed by aligning icons between font's capline and baseline. Aligning relative to x-height is wrong.

With other fonts, everything look good. :)

src/kitemviews/kstandarditemlistwidget.cpp
1026

Please notice, you're doing an integer division. Cast m_scaledPixmapSize.height() to qreal or divide by 2.0.

(it seems like the code above does the same too)

sharvey updated this revision to Diff 30738.EditedMar 27 2018, 6:15 PM
  • Change instances of "2" to "2.0" for proper float division

If division was done by "2", the calculations dropped back to integer math. Correctly using "2.0" ensured the calculation continued to use floating point division.

sharvey marked an inline comment as done.Mar 27 2018, 6:33 PM


If we look at the file listing instead of the Places list for more examples, we can see that there seems to be a size discrepancy between the lineart icons and the fully-rendered icons. In addition, a potential inconsistency (?) between capital letters and lowercase letters. Or maybe it's just a trick of perception. Or maybe there's a reason I don't like Noto Sans. ;)

None of the existing text or font metrics objects have a baseline attribute I can use as a key point. I'm hesitant to go digger deeper into the code and making changes, for the sake of a single pixel.

We might be looking at a case of "as good as it can get", at least without altering things farther back in the code. We've made a 99% improvement over the original state when the bug was reported, and we've avoided introducing any complicated regressions by altering the underlying size & layout calculations.

ngraham accepted this revision.Mar 27 2018, 6:37 PM

Yeah, I'm fine with it now. A definite improvement.

This revision is now accepted and ready to land.Mar 27 2018, 6:37 PM
sharvey added inline comments.Mar 27 2018, 6:37 PM
src/kitemviews/kstandarditemlistwidget.cpp
1026

I'm pretty sure my decimal places got lost during multiple revisions, recompilations, and re-tests, of which there have been MANY. I corrected it, along with the others above. Thanks for catching it.

Yeah, I'm fine with it now. A definite improvement.

And there was much rejoicing!

Please also update the commit message explaining why we are dividing by 2.0

src/kitemviews/kstandarditemlistwidget.cpp
1018

Nitpick: please remove space before the last )

1024

Same here

zzag added a comment.Mar 27 2018, 8:58 PM


If we look at the file listing instead of the Places list for more examples, we can see that there seems to be a size discrepancy between the lineart icons and the fully-rendered icons. In addition, a potential inconsistency (?) between capital letters and lowercase letters. Or maybe it's just a trick of perception. Or maybe there's a reason I don't like Noto Sans. ;)

It's because action icons have inner padding.


As you see, Noto Sans' midline is slightly lower than text rect's midline so that's why text feels bottom-aligned, I guess. It doesn't have anything to do with rounding errors, etc.

cfeck added a comment.Mar 27 2018, 9:05 PM

In Skulpture style I had a user option to shift text up or down a few pixels, so that centered text could be fine tuned depending on the font.

This cannot be done by applications, because they do not know how a font looks. Using mathematical centering and/or Qt::AlignVCenter is all you can do.

sharvey updated this revision to Diff 30763.Mar 27 2018, 9:21 PM
  • Remove extra spaces inside parentheses
sharvey marked 2 inline comments as done.Mar 27 2018, 9:24 PM
In D11650#235654, @zzag wrote:

It's because action icons have inner padding.

Well that's not helpful (in this case)!

As you see, Noto Sans' midline is slightly lower than text rect's midline so that's why text feels bottom-aligned, I guess. It doesn't have anything to do with rounding errors, etc.

I, too, love typography. What did you use to generate that sample?

Between padded icons and questionable midlines... no wonder this "simple" fix became so complicated.

Can you update the Summary section like @elvisangelaccio requested?

sharvey edited the summary of this revision. (Show Details)Mar 27 2018, 10:50 PM

Can you update the Summary section like @elvisangelaccio requested?

Done. I thought he just wanted me to expand on that specific commit message, which I revised.

I've added details to the Summary explaining the method and the use of 2.0.

@elvisangelaccio, are you happy with this now?

zzag added a comment.Mar 27 2018, 11:41 PM
From e1b181dc1974b2ca0e1ecad2135318d82bfc7d63 Mon Sep 17 00:00:00 2001
From: Vlad Zagorodniy <vladzzag@gmail.com>
Date: Wed, 28 Mar 2018 02:37:26 +0300
Subject: [PATCH] kitemview/kstandarditemlistwidget: vertically align icons

---
 src/kitemviews/kstandarditemlistwidget.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/kitemviews/kstandarditemlistwidget.cpp b/src/kitemviews/kstandarditemlistwidget.cpp
index d396e58f8..c9a3fcc67 100644
--- a/src/kitemviews/kstandarditemlistwidget.cpp
+++ b/src/kitemviews/kstandarditemlistwidget.cpp
@@ -1022,8 +1022,10 @@ void KStandardItemListWidget::updatePixmapCache()
         const TextInfo* textInfo = m_textInfo.value("text");
         m_pixmapPos.setX(textInfo->pos.x() - 2 * padding
                          - (scaledIconSize + m_scaledPixmapSize.width()) / 2);
-        m_pixmapPos.setY(padding
-                         + (scaledIconSize - m_scaledPixmapSize.height()) / 2);
+        const qreal midlineShift = m_customizedFontMetrics.height() / 2.0
+            - m_customizedFontMetrics.descent()
+            - m_customizedFontMetrics.capHeight() / 2.0;
+        m_pixmapPos.setY(m_textRect.center().y() + midlineShift - m_scaledPixmapSize.height() / 2.0);
     }
 
     m_iconRect = QRectF(m_pixmapPos, QSizeF(m_scaledPixmapSize));
-- 
2.16.3

I, too, love typography. What did you use to generate that sample?

That's a simple demo app I built.

Between padded icons and questionable midlines... no wonder this "simple" fix became so complicated.

No, that's simple(see the patch above). Feel free to copy it. ;-)

@zzag : That's a nice calculation. I patched it in temporarily and had a look. I get a value of 0.5 for midlineShift (Noto Sans 11), so we're once again looking at fractions of pixels(!)

@ngraham , @elvisangelaccio : Do you want me to update my patch with Vlad's adjustment included? While it may require some re-testing, I think it'll be the final word on getting this alignment correct.

Sure, go ahead and give it a whirl, if it works!

zzag added a comment.EditedMar 28 2018, 2:33 AM

@zzag : That's a nice calculation. I patched it in temporarily and had a look. I get a value of 0.5 for midlineShift (Noto Sans 11), so we're once again looking at fractions of pixels(!)

What's your screen scale value?

Also, I could screw up somewhere while I was simplifying code. IIRC the shift should be 1(or not?) when screen scale is equal to 1.

Update

The patch I posted looks good. I test it tomorrow again. But what really matters whether icons look aligned.

Talking about fractions of pixels. That's perfectly fine. A font is roughly speaking a bunch of bezier curves.

sharvey updated this revision to Diff 30785.Mar 28 2018, 9:01 AM
  • Include adjustment of center point based on font's midline

In some cases (i.e. Noto Sans), the font's midline may be offset from the
center of the text frame. midlineShift takes this into account and adds a
small adjustment to the Y value of the icon as necessary

In D11650#235794, @zzag wrote:

@zzag : That's a nice calculation. I patched it in temporarily and had a look. I get a value of 0.5 for midlineShift (Noto Sans 11), so we're once again looking at fractions of pixels(!)

What's your screen scale value?

Also, I could screw up somewhere while I was simplifying code. IIRC the shift should be 1(or not?) when screen scale is equal to 1.

Screen scale is 1.0 - it's probably a fluke due to running in a virtual machine.

I've added in your adjustment and updated the patch. We'll let @ngraham give us his opinion. I think it looks fine as well, but we'll let him give the final answer.

ngraham accepted this revision.Mar 28 2018, 1:52 PM

This now looks absolutely flawless to me. Congratulations and thank you to all involved!

sharvey edited the summary of this revision. (Show Details)Mar 28 2018, 2:27 PM

This now looks absolutely flawless to me. Congratulations and thank you to all involved!

I updated the Summary to include the midlineShift calculation, and gave credit where it's due.

elvisangelaccio accepted this revision.Mar 29 2018, 8:04 PM
This revision was automatically updated to reflect the committed changes.