-
-
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
feat(Controller): add support for settings #11300
feat(Controller): add support for settings #11300
Conversation
My biggest question is what will the interface be between the GUI and JavaScript? AFAICT this only builds the GUI so far, but it doesn't expose it the device-specific settings to the script? |
Yes, that's correct, I haven't done this part yet, hopefully will get some part of it done today. Update: with this commit , I have made the settings injection into the script. The logic is similar to how |
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 this PR looks very promising. I have added dome comments from my first read.
5fe6d41
to
1a8681c
Compare
1a8681c
to
d5e281b
Compare
3ed3cad
to
e7fe278
Compare
Regarding the layout in the mapping GUI: |
I don't think multiple options per row are the issue here. The issue are the long description texts. Have a look on the narrow layout of the Traktor preferences, with up to 4 options per row. |
I agree, horizontal scrollbars are PITA. |
e7fe278
to
52ae9c4
Compare
I followed @JoergAtGithub 's advice and tried with tool-tip instead of sub-line descriptions. I believe this looks great. |
This looks much cleaner now! 🤩 But for the multi option per row, the allignment could be improved. There is much space between the label and the control belonging to the label. |
Like this? I just want to mention here that it is likely there will be very different options depending devices and mappings. Those settings are meant to cover the greater and be dynamically rendered; IMHO, there is no way we can have as great results as in the statically defined preferences as references :) |
IMHO this looks really good now! You might want to tune the vertical whitespaces to improve the look further (more height for the checkbox options, move the Deck-Labels a few pixels down). But such visual optimizations are beyond what is necessary. |
f1b9766
to
507d0bc
Compare
Great, if that works reliably! Though I suggest to make it depend on the width of In regards to the label/control arrangement: the are usually (always?) layed out horizontally => Label: [control] |
IMHO it looks much cleaner now with the vertical label arrangement and it's consistent with the layout of other preference pages (Waveform, Decks). |
I guess I could add an |
Would yo mind including the dummy mapping for testing here?
Hmm? |
It's in the PR description. I haven't updated the visual, but the XML mapping remains the same |
Yup, sounds good. Should default to horizontal (if absent) so the vertical layout is used only when required. We should keep an eye on it when reviewing mappings. |
I have added the On the 960px choice, I tried a few values, and that one seemed to be a great trade off, but can always be changed if you have a value you think we should stick to. Hopefully, everything looks good now from your perspectives. I will continue unit tests/docstring/guideline and will get started with the wiki update. FYI, the three mappings visible on the video are attached here (had to zip it because Github is sometime stupid), so hopefully it will help for review, or if you guys want to give it a spin locally. video.mp4 |
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.
Now finally everything works as expected! I approve this PR hereby!
@daschuer If your points are addressed too, please merge this PR!
can't wait to make the traktor S3 config use this feature! |
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.
I was not able to create settings for a midi device. Does it also work for midi?
Do you have a working sample?
No, this is written in the PR description. This PR is HID only. MIDI is planned for an follow-up PR. Working sample is in #12995 |
Yes, the original plan was to address this in a future PR. I will look at the leak asap. |
@daschuer I fixed that leak with the suggested patch. For transparency sake, I am not sure I will carry on with the MIDI work right away as this first part wasn't an easy ride and kinda shut my motivation down for the original plan suggested in the issue. I also don't own any MIDI device so it would be hard for me to work and test usability of the feature. Edit: I actually have a Terminal Mix 4 buried in a cupboard which might still work. Perhaps that can help me for testing, but I'd still like a well define direction for the final result. |
716c110
to
3e98e4e
Compare
Oh, you have debased everything :-( This is not permitted under review, because we now have technically 17 commit to review again. What was the reason for it? Can we find a shortcut for merging? I can confirm that the last commit fixes my last comment. |
When I rebased my branch, there was conflict with pretty much every commits (due to my recently merge PR touching on the same files), could it be the reason? Open to suggestions on shortcuts for merging? |
Next time, please just merge upstream/main in a single commit and resolve the conflict in a single merge commit. Did you do a brief test with you example mapping after the rebase? |
3e98e4e
to
d72e012
Compare
Apologies for the git mess - @daschuer I restored the previous commit with reflog and did an upstream merge instead just for peace of mind. |
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.
LGTM, Thank you for this often requested feature.
Wow, thanks to everyone involved, super excited to play with this soon! (If I find time, I'll see if I can update the MC7000 mapping 😄) |
Currently only HID is supported, not yet MIDI. |
Ah, that's a shame. But if I read the initial comment correctly, MIDI support is on the radar. |
It was, but unfortunately the plan for MIDI didn't get a consensus (see the original issue and Zulip |
I think we've the consensus, that we all want the same JavaScript setting API, that we've now for HID, for MIDI (and BULK) too. |
Linking for completeness, I assume MIDI mappings are now supported? |
Yes! |
@acolombier you previously pointed out
Did that ever manifest? Do we have some user-friendly documentation for this on the wiki? |
Yes this was documented here |
Ah, thank you. I will split it into a separate page thats easier to find. |
This PR addresses the feature request #8135
A few capabilities have been added from the original plan:
group
with alabel
attributerow
which will organize settings horizontally if the settings group box is wider than 960px, with support for horizontal (default) and and vertical label/widget dispositionOutstanding work:
Add MIDI supportThis will require refactoring the load function and IMHO it is wiser to do so in a dedicated PRAn update of the Wiki is currently in progress.
Current status
Simple
XML Config
Kontrol S4 MK3
This is an example of how this feature would be used for the support of the S4 Mk3, currently in progress in PR #11284
XML Config
Regarding the previous list, let me know if you would like to split the work in multiple PR to ease reviewing.