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

Improve UX in controller mapping editing workflow #3278

Merged
merged 1 commit into from
Nov 29, 2020

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 9, 2020

This aims to improve the UX both when editing a built-in controller mapping and when continously working on a user mapping.

The reason for this is that I had some headaches with the new workflow which always adds the " (edited)" suffix automatically.
After having saved an edited built-in preset to the user preset folder Mixxx would silently save further changes to MyController (edited).midi.xml (which is okay). But after manually renaming the preset file and loading that user preset Mixxx would not save further changes to that file but create yet another ... (edited) file. This happening unnoticed can create unclear situations in the user preset folder, for example when editing the xml file or when trying to transfer an repeatedly edited preset.

A simple solution would be showing a popup that tells the user about the new file location.

A little more advanced approach ist to ask the user for a file name for saving the preset in the user preset folder, as well as for display name.


I abandoned the more detailed workflow with a QFieInputDialog I proposed earlier since it may lead to confusing situations, like saving a preset outside the user preset folder.

Edit a built-in mapping, click Apply
This is done quickly if you don't want to change the default name: click Save, click Okay

  • Save As ... QFileInputDialog to choose the file location in the user preset folder, suggest current name
    = don't add " (edited)" suffix, user presets already have a distinct icon and an own section in the preset dropbox
  • Choose preset display name simple QInputDialog, suggest current display name. If file name was modified suggest the new file name (strip xml suffix)

Edit a user mapping, click Apply
This is also done quickly if you want to stick with the current name.

QDialog to choose

  • Overwrite
    • save changes to current preset
    • optionally tick checkbox to always overwrite in current session
  • Save As ...
    • same as editing a built-in mapping

@ronso0 ronso0 added this to the 2.3.0 milestone Nov 9, 2020
@ronso0
Copy link
Member Author

ronso0 commented Nov 9, 2020

still WIP, doesn't built yet.

@daschuer
Copy link
Member

It looks like you have accidentally added BACK files. Can you amend and force push to revert it?

@ronso0
Copy link
Member Author

ronso0 commented Nov 12, 2020

yes, it's still WIP, I'll let you know when it can be tested. I had the descriptio written and didn't want to open a bug

@ronso0
Copy link
Member Author

ronso0 commented Nov 12, 2020

If I continue to polish the described workflow this will introduce new translation strings for the additional dialogs.
Is it still okay to make an exception for this?
Alternatively I propose to change the current implementation to not add " (edited)" to user presets since I expect there will be other users being confused by this, especially when editing downloaded mappings and when developing own mappings.
What do you think @Holzhaus @daschuer ?

@Holzhaus
Copy link
Member

Did we even push all new translations from 2.3 to Transifex yet? If not, it'd say it's definitely okay to add new strings.

@ronso0
Copy link
Member Author

ronso0 commented Nov 13, 2020

I got this into a working state incl. the option to 'Always overwrite during this session'. But working on this reveals some issues, and not sure if I should follow this route for 2.3:
To make saving a preset work I have to manually work around issues with QFileDialog. For example we must not allow preset file names consisting of spaces only because that would show 'No preset' in the combobox and a greyed out checkbox (it's checked though), also the 'whitespace preset' cannot be selected in the combobox.
Also we can't ensure presets are saved in the /controllers folder only but related script files and xml script file links are still in/pointing to /controllers.

for the current implememtation I suggest we either go with

a) not add " (edited)" to user presets

b) use a simple QInputDialog for both the file name and the display name to ensure in c++ that presets are saved to the controllers folder only

what do you think?

@ronso0
Copy link
Member Author

ronso0 commented Nov 16, 2020

@Holzhaus What's you're opinion?

for the current implememtation I suggest we either go with

a) not add " (edited)" to user presets

b) use a simple QInputDialog for both the file name and the display name to ensure in c++ that presets are saved to the controllers folder only

@Holzhaus
Copy link
Member

Sounds good.

@Holzhaus
Copy link
Member

To make saving a preset work I have to manually work around issues with QFileDialog. For example we must not allow preset file names consisting of spaces only because that would show 'No preset' in the combobox and a greyed out checkbox (it's checked though), also the 'whitespace preset' cannot be selected in the combobox.

That sounds like bug we should fix.

@ronso0
Copy link
Member Author

ronso0 commented Nov 16, 2020

That sounds like bug we should fix.

Yep, but I didn't yet figure out where that could happen. Maybe when presets are enumerated? since it affects the combobox as well as the preset short name.

a) not add " (edited)" to user presets

b) use a simple QInputDialog for both the file name and the display name to ensure in c++ that presets are saved to the controllers folder only

So I'll work on b) to not revert all of your work ;)

@ronso0 ronso0 marked this pull request as ready for review November 20, 2020 13:30
@ronso0
Copy link
Member Author

ronso0 commented Nov 20, 2020

@Holzhaus This is ready, please take a look.
Now the user is asked to enter

  • a file name for saving the preset in the user preset folder (.midi.xml is added)
  • a display name for the preset description

After closing the dialogs surrounding whitespaces and all characters except alphanumerical characters and _-+() are removed.

@ronso0
Copy link
Member Author

ronso0 commented Nov 24, 2020

ping @Holzhaus

@Holzhaus
Copy link
Member

@ronso0 Yeah, I've seen it, sorry! After working on windows CI my motivation to review PRs vanished. Since that this is off the table now, I'll have a look tomorrow.

@Holzhaus Holzhaus self-requested a review November 25, 2020 00:49
@Holzhaus
Copy link
Member

Thank you. One issue I noticed: If I run the MIDI mapping assistant while the controller is disabled (the default if I attach a new controller), the MIDI assistant doesn't receive messages. We either need to disable the learning assistant button when the controller is not enabled or we auto-enable the controller when starting the MIDI assistant.

Another minor issue: It would be helpful if we listed the XML file in the Scripts: section, too (we should rename it to Files:). But that doesn't need to happen in this PR.

@ronso0
Copy link
Member Author

ronso0 commented Nov 29, 2020

Another minor issue: It would be helpful if we listed the XML file in the Scripts: section, too (we should rename it to Files:). But that doesn't need to happen in this PR.

Thought about that, too. Actually we have the User Preset Folde shortcut, but for completeness an xml link would be good.

If I run the MIDI mapping assistant while the controller is disabled (the default if I attach a new controller), the MIDI assistant doesn't receive messages. We either need to disable the learning assistant button when the controller is not enabled or we auto-enable the controller when starting the MIDI assistant.

Is that a regression from thi sPR?

@ronso0
Copy link
Member Author

ronso0 commented Nov 29, 2020

If I run the MIDI mapping assistant while the controller is disabled (the default if I attach a new controller), the MIDI assistant doesn't receive messages. We either need to disable the learning assistant button when the controller is not enabled or we auto-enable the controller when starting the MIDI assistant.

Is that a regression from thi sPR?

  • I can start the MIDI Wizard
  • I can select actions from the combobox and the menu
  • I can also click in the skin to pikc controls
  • I can edit the input table manually
  • Apply asks for a name, the preset is properly saved to user dir and loaded in DlgMidi

All that was tested with dev mode and MIDI through port0.

@Holzhaus
Copy link
Member

Yes, all of that works. But after picking a control, the MIDI assistant doesn't pick up the MIDI messages when turning a knob or pressing a button on my controller. I guess because the controller is not enabled (i.e. the MIDI port is not open).

It's not a regression from the PR, just wanted to point that out.

@ronso0
Copy link
Member Author

ronso0 commented Nov 29, 2020

ah okay

@ronso0
Copy link
Member Author

ronso0 commented Nov 29, 2020

so this here is ready?

@Holzhaus
Copy link
Member

Yes.

@Holzhaus Holzhaus merged commit 754c0b5 into mixxxdj:2.3 Nov 29, 2020
@ronso0 ronso0 deleted the midi-export branch November 29, 2020 20:36
DlgPrefController::DlgPrefController(QWidget* parent, Controller* controller,
ControllerManager* controllerManager,
UserSettingsPointer pConfig)
const QString kPresetExt(".midi.xml");
Copy link
Member

Choose a reason for hiding this comment

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

Oops, this should go into an anonymous namespace.

Copy link
Member

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.

3 participants