Skip to content
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

[PreviewDeckN],LoadSelectedTrackAndPlay toggles play/pause if the track is already loaded #12920

Merged
merged 5 commits into from
Mar 19, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Mar 4, 2024

Fixes #9819

Due to limited unused hotkeys (in the default mapping) and to allow controlling the preview deck with a few hotkeys only I propose to extend/override the behaviour of [PreviewDeckN], LoadSelectedTrackAndPlay:
if the track is already loaded, toggle play/pause.

This is mapped to P
Shift+P does start_stop (stop and seek to start) (could as well be cue_gotoandstop?)

TODO

@github-actions github-actions bot added the ui label Mar 4, 2024
@daschuer
Copy link
Member

daschuer commented Mar 4, 2024

Good idea.

} else if (isPreviewDeckGroup(group) && play) {
// This extends/overrides the behaviour of [PreviewDeckN],LoadSelectedTrackAndPlay:
// if the track is already loaded, toggle play/pause.
auto pPlayer = getPlayer(group);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have pPlayer and the null check already above.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

Comment on lines 707 to 710
bool isPlaying =
ControlObject::get(ConfigKey(group, QStringLiteral("play"))) > 0.0;
ControlObject::set(ConfigKey(group, QStringLiteral("play")),
isPlaying ? 0.0 : 1.0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid to look up the control object twice.

@ronso0
Copy link
Member Author

ronso0 commented Mar 4, 2024

The corresponding manual PR is mixxxdj/manual#625

@ronso0 ronso0 marked this pull request as draft March 5, 2024 01:14
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor.
Why is this still in n draft?

if (pTrack == pPlayer->getLoadedTrack()) {
auto* pPlay =
ControlObject::getControl(ConfigKey(group, QStringLiteral("play")));
double newPlay = pPlay->get() > 0.0 ? 0.0 : 1.0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
double newPlay = pPlay->get() > 0.0 ? 0.0 : 1.0;
double newPlay = pPlay->toBool() ? 0.0 : 1.0;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@ronso0
Copy link
Member Author

ronso0 commented Mar 5, 2024

Why is this still in n draft?

I noticed a glitch in the keyboard sheet preview ( ✔️ seemst to be an issue with the GH preview) and I didn't check if this affects the Preview delegate ( ✔️ ).

So this is ready to roll.
Thanks for your prompt review!

@ronso0
Copy link
Member Author

ronso0 commented Mar 5, 2024

I noticed an incosistency:

  • the preview Play button right-click is mapped to start (and stops, so this is the same as start_stop?)
  • the tooltip says cue_gotoandplay (LateNight only)
  • Shift+P is mapped to start_stop
  • the Preview delegate always does cue_gotoandplay and play (pause)

I think we should map the Play button right-click and Shift+P to cue_gotoandplay, too.
If the user wants to go back to the the start this can be done by dragging the playhead on the overview, while otoh jumping to Cue requires to click the C icon (which is rather small in the preview deck).
For unprepared tracks Cue = start, and for prepared tracks we may assume Cue has been set to a different position on purpose.

Not going to fix this here, let's discuss it in #12925

@ronso0 ronso0 marked this pull request as ready for review March 5, 2024 09:23
@ronso0
Copy link
Member Author

ronso0 commented Mar 9, 2024

FWIW, with 2.4 I managed to get a mismatch of the preview deck's play button (not lit) and Preview delegate (lit). I continued clicking the delegate so I can't tell whether the deck was actually playing (shouldn't since the Play button CO is reliable IMO).

@ronso0
Copy link
Member Author

ronso0 commented Mar 19, 2024

The manual PR has been merged already, let's merge this, too.
Any objections?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you.

@daschuer daschuer merged commit 9cc57d9 into mixxxdj:main Mar 19, 2024
13 checks passed
@ronso0 ronso0 deleted the preview-load-play-pause branch March 19, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pressing [Enter] does not pause but stops the preview
2 participants