do not extend mouse areas in header bar longer than the text
ClosedPublic

Authored by mgallien on May 6 2019, 5:27 AM.

Details

Summary

avoid having empty zones that respond to mouse click

Test Plan

mouse areas are not much longer than the text in the header bar

Diff Detail

Repository
R255 Elisa
Branch
fixTooLongMouseAreaInHeaderBar
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11616
Build 11634: arc lint + arc unit
mgallien requested review of this revision.May 6 2019, 5:27 AM
mgallien created this revision.
ngraham requested changes to this revision.May 6 2019, 3:30 PM

For me this patch re-introduces the original bug (https://bugs.kde.org/show_bug.cgi?id=406483).

This revision now requires changes to proceed.May 6 2019, 3:30 PM

For me this patch re-introduces the original bug (https://bugs.kde.org/show_bug.cgi?id=406483).

Which font are you using ?

I do not yet have an explanation why the width is wrong but it could be something specific to a font. I will try another approach.

11 pt Ubuntu font. I see the same thing with default 10 pt Noto Sans though:

As you can see, it's even worse with default settings since the four-letter album name gets elided too.

mgallien updated this revision to Diff 57684.May 6 2019, 8:49 PM
  • use the correct way to get the width of the text

should fix the text being truncated when it should not

11 pt Ubuntu font. I see the same thing with default 10 pt Noto Sans though:

As you can see, it's even worse with default settings since the four-letter album name gets elided too.

Yes, I had not understood correctly how to use TextMetrics. That should be fine now.

I'm afraid even with this latest update to the patch, I still see elided text. :(

mgallien updated this revision to Diff 57731.May 7 2019, 5:58 PM
  • fix corner cases by adding an extra length

fix case of Ubuntu font

ngraham accepted this revision.May 7 2019, 6:43 PM

This now works!

However I'm left to wonder why all this rigamarole with TextMetrics is even necessary at all. I notice that if I remove all the TextMetrics components, and let the labels auto-size themselves by removing Layout.fillWidth and Layout.maximumWidth, everything works fine except for the artist label, which gets elided. But I notice that it only gets elided when the point size is set to elisaTheme.defaultFontPointSize * 1.5. If I change the multiplier to 1, 1.4 or 2, it works. However a multiplier of 1.5, 1.75, or any other odd value triggers the bug. This is reproducible for all fonts and sizes that I tried.

I don't understand this bug, but a less invasive workaround might be simply to change the multiplier a bit rather than do all this stuff with TextMetrics.

This revision is now accepted and ready to land.May 7 2019, 6:43 PM
mgallien updated this revision to Diff 57750.May 8 2019, 9:07 AM
  • use Math.round to avoid having a font.pointSize being decimal

thanks for all the feedback

It looks like the font.pixelSize should probably be set to only integer values

This now works!

However I'm left to wonder why all this rigamarole with TextMetrics is even necessary at all. I notice that if I remove all the TextMetrics components, and let the labels auto-size themselves by removing Layout.fillWidth and Layout.maximumWidth, everything works fine except for the artist label, which gets elided. But I notice that it only gets elided when the point size is set to elisaTheme.defaultFontPointSize * 1.5. If I change the multiplier to 1, 1.4 or 2, it works. However a multiplier of 1.5, 1.75, or any other odd value triggers the bug. This is reproducible for all fonts and sizes that I tried.

I don't understand this bug, but a less invasive workaround might be simply to change the multiplier a bit rather than do all this stuff with TextMetrics.

Thanks a lot for your work. You gave me the idea to have a try at rounding the computed pointSize to an integer. During my tests, this has fixed the bug. Let me know if you have time to test.

ngraham accepted this revision.May 8 2019, 1:58 PM

This looks much better, thanks. It makes sense that font.pointSize should be rounded to an integer value.

This revision was automatically updated to reflect the committed changes.