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.

Sign up for GitHub

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

Jump to bottom

fix ToggleSelectedSidebarItem control of the Numark Mixtrack (Pro) II #13543

Open
wants to merge 1 commit into
base: 2.5
Choose a base branch
from
Open

fix ToggleSelectedSidebarItem control of the Numark Mixtrack (Pro) II #13543

wants to merge 1 commit into from

Conversation

timkuijsten
Copy link

@timkuijsten timkuijsten commented Aug 7, 2024 •
edited
Loading

When browsing, press the browse button to toggle opening/closing a folder.

updates PR #1540

@timkuijsten timkuijsten changed the title fix ToggleSelectedSidebarItem control of the Numark Mixtrack 2 fix ToggleSelectedSidebarItem control of the Numark Mixtrack (Pro) II Aug 7, 2024
@JoergAtGithub
Copy link
Member

Welcome at Mixxx!
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

@JoergAtGithub
Copy link
Member

The pre-commit check is failing. The best way to fix pre-commit issues is to install pre-commit locally on your system, as described here: https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking
Than it will fix these issues automatically before every commit.
Alternatively, you can download the pre-commit.patch file from the artifacts of this PR
grafik
unzip it, and apply it using:
git apply pre-commit.patch

ronso0
ronso0 reviewed Aug 8, 2024
View reviewed changes
Copy link
Member

@ronso0 ronso0 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 for fixing this!
(even though I don't understand what broke the function)

I made some suggestions to improve the library mappings.

res/controllers/Numark Mixtrack 2.midi.xml
@@ -481,7 +481,7 @@
</control>
<control>
<group>[Playlist]</group>
<key>NumarkMixTrackII.toggleDirectoryMode</key>
<key>NumarkMixTrackII.toggleSelectedSidebarItem</key>
Copy link
Member

Choose a reason for hiding this comment

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

There is another toggleDirectoryMode binding above (0x77), is that the Back button?

That could use [Library],MoveFocus

Copy link
Author

Choose a reason for hiding this comment

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

Idd, 0x77 is the back button, 0x76 is pressing the big browse button.

res/controllers/Numark-Mixtrack-2-scripts.js
@@ -158,6 +159,12 @@ NumarkMixTrackII.selectKnob = function(channel, control, value, status, group) {
}
}

NumarkMixTrackII.toggleSelectedSidebarItem = function(channel, control, value, status, group) {
if (value) {
engine.setValue("[Playlist]", "ToggleSelectedSidebarItem", 1);
Copy link
Member

Choose a reason for hiding this comment

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

The [Playlist] controls are deprecated, but they still work
It's recommende to use the [Library] instead.

IIUC the Back button should move focus between the library widgets (search, sidebar, tracks).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for letting me know. I might be able to look into this at some point in the future, but not right away.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I see with the propsed changes the 'directory mode' won't work as before. Now it would trigger move focus (like Tab key) and call updateDirectoryAndFileLeds which uses directory_mode.

Nowadays we have [Library],focused_widget
I.e. directory_mode is not required anymore and updateDirectoryAndFileLeds could use [Library],focused_widget: FOLDER whould be lit if it's 2 and FILE if it's 3.

Are yo motivated to do that refactoring?

Copy link
Author

Choose a reason for hiding this comment

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

I might like to do that at some time in the future but I can't promise. Maybe we can get this simple fix in so that controller based navigation works again? Without this fix the mouse or keyboard has to be used to go into a directory/folder when browsing the library.

Copy link
Member

Choose a reason for hiding this comment

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

Oka, given my experience that "might like to do that at some time in the future" likely never happens due to shifted prios etc. (myself included ; ) and the fact that it'll take some weeks or months until 2.4.2 with the fixed mapping will be released (shortly before 2.5 I guess), we might as well try to fix it now.

If you don't have time/motivation to fix it yourself (potentially with another round of review) I may provide a commit this week, but I need you to test it (I don't have access to a Mixtrack Pro II).

Copy link
Author

@timkuijsten timkuijsten Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well, I just bought a Pioneer DDJ-FLX4 so I won't be creating the patch for the Mixtrack. But, as long as I have not given back the Mixtrack to my friend, I can test a patch if you have it.

p.s. I see the behaviour I implemented for the Mixtrack differs from the DDJ, if you press the browse button on the DDJ it toggles between files, search, folders. So for the DDJ I also don't have a way on the controller to open/collapse a folder in the folder view.

@ronso0 ronso0 changed the base branch from main to 2.4 August 8, 2024 09:27
@ronso0
Copy link
Member

ronso0 commented Aug 8, 2024

Controller mappings usually go to the stable branch (2.4 atm), unless they require features from later versions which is not the case here.
I just changed the target branch to 2.4, please rebase locally onto 2.4 and force-push.
Not sure how familiar you're with git, if you need help let us know. Please don't close this PR if you run into issues, those can be fixed.
Thank you!

@timkuijsten
Numark Mixtrack 2: fix ToggleSelectedSidebarItem
57a06a7 
When browsing, press the browse button to toggle opening/closing a
folder.

Tested with Numark Mixtrack Pro II.
@timkuijsten timkuijsten force-pushed the main branch 2 times, most recently from 57a06a7 to 4fbe67b Compare August 8, 2024 09:42
@timkuijsten
Copy link
Author

Whoops, resetting my main to mixxx/main triggered creating a lot of labels. I wanted to update the pr based on my new 2.4 branch but I can't see how to edit that. So now my main == my 2.4 == mixxx/2.4 + my fix.

@timkuijsten
Copy link
Author

timkuijsten commented Aug 8, 2024 •
edited
Loading

It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

I understand the routine, but I just copy/pasted the code (5 lines, mostly boilerplate) from Pioneer-DDJ-SB3-scripts.js so it falls under the copyright of that file:

Copyright (c) 2017 Be (be.0@gmx.com), licensed under GPL version 2 or later
Copyright (c) 2014-2015 various contributors, licensed under MIT license

@github-actions GitHub Actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Nov 20, 2024
@daschuer daschuer changed the base branch from 2.4 to 2.5 January 25, 2025 22:18
@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Jan 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronso0 ronso0 ronso0 left review comments

Assignees
No one assigned
Labels
controller mappings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@timkuijsten @JoergAtGithub @ronso0 @Swiftb0y