-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update Numark-Mixtrack-3-scripts.js #14180
base: 2.5
Are you sure you want to change the base?
Conversation
Improve Browse knob/button behaviour by allowing focus shift between search, sidebar, and track table with Shift+turn Browse. Browse button expands sidebar items or loads track depending on focus. Shift+press Browse maximises the track table. Further discussion: mixxxdj#14165
Thank you, this makes sense. FWIW Shift + turn may also be used to scroll the focused view, and Shift + press does move the focus, but you decided to maximize the library with that, which is fine given the limited buttons on the Mixtrack3. And the usual paperwork: |
Btw mapping improvements and new mappings should target the 'stable' branch (currently 2.5), so please rebase onto 2.5. |
Thank you - rebased onto 2.5, agreement signed and made the requested update to |
Please check endcredits33#2 |
Mixtrack3: add option for Library navigation mode: Focus | Classic
Fix libraryModes reference to focus mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, couple nitpicks
In Classic mode the Browse encoder can select items in the sidebar and tracks table only, but independent | ||
of keyboadr focus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Classic mode the Browse encoder can select items in the sidebar and tracks table only, but independent | |
of keyboadr focus. | |
In Classic mode the Browse encoder can select items in the sidebar and tracks table only, but independent | |
of keyboard focus. |
const libraryModes = { | ||
focus: 0, | ||
classic: 1, | ||
}; | ||
const LibraryMode = libraryModes[engine.getSetting("libraryMode")] || libraryModes.focus; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove this part entirely if you do the below instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of querying the settings each time compared to this const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplicity. As long as its not done in a tight loop, querying the setting should not be expensive. Doing all of this mapping is unnecessarily weird IMO. Additionally, global const
s in mappings are kind off iffy (can suddenly break other things due to the way JS modules work, so thats why its always been encouraged to only add properties to the controller object instead of the global one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmokay.
tight loop = called at high frequency?
So NumarkMixtrack3.libraryMode = engine.getSetting()..
would be okay?
And settings is not an object of NumarkMixtrack3 here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tight loop = called at high frequency?
Yes.
So NumarkMixtrack3.libraryMode = engine.getSetting().. would be okay?
Yes, better than the current one... But I won't insist on this.
script.toggleControl("[Skin]", "show_maximized_library"); | ||
} | ||
} | ||
if (LibraryMode === libraryModes.focus) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (LibraryMode === libraryModes.focus) { | |
if (engine.getSetting("libraryMode") === "focus") { |
Improve Browse knob/button behaviour by allowing focus shift between search, sidebar, and track table with Shift+turn Browse.
Further discussion: #14165
edit(@ronso0)