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

Allow bulk shifting subtitles forward and backward in the editor #1610

Closed
PCF-Testing opened this Issue Jul 19, 2014 · 18 comments

Comments

Projects
None yet
4 participants
@PCF-Testing
Member

PCF-Testing commented Jul 19, 2014

User stories:

  1. As a video content creator, I make an edit to insert some extra content into my video. I want to shift all the subtitles forward after the point where I added the new content, so that I can add new subtitles for that new content.
  2. As a video content creator, I decide to delete the intro or a 10-second piece in the middle of my video. I want to shift all the subtitles backward after the point where I removed content, so that the subtitles align with the video.

Most recent prototype: https://invis.io/WTE5440J2
Design research/decisions: https://docs.google.com/presentation/d/1I7FBApSzmBJ54E4m1F2Kb-pvy034lZVUtPk2LshS0OQ/edit#slide=id.g2b107bdb15_0_68

Implementation:

  • PART 1: Split Wrench menu items into two menus: Wrench + Clock menus

    • Wrench menu includes all items in prototype + "show tutorial" (exists in current wrench menu in editor)
    • Clock menu as shown in prototype
    • clicking either button causes it to have the "depressed" view or not. When button is "depressed," the menu is open. When it is not, the menu is closed.
      • menus should stay open when actions are taken elsewhere in the editor (currently, the menu closes when action is taken) - @syl22-00 just added this, but it can be in a separate issue for later too if it is more complex
      • hovering over the menus should not open them; it should display a tooltip above:
      • for wrench menu: subtitle tools
      • for clock menu: timing tools
        @syl22-00 just added this piece, let me know what you think
    • menus should open in one of these two directions, whichever is easier to implement:
      • wrench to the left, clock to the right
      • wrench and clock both to the bottom
  • PART 2: timeline context menu

    • left-clicking anywhere on the timeline should open a context menu (see in prototype by clicking on red timeline cursor from start screen)
  • PART 3: modals

    • use styles as shown in prototype, but with the following changes
      • helptext for "all subtitles on or after" field should read: hour:min:sec:ms
      • helptext for shift amount fields should read: "hour," "min," "sec," "ms"
      • Instead of "Shift Amount" title - two radio buttons for "shift by amount" and "shift to timestamp." Appropriate data fields appear based on which radio button is selected:
        shift_fwd_modal_open_d1
        shift_fwd_modal_open_d2
    • "shift to timestamp" shifts the start point for the subtitles selected to the designated timestamp
  • PART 4: Error messages for form fields

    • if a shift amount will cause a subtitle to be shifted completely out of the video, show error message: "Please enter a smaller shift amount. Subtitles can't be shifted completely out of the video."
  • style: https://projects.invisionapp.com/d/main#/console/12306945/265725143/preview


From old description:

either all subs or all subtitles from a certain timeline position forward
the option to change all the start and end times by a fixed offset value

  • User selects a start time and an amount of seconds to remove
    • Maybe the start time defaults to the current position in the video
  • User clicks "Delete time" button
  • We delete any subtitles inside that time frame.
    • If a subtitle is partly inside timeframe, then we should clip it so that only the time outside remains
  • Any subtitles after the time frame are moved backward in time by the amount of time selected

@kslottow kslottow changed the title from Add the option to change all the start and end times by a fixed offset value to Allow adding time - the option to change all the start and end times by a fixed offset value Aug 1, 2017

@kslottow

This comment has been minimized.

Contributor

kslottow commented Aug 1, 2017

@kslottow kslottow removed their assignment Aug 1, 2017

@kslottow kslottow added this to the Sprint 19 milestone Aug 1, 2017

@kslottow kslottow modified the milestones: Sprint 20, Sprint 19 Aug 14, 2017

@kslottow kslottow modified the milestones: Sprint 21, Sprint 20 Aug 28, 2017

@kslottow kslottow modified the milestones: Sprint 23, Sprint 21 Sep 8, 2017

@kslottow kslottow assigned syl22-00 and unassigned bendk Oct 5, 2017

@kslottow kslottow modified the milestones: Sprint 23, Sprint 24 Oct 23, 2017

@syl22-00 syl22-00 self-assigned this Nov 2, 2017

@syl22-00 syl22-00 modified the milestones: Sprint 24, Sprint 25 Nov 2, 2017

@kslottow kslottow changed the title from Allow adding time - the option to change all the start and end times by a fixed offset value to Allow shifting subtitles forward and backward, either all subs or all subtitles from a certain timeline position forward Nov 10, 2017

@kslottow kslottow changed the title from Allow shifting subtitles forward and backward, either all subs or all subtitles from a certain timeline position forward to Allow bulk shifting subtitles forward and backward in the editor Nov 10, 2017

@kslottow kslottow modified the milestones: Sprint 25, Sprint 26 Nov 22, 2017

@syl22-00 syl22-00 modified the milestones: Sprint 26, Sprint 27 Nov 29, 2017

@kslottow kslottow removed this from the Sprint 27 milestone Dec 13, 2017

@kslottow kslottow added this to the Sprint 28 milestone Dec 13, 2017

@syl22-00

This comment has been minimized.

Contributor

syl22-00 commented Dec 31, 2017

@syl22-00 syl22-00 removed their assignment Dec 31, 2017

@syl22-00

This comment has been minimized.

Contributor

syl22-00 commented Jan 8, 2018

@PCF-Testing please let me know if you can work out from that branch. I can not change the pipeline of those tickets anymore.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jan 12, 2018

@bendk @syl22-00: took a stab at this one and found the following issues:

  • On the branch, mouseover the menu buttons does not expand the menu, menus only expand when a button is clicked. It matches the req' in the ticket, but is a change in behaviour compared to the production version and makes tools menus less visible.

  • Design of dialogs needs more love (increased padding, better headers matching other headers in the editor, styling of buttons). Does the dialog to be positioned inside the menu drop-down? For other tools commands, we use dialog windows in the center of the editor.

Compare:
screenshot from 2018-01-12 04 23 47
(a typical editor modal)

vs

screenshot from 2018-01-12 04 24 18
(time shift dialog from the branch)

  • Both dialogs need "Cancel" button to hide the dialog and reset the fields in the dialog (clicking outside the dialog just hides it but leaves the entered data intact).

  • Once the user clicks "Shift" and the changes are applied, the fields in the dialog should be reset to their default values (this currently doesn't happen)

  • Since "Shift Amount" fields accept any numbers of hours, minutes etc., there is no need to use the programming abbreviations ("mm", "ss"). Can be "hours", "minutes", "seconds", and "milliseconds". ("All subtitles on or after"should keep the current labelling because in that case it indicates the required time format and the number of digits in each position matters.)

  • Time format for "All subtitles on or after" should be "h:mm:ss.ms" (with a dot before milliseconds)

  • If the user starts typing in the start time field ("h:mm:ss.ms"), could this automatically select "All subtitles on or after" radio button)? Otherwise, the user can easily forget to click it.

  • Shifting subtitles by hour(s) does not work for me (shifting by 60 min does)

  • Shifting back does not make a sanity check before completing the action and can produce invalid time values if the resulting time is negative:

screenshot from 2018-01-12 04 23 09

@bendk

This comment has been minimized.

Member

bendk commented Jan 12, 2018

Talked with Kenzie a bunch about the UI for this feature and came up some decisions:

  • We think we should keep the click-to-open behavior of the buttons, for a couple reasons:
    • We want to use clicking in general for dropdown menus. It's easy to accidentally lose the menu with the hover behavior, and also mobile users can't hover at all.
    • The discoverability concern goes both ways. Right now many user's don't know you can click the menu, since it opens automatically once you hover.
  • We should however, close the menu on the second click
  • We should use a better widget for picking times/durations.
  • We should use a dialog to handle the forms like you suggested
  • The shift backward dialog should be reworked:
    • First input is "start time"
    • The next input is "end time"
    • Below that should be help text that explains what's going on. Something like:
      • All subtitles between 1:04 and 2:04 will be deleted
      • All subtitles after 2:04 will be shifted back by 1:00
    • After a long talk, this one seemed like it was the clearest way to explain to the user what's going on
    • Also, this eliminates the issue of having subtitles shifted before 0 seconds.

@bendk bendk self-assigned this Jan 12, 2018

@bendk bendk modified the milestones: Sprint 28, Sprint 30 Jan 12, 2018

bendk added a commit that referenced this issue Jan 12, 2018

bendk added a commit that referenced this issue Jan 16, 2018

@bendk

This comment has been minimized.

Member

bendk commented Jan 18, 2018

Just pushed an updated version (this on is in the pculture/unisubs repo). It implements the changes I listed above.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jan 19, 2018

The look and feel of the updated version is much nicer. I like the way the dialogs look now, and the explanatory text is very helpful.

A few issues that have cropped up right away:

  • The help text and "Shift" button are not up to date with the user's input in the time pick fields. If a user makes some change, e.g. chooses to add 1 sec to all start and end times, typing 1 in the box for seconds does not update the help text, and a subsequent click on "Shift" won't make anything happen. You need to type in the input, then click somewhere inside the editor, and only then you can proceed. Could the user's input be checked in real time, say, with 1-2 sec delay? So that I type 1 sec, and in a second or two the dialog automatically updates with the appropriate help text and "Shift" button ready for action?

  • The validator checks and enforces that the new timestamps do not get beyond the duration of the video. Does this really need to be enforced, or it would be enough just to inform the user that the subtitles go beyond the end of the video, but still allow performing the action? What if the duration of the video is unknown or outdated (suppose it was auto-retrieved from the original video URL, and then another URL with a different video length was added)?

  • When shifting subtitles back, it is possible to get overlapping subtitles as the result. Consider the original subtitles where a new subtitle start every second:

1
00:00:00,000 --> 00:00:01,000
one

2
00:00:01,000 --> 00:00:02,000
two

3
00:00:02,000 --> 00:00:03,000
three

4
00:00:03,000 --> 00:00:04,000
four

5
00:00:04,000 --> 00:00:05,000
five

6
00:00:05,000 --> 00:00:06,000
six

7
00:00:06,000 --> 00:00:07,000
seven

8
00:00:07,000 --> 00:00:08,000
eight

9
00:00:08,000 --> 00:00:09,000
nine

10
00:00:09,000 --> 00:00:10,000
ten

11
00:00:10,000 --> 00:00:11,000
eleven

12
00:00:11,000 --> 00:00:12,000
twelve

Suppose the user requests to remove the bit between 0:00:05.500 and 0:00:07.000. This action produces the following subtitles:

1
00:00:00,000 --> 00:00:01,000
one

2
00:00:01,000 --> 00:00:02,000
two

3
00:00:02,000 --> 00:00:03,000
three

4
00:00:03,000 --> 00:00:04,000
four

5
00:00:04,000 --> 00:00:05,000
five

6
00:00:05,000 --> 00:00:06,000
six

7
00:00:05,500 --> 00:00:06,500
nine

8
00:00:06,500 --> 00:00:07,500
ten

9
00:00:07,500 --> 00:00:08,500
eleven

10
00:00:08,500 --> 00:00:09,500
twelve

Note that subtitles 6 and 7 overlap.

The solution here, I think, would be to auto-adjust the end time for subtitle 6 that falls into the segment that is being removed.

  • I am also having problems with the TAB key, which doesn't seem to play and pause the video (regular YouTube) anymore.
@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jan 22, 2018

  • In the above scenario with removing a fragment of subtitles from 5.500 to 7.000, the fragment is now removed from the timeline, but not from the editing panel, which still keeps the removed subtitles and allows to "seek" to the beginning of the removed panel. The editor is also displaying "Timing error" after the shift back action:

screenshot from 2018-01-22 03 47 11

The zero length subtitle gets saved:

6
00:00:05,000 --> 00:00:05,500
six

7
00:00:05,500 --> 00:00:05,500
seven

8
00:00:05,500 --> 00:00:06,500
eight

I think these subtitles should be just deleted, not compressed to zero length. If users want to re-adjust subtitles, they can do so before shifting. If they don't like the results of deleting a fragment and shifting back, they can reload a saved version and start all over again.

  • A similar quirk with shifting back - consider the following subtitles:

1
00:00:00,000 --> 00:00:10,000
one

2
00:00:10,000 --> 00:00:20,000
two

3
00:00:20,000 --> 00:00:30,000
three

4
00:00:30,000 --> 00:00:40,000
four

5
00:00:40,000 --> 00:00:50,000
five

6
00:00:50,000 --> 00:01:00,000
six

7
00:01:00,000 --> 00:01:10,000
seven

8
00:01:10,000 --> 00:01:20,000
eight

9
00:01:20,000 --> 00:01:30,000
nine

10
00:01:30,000 --> 00:01:40,000
ten

11
00:01:40,000 --> 00:01:50,000
eleven

12
00:01:50,000 --> 00:02:00,000
twelve

13
00:02:00,000 --> 00:02:10,000
thirteen

Now we remove the fragment between the beginning of the video and to 1 minute into it. This produces the following result with a zero length subtitle:

1
00:00:00,000 --> 00:00:00,000
six

2
00:00:00,000 --> 00:00:10,000
seven

3
00:00:10,000 --> 00:00:20,000
eight

4
00:00:20,000 --> 00:00:30,000
nine

5
00:00:30,000 --> 00:00:40,000
ten

  • When invalid values are entered into the time picker fields (60+ minutes, or 60+ seconds, or 1000+ milliseconds), "Shift" button takes no effect, but there is no error message explaining to the user what is wrong.

  • Time picker fields accept non-digits (letters, specials characters etc.). I think it shouldn't.

bendk added a commit that referenced this issue Jan 22, 2018

@bendk

This comment has been minimized.

Member

bendk commented Jan 22, 2018

For the last two, the field would turn red when you entered an invalid amount. Was that not noticeable? Maybe we should make it stand out more.

In any case, I also added an error message below that says there's an invalid value.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jan 23, 2018

Looks much better now.

  • Noticed that one of the old wrench menu items (Show/Hide Tutorial) is missing now. If it is just an oversight and not an intentional change, in the new menu it should belong under the wrench icon again, at the bottom of the list.

  • The time picker allows entering fractions without an error message, e.g. 0.5 h. "Shift" does not work in this case, and there is no feedback on what is wrong.

I also see a potential issue since with subtitle removal and shifting we can make the remaining subtitles so short (e.g. 1 ms long) that it cannot be corrected in the editor without downloading the subtitles and editing them in a text editor - such short subtitles just cannot be changed in the editor. Not sure if we need to deal with this issue immediately, but this is something worth thinking about.

Example:
1
00:00:00,000 --> 00:00:10,000
one

2
00:00:10,000 --> 00:00:20,000
two

3
00:00:20,000 --> 00:00:30,000
three

4
00:00:30,000 --> 00:00:40,000
four

5
00:00:40,000 --> 00:00:50,000
five

6
00:00:50,000 --> 00:01:00,000
six

7
00:01:00,000 --> 00:01:10,000
seven

Now we shift subtitles back from 00:00:40,001 to 00:00:59,999. This creates two subtitles that are 1 ms long.

I think the rest looks good. The branch now included migrations from 2137-stage1, those stalled again until I killed the app.

@bendk

This comment has been minimized.

Member

bendk commented Jan 23, 2018

For that last part, it seems like we need a more precise way to edit timings. I'm thinking like the feature where you can click on them in the subtitle list and manually set the times.

@bendk

This comment has been minimized.

Member

bendk commented Jan 23, 2018

Just pushed a fix for the last two things, hopefully this is all good to go.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Jan 25, 2018

@bendk: sorry, it is not all fixed yet - there is also a context menu when you right-click the timeline. The menu has Shift forward and Shift back commands, but the dialogs are not invoked when these menu items are selected. They should.

I am not 100% sure if the dialogs should be pre-populated with time values when invoked from the timeline. Since the right-click repositions the cursor on the timeline to the time where it was right-clicked, I guess the correct behaviour would be to pre-populate "starting from" with the time selected by the user. But if that does not sound right or is too hard to implement, then at least the dialogs should appear and work as usual.

bendk added a commit that referenced this issue Feb 1, 2018

@bendk

This comment has been minimized.

Member

bendk commented Feb 1, 2018

Just pushed a fix that should fix those issues. We agreed that:

  • Right click should not seek the timeline, that's confusing if it also opens the context menu
  • We shouldn't pre-populate the starting time when right-clicking on the timeline. It's just to imprecise to be useful.
@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Feb 2, 2018

I think now it looks, feels and functions OK. Moving to Needs PR.

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Feb 2, 2018

A reminder that most KB articles in Transcribing and Translating section will need an update.

asabine added a commit that referenced this issue Feb 2, 2018

@kslottow

This comment has been minimized.

Contributor

kslottow commented Feb 5, 2018

thanks @PCF-Testing ! Working on this today

@PCF-Testing

This comment has been minimized.

Member

PCF-Testing commented Feb 7, 2018

Deployed.

@PCF-Testing PCF-Testing closed this Feb 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment