Hotcue Buttons, Context Menu + Skin Improvements. #2560

daschuer · 2020-03-17T23:27:19Z

Instead of clear, the Hotcue menu is displayed on right click.
This also simplifies styling and skin.

Holzhaus

Thanks for taking care of this. Opening the cue menu on right click is a real usability improvement IMHO.

I'm not super sure if it's smart to introduce inconsistencies for the Hotcue COs at this point (for unset hotcues the color CO value is not -1 anymore), when all other COs behave differently.

Also, I think that qss styles allows much greater flexibility for skin deveĺopers than just changing background and text color, so I think we should discuss how we can implementing WHotcueButton while still allowing that.

Holzhaus · 2020-03-18T15:37:07Z

res/skins/Deere/style.qss

-#HotcueButton[backgroundIsDark=false][hasBackgroundColor=true] {
-    color: #1f1e1e;
+    qproperty-light: #FDFDFD;
+    qproperty-dark: #1f1e1e;


This looks really inflexible. What if I want to modify the border color or set a button icon or do other stuff in QSS depending on the dark/light property? This isn't possible at the moment, we can only set the text color. Can't we do something like this:

#HotCueButton { color: #1F1E1E; } #HotCueButton[hasDarkColor="true"] { color: #FDFDFD; }

This was the original concept.
I found it quite chunky and is suffers bugs that the wrong color was displayed.
We can revert it if it is a real requirement.

Holzhaus · 2020-03-18T15:40:52Z

src/engine/controls/cuecontrol.cpp

@@ -1769,7 +1766,7 @@ HotcueControl::HotcueControl(QString group, int i)

    // The rgba value  of the color assigned to this color.
    m_hotcueColor = new ControlObject(keyForControl(i, "color"));
-    m_hotcueColor->set(kNoColorControlValue);
+    m_hotcueColor->set(QRgb(ColorPalette::kDefaultCueColor));


Hmm, I'm still not really sure that this is a good idea. All other COs contain -1 when the hotcue is not set, and since it's possible to modify the CO value via developer mode we need range checks anyway IMHO.

Holzhaus · 2020-03-18T15:43:05Z

src/widget/whotcuebutton.cpp

+ConfigKey WHotcueButton::createConfigKey(const QString& name) {
+    ConfigKey key;
+    key.group = m_group;
+    // Add one to hotcue so that we don't have a hotcue_0


Suggested change

// Add one to hotcue so that we don't have a hotcue_0

// Increment hotcue index by one so that we don't have a hotcue_0

Holzhaus · 2020-03-18T15:58:14Z

src/widget/whotcuebutton.cpp

+                                 : QString()) +
+            QStringLiteral("; }");
+
+    if (m_hoverCueColor) {


Idea: In addition to a boolean dark/light property, you could set these QPalette colors:

button

highlight

Then we do something like this in QSS:

#HotcueButton { color: black; background-color: palette(button); } #HotcueButton[hasDarkColor="true"] { color: white; } #HotcueButton:hover { background-color: palette(highlight); }

See: https://doc.qt.io/qt-5/stylesheet-reference.html#paletterole

That way we can still be flexible with our qss styles, but also have support for hightlights.

I will give it a try.

palette() is in this case the default QPalette() set by QApplication::setPalette().
We may change it but this restyles the whole Application, so this is no option yet.

palette() is in this case the default QPalette() set by QApplication::setPalette().
We may change it but this restyles the whole Application, so this is no option yet.

Ok, that unfortunate. @ronso0 Do you have any ideas how to proceed? Do you think these two properies are sufficient?

Actually I'm not happy with just light and dark. For the 'white border' example below it would be more convenient to have dark, medium & bright so that I could use the white border for dark and medium and only change it to grey if we have a bright hotcue.

Nevertheless, I think this is okay fo now. Also I think brightness() is used elsewhere in Mixxx, isn't it?

daschuer · 2020-03-18T17:28:06Z

I'm not super sure if it's smart to introduce inconsistencies for the Hotcue COs at this point (for unset hotcues the color CO value is not -1 anymore), when all other COs behave differently.

The value color is irrelevant if enabled is 0.
If we set color to -1 as well, we have two possible concurrent states indicating "not set". The mapping author might be tempted to use only color, which is wrong. That's why I prefer not using a no-color state.

Holzhaus · 2020-03-20T11:34:05Z

The value color is irrelevant if enabled is 0.
If we set color to -1 as well, we have two possible concurrent states indicating "not set". The mapping author might be tempted to use only color, which is wrong. That's why I prefer not using a no-color state.

Yes, but all other COs do that, too. If we change that behaviour, we should do it for all these COs, not just color.

daschuer · 2020-03-20T14:51:56Z

what are "All" other COs? Only the position CO is set to -1 because it represents the invalid value, which has a programmatic use case.
Unfortunately this is actually a valid value. So We have already discussed to set it to something more invalid like -INF or DBL_MIN.
Compared to this, initializing the color CO with the default color is a good choice.

daschuer · 2020-03-20T15:03:44Z

Done.

Now you can

#HotcueButton[light="true"] {
    color: #1f1e1e;
}

or/and

#HotcueButton[dark="true"] {
    color: #1f1e1e;
}

to style the colored state

Holzhaus · 2020-03-20T17:46:36Z

what are "All" other COs? Only the position CO is set to -1 because it represents the invalid value, which has a programmatic use case.

You're right. I was under the impression that more CO's were using -1 but it looks like that's not the case.

Compared to this, initializing the color CO with the default color is a good choice.

Ok, I'd prefer 0 though which is also a valid color.

ronso0

In Deere for example most buttons have a hover effect and for hotcue buttons this is 'stuck' as soon as the cue popup is opened.
what about emulating a leaveEvent to force [hover="false"] & repaint when the popup is opened?

res/skins/LateNight/style.qss

…oids some unnecessary code cycles.

daschuer · 2020-03-20T19:52:07Z

Ups, Hopefully Latenight Looks good again.
I have also kept the 0 to initialize the the hotcue_nn_color control.

daschuer · 2020-03-20T19:56:20Z

It we agree to the qss and xml fromat, I will

In Deere for example most buttons have a hover effect and for hotcue buttons this is 'stuck' as soon as the cue popup is opened.
what about emulating a leaveEvent to force [hover="false"] & repaint when the popup is opened?

I cannot confirm this with Ubuntu Xenial. On which OS did you experience this.

daschuer · 2020-03-20T20:06:16Z

Is it a temporary issue? Can we live with it for now?

It is hard to fix this remote, I had a brief look without a Idea.

I can remember that we had a similar problem with the microphone pop up and the pressed state.
Unfortunately the fix did not work there.

src/widget/whotcuebutton.cpp

ronso0 · 2020-03-20T19:12:31Z

res/skins/Tango/style.qss

-#HotcueButton[backgroundIsDark=false][hasBackgroundColor=true] {
-    color: #0f0f0f;
+#HotcueButton[light="true"]:hover {
+    border: 1px solid #0f0f0f;


this blends with the bg and makes the button shrink.
also, the distinction between light & dark should be shifted: dark is applied to default orange already.

actually I think we could stick to the white border for all colors, except for white ;) where I'd use 50% grey for example

ronso0 · 2020-03-20T22:35:25Z

In Deere for example most buttons have a hover effect and for hotcue buttons this is 'stuck' as soon as the cue popup is opened.
what about emulating a leaveEvent to force [hover="false"] & repaint when the popup is opened?

I cannot confirm this with Ubuntu Xenial. On which OS did you experience this.

Ubuntu Studio 18.04, Qt 5.9

daschuer · 2020-03-21T17:00:35Z

How about this:

daschuer · 2020-03-21T17:09:51Z

Just notice, the pause button is also orange with a black sign. So I think we should not make orange dark.

daschuer · 2020-03-21T20:09:33Z

This could be a related bug:
https://bugreports.qt.io/browse/QTBUG-44400
However I cannot reproduce it with Qt 5.5.1

ronso0 · 2020-03-22T17:17:49Z

When I worked on the Mixxx menu styles someone posted a link to this very useful tool
WebAIM Contrast Checker

I'm not really happy with light/dark distinction for hover borders in Tango. I'd rather switch to changing the background color on hover the way this happens in Deere. Though that effect is not from our qss, it's probably the default QPushbutton behaviour: (gif is entirely without Deere/style.qss)

I hope I can work out how to get this into Tango, will do so in separate PR

res/skins/LateNight/style.qss

daschuer · 2020-03-22T18:49:28Z

Done

Be-ing · 2020-03-23T02:19:48Z

I am highly doubtful we should this. If we do, we lock ourselves out of using right click as an alternate action for loop cues (#2194).

Be-ing · 2020-03-23T04:49:06Z

I'm not saying we definitely shouldn't do this. But let's not rush it before we have decided on all the UX details for #2194. I don't want to get into that discussion now; let's keep focusing on 2.3. Maybe if we figure out the design for #2194 before the final 2.3 release we can merge this PR during the 2.3 beta period.

Holzhaus · 2020-03-23T10:53:12Z

I am highly doubtful we should this. If we do, we lock ourselves out of using right click as an alternate action for loop cues (#2194).

Right click is the standard way to open context menus. I accidently deleted cues due to this when I started to use mixxx and was looking for a way to set cue labels. Secondary action could be triggered with middle click. Just sayin'.

The current master branch has issues with this, e.g. it's not possible to edit cues that are too close next to each other. Additionally, cues at position 0 also aren't editable by right click for some reason. So we need some kind of fix for 2.3 anyway.

We could move the right click menu change to another PR that can be merged separately.

ronso0 · 2020-03-23T11:32:09Z

Right click is the standard way to open context menus. I accidently deleted cues due to this when I started to use mixxx and was looking for a way to set cue labels. Secondary action could be triggered with middle click. Just sayin'.

Yup. At first I was skeptical about this, too, because I was used to right-click for clearing a hotcue.
But now I notice my intuitive behaviour was shifted somehow after right-click was introduced to overview markers and I often right-clicked a hotcue button...and whoops, no menu but cleared hotcue :/
That said, I think this is the right way.

Re: middle-click
We shouldn't consider middle-click for anything. It's available on mice and some touchpads only (sometimes it's as akward as pressing the mouse wheel), but not touchscreens, let alone finding an equivalent on controllers where it could at most be emulated by another level modifier like Shift.

Holzhaus · 2020-03-23T12:52:34Z

let alone finding an equivalent on controllers where it could at most be emulated by another level modifier like Shift.

Since we don't need to map right click on controllers if that just opens the cue menu (which is not usable on controllers anyway), we can just map it like this:

Hotcue Action	Mouse	Controller	Touchscreen
Activate	Left click	Pad press	Short press
Activate Secondary	Middle click	SHIFT + Pad press
Cue Menu	Right Click		Long press

No idea how to map secondary action touch screens, but if you use limited hardware you get limited functionality I guess. Mapping the secondary action to long press makes no sense because your timing would be way off.

I think the cue menu is more useful for touch screens. That way you can take your tablet, get on a train and start preparing your set, color and label cues, etc. while traveling.

Swiftb0y · 2020-03-23T13:00:42Z

Have we considered setting loop cues similar to how its done in rekordbox / CDJs? You can set loop cues by creating a cue inside a loop (the loop cue then also has the length of that loop) and regular cues are set outside of a loop.

Holzhaus · 2020-03-23T13:16:14Z

Have we considered setting loop cues similar to how its done in rekordbox / CDJs? You can set loop cues by creating a cue inside a loop (the loop cue then also has the length of that loop) and regular cues are set outside of a loop.

Yes, that's what we do as well. This about secondary actions, e.g. when you have a loop cue, then primary would activate the loop and secondary would activate and jump to it fo example.

daschuer · 2020-03-23T15:29:49Z

This PR is mendarory for 2.3, be because it fixes some bugs and changes the Skin interface.

I am open to comment out the context menu. This is only a single line of code.

However I am confident that this is correct to continue with the context menu.
I had the same experiences like @ronso0.

The loop cue controls can also be placed in the context menu as a workaround.

IMHO this is mature and can be merged now.
@Be-ing what do you think?

Holzhaus

Code and manual test LGTM, thank you!

Be-ing · 2020-03-23T18:02:21Z

In general I agree with @ronso0 that we should be reluctant to use middle click. But I tested this and I can't help but agree that it's just so intuitive to show the menu on right click. I think it's acceptable to use middle click as the alternate action for loop cues. It breaks our usual design pattern of right click on buttons matches shift + button press on controllers, but that's not a strict rule anyway.

We shouldn't consider middle-click for anything. It's available on mice and some touchpads only (sometimes it's as akward as pressing the mouse wheel), but not touchscreens

Middle click is three finger click with touchpads. Mice that don't have a scroll wheel or third button can emulate middle click by simultaneously pressing both buttons. Right click doesn't exist either on touchscreens, so I don't think that should block anything here.

Be-ing · 2020-03-23T18:03:13Z

One small detail I'd like to see addressed before this is merged: clicking on the hotcue button while the right click menu is open does nothing. I expect it to close the menu.

daschuer · 2020-03-23T21:30:47Z

@Be-ing: Here the menu closes if you click outside the menu. It does not matter if there is a hotcue button or anything else. When does it close in your setup?
Does it close if you click outside the menu if you have enabled it via the WOverview?
Can you confirm the sticky hoover state?

Be-ing · 2020-03-23T22:12:04Z

The menu closes if I click another widget. But if I click the hotcue button it doesn't close.

daschuer · 2020-03-23T22:28:08Z

Does it also not close if you click another hotcue button? Can you confirm the sticky hoover state?
Do you have an idea what might happen?

Be-ing · 2020-03-23T23:21:21Z

Sorry, I was confused. There isn't actually a bug. I right clicked then got confused when left clicking immediately didn't close the menu, but my cursor was slightly inside the menu area. So, looks good to me. 👍

@ronso0 do you think this is ready to merge? If you want changes to the hover behavior, I suggest we merge this now and you can work on that in a new PR.

ronso0 · 2020-03-24T10:20:02Z

Exactly. LGTM

Holzhaus · 2020-03-26T17:28:40Z

I get a skin error with LateNight now: https://github.com/mixxxdj/mixxx/projects/2#card-35230536

ronso0 · 2020-03-26T17:42:11Z

can't reproduce with master.
does it happen with other skins as well?

Holzhaus · 2020-03-26T17:58:13Z

Strange, now I cannot reproduce it anymore.

ronso0 · 2020-04-02T00:10:21Z

In Deere for example most buttons have a hover effect

Re: documentation
It's worth mentioning that <Hover>true</Hover> in HotcueButton node automatically picks a dim version of the hotcue color as hover background color. Took me a while to figure that out, thought this was due to some general WPushButton rule in Deere...

daschuer added 4 commits March 17, 2020 23:51

Added WHotcueButton featuring a right click context menu

d13f3ab

Adjust tooltips for the right click contex menu.

25fc643

Improved Hotcue button for Shade and autoconnect the controls.

11ba850

Stream line HotcueButton styling

4ad16d7

Holzhaus added skins ui labels Mar 17, 2020

Holzhaus added this to In progress in 2.3 release via automation Mar 17, 2020

Holzhaus added this to the 2.3.0 milestone Mar 17, 2020

Holzhaus requested a review from ronso0 March 17, 2020 23:37

Get rid of -1 color

747de83

Holzhaus requested changes Mar 18, 2020

View reviewed changes

2.3 release automation moved this from In progress to In Review Mar 18, 2020

Introduce HotcueButton[light/dark] for qss

51c00cd

ronso0 requested changes Mar 20, 2020

View reviewed changes

res/skins/LateNight/style.qss Outdated Show resolved Hide resolved

daschuer added 2 commits March 20, 2020 20:37

Restored lost background color in LateNight

8172241

Don't set hotcue_nn_color control to default if not required. This av…

1a1bf0a

…oids some unnecessary code cycles.

Holzhaus reviewed Mar 20, 2020

View reviewed changes

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

Verify incomming colors in a valueChangeRequest

90c1e54

ronso0 reviewed Mar 20, 2020

View reviewed changes

mixxxdj deleted a comment from Holzhaus Mar 21, 2020

ronso0 reviewed Mar 22, 2020

View reviewed changes

res/skins/LateNight/style.qss Show resolved Hide resolved

Restore lost #SpecialCueButton[value="1"] style

ddc96b9

Holzhaus approved these changes Mar 23, 2020

View reviewed changes

Holzhaus merged commit 041222c into mixxxdj:master Mar 24, 2020

2.3 release automation moved this from In Review to Done Mar 24, 2020

ronso0 mentioned this pull request Apr 2, 2020

Deere: suppress blue hotcue button border #2620

Merged

daschuer deleted the hotcue-rgb-colors branch September 26, 2021 17:37

	// Add one to hotcue so that we don't have a hotcue_0
	// Increment hotcue index by one so that we don't have a hotcue_0

Hotcue Buttons, Context Menu + Skin Improvements. #2560

Hotcue Buttons, Context Menu + Skin Improvements. #2560

Conversation

daschuer commented Mar 17, 2020

Holzhaus left a 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

Choose a reason for hiding this comment

daschuer commented Mar 18, 2020

Holzhaus commented Mar 20, 2020

daschuer commented Mar 20, 2020

daschuer commented Mar 20, 2020

Holzhaus commented Mar 20, 2020

ronso0 left a comment

Choose a reason for hiding this comment

daschuer commented Mar 20, 2020

daschuer commented Mar 20, 2020

daschuer commented Mar 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Mar 20, 2020 • edited

daschuer commented Mar 21, 2020

daschuer commented Mar 21, 2020

daschuer commented Mar 21, 2020

ronso0 commented Mar 22, 2020 • edited

daschuer commented Mar 22, 2020

Be-ing commented Mar 23, 2020

Be-ing commented Mar 23, 2020

Holzhaus commented Mar 23, 2020 • edited

ronso0 commented Mar 23, 2020

Holzhaus commented Mar 23, 2020 • edited

Swiftb0y commented Mar 23, 2020

Holzhaus commented Mar 23, 2020

daschuer commented Mar 23, 2020

Holzhaus left a comment • edited

Choose a reason for hiding this comment

Be-ing commented Mar 23, 2020 • edited

Be-ing commented Mar 23, 2020

daschuer commented Mar 23, 2020

Be-ing commented Mar 23, 2020

daschuer commented Mar 23, 2020

Be-ing commented Mar 23, 2020

ronso0 commented Mar 24, 2020

Holzhaus commented Mar 26, 2020

ronso0 commented Mar 26, 2020

Holzhaus commented Mar 26, 2020

ronso0 commented Apr 2, 2020

ronso0 commented Mar 20, 2020 •

edited

ronso0 commented Mar 22, 2020 •

edited

Holzhaus commented Mar 23, 2020 •

edited

Holzhaus commented Mar 23, 2020 •

edited

Holzhaus left a comment •

edited

Be-ing commented Mar 23, 2020 •

edited