-
-
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
Controllers: Don't needlessly copy presets into user directory #2569
Conversation
The copying and duplicate devices in the list boxes with the same name cause so much confusion, even for me. This would be a huge improvement! |
I guess this should to be included in 2.3.0? |
I didn't built this, yet. |
I think you can just edit the file and save it in |
I wasn't sure since @ywwg was keen on reducing the scope of 2.3. If nobody encounters any problems during testing I'd like to get this merged ASAP though ;-) |
Thank you for working on this obnoxious UX issue.
Ehhh maybe we shouldn't rush this. We have enough on our plate for 2.3. |
I'd propose this for 2.3 (not as a blocker though) because it makes controller mapping development much easier. Right now, you can't really develop controller mappings from the git source tree, because Mixxx will always copy them to the user directory. On the wiki, we recommend symlinking You can still develop from the user directory, but then there is no way to use pre-commit hooks. |
Awesome! I recently started working on a component based mapping for the Pioneer DDJ-SX2. I rebased your PR onto my working branch and everything is going so much smoother now. |
Great! I hope we can get this reviewed and merged before we branch off 2.3. This makes it possible to develop mappings inside the git repo and use the pre-commit hooks to ensure proper code style. |
The Controller page now has a working (!) apply button. Before, mappings would be applied instantly when selecting it on the combobox. Please test. |
m_ui.chkEnabledDevice->setEnabled(false); | ||
} else { | ||
// User picked a preset | ||
m_ui.chkEnabledDevice->setEnabled(true); |
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.
add
m_ui.chkEnabledDevice->setChecked(true);
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.
Hmm, so that selecting a preset means activating the controller? In that case we can remove the enabled checkbox completely and just use "no preset" for disabled controllers.
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.
Is there a use case for keeping a mapping selected but disabling the controller? The only time I have needed to disable and re-enable a controller was when the USB cable came unplugged during a set.
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.
Wait, this works? I always restarted mixxx in that case.
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.
Indeed, it does. It's not obvious at all though.
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.
Hmm, thats a good idea but maybe we should leave it as-is for 2.3. Users that upgrade from 2.2 will have a preset defined for all controllers because Mixxx was writing custom presets everywhere. This means that all controllers would be active.
During development I made a little mistake that also activated all controllers - this made both my keyboard and my mouse unresponsive since they are also HID devices. I had to restart X11 via SSH to make my computer usable again 😕
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.
Oof, yeah. I was wondering if those legacy files laying in the user folders would have consequences. They sure would if we automatically enabled them. I still think it would help to add m_ui.chkEnabledDevice->setChecked(true);
here in slotPresetSelected so users don't have to think about that checkbox.
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 think we can do this without automatically activating those legacy presets in the user folder. Here is what a fresh mixxx.cfg looks like from master after activating one of my Xone K2s:
Babyface_Pro_(70785713)_MIDI_1 /home/be/.mixxx/controllers/Babyface_Pro_(70785713)_MIDI_1.midi.xml
Babyface_Pro_(70785713)_MIDI_2 /home/be/.mixxx/controllers/Babyface_Pro_(70785713)_MIDI_2.midi.xml
XONE:K2_MIDI_1 /home/be/.mixxx/controllers/Allen and Heath Xone K2.midi.xml
[Controller]
XONE:K2_MIDI_1 1
I propose still relying on the [Controller]
ConfigKey to determine which controllers to enable on Mixxx startup. Then automatically enable the ConfigKey for a device when a preset is applied. This would allow us to get rid of the Enabled checkbox. I would still want a Reconnect button.
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.
Ok, this sounds like it could work. However, let's do this in a separate PR, this one is already big enough and I have the impression that PRs always take a few days without any new commits before they get merged here 😉
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.
Okay
The buttons to add and remove entries from the Input Mapping and Output Mapping tables have disappeared?? |
No, I didn't touch them and they are still there. |
Is that a scrollbar on the right side of the window? |
Ah, they are hidden by the vertical scrollbar. How did that happen? |
I'm doing a git bisect... |
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.
Some more comments
After some more testing, I can sometimes reproduce the vertical scrollbar issue with the Inputs/Outputs tabs with master, so it is not a regression introduced by this branch and there is no need to fix it here. I am not sure why it appears sometimes but other times it does not. Maybe @ronso0 has some idea about it? |
This allows editing user controller scripts (e.g. downloaded from the forum) as well.
There's still a lot that could be done to improve handling of controllers in Mixxx, but I consider this PR ready to merge. Please test. |
Great work! Thanks for cleaning up this mess :) |
I still have a question. So what is the workflow if I want to modify the mapping? Do it in the mixxx's controllers folder? |
As soon as you change a control in Pref > Controllers > YourController > In/Output mapppings the default mapping XML is copied to the controllers folder. (js mapping still has to be copied there manually) If you want change a mapping for a PR you can simply work in the git res/controllers folder. |
Based on PR: #2567For XML, we now check if the preset has been edited and only save it to the user directory in that case. Since we don't allow to edit JavaScript from within Mixxx, these are never copied anymore.
Now I can actually use Mixxx with an empty
~/.mixxx/controllers/
directory 🎉