-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add type toggle to cue popup #13215
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment. The rest looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, much appreciated!
I've lef some comments.
Please also add some styling for Deere. |
When I just toggle a loop cue's type accidentally, the loop size changes, right? Seems like sub optimal UX. |
Done
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I also have a commit (almost) ready to fix the 'active' icon. For some reason edit: yeah, I'll share that as soon as I find time next days. |
This is the button style commit |
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. mixxx/src/engine/controls/cuecontrol.cpp Lines 936 to 950 in 4164341
|
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. |
This is taken care of.
|
Yes, that was fixed in the follow up PR (the saved jump). Going to fix that one instead.
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM nox, thank you!
@daschuer wanna take a look? |
Looks like @daschuer already approved - merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/widget/wcuemenupopup.cpp
Outdated
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")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works good now. Thank you.
@ronso0 friendly ping, in case you missed the notification! |
Ouh sorry, thanks for the reminder. LGTM, thank you very much! |
WCueMenuPopup #CueDeleteButton:pressed { | ||
background-color: #b24c12; | ||
outline: none; | ||
image: url(skins:LateNight/palemoon/buttons/btn__delete_active.svg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Randomly stumbled upon this while building for iOS, this file seems to be missing, which results in a bunch of warnings of the form
qt.svg: warning Main Cannot open file '/private/var/containers/Bundle/Application/D577115B-62DB-4FFE-B072-4133259002FE/Mixxx.app/skins/LateNight/palemoon/buttons/btn__delete_active.svg', because: No such file or directory
(I know this PR didn't introduce this styling, but this is where the code was last touched)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can also see this warning on Linux. Happy to create an issue? Should be an easy one to address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: #13492
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