PICARD-9: Add support for user profiles #1851

rdswift · 2021-06-21T00:47:00Z

Summary

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other
Describe this change in 1-2 sentences: Add support for user profiles.

Problem

JIRA ticket (optional): PICARD-9

There has been a long-standing request to allow for different user settings profiles, with a quick and easy method for switching between profiles. The plan for implementing user profiles involves maintaining a stack of profiles that would be applied sequentially (for active profiles in the stack). See the discussion on the JIRA ticket.

Solution

Implement a new subclass of ConfigSection to use for user settings that applies the user profile stack automatically to all config.setting assignments and retrievals. Add new keys to store the user profiles list and settings (overrides) for each profile in a new config.profiles section. Add a new dialog accessed from the main screen menu to manage the user profiles and settings.

Action

Additional work to be completed when this is finalized:

Modify the File Naming Script Editor to block deleting a file naming script that is included in a profile's settings. See Check is naming script is used in a profile before deleting #1859
Update the documentation to explain the use of profiles and the profile editor. Much of this can be copied from @zas's comments on PICARD-9.

picard/config.py

picard/profile.py

picard/config.py

picard/profile.py

picard/config.py

picard/profile.py

picard/config.py

rdswift · 2021-06-24T15:55:16Z

I added a working version of the profile editor dialog, which I've been using for local testing. Once we firm up structures and such, I'll see about adding some tests as well.

EDIT: I suppose the dialog should have been in a separate PR, but I thought it might prove useful for any testing that you might want to do.

EDIT 2: I think the system now works as described by @zas in https://tickets.metabrainz.org/browse/PICARD-9

zas · 2021-06-24T21:26:29Z

I did a quick test, it works well.
I'm a bit bothered by the fact profile-modified options are stored in an option, rather than in their own config sub-section.
Also we'll need to handle config upgrades (when an option name is modified, we'll need to propagate the change to profiles).

That's a good improvement already. Good job!

rdswift · 2021-06-24T22:26:35Z

I'm a bit bothered by the fact profile-modified options are stored in an option, rather than in their own config sub-section.

It should be fairly easy to store the profile settings dictionaries by profile id under a new config.profiles section, or were you thinking about having separate ConfigSection objects for each profile?

rdswift · 2021-06-25T00:15:04Z

After moving the profile settings to a new config.profiles group, it seems that it broke all of the tests that use the config.setting, even though it seems to work perfectly when running Picard. The tests all seem to be failing because config.setting can't see config.profiles and fail with a KeyError. I suspect there's something that needs to be fixed in the PicardTestCase module, but I'm not finding it.

zas · 2021-06-25T06:53:31Z

After moving the profile settings to a new config.profiles group, it seems that it broke all of the tests that use the config.setting, even though it seems to work perfectly when running Picard. The tests all seem to be failing because config.setting can't see config.profiles and fail with a KeyError. I suspect there's something that needs to be fixed in the PicardTestCase module, but I'm not finding it.

I think that's due to the use of global config in SettingConfigSection.

picard/picard/config.py

Line 142 in 2ea04a4

self.__qt_config = config

Use self.__qt_config instead of config:

picard/picard/config.py

Line 150 in 2ea04a4

profiles = config.profiles[self.PROFILES_KEY]

picard/picard/config.py

Line 162 in 2ea04a4

profile_settings = config.profiles[self.SETTINGS_KEY][id]

picard/picard/config.py

Line 185 in 2ea04a4

all_settings = config.profiles[self.SETTINGS_KEY]

picard/picard/config.py

Line 187 in 2ea04a4

config.profiles[self.SETTINGS_KEY] = all_settings

rdswift · 2021-06-25T09:41:31Z

I think that's due to the use of global config in SettingConfigSection.

That was exactly the problem. THANK-YOU!

zas

Just (few more) stylistic remarks, but the patch is very good ;)

picard/ui/profileeditor.py

test/test_profiles.py

picard/ui/profileeditor.py

Named tuple profile settings

In `picard/ui/profileeditor.py`: - Remove unnecessary `deepcopy()`. - Use `set.difference()` for comparing key sets. - Add explanation for creating missing key in `get_settings_for_profile()` method. - Move `settings` declaration into `if` section where it is used. - Remove unnecessary `.keys()` call when creating set of profile IDs. - Use `set.difference()` for comparing setting sets. - Remove unnecessary `self.loading` check. - Add class constant for which QTreeWidgetItem column to use. In test/test_profiles.py: - Use keys defined in `SettingConfigSection` class.

zas

LGTM

picard/config.py

phw

Good work. This works really well, and I am especially glad that this was possible with rather small and clean changes to the config module itself.

From a user perspective I find the settings concept rather difficult to grasp, but it is very flexible. Before deep diving into the code itself I tried this functionality from a user's perspective. But I definitely had to read the documentation to fully understand it (even though I had kind of suspected how this works based on our previous discussions).

I have a few notes about the profile editor dialog itself:

The buttons should be placed in QDialogButtonBox and should be added with appropriate roles. This ensures the buttons render consistent with other dialogs and platform dependent.
I find the separate "Save" and "Make it so!" buttons really confusing and not easy to use. I'd favor having this behave like options themselves: Have a "Cancel" button and a single "Make it so!" button to save and close
I would consider moving the "New", "Copy" and "Delete" buttons below the widget, next to the ordering buttons. That would make it consistent with other widgets we use in options where the buttons to control a list of things (e.g. scripts) are next to the widget presenting this list. It would also make the main dialog buttons look less cluttered

I would also add a keyboard shortcut to open the profile editor. Ctrl+Shift+P maybe? That would allow the user to quickly change profiles without the menu.

For a future refinement to make the behavior of profiles more clear I think it would be beneficial to indicate in the profile editor which settings actually have distinct values. Maybe even show the value itself (which would probably require that we have a useful rendering depending on the option type, e.g. show BoolOption as "Yes" / "No" or something.

But we can do this in a follow up PR.

ui/profileeditor.ui

picard/ui/profileeditor.py

rdswift · 2021-06-28T15:26:11Z

I have a few notes about the profile editor dialog itself:

The buttons should be placed in QDialogButtonBox and should be added with appropriate roles. This ensures the buttons render consistent with other dialogs and platform dependent.

Most were in the buttonbox, except "Make It So!". Now are all in buttonboxes with the appropriate roles assigned.

I find the separate "Save" and "Make it so!" buttons really confusing and not easy to use. I'd favor having this behave like options themselves: Have a "Cancel" button and a single "Make it so!" button to save and close

I actually simplified it by removing the "Save" button and auto-save the changes locally (within the dialog) as they are made. Do we still need a "Reset" button?

I would consider moving the "New", "Copy" and "Delete" buttons below the widget, next to the ordering buttons. That would make it consistent with other widgets we use in options where the buttons to control a list of things (e.g. scripts) are next to the widget presenting this list. It would also make the main dialog buttons look less cluttered

Done. I agree that they look better there.

I would also add a keyboard shortcut to open the profile editor. Ctrl+Shift+P maybe? That would allow the user to quickly change profiles without the menu.

Done. At the same time I also added one for the file naming script editor to be consistent. I hope that's okay.

picard/ui/profileeditor.py

phw

The dialog now looks much clearer with re-arranged buttons.

LGTM

Create subclass of ConfigSection to support user profiles

405ad2a

zas requested changes Jun 21, 2021

View reviewed changes

picard/config.py Outdated Show resolved Hide resolved

picard/config.py Outdated Show resolved Hide resolved

picard/profile.py Outdated Show resolved Hide resolved

Refactor. Correct problem getting value from a profile.

eba6306

zas requested a review from phw June 21, 2021 15:45

Change variable and method names for clarification and add docstring

f30ea31

zas requested changes Jun 21, 2021

View reviewed changes

picard/config.py Outdated Show resolved Hide resolved

picard/profile.py Outdated Show resolved Hide resolved

picard/profile.py Outdated Show resolved Hide resolved

rdswift added 2 commits June 21, 2021 14:54

Refactor and use sets to combine settings

21c7804

Store profile list and profile settings separately

ea4fec1

zas requested changes Jun 23, 2021

View reviewed changes

picard/config.py Outdated Show resolved Hide resolved

picard/profile.py Outdated Show resolved Hide resolved

picard/config.py Outdated Show resolved Hide resolved

picard/config.py Outdated Show resolved Hide resolved

picard/config.py Outdated Show resolved Hide resolved

Clean up saving profile settings updates. Reduce duplication.

e9a90cf

zas requested changes Jun 24, 2021

View reviewed changes

picard/config.py Outdated Show resolved Hide resolved

rdswift added 2 commits June 24, 2021 07:29

Make keys into class constants

16e6a94

Add user profile editor dialog (WIP)

583058e

rdswift force-pushed the add_user_profiles branch 2 times, most recently from c2e5a8d to 2b0f0f1 Compare June 24, 2021 21:10

Add tests

20ca5ec

rdswift force-pushed the add_user_profiles branch from 2b0f0f1 to 20ca5ec Compare June 24, 2021 21:21

Retain selections tree expanded state between selected profiles.

7284904

rdswift force-pushed the add_user_profiles branch from 00f1cc2 to 6ea6136 Compare June 24, 2021 23:55

Move profiles to config.profiles. Need to resolve failing tests.

2ea04a4

rdswift force-pushed the add_user_profiles branch from 6ea6136 to 2ea04a4 Compare June 25, 2021 03:26

Use self.__qt_config when checking profiles and profile settings.

68b39ec

rdswift changed the title ~~Create subclass of ConfigSection to support user profiles~~ PICARD-9: Add support for user profiles Jun 25, 2021

rdswift marked this pull request as ready for review June 25, 2021 15:37

rdswift and others added 2 commits June 25, 2021 17:37

Fix crash after deleting the last profile in the list

5af5024

Convert profile settings to named tuple

737c746

zas requested changes Jun 26, 2021

View reviewed changes

rdswift added 2 commits June 26, 2021 08:32

Merge pull request #9 from zas/namedtuplesetting

7c1a508

Named tuple profile settings

zas previously approved these changes Jun 26, 2021

View reviewed changes

Correct the url to the help page

e01b978

rdswift dismissed zas’s stale review via e01b978 June 26, 2021 18:35

zas previously approved these changes Jun 26, 2021

View reviewed changes

zas reviewed Jun 27, 2021

View reviewed changes

picard/config.py Outdated Show resolved Hide resolved

Update settings for id directly

4f10a47

rdswift dismissed zas’s stale review via 4f10a47 June 27, 2021 14:01

zas previously approved these changes Jun 27, 2021

View reviewed changes

phw requested changes Jun 28, 2021

View reviewed changes

ui/profileeditor.ui Outdated Show resolved Hide resolved

picard/ui/profileeditor.py Outdated Show resolved Hide resolved

rdswift added 3 commits June 28, 2021 07:41

Add keyboard shortcut to MainWindow

8c967ef

Revise and rearrange buttons. Auto-save changes within dialog.

d9934ad

Rename ProfileEditorPage to ProfileEditorDialog

85ce4b1

rdswift dismissed zas’s stale review via 85ce4b1 June 28, 2021 15:17

zas reviewed Jun 28, 2021

View reviewed changes

picard/ui/profileeditor.py Show resolved Hide resolved

Add comment explaining why deepcopy() is required.

22d91a4

zas previously approved these changes Jun 28, 2021

View reviewed changes

rdswift dismissed zas’s stale review via 22d91a4 June 28, 2021 17:22

zas previously approved these changes Jun 28, 2021

View reviewed changes

Fix crash when deleting the last profile from the list.

673b7a7

rdswift dismissed zas’s stale review via 673b7a7 June 28, 2021 17:56

zas approved these changes Jun 28, 2021

View reviewed changes

phw approved these changes Jun 28, 2021

View reviewed changes

phw merged commit 75f8d93 into metabrainz:master Jun 28, 2021

rdswift deleted the add_user_profiles branch June 28, 2021 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PICARD-9: Add support for user profiles #1851

PICARD-9: Add support for user profiles #1851

rdswift commented Jun 21, 2021 •

edited

Loading

rdswift commented Jun 24, 2021 •

edited

Loading

zas commented Jun 24, 2021

rdswift commented Jun 24, 2021

rdswift commented Jun 25, 2021

zas commented Jun 25, 2021

rdswift commented Jun 25, 2021

zas left a comment

zas left a comment

phw left a comment

rdswift commented Jun 28, 2021

phw left a comment

PICARD-9: Add support for user profiles #1851

PICARD-9: Add support for user profiles #1851

Conversation

rdswift commented Jun 21, 2021 • edited Loading

Summary

Problem

Solution

Action

rdswift commented Jun 24, 2021 • edited Loading

zas commented Jun 24, 2021

rdswift commented Jun 24, 2021

rdswift commented Jun 25, 2021

zas commented Jun 25, 2021

rdswift commented Jun 25, 2021

zas left a comment

Choose a reason for hiding this comment

zas left a comment

Choose a reason for hiding this comment

phw left a comment

Choose a reason for hiding this comment

rdswift commented Jun 28, 2021

phw left a comment

Choose a reason for hiding this comment

rdswift commented Jun 21, 2021 •

edited

Loading

rdswift commented Jun 24, 2021 •

edited

Loading