feat: add type toggle to cue popup #13215

acolombier · 2024-05-06T13:59:34Z

This adds the ability to changes the cue type without having to reset it. It also allow updating the saved loop end using the beatloop size.

I would like to also allow right-click on the button to set the end loop to the current play position to allow more granular control, and prepare the field for saved beatjump. Any component I can use or should I make a custom QPushButton?

Kooha-2024-05-06-14-54-24.mp4

This is a dependency for #13161

daschuer

Just a minor comment. The rest looks good.

src/widget/wcuemenupopup.h

ronso0

Thanks for working on this, much appreciated!

I've lef some comments.

src/widget/wcolorpicker.cpp

src/widget/wcuemenupopup.cpp

ronso0 · 2024-05-07T10:03:35Z

Please also add some styling for Deere.

Swiftb0y · 2024-05-07T12:31:09Z

When I just toggle a loop cue's type accidentally, the loop size changes, right? Seems like sub optimal UX.

acolombier · 2024-05-07T16:41:38Z

Please also add some styling for Deere.

Done

When I just toggle a loop cue's type accidentally, the loop size changes, right? Seems like sub optimal UX.

Done

daschuer · 2024-05-08T21:51:10Z

@ronso0 and @Swiftb0y, It this ready to merge now?

Swiftb0y

I don't have the time to test this myself right now. Can you double check that the loopsize still doesn't change when toggling the type and the loop is not aligned to a beat. For example when a user sets up a loop with the in-out buttons (while having quantize disabled) and then assigns a hotcue to that loop.

ronso0

Some remarks.

Shall we also change the cue color when changing the type?
if (type == loop && color == defaultLoopCueColor) { cue->setColor(defaultHotcueColor }
and vice versa for hotcue -> loopcue.

src/widget/wcuemenupopup.cpp

ronso0 · 2024-05-11T00:47:50Z

I also have a commit (almost) ready to fix the 'active' icon. For some reason qproperty-icon: url(..); does not style the checked state, but image: .. works (though doesn't allow size adjustments like with the qproperty).

edit: yeah, qproperty-icon is evauated only once, so it can't be used for different state icons.
https://doc.qt.io/qt-6/stylesheet-syntax.html#setting-qobject-properties

I'll share that as soon as I find time next days.

src/widget/wcuemenupopup.cpp

ronso0 · 2024-05-11T11:26:32Z

This is the button style commit
2af95df

acolombier · 2024-05-11T18:22:54Z

Shall we also change the cue color when changing the type?

Good question. I would suggest we don't, but I guess we could change it if the current color matches the current cue type as you suggested, before change? What do you think?

ronso0 · 2024-05-11T19:59:39Z

Shall we also change the cue color when changing the type?

Good question. I would suggest we don't, but I guess we could change it if the current color matches the current cue type as you suggested, before change? What do you think?

Yup, that's what I meant.
Related code we can adopt is here

mixxx/src/engine/controls/cuecontrol.cpp

Lines 936 to 950 in 4164341

    
           if (cueType == mixxx::CueType::Loop) { 
        
               ConfigKey autoLoopColorsKey("[Controls]", "auto_loop_colors"); 
        
               if (getConfig()->getValue(autoLoopColorsKey, false)) { 
        
                   color = m_colorPaletteSettings.getHotcueColorPalette().colorForHotcueIndex(hotcueIndex); 
        
               } else { 
        
                   color = colorFromConfig(ConfigKey("[Controls]", "LoopDefaultColorIndex")); 
        
               } 
        
           } else { 
        
               ConfigKey autoHotcueColorsKey("[Controls]", "auto_hotcue_colors"); 
        
               if (getConfig()->getValue(autoHotcueColorsKey, false)) { 
        
                   color = m_colorPaletteSettings.getHotcueColorPalette().colorForHotcueIndex(hotcueIndex); 
        
               } else { 
        
                   color = colorFromConfig(ConfigKey("[Controls]", "HotcueDefaultColorIndex")); 
        
               } 
        
           }

src/widget/wcuemenupopup.h

ronso0 · 2024-05-11T20:06:29Z

Can we remove the obsolete cue end position after switching from loop -> hotcue when closing the menu?

It just feel weird to be left with hotcues that have an end position.

ronso0 · 2024-05-11T20:08:50Z

Can you double check that the loopsize still doesn't change when toggling the type and the loop is not aligned to a beat. For example when a user sets up a loop with the in-out buttons (while having quantize disabled) and then assigns a hotcue to that loop.

This is taken care of.
The only way to screw up a loop is

have a loop cue
left-click loop cue button
Whoopsy!
right-click loop cue button

src/widget/wcuemenupopup.cpp

acolombier · 2024-05-12T13:45:04Z

Can we remove the obsolete cue end position after switching from loop -> hotcue when closing the menu?

Yes, that was fixed in the follow up PR (the saved jump). Going to fix that one instead.

Can you double check that the loopsize still doesn't change when toggling the type and the loop is not aligned to a beat. For example when a user sets up a loop with the in-out buttons (while having quantize disabled) and then assigns a hotcue to that loop.

This is taken care of. The only way to screw up a loop is
* have a loop cue

* **left**-click loop cue button

* _Whoopsy!_

* **right**-click loop cue button

This would require to implement an history of cue modification and I'm not going to do this here. If this a deal breaker, happy to close the PR.

ronso0 · 2024-05-12T15:29:26Z

This would require to implement an history of cue modification and I'm not going to do this here. If this a deal breaker, happy to close the PR.

I wanted to point out that this is unlikely to happen by accident, and definitely not blocking this.

acolombier · 2024-05-12T16:10:12Z

Ah, I see. I thought you meant there was a missing edge case - I think we slightly lost in translation :)

I think all comments should have been addressed now.

ronso0

LGTM nox, thank you!

ronso0 · 2024-05-12T23:09:29Z

@daschuer wanna take a look?

acolombier · 2024-05-13T00:56:10Z

Looks like @daschuer already approved - merge?

daschuer

During testing I had the situation that the cue type toggle button was lit, but the cue button in the skin did not change the state to loop. Unfortunately I was not able to reproduce the issue.

At least we should make sure the toogle button state is corrected when changing the cue type fails for some reason.

daschuer · 2024-05-13T04:33:12Z

src/widget/wcuemenupopup.cpp

+            tr("Toggle this cue type between normal cue and saved loop, using "
+               "the current beatloop size or the current play position") +
+            "\n\n" +
+            tr("Left-click: Toggle between normal cue and saved loop, using"
+               "the current beatloop size as the loop size") +
+            "\n" +
+            tr("Right-click: If the current play position is after the cue,"
+               "set that position as loop end and make the cue a saved loop "
+               "if not"));


Suggested change

tr("Toggle this cue type between normal cue and saved loop, using "

"the current beatloop size or the current play position") +

"\n\n" +

tr("Left-click: Toggle between normal cue and saved loop, using"

"the current beatloop size as the loop size") +

"\n" +

tr("Right-click: If the current play position is after the cue,"

"set that position as loop end and make the cue a saved loop "

"if not"));

tr("Toggle this cue type between normal cue and saved loop")

"\n\n" +

tr("Left-click: Use the old size or the current beatloop size as the loop size") +

"\n" +

tr("Right-click: Use the current play position as loop end if it is after the cue);

I am not sure if keeping the old loop size is the intended behaviour, because it is invisible, when the cue is an normal cue. We loose the "feature" to adjust the beatloops size of a saved loop by this.

This was requested by @Swiftb0y
Since you can change the loop size with right-click, I guess it is okay.
Perhaps I could also make double-click to change the loop size with the beatloops size value?

Perhaps I could also make double-click

I suggest to keep it simple.
The purpose of this button is to quickly change the type, with two loop options.
Adjusting saved loops should better be done with the looping controls IMO.

Adjusting the right-click behaviour might work:

if before cue: reject

if at cue: use beatloop size NEW

if after cue: use play pos

That should be the behaviour. Also with your commit, the end pos is now removed after popup close so the left button may use the beatloop size again on next attempt

I suggest to keep it simple.

I also think thata important. if the functionality for retaining the previous size results in code that is too complicated, we should ensure the code remains understandable. In the worst case, the user accidentally clicks the wrong button (that is not easy to access in the first place) and they learn that it changes the length the hard way.

I'm sorry for being so indecisive on this.

No problem. Could you please have a look with the current solution? It will allow to quickly revert the saved loop, and will "clean" the dangling end position when the popup is closed, thanks to @ronso0's work. I think the code remain quite simple, so hopefully we have a good trade-off between usability and code simplicity.

src/widget/wcuemenupopup.h

ronso0 · 2024-05-13T09:27:22Z

Can we remove the obsolete cue end position after switching from loop -> hotcue when closing the menu?

It just feel weird to be left with hotcues that have an end position.

49c052d

daschuer

Works good now. Thank you.

acolombier · 2024-05-23T09:33:43Z

@ronso0 friendly ping, in case you missed the notification!

ronso0 · 2024-05-23T13:55:48Z

Ouh sorry, thanks for the reminder.

LGTM, thank you very much!

feat: add type toggle to cue popup

a9e6998

github-actions bot added skins ui labels May 6, 2024

daschuer requested changes May 6, 2024

View reviewed changes

src/widget/wcuemenupopup.h Show resolved Hide resolved

acolombier mentioned this pull request May 6, 2024

Saved beat jumps #13216

Draft

Use PollingControlProxy and custom action on right click

d6479b6

acolombier marked this pull request as ready for review May 6, 2024 21:15

acolombier requested a review from daschuer May 6, 2024 21:37

ronso0 reviewed May 7, 2024

View reviewed changes

src/widget/wcolorpicker.cpp Show resolved Hide resolved

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

Right click changes the cue type and the loop size is persisted

3f9157e

github-actions bot added the waveform label May 7, 2024

Add Deere styling

09d6433

Merge remote-tracking branch 'upstream/main' into feat/cuepopup

cc9555b

daschuer approved these changes May 8, 2024

View reviewed changes

Swiftb0y reviewed May 9, 2024

View reviewed changes

ronso0 requested changes May 11, 2024

View reviewed changes

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

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

ronso0 requested changes May 11, 2024

View reviewed changes

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

Antoine C and others added 2 commits May 11, 2024 19:32

Do not change cue type if position is invalid

49d1bc1

Cue popup: refine button styles

f4d5219

ronso0 reviewed May 11, 2024

View reviewed changes

src/widget/wcuemenupopup.h Outdated Show resolved Hide resolved

ronso0 requested changes May 11, 2024

View reviewed changes

src/widget/wcuemenupopup.cpp Show resolved Hide resolved

Add missing null check and comments

e685999

Rename popup button slots

7a43177

ronso0 self-requested a review May 12, 2024 23:08

ronso0 approved these changes May 12, 2024

View reviewed changes

daschuer requested changes May 13, 2024

View reviewed changes

ronso0 requested changes May 13, 2024

View reviewed changes

src/widget/wcuemenupopup.h Outdated Show resolved Hide resolved

Antoine C and others added 2 commits May 13, 2024 12:16

Use unique_ptr and fix position issue

a942386

Cue menu: clear end position if obsolete

e7dcd1c

acolombier requested review from ronso0 and daschuer May 13, 2024 21:06

daschuer approved these changes May 13, 2024

View reviewed changes

ronso0 approved these changes May 23, 2024

View reviewed changes

ronso0 merged commit ebd51e1 into mixxxdj:main May 23, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add type toggle to cue popup #13215

feat: add type toggle to cue popup #13215

acolombier commented May 6, 2024

daschuer left a comment

ronso0 left a comment

ronso0 commented May 7, 2024

Swiftb0y commented May 7, 2024 •

edited

acolombier commented May 7, 2024

daschuer commented May 8, 2024

Swiftb0y left a comment •

edited

ronso0 left a comment

ronso0 commented May 11, 2024 •

edited

ronso0 commented May 11, 2024

acolombier commented May 11, 2024 •

edited

ronso0 commented May 11, 2024

ronso0 commented May 11, 2024

ronso0 commented May 11, 2024

acolombier commented May 12, 2024

ronso0 commented May 12, 2024

acolombier commented May 12, 2024

ronso0 left a comment

ronso0 commented May 12, 2024

acolombier commented May 13, 2024

daschuer left a comment

daschuer May 13, 2024

daschuer May 13, 2024

acolombier May 13, 2024

ronso0 May 13, 2024

acolombier May 13, 2024

Swiftb0y May 13, 2024 •

edited

acolombier May 13, 2024

ronso0 commented May 13, 2024

daschuer left a comment

acolombier commented May 23, 2024

ronso0 commented May 23, 2024

feat: add type toggle to cue popup #13215

feat: add type toggle to cue popup #13215

Conversation

acolombier commented May 6, 2024

daschuer left a comment

Choose a reason for hiding this comment

ronso0 left a comment

Choose a reason for hiding this comment

ronso0 commented May 7, 2024

Swiftb0y commented May 7, 2024 • edited

acolombier commented May 7, 2024

daschuer commented May 8, 2024

Swiftb0y left a comment • edited

Choose a reason for hiding this comment

ronso0 left a comment

Choose a reason for hiding this comment

ronso0 commented May 11, 2024 • edited

ronso0 commented May 11, 2024

acolombier commented May 11, 2024 • edited

ronso0 commented May 11, 2024

ronso0 commented May 11, 2024

ronso0 commented May 11, 2024

acolombier commented May 12, 2024

ronso0 commented May 12, 2024

acolombier commented May 12, 2024

ronso0 left a comment

Choose a reason for hiding this comment

ronso0 commented May 12, 2024

acolombier commented May 13, 2024

daschuer left a comment

Choose a reason for hiding this comment

daschuer May 13, 2024

Choose a reason for hiding this comment

daschuer May 13, 2024

Choose a reason for hiding this comment

acolombier May 13, 2024

Choose a reason for hiding this comment

ronso0 May 13, 2024

Choose a reason for hiding this comment

acolombier May 13, 2024

Choose a reason for hiding this comment

Swiftb0y May 13, 2024 • edited

Choose a reason for hiding this comment

acolombier May 13, 2024

Choose a reason for hiding this comment

ronso0 commented May 13, 2024

daschuer left a comment

Choose a reason for hiding this comment

acolombier commented May 23, 2024

ronso0 commented May 23, 2024

Swiftb0y commented May 7, 2024 •

edited

Swiftb0y left a comment •

edited

ronso0 commented May 11, 2024 •

edited

acolombier commented May 11, 2024 •

edited

Swiftb0y May 13, 2024 •

edited