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

Convert QML Menus to QWidgets and QActions #1299

Merged
merged 5 commits into from
Aug 5, 2022

Conversation

bmatherly
Copy link
Member

This is a work in progress submitted for review on the approach before I commit to converting all the toolbars. So far I have converted the Timeline dock toolbar, Timeline main menu and Timeline right click context menu. Every action that can be performed from a timeline toolbar or menu has been converted to a QAction which will later be indexed and managed to allow the user to change default keyboard shortcuts. I replicated the timeline toolbar as closely as possible to the QML based implementation. The menus changed slightly. In particular, the main menu has many new entries to account for shortcuts that did not have corresponding menu entries. I am interested in feedback about the names and organization of the menus. I also moved all timeline related settings into the top level Settings menu.

New Timeline and Hamburger Menu:
image

New Right-Click Context Menu:
image

New Timeline section in the settings menu:
image

@ddennedy
Copy link
Member

ddennedy commented Jul 30, 2022

Blend/Unblend Selected Track is new, right?

  • I think Record Audio by itself not in a sub-menu is awkward. Probably put it in Edit until something better comes along.
  • We need to do something about removing a timeline blank. Can this not be detected to show a different menu?
  • Since we no longer need to worry about the QQuickWidget viewport confining the menus, I think we can remove some submenus, in particular More on the clip.
  • There is too much redundant Cut, Copy, and Paste actions: Remove them from the Timeline menu since they are in the clip context menu. To be congruent, I think Ripple Delete, Lift, and Replace should be removed from Timeline > Edit as well.
  • Change "Merge" to "Rejoin".
  • Move Playlist toolbar to bottom of view.
  • Remove Select Clip 10 ("[Warning] <> QAction::event: Ambiguous shortcut overload: 0") That shortcut is now for Zoom Timeline to Fit*.

@bmatherly
Copy link
Member Author

Thanks for your review

Blend/Unblend Selected Track is new, right?

It has been in the code for a while. It is not in the online shortcut reference. I am happy to remove it if it is not needed.
https://github.com/mltframework/shotcut/blob/master/src/mainwindow.cpp#L2249

I think Record Audio by itself not in a sub-menu is awkward. Probably put it in Edit until something better comes along.

Done

We need to do something about removing a timeline blank. Can this not be detected to show a different menu?

I have implemented a solution for this. It is now possible to select a blank clip when right clicking on it. This allows the context menu to popup and the only action enabled is "Ripple Delete". Blanks and Clip can not be selected at the same time. Multiple blanks can not be selected at the same time. I think this is more intuitive than the previous behavior because there is a visible selection indicator (red outline).

Since we no longer need to worry about the QQuickWidget viewport confining the menus, I think we can remove some submenus, in particular More on the clip.

Done. Let me know if you see any more. They are easy to move around.

There is too much redundant Cut, Copy, and Paste actions: Remove them from the Timeline menu since they are in the clip context menu. To be congruent, I think Ripple Delete, Lift, and Replace should be removed from Timeline > Edit as well.

Done

Change "Merge" to "Rejoin".

Done

Move Playlist toolbar to bottom of view.

Done

Remove Select Clip 10 ("[Warning] <> QAction::event: Ambiguous shortcut overload: 0") That shortcut is now for Zoom Timeline to Fit*.

Done

@ddennedy
Copy link
Member

ddennedy commented Jul 31, 2022

  • The enable state of the Overwrite action is incorrect. It seems to depend on the Project player being active, but that is wrong. I should be able to trim a shot in Source and overwrite at the playhead.
  • Select Track Above and Select Track Below incorrectly use the term "select" because that is not what they do (I realize the reference doc is wrong). Tracks have independent "current" and "selected" states. These actions change the current state and do not select the track. I do not think we have any shortcuts currently to select a track. We can add an action to select the current track later, and these should be renamed like Set Current Track Above etc. unless you think of a better wording. It is OK to leave these in the Selection submenu since it is similar to selection if not exactly the same.
  • Move Select Clip Under Playhead upward to below Select Clip Below.
  • The C and V shortcuts are missing. They are aliases for Copy and Paste.
  • The Paste action seems to be dependent upon the clipboard being not empty. However, it is supposed to the the Source player in that case. See TimelineDock::insert().
  • In Jobs panel move the menu button to the left.
  • In Markers panel move the menu button to the left.
  • In Properties panel move the menu button to the left.

@bmatherly
Copy link
Member Author

The enable state of the Overwrite action is incorrect. It seems to depend on the Project player being active, but that is wrong. I should be able to trim a shot in Source and overwrite at the playhead.

Done. I removed disabling the action. I incorrectly thought Overwrite would occur on the selected clip.

Select Track Above and Select Track Below incorrectly use the term "select" because that is not what they do (I realize the reference doc is wrong). Tracks have independent "current" and "selected" states. These actions change the current state and do not select the track. I do not think we have any shortcuts currently to select a track. We can add an action to select the current track later, and these should be renamed like Set Current Track Above etc. unless you think of a better wording. It is OK to leave these in the Selection submenu since it is similar to selection if not exactly the same.

Done

Move Select Clip Under Playhead upward to below Select Clip Below.

Done

The C and V shortcuts are missing. They are aliases for Copy and Paste.

Done. After looking more closely, I see that Paste calls Insert which will use either the clipboard or the source player. Will this confuse people? I wonder if we need two actions:

  • Paste - only insert from the clipboard (CTRL-V)
  • Insert - only insert from the source player (V)

Alternately, we could change the text from "Paste" to "Paste/Insert".

The Paste action seems to be dependent upon the clipboard being not empty. However, it is supposed to the the Source player in that case. See TimelineDock::insert().

Done. But see my previous comment.

In Jobs panel move the menu button to the left.
In Markers panel move the menu button to the left.
In Properties panel move the menu button to the left.

Done

FYI: There will be more PRs to come after this one. I aspire to convert all the docks to eventually use the new DockToolBar widget for visual consistency. Also, there are still more actions that should be converted. Finally, I will create a singleton class to manage all the actions so that it can be responsible for providing user configuration of shortcuts. My main goals for this PR are to remove the QML menus and to get general agreement on the patterns being used.

Thanks for your review.

@ddennedy
Copy link
Member

ddennedy commented Aug 2, 2022

Sorry that I told you to remove Paste from the timeline menu. That does belong in its Edit submenu.
Both shortcuts for Paste/Insert are still not working with nothing in the clipboard and something in the Source player. I do not want to separate them at this time. It has been like this for awhile.

@ddennedy
Copy link
Member

ddennedy commented Aug 3, 2022

FYI after you add a class to list all actions, I will add an action search.

@bmatherly
Copy link
Member Author

Sorry that I told you to remove Paste from the timeline menu. That does belong in its Edit submenu.
Both shortcuts for Paste/Insert are still not working with nothing in the clipboard and something in the Source player.

Done

FYI after you add a class to list all actions, I will add an action search.

OK. I will work on that next - then we can work in parallel: you can work on search while I continue to convert actions and toolbars. I wold like to submit that in a new PR if you are agreeable to that.

@ddennedy ddennedy changed the title WIP Convert QML Menus to QWidgets and QActions Convert QML Menus to QWidgets and QActions Aug 5, 2022
@ddennedy ddennedy added this to the v22.08 milestone Aug 5, 2022
@ddennedy ddennedy merged commit 783b811 into mltframework:master Aug 5, 2022
@bmatherly bmatherly deleted the shortcuts branch November 6, 2023 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants