mltframework / shotcut

cross-platform (Qt), open-source (GPLv3) video editor
https://www.shotcut.org
GNU General Public License v3.0
10.84k stars 1.12k forks source link

Add player looping #1500

Closed bmatherly closed 9 months ago

bmatherly commented 9 months ago

Includes various loop range options and indicators in the player and timeline to show the loop range.

Add new menu option in the Player menu and as a button menu on the player image

image I am not sure I love the menu on the play button. It throws off the symmetry of the player icons. Comments welcome

When loop mode is enabled, the normal triangle play button is changed to an arrow loop icon image

When looping is activated, I have added a semi-transparent highlight to the player and timeline rulers. I know there is some sensitivity to making things cluttered. So I am open minded about comments here:

image

image

The loop mode is currently not saved in the settings. Should it be? I think some people will expect it to be saved. Others will be surprised when they forget about the setting and the player loops unexpectedly. Alternately, maybe the loop mode should be cleared whenever the producer is changed (like when changing from timeline to playlist or a different clip).

The shortcut for the loop mode is ALT+L

ddennedy commented 9 months ago

It throws off the symmetry of the player icons.

Yeah, but it is not too bad. However, it is rather unusual to have loop and play buttons combined, and that will throw people (I know I suggested it). Usually transport controls have a separate loop button that shows itself as on or not. I am concerned about consuming more space with another button. I am starting to consider to move all time-related stuff to a second row. I rarely see the in point & selected duration times. Maybe to reduce the space occupied always make the icons the small ones, and use smaller text for the timecode and time values. What do you think?

The loop mode is currently not saved in the settings. Should it be?

No, this is generally a session-oriented option, not a sticky one.

The shortcut for the loop mode is ALT+L

Again, I frown on Alt+letter shortcuts because they can conflict with the main menu shortcuts. Some UIs underline a letter in a form field label to make Alt+letter a shortcut to give that field focus. Shotcut does not do this because it's tabbed multi-panel with soft focus making it complicated and unreliable. However, I want to remind you again of this general UI convention - at least for traditional Windows apps - for the rationale. Maybe we will have a Library menu in the future. Besides, I have noticed an app or two use backslash as the loop toggle and have always wanted to use that.

bmatherly commented 9 months ago

I have noticed an app or two use backslash as the loop toggle and have always wanted to use that.

Done

Here is a suggestion to move the transport controls to their own row (not committed yet, just in my working copy). I do not personally miss the vertical space because I always have extra unused space above and below my video preview. image You can see that the loop action gets its own stateful button and the play button does not change to the loop icon.

Comments welcome. I would be happy to push the change if you would like to try it.

ddennedy commented 9 months ago

Here is a suggestion to move the transport controls to their own row

That is one way that I was not exactly think of, but I am not sure it addresses the concern that it runs out of room for the in/selected time values. I was thinking to put all buttons on one row and all time values on a second row. I even think it can be adaptive. In the Player layout there is usually room for everything even with non-small icons. So, create 4 horizontal layouts as groups: current/duration times, transport controls, option buttons, in/selected times. Then, only if there is not enough room for all 4, move the 2 time layouts to the second row layout. There is an expander between each group to evenly distribute them. If two rows are needed, all of the expanders thus far stay on the first row, but an extra expander is needed between the 2 time groups on the second row. Something like that. What do you think?

bmatherly commented 9 months ago

I was thinking to put all buttons on one row and all time values on a second row. I even think it can be adaptive.

This is a good idea - but tricky to implement because it does not conform to the built-in layout wrapping behavior. I think I came up with something that works pretty good (pushed in the last commit)

When there is room for everything on one line: image

When it has to overflow to a second line: image

I'm pretty happy with it.