Adding an option to hide In/Start Columns in Playlist Detail View

[dderjoel]

In my workflow I am not trimming the files that I add to the playlist. I just add all my clips to the playlist and then drag them into the timeline as needed. This results in those columns just wasting real estate because all just display '00:00:00:00' and pushing the 'Date' column to the side, that I do need, and need to vertical-scroll because of the In/Start columns.

This PR adds an option to the Playlist-Menu that allows the user to hide the In/Start columns.

I've tried to add the setting to the Settings class for it to persist, but I am missing where it is being loaded to 'check' the persisted selection, and trigger the action. (Hence this PR is in a DRAFT).

In the default Settings, nothing changes.

Please let me know where I need to look for the correct way to use the Settings-persistence.

[ddennedy]

I probably will not merge this because I don’t want to start letting others litter the UI with their personal preferences (not many people have asked specifically for this). Also, you add this, and people will ask for options to hide any column as well as add more columns and click header to sort like a full-on grid view. So, this is either do the bigger project (minus adding unspecified columns), or remove the UI option and use the Settings object only (manual config change to use it).

[bmatherly]

I agree that this implementation is too specific. I would support a more generalized approach that allows per-column hiding. We have a couple of examples in the UI where that is supported.

For example, the Markers Panel has per-column configuration:

Also, a little different but still related, we have configuration for visibility of each audio loudness metric in the Audio Loudness Scope:

I would probably support a patch for the Playlist that mimics the implementation in the markers panel.

[dderjoel]

@ddennedy 100% understand and also agree. This was kinda just the first spot where I thought it would make sense to have that setting.

@bmatherly This is actually what I wanted to implement, too. Should've asked here earlier for hints, I guess. I'd then try to get the Markers Column Hide/Show style implemented for the Playlist.

[dderjoel]

Something along those lines? I've updated my master branch. I am unsure about translations, if they would be caught already of if that would need some additional work.

[ddennedy]

OK, I tried it out, and it is good, and I agree to merge it. Don't worry about translations; I coordinate that. We run the code through astyle 3.1 for code formatting, and it suggested these line breaks and indentation. Then, you can take it out of draft for review and merge.

--- src/docks/playlistdock.cpp
+++ src/docks/playlistdock.cpp
@@ -1383,8 +1383,11 @@ void PlaylistDock::updateViewMode()
         ui->tableView->setModel(&m_model);

-        ui->tableView->setColumnHidden(PlaylistModel::COLUMN_THUMBNAIL, !Settings.playlistShowColumn("thumbnails"));
-        ui->tableView->setColumnHidden(PlaylistModel::COLUMN_RESOURCE, !Settings.playlistShowColumn("clip"));
+        ui->tableView->setColumnHidden(PlaylistModel::COLUMN_THUMBNAIL,
+                                       !Settings.playlistShowColumn("thumbnails"));
+        ui->tableView->setColumnHidden(PlaylistModel::COLUMN_RESOURCE,
+                                       !Settings.playlistShowColumn("clip"));
         ui->tableView->setColumnHidden(PlaylistModel::COLUMN_IN, !Settings.playlistShowColumn("in"));
-        ui->tableView->setColumnHidden(PlaylistModel::COLUMN_DURATION, !Settings.playlistShowColumn("duration"));
+        ui->tableView->setColumnHidden(PlaylistModel::COLUMN_DURATION,
+                                       !Settings.playlistShowColumn("duration"));
         ui->tableView->setColumnHidden(PlaylistModel::COLUMN_START, !Settings.playlistShowColumn("start"));
         ui->tableView->setColumnHidden(PlaylistModel::COLUMN_DATE, !Settings.playlistShowColumn("date"));

[ddennedy]

Brian, I think this new submenu should be moved to Settings > Playlist. Do you agree? If feels more like a setting than an action to me.

[bmatherly]

Brian, I think this new submenu should be moved to Settings > Playlist. Do you agree? If feels more like a setting than an action to me.

I agree that it is a setting. I do not feel strongly about where it goes. But it would be nice to be consistent across the Playlist, Markers and (future) Subtitles panels. If we don't add submenus for all those panels in the Settings menu, then it would be nice if they are all in the same panel hamburger menu so that it is familiar for users.

[bmatherly]

I have no comments on the code. LGTM.

[ddennedy]

Oh yeah, that consistency of placement is a good point. So, this is currently more consistent, but in the future we could move all of these sub-menus to Settings. A lot of programs place this as the context menu for the column header (typical but less discoverable; so, mainly for power users).

[ddennedy]

Thanks @dderjoel! Sorry I squash merged, which makes managing your fork of master more difficult. I usually check if the source branch is master and if so do a merge commit to make your life easier, but I guess I fell out of habit since that is rare nowadays.

[dderjoel]

no worries @ddennedy. Squashing makes sense because there was also the revert and the useless merge-commit in there. Thanks for the support and quick merging!