[RFC/WIP] Optional per-playlist column settings #698
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
is_column_selected() and on_column_item_activate() within the __register_playlist_columns_menuitems() are unused. The actual callbacks used with ColumnMenuItem instances are the class-provided is_selected() and on_item_activate().
…ings within PlaylistView The playlist columns and their sizing settings (resizable flag and individual column sizes) are now managed by PlaylistView instead of direct access to settings. This will allow us to implement the option to switch between global column settings and individual, per playlist column settings.
…ettings For now, we will piggyback on the existing playlist save/restore infrastructure for persistent global/custom playlist column settings.
…t columns switch The columns context menu now has a "Global playlist columns" checkbox, which allows user to switch between using global and playlist-specific columns settings.
sjohannes
reviewed
Apr 23, 2020
| 'global_playlist_columns', | ||
| 'playlist_columns', | ||
| 'resizable_cols', | ||
| 'playlist_column_widths', |
There was a problem hiding this comment.
What do you think about grouping these under sort of a namespace, e.g. custom_columns/...?
| resizable_cols = property(lambda self: self.__resizable_cols, _set_resizable_cols) | ||
| playlist_column_widths = property( | ||
| lambda self: self.__playlist_column_widths, _set_playlist_column_widths | ||
| ) |
There was a problem hiding this comment.
I think this would look nicer with the @property and @....setter pattern.
| global_columns_item = menu.check_menu_item( | ||
| 'global-columns', | ||
| after, | ||
| _('Global columns settings'), |
There was a problem hiding this comment.
Hm, I'm not sure about this. I'd probably prefer this be the reverse ("custom columns settings"), but I still don't find that clear enough.
|
Regarding the design, I'm fine with it; seems the most reasonable option. It's probably not ideal having a GUI option leaking to what is part of "core", but it's not a big deal. But I think grouping the settings (as mentioned in my review comment) would make it better. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements an option to detach the playlist's column settings from the global state, and use private playlist-specific settings instead. The switch is added at the top of the columns context menu.
My primary use case is having the default columns (#, Title, Album, Artist, Length) for album-based playlists, and having only Title and Artist for custom-made playlists.
This involves reworking of how the column settings are managed; instead of direct access to the properties in the settings handle, the settings are now managed by the
PlaylistViewobject in the GUI/widgets layer. However, to minimize the changes on the playlist save/restore codepath, the settings are also mirrored to the underlyingxl.playlist.Playlistfor storage purposes. I'm not entirely happy with this design, but it seems less intrusive than either trying to serialize/deserialize the column settings at the GUI layer, or trying to push the settings management all the way down to thexl.playlist.Playlist.