Skins: allow custom brightness threshold for hotcue colors #2928

ronso0 · 2020-07-10T15:35:27Z

The style of WHotcueButton can be adjusted for dim & bright hotcue colors.
In LateNight PaleMoon theme which is rather dim the default brightness threshold 127 doesn't work well; colors are considered 'dark' too early.

This change allows to set a custom <DimBrightThreshold> (int 0-256) for the following widgets:

HotcueButton
Overview
Visual (scrolling waveforms)

In the PaleMoon example I used the default 127 in the overview to keep the contrasting marker strokes.
(see also #3003)

ronso0 · 2020-08-10T16:52:54Z

Now waveforms and overviews can read the <DimBrightThreshold> node, too. Default is 127.

The sole purpose is to adjust cue colors to dim PaleMoon theme. Please test!

Holzhaus

Thanks. I can't test properly, because it hangs very long when starting and loading a track doesn't work either, but I doubt this was caused by your changes. Probably this is because this branch is so old. I suggest you rebase on latest 2.3 and also change the target branch to 2.3.

src/waveform/renderers/waveformsignalcolors.h

src/widget/wwaveformviewer.cpp

ronso0 · 2020-08-17T20:48:49Z

Thanks. I can't test properly, because it hangs very long when starting and loading a track doesn't work either, but I doubt this was caused by your changes. Probably this is because this branch is so old. I suggest you rebase on latest 2.3 and also change the target branch to 2.3.

This is based on 2.3 already and FWIW I didn't encounter the issues you did.
I rebased and targeted at 2.3

Holzhaus · 2020-08-17T21:07:33Z

Thanks. I can't test properly, because it hangs very long when starting and loading a track doesn't work either, but I doubt this was caused by your changes. Probably this is because this branch is so old. I suggest you rebase on latest 2.3 and also change the target branch to 2.3.

This is based on 2.3 already and FWIW I didn't encounter the issues you did.
I rebased and targeted at 2.3

Yes, I got some weird ACPI issues on my new laptop. A reboot fixed them.

This is how it looks on my machine. I think we need light hotcue button labels, but other than it it looks fine.

ronso0 · 2020-08-17T21:54:09Z

...and again the SVGs are rendered diferently it seems:

now that it works I can increase the blur, but I'm a bit tired of this cross-computer testing by now, so I may as well switch to bright icons, even if that's what I wanted to avoid in the first place.

Thanks for testing!

Holzhaus · 2020-08-17T22:13:41Z

...and again the SVGs are rendered diferently it seems:

now that it works I can increase the blur, but I'm a bit tired of this cross-computer testing by now, so I may as well switch to bright icons, even if that's what I wanted to avoid in the first place.

Whoops, the low contrast might actually not be Qt's fault. It's already late and I forgot that I have redshift enabled on my computer, so this might have caused this. Anyway, I think the contrast is a bit too low especially when you turn down screen brightness in a club setting, so using white labels or conderably more blur would probably be a good idea. I'd probably prefer white text instead of a strong blue effect, but you decide 😃

ronso0 · 2020-08-17T22:52:13Z

Okay, good to know :)
I'll play around with a bit, maybe bright icons are more cnsistent in the end considering the waveform labels also switch to bright fonts. I'm just glad it doesn't pick a bright text for the orange in that picture, that was really a hard contrast at night.

daschuer

Thank you for this useful PR. I have left some comments.

src/waveform/renderers/waveformsignalcolors.cpp

src/waveform/renderers/waveformsignalcolors.h

src/waveform/renderers/waveformwidgetrenderer.cpp

src/widget/whotcuebutton.cpp

Holzhaus · 2020-08-21T21:43:34Z

There are merge conflicts now.

ronso0 · 2020-08-22T20:18:59Z

sometimes I wonder why changes like that are considered 'conflicts'...

Holzhaus · 2020-08-22T22:01:05Z

Because these lines were changed in one branch and removed in the other. You should enable diff3 conflict style in your git settings to see it properly:

git config --global merge.conflictstyle diff3

Then it will look like this:

<<<<<<< HEAD
Content in this branch. 
||||||| merged common ancestors
Content of common ancestor. 
=======
Content in other branch. 
>>>>>>> abdef12....

No idea why this isn't the default.

ronso0 · 2020-08-31T22:59:06Z

Just to let you know: I addressed the comments but didn't push, yet, because I need some continous time to thoroughly test the thresholds with updated hotcue icons and the waveforms.
This won't happen before next week, though.

…ks and hotcues, refine light hotcue icons

ronso0 · 2020-09-23T13:57:07Z

A TMP commit slipped in so I finally cleaned up and rebased onto 2.3. Please have a look!

I adjusted the thresholds in LateNight PaleMoon and made the hotcue button icons more clear.
The comparison uses the new icons for demonstration.
before (general threshold = 127):

this PR (custom threshold for buttons and waveform = 90):

also reasonable: threshold for buttons and waveform = 80

What do you think?

Holzhaus · 2020-09-23T14:03:58Z

90 is okay, 80 makes the "6" barely readable.

Holzhaus

There are merge conflicts.

Also, I think the contrast for the blue hotcue colors from the default palette is bad. And maybe the red and purple button should be considered "dark", too?

ronso0 · 2020-10-18T16:14:24Z

you can try a different threshold, set in skin.xml in PaleMoon color scheme

Holzhaus · 2020-10-19T21:10:21Z

There are merge conflicts.

Also, I think the contrast for the blue hotcue colors from the default palette is bad. And maybe the red and purple button should be considered "dark", too?

I tried, using the light grey text on red make the contrast even worse.

ronso0 · 2020-10-19T21:25:17Z

I'll check if it can be fixed with a brighter font for dark buttons.

ronso0 · 2020-10-20T13:28:27Z

Oh, the first time for quite a while I got a "Build passed" message from our friend Travis :)

ronso0 · 2020-10-20T23:33:35Z

I think the contrast for the blue hotcue colors from the default palette is bad

I made the 'dark' icons a bit brighter:

And maybe the red and purple button should be considered "dark", too?

I don't want to tweak our dark/light algorythm here. Would you mind fiing a bug report?

Holzhaus · 2020-10-21T09:59:00Z

And maybe the red and purple button should be considered "dark", too?

I don't want to tweak our dark/light algorythm here. Would you mind fiing a bug report?

The algorithm is okay, I tweaked the threshold to 105 and now it looks like this:

I think that has better contrast, but you decide. Just let me know, so we can merge this.

daschuer

Thank you.

ronso0 · 2020-10-21T13:05:53Z

And maybe the red and purple button should be considered "dark", too?

I don't want to tweak our dark/light algorythm here. Would you mind fiing a bug report?

The algorithm is okay, I tweaked the threshold to 105 and now it looks like this:

I get the impression what I'm trying to improve here is not matter of the dim/bright threshold, but it also seems to depend a lot on the screen used and maybe also the algorhythm, because when I tested with Mixxx Hotcue palette yesterday I perceived the Red indeed darker than the Blue.

I did check again with https://webaim.org/resources/contrastchecker/ and you're right: with the new threshold we get ~4.1-3 for the Red, Blue, Purple which is considered okay for normal text.

Did you try 105 also for the waveforms?
If you find that value working for both Hotcues and waveforms, let me know.

Holzhaus · 2020-10-25T21:04:25Z

Did you try 105 also for the waveforms?
If you find that value working for both Hotcues and waveforms, let me know.

Yes, looks good to me.

ronso0 · 2020-10-25T21:08:21Z

Thanks!
Are we done here?

Holzhaus

There are some pre-commit issues, please check.

ronso0 · 2020-10-25T21:19:34Z

those are about

-    int getDimBrightThreshold() { return m_dimBrightThreshold; }
+    int getDimBrightThreshold() {
+        return m_dimBrightThreshold;
+    }

Either I reformat all surrounding one-liners or we ignore the complaint. I don't care but I'd like to have some consitency in that file.
(IIRC we already discussed that specific spot and agreed to ignore, but I'm not sure)

Holzhaus · 2020-10-25T21:20:45Z

I'm okay with reformatting all of them.

…unctions

ronso0 · 2020-10-27T19:42:57Z

Ready.

Holzhaus

LGTM, thank you

Holzhaus · 2020-10-27T19:51:59Z

src/waveform/renderers/waveformwidgetrenderer.h

+    double getGain() const {
+        return m_gain;
+    }


Not your fault, but shouldn't this be CSAMPLE_GAIN instead of double? Anyway, fixing this is out of scope for this PR.

ronso0 requested a review from daschuer July 10, 2020 15:37

ronso0 added the skins label Jul 10, 2020

ronso0 added this to the 2.3.0 milestone Jul 10, 2020

ronso0 mentioned this pull request Jul 15, 2020

LateNight :: follow-up fixes / update #2913

Merged

20 tasks

ronso0 force-pushed the hotcue-dim-bright branch from 43f82fa to 85c998e Compare August 10, 2020 16:48

ronso0 marked this pull request as ready for review August 10, 2020 16:50

Holzhaus requested changes Aug 16, 2020

View reviewed changes

src/waveform/renderers/waveformsignalcolors.h Outdated Show resolved Hide resolved

src/widget/wwaveformviewer.cpp Outdated Show resolved Hide resolved

ronso0 changed the base branch from master to 2.3 August 17, 2020 19:38

ronso0 force-pushed the hotcue-dim-bright branch from 85c998e to 3ca86b9 Compare August 17, 2020 20:48

daschuer requested changes Aug 19, 2020

View reviewed changes

ronso0 added 3 commits September 23, 2020 14:50

color util: rename isDimmColor > is DimColor, add 'b' to bool variables

c4f6220

WHotcueButton/color.h: allow custom dim/bright threshold

8f0962e

WOverView/WVisual: allow custom dim/bright treshold for marks

53c2bc6

ronso0 force-pushed the hotcue-dim-bright branch from bc19a80 to 53c2bc6 Compare September 23, 2020 13:37

LateNight PaleMoon: set custom dim/bright thresholds for waveform mar…

2415003

…ks and hotcues, refine light hotcue icons

ronso0 requested a review from Holzhaus October 12, 2020 16:40

ronso0 requested a review from daschuer October 12, 2020 16:40

Holzhaus reviewed Oct 18, 2020

View reviewed changes

Merge branch '2.3' into hotcue-dim-bright

9272660

Latenight PaleMoon: brighter icons for dark hotcues

8decddb

daschuer approved these changes Oct 21, 2020

View reviewed changes

LateNight: adjust dim/bright threshold to 105

faa13ae

Holzhaus approved these changes Oct 25, 2020

View reviewed changes

Holzhaus requested changes Oct 25, 2020

View reviewed changes

waveformsignalcolors.h/waveformwidgetrenderer.h: break one-line get f…

45eade2

…unctions

Holzhaus approved these changes Oct 27, 2020

View reviewed changes

Holzhaus merged commit b2efa10 into mixxxdj:2.3 Oct 27, 2020

ronso0 deleted the hotcue-dim-bright branch October 27, 2020 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skins: allow custom brightness threshold for hotcue colors #2928

Skins: allow custom brightness threshold for hotcue colors #2928

ronso0 commented Jul 10, 2020 •

edited

Loading

ronso0 commented Aug 10, 2020

Holzhaus left a comment

ronso0 commented Aug 17, 2020

Holzhaus commented Aug 17, 2020

ronso0 commented Aug 17, 2020

Holzhaus commented Aug 17, 2020 •

edited

Loading

ronso0 commented Aug 17, 2020

daschuer left a comment

Holzhaus commented Aug 21, 2020

ronso0 commented Aug 22, 2020

Holzhaus commented Aug 22, 2020 •

edited

Loading

ronso0 commented Aug 31, 2020

ronso0 commented Sep 23, 2020 •

edited

Loading

Holzhaus commented Sep 23, 2020

Holzhaus left a comment

ronso0 commented Oct 18, 2020

Holzhaus commented Oct 19, 2020

ronso0 commented Oct 19, 2020

ronso0 commented Oct 20, 2020

ronso0 commented Oct 20, 2020

Holzhaus commented Oct 21, 2020

daschuer left a comment

ronso0 commented Oct 21, 2020

Holzhaus commented Oct 25, 2020

ronso0 commented Oct 25, 2020

Holzhaus left a comment

ronso0 commented Oct 25, 2020

Holzhaus commented Oct 25, 2020

ronso0 commented Oct 27, 2020

Holzhaus left a comment

Holzhaus Oct 27, 2020

Skins: allow custom brightness threshold for hotcue colors #2928

Skins: allow custom brightness threshold for hotcue colors #2928

Conversation

ronso0 commented Jul 10, 2020 • edited Loading

ronso0 commented Aug 10, 2020

Holzhaus left a comment

Choose a reason for hiding this comment

ronso0 commented Aug 17, 2020

Holzhaus commented Aug 17, 2020

ronso0 commented Aug 17, 2020

Holzhaus commented Aug 17, 2020 • edited Loading

ronso0 commented Aug 17, 2020

daschuer left a comment

Choose a reason for hiding this comment

Holzhaus commented Aug 21, 2020

ronso0 commented Aug 22, 2020

Holzhaus commented Aug 22, 2020 • edited Loading

ronso0 commented Aug 31, 2020

ronso0 commented Sep 23, 2020 • edited Loading

Holzhaus commented Sep 23, 2020

Holzhaus left a comment

Choose a reason for hiding this comment

ronso0 commented Oct 18, 2020

Holzhaus commented Oct 19, 2020

ronso0 commented Oct 19, 2020

ronso0 commented Oct 20, 2020

ronso0 commented Oct 20, 2020

Holzhaus commented Oct 21, 2020

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Oct 21, 2020

Holzhaus commented Oct 25, 2020

ronso0 commented Oct 25, 2020

Holzhaus left a comment

Choose a reason for hiding this comment

ronso0 commented Oct 25, 2020

Holzhaus commented Oct 25, 2020

ronso0 commented Oct 27, 2020

Holzhaus left a comment

Choose a reason for hiding this comment

Holzhaus Oct 27, 2020

Choose a reason for hiding this comment

ronso0 commented Jul 10, 2020 •

edited

Loading

Holzhaus commented Aug 17, 2020 •

edited

Loading

Holzhaus commented Aug 22, 2020 •

edited

Loading

ronso0 commented Sep 23, 2020 •

edited

Loading