src/widget: Replace CueMenu implementation with a custom popup

[Holzhaus]

As discussed on Zulip, here's my initial implementation of a more straightforward cue menu. The last time I did some GUI programming was a few years ago (and I was using GTK+), so I'd appreciate any hints how to get rid of the padding/margin around the color picker. This is also missing an icon for removing cues and is not styled yet.

EDIT: This PR introduces some new styleable widgets:

WCueMenuPopup
#CueNumberLabel
#CuePositionLabel
#CueLabelEdit
#CueColorPicker
#CueDeleteButton

[ronso0]

I'd appreciate any hints how to get rid of the padding/margin around the color picker.

According to https://doc.qt.io/qt-5/qgridlayout.html you could try to use setVerticalSpacing(N), setHorizontalSpacing(M) or simply setSpacing(N) to override the default spacing (or values inherited from parent layout respectively) and have equal margins in between the colors.
Maybe that eleminates the sourrounding spaces as well and pLayout->setContentsMargins(0,0,0,0); becomes effective.

[ronso0]

When I mentioned the cue label lineedit I considered Apply,Clear and Revert / Cancel buttons. Icons would be checkmark, Backspace and Rewind icon.

Do we need those buttons?
When are label string changes applied? (didn't build yet)
Instantly? When the lineedit loses focus? Or when the menu is closed? which would be the same when there's virtually no free space to click in the menu.
With an Apply button, pressing Enter on real or on-screen keyboards would be obsolete.

[Holzhaus]

I'm using the textEdited signal, which fires as soon as the text is changed. No need to press enter. The widget behaves like a regular QMenu pop-up. You can close it by clicking anywhere outside of it, by pressing ESC, etc.

[ronso0]

while this is probably no big deal with a hotcue label, there's no way to revert the changes if I screwed up the label text? the current dialog has a cancel button btw.

[Holzhaus]

You can still use Ctrl-Z before the pop-up window is closed. And usually you don't really type long texts into the label field, just a few words like "before drop" or something like that. Apply/Cancel buttons are not really necessary for this use case IMHO.

[Holzhaus]

@ronso0

According to https://doc.qt.io/qt-5/qgridlayout.html you could try to use setVerticalSpacing(N), setHorizontalSpacing(M) or simply setSpacing(N) to override the default spacing (or values inherited from parent layout respectively) and have equal margins in between the colors.
Maybe that eleminates the sourrounding spaces as well and pLayout->setContentsMargins(0,0,0,0); becomes effective.

Thanks. It turns that this was caused by setting the QPushButtons to a fixed size 🤦‍♂️

This is how it looks now:

[Holzhaus]

Due to the long button label the "Remove" button is too large. It should use an icon instead IMHO. What is the policy regarding icons? I thought about using this one: https://github.com/feathericons/feather/blob/master/icons/trash-2.svg

But this might look a bit out of place depending on the skin. Can we make the button icon styleable by skins/qss?

[Holzhaus]

Ok, just used the icon from res/images/library/ic_library_cross.svg. I googled at bit and found that the icon should be overwritable via QSS. This is the current state:

@ferranpujolcamins What do you think? This isn't the cue over you outlined in your GSoC proposal but it should improve the workflow for the time being.
@ronso0 Since a lot of your PRs are skin improvements: Do I need to make any changes to make this properly skinnable via qss?

IMHO this is ready to merge now.

[ronso0]

Do I need to make any changes to make this properly skinnable via qss?

As long as this is a regular QMenu with QPushButtons it can be styled.
I didn't try it yet but I think it would help (it definitely doesn't do any harm) to grant the QLineEdit a distinct ObjectName so we can address it stylesheets with something like QLineEdit#CueLabelEdit, because there are other lineedits in the Library, as well as WSearchLineEdit.

Nice to have: each button 'knows' if its color is currently assigned to the hotcue in question so the button's 'pressed' state can be styled, too.

[ronso0]

Also, I somehow fear the clear button could be mistaken as Clear button for the lineedit. That can be resolved by some style that clearly separates it from the lieedit, or simply with a different icon, something like the rubbish bin icon you suggested.

[ronso0]

Do I need to make any changes to make this properly skinnable via qss?

Almost forgot: qss icons wouldn't override c++ icons, but be drawn over it (or underneath), so please also name the Remove button (HotcueRemoveButton?) and add a style snippet to all skins, much like for the searchbox' Clear button: https://github.com/mixxxdj/mixxx/blob/master/res/skins/LateNight/style.qss#L1970 (I removed the c++ style to properly paint the focused state)

[ronso0]

@Holzhaus What could be the reason I only get grey buttons?

btw the button with the previously seleted cue color has some kind of outline so I guess that can be styled with qss.

[daschuer]

The remove button is quite ambiguous.
Since the dialog edits cue properties it is not clear that the x clears the whole cue.

How about adding a status bar on top of the dialog with the hot cue number and current color and time if you like. There is likely enough space for a labeled "clear hot cue" button than.

[Holzhaus]

@daschuer I was just deleting duplicate comments (bad internet connection?).

[Holzhaus]

I think the "Delete Cue" button needs to be shifted up a bit.

Can you elaborate? Do you mean pushing everything left of it down? Or reduce margins/padding so that it's closer to the top window border?

[daschuer]

I think the "Delete Cue" button needs to be shifted up a bit.

Can you elaborate? Do you mean pushing everything left of it down? Or reduce margins/padding so that it's closer to the top window border?

I don't really mind. My initial idea was pushing the delete button up a bit and keep the overall window size.

[daschuer]

I'm a bit torn apart: I agree the missing OK button makes the ...

Ok I agree that It can remain like this. It is unusual on the first sight, but you can quickly get used to it.

[Holzhaus]

OK, I made the requested changes. Please retest.

@ronso0 Please verify that you can overwrite the styles from default.qss in skins, too. It works for me.

[ronso0]

I use mm:ss for the time displays but the cue menu shows mm:ss:zz

Edit not related to tis PR: there seems to be no rounding of the time values in the overview, just cropping the decimals

[ronso0]

Please verify that you can overwrite the styles from default.qss in skins, too. It works for me.

👍 Yup that works.

[daschuer]

Edit not related to tis PR: there seems to be no rounding of the time values in the overview, just cropping the decimals

Clock values are IMHO not rounded. For example the minute pointer jumps to the next minute if the first one is over, independent of the presence of a seconds pointer.

[daschuer]

AppVeyor has timed out. So we can merge. Thank you to all involved here. :-)

[Holzhaus]

Thanks @daschuer and @ronso0!

While looking for a location to put the default.qss loading logic, I stumbled over the LegacySkinParser::getLibraryStyle() method. Should we try to move some style declarations from there to default.qss, too?

[ronso0]

Yeah, that won't hurt.
I guess you noticed it's just legacyskinparser.cpp#L1410-L1429, the color stuff below is read from nodes within the Library element of each skin.

[Be-ing]

Now we have no way for users to pick an arbitrary color outside of the default palette in #2345.

[Holzhaus]

@Be-ing:

Now we have no way for users to pick an arbitrary color outside of the default palette in #2345.

We didn't have a way to do that before either. And #2345 will have to makes a few changes to the cue menu popup code anyway. IMHO we could add a new button with a "..." icon that opens a QColorDialog in #2345 or after #2345 has been merged.

[ronso0]

@Holzhaus Could you please add the stylable widgets to PR description?

WCueMenuPopup
#CueNumberLabel
#CuePositionLabel
#CueLabelEdit
#CueColorPicker
#CueDeleteButton
It would be helpful to mention those selectors in the PR description so everyone can easily find them afterwards without having to trawl through the PR discussion and the file changes.
Later on (after releasing 2.3 I guess, when there's less pressure) we should also add this to the skin documentation https://www.mixxx.org/wiki/doku.php/creating_skins

[Holzhaus]

@ronso0 OK, done

[ronso0]

thx!

[ronso0]

I think we need to tweak the button styles: color doesn't work like WYSIWYG for me, color buttons appear pale compared to the actual hotcue button colors.