Beautify Shade labels #26

daschuer · 2019-09-18T21:27:34Z

No description provided.

… the color schemes.

Be-ing · 2019-09-19T00:50:41Z

src/widget/woverview.cpp

@@ -402,8 +402,8 @@ void WOverview::mouseMoveEvent(QMouseEvent* e) {
    // the hotcue rendered on top must be assigned to m_pHoveredMark to show
    // the CueMenu. To accomplish this, m_marksToRender is iterated in reverse
    // and the loop breaks as soon as m_pHoveredMark is set.
-    for (auto it = m_marksToRender.crbegin(); it != m_marksToRender.crend(); ++it) {
-        auto pMark = *it;
+    for (int i = m_marksToRender.size() - 1; i >= 0 ; --i) {


Thanks, this is easier to understand anyway.

Be-ing · 2019-09-19T01:25:30Z

res/skins/Shade/deck_overview.xml

@@ -14,9 +14,6 @@
    <PlayPosColor>#00FF00</PlayPosColor>
    <EndOfTrackColor>#EA0000</EndOfTrackColor>
    <PlayedOverlayColor>#60000000</PlayedOverlayColor>
-    <LabelBackgroundColor>#80000000</LabelBackgroundColor>
-    <LabelTextColor>#ffffff</LabelTextColor>
-    <LabelFontSize>9</LabelFontSize>


This breaks Shade, the label disappears when hovering:

9 is the maximum font size that fits

If you want to increase the vertical height of the overview to allow a bigger text size, that would be fine with me.

Not for me:

Does this overlap also happen in a unscaled version? ... Or is it a different font issue?

This also happens for me with 1x scaling:

Ok, so I guess we need to set a font as well.

You're right, I still had automatic scaling enabled so that was at 2x. Here it is at 1x; the problem is still there:

Got it. See the latest commit. Your font seems to have higher charters for Î or such. But this is not relevant here, so we can use the pixel high, which only encounters the height of "Hg"

Why should we assume that the user isn't using tall characters like Î? The label can be any text the user wants.

The user can still use Î, depending to the font it may now hit or exceed the background. This is OK for me since the majority of characters are looking well.

I don't think this is a good idea. The area of the background should cover any possible character.

Be-ing · 2019-09-19T01:28:56Z

src/waveform/waveformmarklabel.cpp

    painter.setBrush(QBrush(backgroundColor));
-    painter.drawRoundedRect(0, 0, pixmapRect.width(), pixmapRect.height(), 5.0, 5.0);
+    painter.drawRoundedRect(0, 0, pixmapRect.width(), pixmapRect.height(), 2.0, 2.0);


Why was this changed?

Because with 5.0, the labels are too much rounded, almost a circle in case of a single character.
See:

src/waveform/waveformmarklabel.cpp

Be-ing · 2019-09-19T17:47:29Z

src/waveform/waveformmarklabel.cpp

+    // We take the pixelSize for background height, to not consider exotic
+    // high characters.
+    pixmapRect.setHeight(
+            math_max(font.pixelSize() * 1.2,


As far as I can tell this makes no difference. The label is still hidden in Shade when I hover a hotcue.

daschuer · 2019-09-19T22:47:19Z

OK, I have forced pushed a better solution. m_labelFontSize is now adjusted to be Font Type independent.

Be-ing · 2019-09-20T02:56:23Z

src/widget/woverview.cpp

-                || pMark->m_label.intersects(m_timeRulerPositionLabel)
-                || pMark->m_label.intersects(m_timeRulerDistanceLabel)) {
-            continue;
-        }


Revert this. Labels should not overlap.

It is not the right place to force this here. This code skips to draw the label just because the font is too big or the waveform too small. I agree that the labels should not overlap, but if there is a skin issue it should be rendered anyway to see the overlap.

This is required for normal functionality. It is not to work around bugs in skins. It is normal for the intro/outro cue markers and durations which are rendered on top, and the main cue label which is rendered at vertical center in Tango, to overlap the labels when holding right click.

if there is a skin issue it should be rendered anyway to see the overlap.

I disagree. The label disappearing makes bugs easier to catch.

Some pixel vertical overlap is no issue, rectifies not drawing the lable, especially because the position and distance labels are still painted overlapping. We need here a solution that detects the cue type and hides the labels based on that.

No, we need to not have overlap. Please do not make this any more complicated. It was already working well. I will not merge this without this commit being reverted (or removed with a rebase).

We need a solution for the root issue first: Different font heights on different systems. Will you fix it in your branch?
Than I can finally adjust this code here.

Be-ing · 2019-09-20T02:57:56Z

src/widget/woverview.cpp

+    QFont refFont;
+    refFont.setPixelSize(labelFontSize);
+    QFontMetricsF fontMetrics(refFont);
+    m_labelFontSize = labelFontSize / fontMetrics.height() * labelFontSize;


I don't understand this math. Why not just use fontMetrics.height()?

The labelFontSize is the high of "Hg" decent() + capitalHight()
My system font is Ubuntu, you have a different font. The ascent() is different.
Here we adjust the m_labelFontSize for an almost equal font independent hight().

I think I need to improve the comment.

Does this produce a good result on you side?

almost equal font independent hight()

This goal does not make sense to me. The actual height of the text as it is rendered is what is needed.

My system font is Ubuntu, you have a different font.

This seems to be the root of the issues. Does Shade not specify a font? If so, let's fix that issue so we get consistent results on both our computers.

almost equal font independent hight()

This goal does not make sense to me. The actual height of the text as it is rendered is what is needed.

Yes, but the actual height is finally kind of random since the skin designer does not know the system font. The skin designer can optimize the looking with the pixel size of its local font. If user uses a font with the same pixel size but taller or lower characters, the labels might overlap. This has happened on your and my system. This math here takes care, that the actual pixel size for the tall font is lower in a way that Üg = Üg and not only Ug = Ug

Does Shade not specify a font?

It does: font-family: "Open Sans";
But as long as we do not install the font it can be any fallback.

How about setting the maximum label height in the skin xml rather then setting the font pixel size?
This way the font can defined by style sheets and if it exceeds the lable the size is adjusted.

I have tried replacing font-family: "Open Sans" with font-family: Ubuntu in Shade's style.qss and the label still gets hidden when hovering.

The font used in my case is already Ubuntu. We need to find a way to define the Mixxx shipped font on all systems.

With font-family: Ubuntu, I am able to increase the LabelFontSize for Shade up to 12. With "Open Sans", 11 is the max size that fits. Either we can decrease the font size to 11 or explicitly specify to use the Ubuntu font for Shade (which we also use for Tango).

Ping. You can either explicitly specify the Ubuntu font or change the font size to 11. I think explicitly specifying the font would be better to ensure a consistent appearance.

Be-ing · 2019-09-28T18:29:00Z

@ronso0 do you have any hints why we are seeing different results with the labels being hidden on our different computers?

I think if we cannot figure out why this is happening soon we should just stick with a font size of 9 for Shade's labels and get 2.3 beta out already. I don't think increasing the font size by a pixel or two is worth continuing to delay this.

ronso0 · 2019-10-01T16:54:29Z

@ronso0 do you have any hints why we are seeing different results with the labels being hidden on our different computers?

Not at first glance. I can have a look at it soonish, like tomorrow night.
Did you make sure the font include is working in other places of Shade? We deliver OpenSans with Mixxx so why should it fall back to some system font..

daschuer · 2019-10-01T18:50:30Z

Looking on this again, I think it is a conceptual issue to set the font size via skin.xml but the font type via css. Can we remove the size feature form the skin?

daschuer · 2019-10-03T21:13:54Z

Ok, WOverview has the cue labels and the status messages, so it makes sense the set the label font size via skin xml. I have reverted the critical commits and set the font to "Open Sans" for WOverview explicit. Finally it looks nice. @Be-ing, ok for merging?

Be-ing · 2019-10-05T22:09:37Z

With font-family: Ubuntu, I am able to increase the LabelFontSize for Shade up to 12. With "Open Sans", 11 is the max size that fits.

Hmm, I can't reproduce these results now. I'm not sure what I did to get them. The maximum font size with Open Sans is 8 and it is 10 with Ubuntu. I tested this on my branch and got the same results. Without specifying a font-family for WOverview in style.qss, the max font size is 9 as it is in my branch. I do not know what default font that is falling back to.

daschuer · 2019-10-11T11:15:09Z

@Be-ing: Does this mean the issue is fixed for you as well? Merge?

Be-ing · 2019-10-11T13:07:03Z

No, this is still not working.

daschuer · 2019-10-11T16:57:41Z

Why not? Can you attach a screenshot?
Do you have an idea how to fix the issue?

Be-ing · 2019-10-12T14:08:03Z

I stated the fix before: change the font size to 8 with Open Sans or 10 with Ubuntu.

daschuer · 2019-10-12T15:10:36Z

But why? Here it looks nice now. How does it look at you side?

Be-ing · 2019-10-12T16:19:06Z

The problem is as it has been whole time of this PR. The labels disappear when hovered with the mouse if the text is too big because of the overlap.

daschuer · 2019-10-12T16:40:40Z

With my latest changes it doesn't happen any more and it is independent from the system font now.
That's why screenshot is crucial to compare differences pixel by pixel.
Does it happen with 100% scaling as well?

Be-ing · 2019-10-12T17:23:15Z

daschuer · 2019-10-13T14:55:01Z

Oh sorry, I can reproduce. I must have pushed the wrong version. Now I have switched back to Ubuntu 11.

All skins benefit from the sightly bigger font.

Be-ing · 2019-10-13T20:24:53Z

The text is still too big. Again, the maximum font size for Ubuntu that fits with Shade is 10. Here is a screenshot of how it looks with this PR currently:

daschuer · 2019-10-13T20:43:42Z

Does the 100% version works for you now?

daschuer · 2019-10-13T21:01:58Z

Here is mine

daschuer · 2019-10-13T21:18:44Z

and 300 %

daschuer · 2019-10-13T21:29:49Z

The font has the same size, but the label is bigger. Maybe it depends on the Qt version.

daschuer · 2019-10-13T22:03:23Z

Ups, this is a bigger issue, because the overview deck and the mini decks of every skin are suffering the same issue.
So the difference seams either be a bug in your or mine Qt version.
It feels odd to user such big labels, don't allow to overlap, just for the rare case of a very tall character.
I have tried various exotic characters, but I did not find one which is able to exceed my size.
I think it is reasonable to reduce the label size in favour of readable numbers.

Be-ing · 2019-10-13T23:35:16Z

Does the 100% version works for you now?

No. As before, the scale factor has no impact on this bug.

Be-ing · 2019-10-13T23:36:13Z

Maybe it depends on the Qt version.

Perhaps? I can't come up with any other explanation for why we are getting different results on our computers.

Be-ing · 2019-10-14T00:22:51Z

overview deck and the mini decks of every skin are suffering the same issue.

Okay, so decrease the font size.

I think it is reasonable to reduce the label size in favour of readable numbers.

Again, I don't think we should mess with this. This PR for trivial polishing details has been open for 3 weeks now, IMO it was never necessary in the first place, meanwhile we still have no beta release. Can we please just decrease the font size and move on already?

daschuer · 2019-10-14T15:13:20Z

This PR discovers an other system dependent relationship than we have first discussed. This means the skin designer has no control if the skin works or not. This needs to be fixed before merge.

Reducing the font size for the mimum decks a small value makes the info ugly or even unreadable. With 300 % scaling, you will of course be able to read any font size this is not the case with 100 %.

For a complete solution, I think we need to dig down the issue first. I have already tried to find differences caused by the Qt Version, but reviewing the Qt git history did not discover anything so far. I think I will add some debug statements, to get it finally.

As a quick hack we may limit the lable high to 1/3 of the overview hight. This will at least stop the "disappearing" bug. Cutting some pixels of some exotic characters is a neglegtable issue compared to this.

What do you think.

Be-ing · 2019-10-14T16:11:58Z

This needs to be fixed before merge.

No it does not. Please keep perspective here. Nobody expects perfection from a beta release. We are already going on a year without a release. We are talking about literally 1 pixel difference in font sizes.

What do you think.

No more hacks. Just reduce the font size and let's get this release out already.

daschuer · 2019-10-14T17:15:13Z

We are talking about literally 1 pixel difference in font sizes.

That would be no issue for me with the Shade main overview. The real issue is the unreliable label background size and the unreadable font size in the overview and preview player. I cannot fix this here, because the label too big issue on your side.

Be-ing · 2019-10-14T17:17:34Z

How about we introduce a boolean skin element to disable showing the times on cue hover for the preview deck and mini decks?

daschuer · 2019-10-14T22:02:45Z

Good idea. So we need to find only the bug for the different label heights.

Be-ing · 2019-10-14T22:04:12Z

No we don't. Just make the text one pixel size smaller.

daschuer · 2019-10-14T22:06:09Z

OK

Be-ing · 2019-10-14T22:07:25Z

If you want to look into the differences in rendering later, you can do that. But continuing to hold this up over literally one pixel in font size is IMO absurd.

…the cue lable disappear on some systems.

daschuer added 3 commits September 18, 2019 23:22

Fix building on Xenial

a26999e

Remove border of cue labels and increase font size

7c320fa

Shade: set a overview bg color to alter the cue label background with…

7c304d1

… the color schemes.

daschuer mentioned this pull request Sep 18, 2019

add hotcue text labels, position, and right click menu to overview waveform mixxxdj/mixxx#2238

Merged

Be-ing reviewed Sep 19, 2019

View reviewed changes

src/waveform/waveformmarklabel.cpp Show resolved Hide resolved

daschuer changed the title ~~Beatify Shade labels~~ Beautify Shade labels Sep 19, 2019

Be-ing reviewed Sep 19, 2019

View reviewed changes

daschuer force-pushed the hotcue_labels branch from 2fe49ae to 651aa27 Compare September 19, 2019 22:44

daschuer force-pushed the hotcue_labels branch from 651aa27 to 0800962 Compare September 19, 2019 22:48

Be-ing reviewed Sep 20, 2019

View reviewed changes

daschuer force-pushed the hotcue_labels branch from 0800962 to 8ab55f4 Compare October 3, 2019 21:08

daschuer force-pushed the hotcue_labels branch from 8ab55f4 to 1745f3c Compare October 13, 2019 14:37

Use "Ubuntu" font for WOverview

1e947ab

daschuer force-pushed the hotcue_labels branch from 1745f3c to 1e947ab Compare October 13, 2019 14:54

Revert the default font size to 10px, to not trigger a bug that lets …

02eb82a

…the cue lable disappear on some systems.

Be-ing merged commit 71aca86 into Be-ing:hotcue_labels Oct 14, 2019

daschuer deleted the hotcue_labels branch September 26, 2021 17:37

Beautify Shade labels #26

Beautify Shade labels #26

Conversation

daschuer commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Sep 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing Sep 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Sep 28, 2019

ronso0 commented Oct 1, 2019

daschuer commented Oct 1, 2019

daschuer commented Oct 3, 2019

Be-ing commented Oct 5, 2019

daschuer commented Oct 11, 2019

Be-ing commented Oct 11, 2019

daschuer commented Oct 11, 2019

Be-ing commented Oct 12, 2019

daschuer commented Oct 12, 2019

Be-ing commented Oct 12, 2019

daschuer commented Oct 12, 2019

Be-ing commented Oct 12, 2019

daschuer commented Oct 13, 2019

Be-ing commented Oct 13, 2019

daschuer commented Oct 13, 2019

daschuer commented Oct 13, 2019

daschuer commented Oct 13, 2019

daschuer commented Oct 13, 2019

daschuer commented Oct 13, 2019

Be-ing commented Oct 13, 2019

Be-ing commented Oct 13, 2019

Be-ing commented Oct 14, 2019

daschuer commented Oct 14, 2019

Be-ing commented Oct 14, 2019 • edited Loading

daschuer commented Oct 14, 2019

Be-ing commented Oct 14, 2019

daschuer commented Oct 14, 2019

Be-ing commented Oct 14, 2019

daschuer commented Oct 14, 2019

Be-ing commented Oct 14, 2019 • edited Loading

Be-ing Sep 19, 2019 •

edited

Loading

Be-ing Sep 27, 2019 •

edited

Loading

Be-ing Sep 28, 2019 •

edited

Loading

Be-ing commented Oct 14, 2019 •

edited

Loading

Be-ing commented Oct 14, 2019 •

edited

Loading