Option to keep deck playing on track load #10944

mbacarella · 2022-10-06T19:39:49Z

This is a proposed solution as discussed in issue #10548.

This is my first proposed contribution to this project and first to an open source C++ project. I apologize if I missed something important or did something dumb. Thanks for your consideration :)

ronso0 · 2022-10-06T21:14:53Z

Thank you for working on this! And congratulations for your first contribution : )

Since this is your first contribution we need you to sign the contributor agreement and thereby allows Mixxx to include your changes.
Please comment here when you filled the form and let us know which name should appear in the contributor list.

I apologize if I missed something important

Please install pre-commit as described in CONTRIBUTING.md#git-workflow (I think wiki/Using-Git#set-up-automatic-code-checking is more comprehensive)

ronso0 · 2022-10-06T21:16:59Z

Please install pre-commit as described in CONTRIBUTING.md#git-workflow (I think wiki/Using-Git#set-up-automatic-code-checking is more comprehensive)

As long as no one started to review this PR you may as well incorporate the pre-commit fixes and sqash everything into one commit and rebase onto mixxx/main.

mbacarella · 2022-10-06T21:35:20Z

Please comment here when you filled the form and let us know which name should appear in the contributor list.

I filled out the form. Michael Bacarella <[email protected]> is fine.

mbacarella · 2022-10-06T21:52:49Z

As long as no one started to review this PR you may as well incorporate the pre-commit fixes and sqash everything into one commit and rebase onto mixxx/main.

I installed the pre-commit hooks, squashed, rebased and pushed. It complained about qmlformat but I couldn't figure out how to install it, so I skipped. I assume the server side may run it and let me know if there are any problems.

mbacarella · 2022-10-06T21:58:09Z

I apologize for missing the instructions on installing Git pre-commit hooks the first go-round.

mbacarella · 2022-10-06T22:19:51Z

After the rebase, I noticed and fixed two more issues as separate commits and pushed. I thought the clang-format pre-commit hook would tell me to fix things but it passed for me locally without complaint. Not sure what to expect.

ronso0 · 2022-10-06T22:38:42Z

src/preferences/dialog/dlgprefdeckdlg.ui

        </property>
        <property name="buddy">
-         <cstring>checkBoxDisallowLoadToPlayingDeck</cstring>
+         <cstring>comboBoxLoadWhenDeckPlaying</cstring>


I'm not sure why you changed this (changing translation texts requires new translation for every language).

Also, I thought the new option would a) load a track into a playing deck and b) play that right away.
Even though b) requires a), a) and b) are unrelated (I might want to load tracks into playing decks, but I don not want auto-play)

It's actually deleting the checkbox entirely and replacing it with a combobox that offers more functionality:

load track into playing deck and play that right away

load track into playing deck but stop

don't allow loading into playing deck

See the discussion at the end of this issue? #10548 (comment)

Of course, I should have taken a closer look, not just skimming through...

ronso0 · 2022-10-06T22:41:11Z

If did git rebase -i [mixxx repo]/main Catch up to main (resolve merge conflict)
1c79403 would not be necessary (you didn't touch legacyskinparser).

mbacarella · 2022-10-06T22:50:14Z

If did git rebase -i [mixxx repo]/main Catch up to main (resolve merge conflict) 1c79403 would not be necessary (you didn't touch legacyskinparser).

I can rebase this now, but I notice you have started a review.

ronso0 · 2022-10-07T08:07:44Z

Even though I explicitely just added a comment it is marked as "reviewed", this is irritating.
Feel free to rebase!

src/mixer/playermanager.cpp

src/mixer/playermanager.h

daschuer · 2022-10-07T12:34:24Z

src/preferences/dialog/dlgprefdeck.cpp

+    comboBoxLoadWhenDeckPlaying->addItem(tr("Reject"), static_cast<int>(LoadWhenDeckPlaying::Reject));
+    int loadWhenDeckPlaying;
+    if (m_pConfig->exists(kConfigKeyLoadWhenDeckPlaying)) {
+        loadWhenDeckPlaying = m_pConfig->getValueString(kConfigKeyLoadWhenDeckPlaying).toInt();


Do we need to use the default as well, if the value does not exists?
It would be grate to have a solution that works forward and backward with old an new versions.

Sorry, I don't follow. If it doesn't exist, won't it then get the default from the else-else?

Yes, right.
It is just the issue that "m_pConfig->exists" is a redundant lookup of the value. It is better to use getValue() with a fallback value like purposed above

Well yes but I'm trying to be mindful of not losing behavior previously configured by the user in a previous version. If the user disabled (or enabled) playing track protection I want to import that.

I realize this is confusing because, in a different part of the code, I didn't use getValue with default when I should have, but I think in this case it makes sense to probe for existence of the new config variable first and fall back to loading the older config variable.

src/preferences/dialog/dlgprefdeck.cpp

mbacarella · 2022-10-11T14:40:59Z

Did a bunch of the code review driven changes. Didn't rebase since a review actually started after @ronso0 said to rebase away :)

Anything else?

ronso0 · 2022-10-11T14:55:19Z

Just a hint: usually code conversations (conversations pinned to a specific spot in the code) are marked as resolved by the reviewer. That makes it easier to check if/how the discussed changes were applied.

mbacarella · 2022-10-11T14:57:34Z

Just a hint: usually code conversations (conversations pinned to a specific spot in the code) are marked as resolved by the reviewer. That makes it easier to check if/how the discussed changes were applied.

My bad! Will bear that in mind for next time :)

mbacarella · 2022-10-16T19:11:12Z

@ronso0 just to be clear I think this is ready to merge if there are no other blockers

(not sure what to do about the code style check; should I run clang-format on my code and push?)

ronso0 · 2022-10-16T20:08:11Z

I only che.cked the UI until now, @daschuer has the final word re code here.

Update pre-commit maybe? Idk why it didn't complain about the indentations locally.
You may also download the generated patch and apply it manually. Click on Details next to the pre-commit check, on the top left click Summary, scroll down to Artifacts and grab pre-commit.patch

daschuer

I have just tested this and it works good. Thank you.
Just one comment for improvement and the pre-commit patch and we can merge this nice addition.

daschuer · 2022-10-17T15:27:16Z

src/util/dnd.cpp

+    } else {
+        // support older version of this flag
+        allowLoadTrackIntoPlayingDeck =
+            pConfig->getValue<bool>(ConfigKey("[Controls]", "AllowTrackLoadToPlayingDeck"));


Can you also introduce a constant for this?

daschuer

LGTM, Thank you.

github-actions bot added the ui label Oct 6, 2022

Option to keep deck playing on track load (closes mixxxdj#10548)

87c5582

mbacarella force-pushed the keep-deck-playing-on-track-load branch from 7e77f11 to 87c5582 Compare October 6, 2022 21:49

mbacarella added 2 commits October 6, 2022 15:12

Catch up to main (resolve merge conflict)

1c79403

Tabstops: remove old item from tabstop, add comboBoxLoadWhenDeckPlaying

68ecf10

ronso0 reviewed Oct 6, 2022

View reviewed changes

daschuer reviewed Oct 7, 2022

View reviewed changes

src/mixer/playermanager.cpp Outdated Show resolved Hide resolved

daschuer reviewed Oct 7, 2022

View reviewed changes

src/mixer/playermanager.h Show resolved Hide resolved

daschuer reviewed Oct 7, 2022

View reviewed changes

src/preferences/dialog/dlgprefdeck.cpp Outdated Show resolved Hide resolved

ronso0 reviewed Oct 9, 2022

View reviewed changes

src/preferences/dialog/dlgprefdeck.cpp Outdated Show resolved Hide resolved

Incorporate code review feedback

fbff36e

daschuer reviewed Oct 17, 2022

View reviewed changes

mbacarella added 3 commits October 17, 2022 08:51

Make AllowTrackLoadToPlayingDeck a const ConfigKey

7b85652

Run git-clang-format

59287c8

Actually try using the pre-commit.patch this time

ed7001e

daschuer approved these changes Oct 19, 2022

View reviewed changes

daschuer added the changelog This PR should be included in the changelog label Oct 19, 2022

daschuer added this to the 2.4.0 milestone Oct 19, 2022

daschuer merged commit 8cd8908 into mixxxdj:main Oct 19, 2022

ronso0 mentioned this pull request Mar 4, 2024

Deck playback continues to play after new track is loaded #10733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to keep deck playing on track load #10944

Option to keep deck playing on track load #10944

mbacarella commented Oct 6, 2022

ronso0 commented Oct 6, 2022

ronso0 commented Oct 6, 2022

mbacarella commented Oct 6, 2022

mbacarella commented Oct 6, 2022

mbacarella commented Oct 6, 2022

mbacarella commented Oct 6, 2022

ronso0 Oct 6, 2022

mbacarella Oct 6, 2022

ronso0 Oct 7, 2022 •

edited

Loading

ronso0 commented Oct 6, 2022

mbacarella commented Oct 6, 2022

ronso0 commented Oct 7, 2022

daschuer Oct 7, 2022 •

edited

Loading

mbacarella Oct 7, 2022

daschuer Oct 7, 2022

mbacarella Oct 8, 2022

mbacarella Oct 8, 2022

mbacarella commented Oct 11, 2022

ronso0 commented Oct 11, 2022

mbacarella commented Oct 11, 2022

mbacarella commented Oct 16, 2022

ronso0 commented Oct 16, 2022

daschuer left a comment

daschuer Oct 17, 2022

mbacarella Oct 17, 2022

daschuer left a comment

Option to keep deck playing on track load #10944

Option to keep deck playing on track load #10944

Conversation

mbacarella commented Oct 6, 2022

ronso0 commented Oct 6, 2022

ronso0 commented Oct 6, 2022

mbacarella commented Oct 6, 2022

mbacarella commented Oct 6, 2022

mbacarella commented Oct 6, 2022

mbacarella commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

ronso0 commented Oct 6, 2022

mbacarella commented Oct 6, 2022

ronso0 commented Oct 7, 2022

daschuer Oct 7, 2022 • edited Loading

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

mbacarella commented Oct 11, 2022

ronso0 commented Oct 11, 2022

mbacarella commented Oct 11, 2022

mbacarella commented Oct 16, 2022

ronso0 commented Oct 16, 2022

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

ronso0 Oct 7, 2022 •

edited

Loading

daschuer Oct 7, 2022 •

edited

Loading