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.

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

Merged
merged 57 commits into from Apr 9, 2020

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Mar 19, 2020

Based on PR: #2567

For 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 🎉

@uklotzde
Copy link
Contributor

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!

@uklotzde
Copy link
Contributor

I guess this should to be included in 2.3.0?

@ronso0
Copy link
Member

ronso0 commented Mar 19, 2020

I didn't built this, yet.
How's the workflow now if I want to alter the js file only?

@Holzhaus
Copy link
Member Author

I didn't built this, yet.
How's the workflow now if I want to alter the js file only?

I think you can just edit the file and save it in ~/.mixxx/controllers/.

@Holzhaus
Copy link
Member Author

I guess this should to be included in 2.3.0?

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 ;-)

@Be-ing
Copy link
Member

Be-ing commented Mar 23, 2020

Thank you for working on this obnoxious UX issue.

I guess this should to be included in 2.3.0?

Ehhh maybe we shouldn't rush this. We have enough on our plate for 2.3.

@Be-ing Be-ing added this to the 2.4.0 milestone Mar 23, 2020
@Holzhaus Holzhaus removed this from the 2.4.0 milestone Mar 25, 2020
@Holzhaus
Copy link
Member Author

Holzhaus commented Mar 25, 2020

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 ~/.mixxx/controllers to <gitroot>/res/controllers, but the XML file will be copied anyway, so you need to change the controller mapping every time you restart mixxx.

You can still develop from the user directory, but then there is no way to use pre-commit hooks.

@Holzhaus Holzhaus added this to In progress in 2.3 release via automation Apr 2, 2020
@Holzhaus Holzhaus added this to the 2.3.0 milestone Apr 2, 2020
@Holzhaus Holzhaus moved this from In progress to In Review in 2.3 release Apr 2, 2020
@goddisignz
Copy link
Contributor

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.
😎

@Holzhaus
Copy link
Member Author

Holzhaus commented Apr 4, 2020

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.

@Holzhaus
Copy link
Member Author

Holzhaus commented Apr 8, 2020

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);
Copy link
Member

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);

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

@Holzhaus Holzhaus Apr 8, 2020

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 😕

Copy link
Member

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.

Copy link
Member

@Be-ing Be-ing Apr 8, 2020

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.

Copy link
Member Author

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 😉

Copy link
Member

Choose a reason for hiding this comment

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

Okay

@Be-ing
Copy link
Member

Be-ing commented Apr 8, 2020

The buttons to add and remove entries from the Input Mapping and Output Mapping tables have disappeared??

@Holzhaus
Copy link
Member Author

Holzhaus commented Apr 8, 2020

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.

@Be-ing
Copy link
Member

Be-ing commented Apr 8, 2020

master:
Screenshot from 2020-04-08 15-08-15

this branch:
Screenshot from 2020-04-08 15-31-28

Add, Remove, and Clear All buttons have disappeared

@Holzhaus
Copy link
Member Author

Holzhaus commented Apr 8, 2020

Is that a scrollbar on the right side of the window?

@Be-ing
Copy link
Member

Be-ing commented Apr 8, 2020

Ah, they are hidden by the vertical scrollbar. How did that happen?

@Holzhaus
Copy link
Member Author

Holzhaus commented Apr 8, 2020

I have no idea. I didn't touch the UI files. I just built this on my laptop, this is how it looks for me:

2020-04-08-22-39-52_1366x768

@Be-ing
Copy link
Member

Be-ing commented Apr 8, 2020

I'm doing a git bisect...

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Some more comments

src/controllers/controllerinputmappingtablemodel.h Outdated Show resolved Hide resolved
src/controllers/controllermanager.cpp Outdated Show resolved Hide resolved
src/controllers/controllermanager.cpp Show resolved Hide resolved
@Be-ing
Copy link
Member

Be-ing commented Apr 8, 2020

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?

@Holzhaus Holzhaus requested a review from ronso0 April 9, 2020 09:25
@Holzhaus
Copy link
Member Author

Holzhaus commented Apr 9, 2020

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.

@Be-ing Be-ing merged commit fec775d into mixxxdj:master Apr 9, 2020
2.3 release automation moved this from In Review to Done Apr 9, 2020
@Be-ing
Copy link
Member

Be-ing commented Apr 9, 2020

Great work! Thanks for cleaning up this mess :)

@ghost
Copy link

ghost commented Apr 16, 2020

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?

@ronso0
Copy link
Member

ronso0 commented Apr 16, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.3 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants