Make deck cloning via Load button optional (opt-out) #2042

ronso0 · 2019-02-28T20:26:31Z

This adds an option to Preferences > Decks > Deck Options where users can disable Load double-tap to clone a playing deck (keyboard and controllers) (introduced in #1892).

I added this option because I accidentially cloned the adjacent deck way to often due to an unreliable Load button on controller, but also when I was in a hurry. Until now loading just worked, but now I had to double check that I actually loaded the track from the library and did NOT clone the other deck, which is annoying and drew too much attention from the actual task of DJing.

The new CO is called [Controls],CloneDeckOnLoadDoubleTap and defaults to true so the existing implementation remains unaffected for everyone.

Any suggestions for the wording in Deck Options? I'm not happy with it..

Should the option only affect the Load button in Midi mapppings and always work on the keyboard?

ronso0 · 2019-02-28T23:35:46Z

daschuer · 2019-03-01T06:16:22Z

How about that:
"Clone deck when a track is loaded twice"
A tooltip can explain the details.
IMHO this feature should effect keyboard and midi.
The user has still the deck to deck drag and drop option.

An alternative solution would be a not cloning load track Co.
The mapper can than decide if the hardware is sufficient precise to do the double load trick.

ronso0 · 2019-03-01T14:36:45Z

How about that:
"Clone deck when a track is loaded twice"

Clone deck . . [_] Create deck clone when Load is pressed twice
a bit better as it doesn't include drag'n'drop

A tooltip can explain the details.

👍

IMHO this feature should effect keyboard and midi.
The user has still the deck to deck drag and drop option.

👍

An alternative solution would be a not cloning load track Co.
The mapper can than decide if the hardware is sufficient precise to do the double load trick.

👍 perfect!

ronso0 · 2019-04-12T19:45:18Z

What about
Clone deck [_] Double-press Load button to clone

removes the duplicate "Clone Deck" from previous string, and also limits the option to MIDI & keyboard buttons (no Load button in skins that could be double-clicked)

daschuer

Playing with this function I think the original implementation is too hacky.
"LoadSelectedTrack" should do what it is saying, load the selected track.

Currently it load the selected track first and replaces it with the playing track on the second press.
I understand that this could be a desired behavior, but it is up to the mapping to decide what should clone a deck. So I think the cleanest solution would be a dedicated control.

daschuer · 2019-04-20T13:05:24Z

src/mixer/playermanager.cpp

@@ -580,8 +580,11 @@ void PlayerManager::slotLoadTrackToPlayer(TrackPointer pTrack, QString group, bo
    }

    mixxx::Duration elapsed = m_cloneTimer.restart();
-    if (m_lastLoadedPlayer == group && elapsed < mixxx::Duration::fromSeconds(0.5)) {
-        // load pressed twice quickly, clone instead of loading
+    bool cloneOnDoubleTap = m_pConfig->getValue<bool>(


I think we should here use the defaut value if the config is missing. There should be a function that allows to set a default as fall back.

using the default (off) now.
~~but shouldn't defaults be set only in dlgprefdeck.cpp?~~
alright. let's do it here, as well as in dlgprefdeck > reset to defaults only (found the respective TODO there)

src/preferences/dialog/dlgprefdeck.cpp

daschuer · 2019-04-20T13:08:35Z

src/preferences/dialog/dlgprefdeck.cpp

@@ -566,6 +586,9 @@ void DlgPrefDeck::slotApply() {
    m_pConfig->setValue(ConfigKey("[Controls]", "AllowTrackLoadToPlayingDeck"),
                        !m_bDisallowTrackLoadToPlayingDeck);

+    m_pConfig->setValue(ConfigKey("[Controls]", "CloneDeckOnLoadDoubleTap"),
+                        m_bCloneDeckOnLoadDoubleTap);


Why not read the checkbox here and omit the m_bCloneDeckOnLoadDoubleTap vairable?

because we do it via variables for all the other settings?

It is maybe we can improve this as well one day. It jumps into my eyes, that the syncing of the checkbox and the variable has no value.

src/preferences/dialog/dlgprefdeckdlg.ui

daschuer · 2019-04-20T13:30:24Z

It is OK to merge this as an intermediate solution. A new generic LoadDeck control can be the preferences configurable one if we really need this.

…deck-opt-out

daschuer

Some more comments

daschuer · 2019-06-26T17:16:44Z

src/mixer/playermanager.cpp

-        // load pressed twice quickly, clone instead of loading
+    // If not present in the config, use & set the default value
+    if (!m_pConfig->exists(ConfigKey("[Controls]","CloneDeckOnLoadDoubleTap"))) {
+        bool cloneOnDoubleTap = true;


this variable falls out of scope in line 587

fixed it already, waiting for local build because of the ui changes about hotcue colors

src/mixer/playermanager.cpp

daschuer · 2019-06-26T17:20:48Z

src/preferences/dialog/dlgprefdeck.cpp

@@ -413,6 +425,8 @@ void DlgPrefDeck::slotResetToDefaults() {
    // Don't load tracks into playing decks.
    checkBoxDisallowLoadToPlayingDeck->setChecked(true);

+    // Clone decks by double-tapping Load button.
+    checkBoxCloneDeckOnLoadDoubleTap->setChecked(true);


it would be nice to use a constant for the default true

please explain what you mean.

In a anonymous namespace:

contexpr bool kDefaultCloneDeckOnLoad = true;

This helps to set the default value only in one place.

…deck-opt-out

ronso0 · 2019-06-26T20:35:00Z

never again I'll resolve conflicts via the web interface. potentially screws up ui files, git pull never really worked afterwards

have to fix the ui file as there are now duplicate layout cells..

ronso0 · 2019-07-03T13:26:36Z

src/mixer/playermanager.cpp

@@ -29,6 +29,8 @@ namespace {

 const mixxx::Logger kLogger("PlayerManager");

+constexpr bool kDefaultCloneDeckOnLoad;


is this sufficient to make kDefaultCloneDeckOnLoad available from src/preferences/dialog/dlgprefdeck.cpp ?

appearantly not (or it's just wrong) because comilation fails.
do I need to put in src/preferences/dialog/dlgprefdeck.h and include that in playermanager ?

Yes, the easy route would be OK for me here.

ronso0 · 2019-08-06T10:33:22Z

all comments addressed I guess.
current state:

Is the wording okay now?

ronso0 · 2019-08-16T11:36:28Z

@Be-ing Do you have ideas about the wording?

Clone Deck [_] Double-press Load button to clone playing track into next stopped deck

Be-ing · 2019-08-16T14:07:12Z

It looks like there are unrelated files in this branch. Can you rebase on master?

ronso0 · 2019-08-16T16:08:15Z

It looks like there are unrelated files in this branch. Can you rebase on master?

yup, done.

ronso0 · 2019-08-16T16:10:24Z

The wording for the checkbox and the tooltip look good to me.

Current status or my last proposal?

Clone Deck [_] Double-press Load button to clone playing track into next stopped deck

Be-ing · 2019-08-16T16:14:22Z

How about "Double-press Load button to clone playing track"?

ronso0 · 2019-08-16T16:22:21Z

Clone Deck [_] Double-press Load button to clone playing track
is okay for me.

But looking closer, I think we need to make the tooltip more clear:
If the track currently selected in the Tracks table is already playing in a deck, Mixxx creates a playing clone of it in the first stopped deck it finds when double-pressing the Load button on controlllers or keyboard.
You can always drag-and-drop tracks on screen to clone a deck.

ronso0 · 2019-08-16T16:53:52Z

last commits where not pushed as I was in detached mode.

Should be fine now, also the label & tooltip are finalized.
~~Ready to merge.~~

ronso0 · 2019-08-16T16:57:26Z

aaargh..
"If the track currently selected in the Tracks table is already playing in a deck, Mixxx creates a playing clone ..."
is wrong.
Need to restore the tooltip..

ronso0 · 2019-08-16T17:02:58Z

okay.
tooltip:
Create a playing clone of the first playing deck Mixxx finds
by double-tapping a Load button on a controller or keyboard.
You can always drag-and-drop tracks on screen to clone a deck.

Checkbox Label:
Clone Deck [_] Double-press Load button to clone playing track

Puh, ready to merge.

Be-ing · 2019-08-16T18:32:20Z

How about removing the words "Mixxx finds" from the tooltip?

src/preferences/dialog/dlgprefdeckdlg.ui

Be-ing · 2019-08-16T19:38:09Z

I tested and this works as described. 👍

Be-ing · 2019-08-17T18:05:36Z

How about removing the words "Mixxx finds" from the tooltip?

ping

Be-ing · 2019-08-17T18:07:43Z

I'm wondering of double-load cloning should be off by default. It can interfere with a performance by making the volume jump unexpectedly, which would be especially surprising if you don't know about the feature.

ronso0 · 2019-08-17T18:13:46Z

How about removing the words "Mixxx finds" from the tooltip?

done.

Be-ing · 2019-08-17T18:14:08Z

Thanks for this. I have occasionally been annoyed by accidentally triggering this feature.

ronso0 · 2019-08-17T18:15:23Z

I'm wondering of double-load cloning should be off by default. It can interfere with a performance by making the volume jump unexpectedly, which would be especially surprising if you don't know about the feature.

Surprises make a steep learning curve :)
I fear the feature will not be discovered if Off bey default

Be-ing · 2019-08-17T18:16:55Z

I think double pressing a load button to trigger this is less discoverable than dragging and dropping from deck to deck.

ronso0 · 2019-08-17T18:22:08Z

hmm..maybe.
if the mouse action is discoverable this wouldn't indicate there's keyboard/controller way, too.

I can't recall: besides the "bugs fixed" list, do we have a "New features" list for new major Mixxx versions?

Be-ing · 2019-08-17T18:23:49Z

The release announcements serve that purpose.

We could make dragging and dropping more discoverable by changing the cursor over draggable widgets.

ronso0 · 2019-08-17T18:48:16Z

I'd like to change this, too.
would be nice to have different cursors for each drop target:

load to deck/sampler
add to playlists and crates
clone to deck
(anything else?)

Holzhaus · 2019-08-23T11:51:11Z

Current master fails to build, I'm getting:

src/preferences/dialog/dlgprefdeck.cpp: In constructor 'DlgPrefDeck::DlgPrefDeck(QWidget*, MixxxMainWindow*, PlayerManager*, UserSettingsPointer)':
src/preferences/dialog/dlgprefdeck.cpp:173:5: error: 'checkBoxCloneDeckOnLoadDoubleTap' was not declared in this scope; did you mean 'm_bCloneDeckOnLoadDoubleTap'?
  173 |     checkBoxCloneDeckOnLoadDoubleTap->setChecked(m_bCloneDeckOnLoadDoubleTap);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |     m_bCloneDeckOnLoadDoubleTap
src/preferences/dialog/dlgprefdeck.cpp: In member function 'virtual void DlgPrefDeck::slotUpdate()':
src/preferences/dialog/dlgprefdeck.cpp:349:5: error: 'checkBoxCloneDeckOnLoadDoubleTap' was not declared in this scope; did you mean 'm_bCloneDeckOnLoadDoubleTap'?
  349 |     checkBoxCloneDeckOnLoadDoubleTap->setChecked(m_pConfig->getValue(
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |     m_bCloneDeckOnLoadDoubleTap
src/preferences/dialog/dlgprefdeck.cpp: In member function 'virtual void DlgPrefDeck::slotResetToDefaults()':
src/preferences/dialog/dlgprefdeck.cpp:429:5: error: 'checkBoxCloneDeckOnLoadDoubleTap' was not declared in this scope; did you mean 'm_bCloneDeckOnLoadDoubleTap'?
  429 |     checkBoxCloneDeckOnLoadDoubleTap->setChecked(kDefaultCloneDeckOnLoad);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |     m_bCloneDeckOnLoadDoubleTap

ronso0 · 2019-08-23T12:15:46Z

checkBoxCloneDeckOnLoadDoubleTap is included from src/preferences/dialog/dlgprefdeckdlg.ui.
In eclipse I see warnings for all the other checkboxes built it's built without errors here.

Be-ing · 2019-08-23T12:17:05Z

@Holzhaus maybe try running scons -c?

Holzhaus · 2019-08-23T13:42:12Z

Sorry for the noise, this was caused by using Scons with Python 3 instead of Python 2.7. Should be fixed by uklotzde@03fad27.

ronso0 added 2 commits February 28, 2019 21:09

add clone option to Pref > Decks

52f60bc

make playermanager respect clone option

162c742

esbrandt mentioned this pull request Mar 3, 2019

implement instant doubles functionality (clone deck) #1892

Merged

Double-press Load button to clone, add tooltip

2ce61a5

daschuer requested changes Apr 20, 2019

View reviewed changes

ronso0 added 6 commits May 3, 2019 16:19

Merge branch 'master' of https://github.com/mixxxdj/mixxx into clone-…

a6968e6

…deck-opt-out

use default FALSE if no config value exists yet

6a17f0a

set cloneOnLoadDoubleTap default also in playermanager

08c793e

fix tooltip of cloneDeckOnLoadDoubelTap checkbox

8ee8543

restore tooltip of checkBoxSeekToCue checkbox

9693de5

declare cloneOnDoubleTap earlier

bc40883

daschuer reviewed Jun 26, 2019

View reviewed changes

Merge branch 'master' of https://github.com/mixxxdj/mixxx into clone-…

7213704

…deck-opt-out

ronso0 force-pushed the clone-deck-opt-out branch from d2634d7 to 7213704 Compare June 26, 2019 17:58

repair failed conflict resolution

8e06f36

ronso0 added 2 commits June 26, 2019 22:46

fix & sort ui cells in Preferences > Decks

6cf252f

simplify reading &/ setting the default of bool cloneOnDoubleTap

a8a41f5

ronso0 force-pushed the clone-deck-opt-out branch from fab0079 to 0a3f0cd Compare July 3, 2019 13:25

ronso0 commented Jul 3, 2019

View reviewed changes

ronso0 force-pushed the clone-deck-opt-out branch 2 times, most recently from 80dc03a to aae48b2 Compare July 5, 2019 16:50

finalize label in Preferences

5032a3f

ronso0 force-pushed the clone-deck-opt-out branch from 86b1892 to 5032a3f Compare August 16, 2019 16:59

Be-ing reviewed Aug 16, 2019

View reviewed changes

src/preferences/dialog/dlgprefdeckdlg.ui Outdated Show resolved Hide resolved

consistent notation in Deck preferences page

80124dd

tweak tooltip

4097f15

Be-ing merged commit 6962ce1 into mixxxdj:master Aug 17, 2019

ronso0 deleted the clone-deck-opt-out branch August 17, 2019 18:14

iamcodemaker mentioned this pull request Apr 22, 2020

release 2.3 beta #2697

Merged

		@@ -29,6 +29,8 @@ namespace {

		const mixxx::Logger kLogger("PlayerManager");

		constexpr bool kDefaultCloneDeckOnLoad;

Make deck cloning via Load button optional (opt-out) #2042

Make deck cloning via Load button optional (opt-out) #2042

Conversation

ronso0 commented Feb 28, 2019 • edited Loading

ronso0 commented Feb 28, 2019

daschuer commented Mar 1, 2019

ronso0 commented Mar 1, 2019

ronso0 commented Apr 12, 2019

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 Jun 26, 2019 • 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

daschuer commented Apr 20, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

ronso0 Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Aug 6, 2019

ronso0 commented Aug 16, 2019

Be-ing commented Aug 16, 2019

ronso0 commented Aug 16, 2019

ronso0 commented Aug 16, 2019

Be-ing commented Aug 16, 2019

ronso0 commented Aug 16, 2019

ronso0 commented Aug 16, 2019 • edited Loading

ronso0 commented Aug 16, 2019

ronso0 commented Aug 16, 2019

Be-ing commented Aug 16, 2019

Be-ing commented Aug 16, 2019

Be-ing commented Aug 17, 2019

Be-ing commented Aug 17, 2019

ronso0 commented Aug 17, 2019

Be-ing commented Aug 17, 2019

ronso0 commented Aug 17, 2019

Be-ing commented Aug 17, 2019

ronso0 commented Aug 17, 2019

Be-ing commented Aug 17, 2019

ronso0 commented Aug 17, 2019

Holzhaus commented Aug 23, 2019

ronso0 commented Aug 23, 2019

Be-ing commented Aug 23, 2019

Holzhaus commented Aug 23, 2019

ronso0 commented Feb 28, 2019 •

edited

Loading

ronso0 Jun 26, 2019 •

edited

Loading

ronso0 commented Jun 26, 2019 •

edited

Loading

ronso0 Jul 3, 2019 •

edited

Loading

ronso0 commented Aug 16, 2019 •

edited

Loading