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.

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

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.

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!

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?)

In D11650#235186, @sharvey wrote:

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.

In D11650#235016, @ngraham wrote:

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. :)

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.

In D11650#235492, @ngraham wrote:

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

In D11650#235489, @sharvey wrote:

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.

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.

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?

In D11650#235698, @ngraham wrote:

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?

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

In D11650#235679, @sharvey wrote:

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!

In D11650#235747, @sharvey 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.

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.

In D11650#235794, @zzag wrote:

In D11650#235747, @sharvey 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.

In D11650#235928, @ngraham wrote:

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.