feat: add setting definition for Traktor S4 MK3 #12995

acolombier · 2024-03-23T19:25:33Z

This PR adds the setting definition for Traktor S4 MK3.

Manual PR to come soon

Depends on #11300

acolombier · 2024-03-23T19:46:11Z

Added a second commit to provide backward compatibility with 2.4 as an option. Happy to remove or rebase to target 2.4 instead.

Tests are expected to fail on the first commit otherwise till the dependency is merged.

res/controllers/Traktor-Kontrol-S4-MK3.js

ywwg · 2024-03-28T21:18:04Z

"chore" is for low-level repository tasks. This is a "feat"

ywwg

Thank you for doing this! It's very fun to see these get used right away. I mostly made some language edits and notes, as well as some naming nits.

res/controllers/Traktor Kontrol S4 MK3.hid.xml

ywwg · 2024-03-28T21:29:23Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+                default="true"
+                label="Keep one-color LED dimmed instead of off when inactive">
+                <description>
+                    This will always ensure every button has some lighting which may help in dark environment, but may put off the lead color of the deck.


not sure what you mean by "put off the lead color"? That doesn't quite scan for my own grammar.

Just for context here (and for some other comments you've written), when I did this original mapping, it was received with various opinion of how things should look like and work (perfect to leverage these new setting capabilities hey!)
What I tried to say was that by keeping those single color LED backlit (white and green mainly if I recall properly), the colour of the deck may look slightly off (say, you've picked a nice Lime coloring, there is some red and white in the way, which may make it hard to see what is the actual deck color (thus what deck is currently selected)

res/controllers/Traktor Kontrol S4 MK3.hid.xml

ywwg · 2024-03-28T21:42:40Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+                    <value label="Times Played">18</value>
+                    <value label="Rating">19</value>
+                    <value label="Key">20</value>
+                    <description>Second column when iterating over sortable library columns using VIEW+Library Encoder press</description>


we don't capitalize any other buttons, should we?

I'm happy either way. What would you prefer? All caps or not?
Since the fields should support rich text, we could also just put some emphasis like View or View

res/controllers/Traktor-Kontrol-S4-MK3.js

acolombier

It's very fun to see these get used right away

That branch has been sitting on my local for almost a year, so it is a relief to finally seeing being useful to other :)
Will do the suggested changed changes in a bit. How hard would it be to help me find all grammar/spelling mistakes? If too hard, I can try an see if ChatGPT can help correcting this in place 🤣

res/controllers/Traktor Kontrol S4 MK3.hid.xml

acolombier · 2024-03-28T22:38:28Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+                default="true"
+                label="Keep one-color LED dimmed instead of off when inactive">
+                <description>
+                    This will always ensure every button has some lighting which may help in dark environment, but may put off the lead color of the deck.


Just for context here (and for some other comments you've written), when I did this original mapping, it was received with various opinion of how things should look like and work (perfect to leverage these new setting capabilities hey!)
What I tried to say was that by keeping those single color LED backlit (white and green mainly if I recall properly), the colour of the deck may look slightly off (say, you've picked a nice Lime coloring, there is some red and white in the way, which may make it hard to see what is the actual deck color (thus what deck is currently selected)

res/controllers/Traktor Kontrol S4 MK3.hid.xml

acolombier · 2024-03-28T22:45:45Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+                </description>
+            </option>
+            <option
+                variable="mixerControlsMixAuxOnShift"


Yes, we do.

It happens quite often that I would use the mixer to adjust track volumes (e.g fade out) while pitching down (shift+left encoder) a track.

acolombier · 2024-03-28T22:47:56Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+                    <value label="Times Played">18</value>
+                    <value label="Rating">19</value>
+                    <value label="Key">20</value>
+                    <description>Second column when iterating over sortable library columns using VIEW+Library Encoder press</description>


I'm happy either way. What would you prefer? All caps or not?
Since the fields should support rich text, we could also just put some emphasis like View or View

acolombier · 2024-03-28T22:55:10Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+                    type="enum"
+                    label="Sixth column">
+                    <value label="---" default="true">0</value>
+                    <value label="Artist">1</value>


These values come from here. I would expect it won't be changed (potentially new value may be added tho, but no breaking change)
I don't know how to put test for that, what would you suggest? GTest case?

acolombier · 2024-03-28T22:57:44Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+            <option
+                variable="loopWheelMoveFactor"
+                type="integer"
+                min="20"


I'm not sure I understand. Do you mean you would want a bigger max? is 10x of default enough in your opinion?

res/controllers/Traktor-Kontrol-S4-MK3.js

acolombier · 2024-03-29T11:06:57Z

I have removed 2.4 backward compatibility and tried and rework phrasing and spelling (with a limited success - sorry)

I have also experienced with custom style around button names, similar to how it is done on the manual. What are your thoughts?

ywwg

looking good! just a few more questions and then I think we're good to go

ywwg · 2024-03-29T14:09:15Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+                </description>
+            </option>
+            <option
+                variable="mixerControlsMixAuxOnShift"


oh is that a typo?

Suggested change

variable="mixerControlsMixAuxOnShift"

variable="mixerControlsMicAuxOnShift"

ywwg · 2024-03-29T14:09:34Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+                </description>
+            </option>
+            <option
+                variable="mixerControlsMixAuxOnShift"


I mean, why not make this true by default and not be an option?

ywwg · 2024-03-29T14:12:33Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+                    type="enum"
+                    label="Sixth column">
+                    <value label="---" default="true">0</value>
+                    <value label="Artist">1</value>


It could be a gross gtest in c++ that loads the xml and compares it to the C++ code. or maybe a better option is we could make this file somehow templated (qtxml?) and generated as part of the build process.

The library changes not very often, but often enough. Without an automated way of detecting the change I'm afraid we will never know that we broke this config.

ywwg · 2024-03-29T14:13:31Z

res/controllers/Traktor Kontrol S4 MK3.hid.xml

+            <option
+                variable="loopWheelMoveFactor"
+                type="integer"
+                min="20"


yeah probably a bigger max. I have seen some videos of djs cranking the CDJ wheel sensitivity way way up and getting some really cool effects. Sure let's try 10x!

res/controllers/Traktor-Kontrol-S4-MK3.js

ywwg · 2024-03-29T14:15:12Z

src/controllers/legacycontrollersettings.cpp

@@ -118,7 +120,21 @@ QWidget* LegacyControllerBooleanSetting::buildInputWidget(QWidget* pParent) {
        emit changed();
    });

-    return pCheckBox;
+    auto pLabelWidget = make_parented<QLabel>(pWidget);


what is this change for?

This change was requried as the QCheckBox built-in label doesn't support rich text. Happy to move that to an other PR is we are happy about the way it looks with rich text (that is the emphasis on button )

ywwg

thank you for the regression test! This looks good to me

ywwg · 2024-03-30T17:28:23Z

src/test/controllers/controller_s4mk3_settings_test.cpp

+};
+
+QHash<QString, TrackModel::SortColumnId> ControllerS4MK3SettingTest::COLUMN_MAPPING = {
+        {"Artist", TrackModel::SortColumnId::Artist},


It is unfortunate that we don't have enum-to-string mapping in the actual code. https://github.com/mixxxdj/mixxx/blob/main/src/library/browse/browsetablemodel.cpp#L60

There is an issue with internationalization, we will need a solution for that too eventually. But for now this is good enough, thank you!

Maybe add a comment to this file that if other controllers want to add the same type of feature they can also use this test? Maybe rename the test to columnid_regression_test.cpp or something like that?

ywwg · 2024-03-30T18:00:35Z

As is tradition, can you prepare the manual PR? we have to insist on having both ready to enforce that documentation stays up to date <3

acolombier · 2024-03-30T18:03:01Z

As is tradition, can you prepare the manual PR? we have to insist on having both ready to enforce that documentation stays up to date <3

Already done and waiting for your review :D

ywwg · 2024-03-31T21:24:34Z

merging, and then we'll make sure to merge the manual PR in the right place.

github-actions bot added the controllers label Mar 23, 2024

acolombier force-pushed the chore/add-setting-to-traktor-s4mk3 branch from 56e2929 to cb4b03d Compare March 23, 2024 20:04

acolombier commented Mar 23, 2024

View reviewed changes

res/controllers/Traktor-Kontrol-S4-MK3.js Show resolved Hide resolved

JoergAtGithub mentioned this pull request Mar 27, 2024

feat(Controller): add support for settings #11300

Merged

6 tasks

JoergAtGithub requested a review from ywwg March 28, 2024 20:00

acolombier force-pushed the chore/add-setting-to-traktor-s4mk3 branch from cb4b03d to 31d1706 Compare March 28, 2024 21:35

acolombier changed the title ~~chore: add setting definition for Traktor S4 MK3~~ feat: add setting definition for Traktor S4 MK3 Mar 28, 2024

ywwg requested changes Mar 28, 2024

View reviewed changes

acolombier commented Mar 28, 2024

View reviewed changes

acolombier force-pushed the chore/add-setting-to-traktor-s4mk3 branch from 16b015a to dd13e00 Compare March 29, 2024 10:56

feat: add setting definition for Traktor S4 MK3

53ef29e

acolombier force-pushed the chore/add-setting-to-traktor-s4mk3 branch from dd13e00 to 53ef29e Compare March 29, 2024 11:04

ywwg reviewed Mar 29, 2024

View reviewed changes

github-actions bot added build code quality labels Mar 30, 2024

acolombier mentioned this pull request Mar 30, 2024

S4MK3: update setting change instruction mixxxdj/manual#634

Merged

ywwg approved these changes Mar 30, 2024

View reviewed changes

acolombier force-pushed the chore/add-setting-to-traktor-s4mk3 branch 2 times, most recently from 1237dc3 to 3bf2f62 Compare March 30, 2024 17:49

Add a testcase and addressing couple of nits

a89e141

acolombier force-pushed the chore/add-setting-to-traktor-s4mk3 branch from 3bf2f62 to a89e141 Compare March 30, 2024 17:49

ywwg approved these changes Mar 30, 2024

View reviewed changes

ywwg merged commit 9f68007 into mixxxdj:main Mar 31, 2024
13 checks passed

acolombier mentioned this pull request Apr 3, 2024

feat: enable controller setting on MIDI #13045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add setting definition for Traktor S4 MK3 #12995

feat: add setting definition for Traktor S4 MK3 #12995

acolombier commented Mar 23, 2024 •

edited

acolombier commented Mar 23, 2024 •

edited

ywwg commented Mar 28, 2024

ywwg left a comment

ywwg Mar 28, 2024

acolombier Mar 28, 2024

ywwg Mar 28, 2024

acolombier Mar 28, 2024

acolombier left a comment

acolombier Mar 28, 2024

acolombier Mar 28, 2024

acolombier Mar 28, 2024

acolombier Mar 28, 2024

acolombier Mar 28, 2024

acolombier commented Mar 29, 2024

ywwg left a comment

ywwg Mar 29, 2024

ywwg Mar 29, 2024

ywwg Mar 29, 2024

ywwg Mar 29, 2024

ywwg Mar 29, 2024

acolombier Mar 30, 2024

ywwg left a comment

ywwg Mar 30, 2024

ywwg Mar 30, 2024

acolombier Mar 30, 2024

ywwg commented Mar 30, 2024

acolombier commented Mar 30, 2024

ywwg commented Mar 31, 2024

	variable="mixerControlsMixAuxOnShift"
	variable="mixerControlsMicAuxOnShift"

feat: add setting definition for Traktor S4 MK3 #12995

feat: add setting definition for Traktor S4 MK3 #12995

Conversation

acolombier commented Mar 23, 2024 • edited

acolombier commented Mar 23, 2024 • edited

ywwg commented Mar 28, 2024

ywwg 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

acolombier 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

acolombier commented Mar 29, 2024

ywwg 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

Choose a reason for hiding this comment

ywwg 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

ywwg commented Mar 30, 2024

acolombier commented Mar 30, 2024

ywwg commented Mar 31, 2024

acolombier commented Mar 23, 2024 •

edited

acolombier commented Mar 23, 2024 •

edited