search

biigle / core

:large_blue_circle: Application core of BIIGLE
https://biigle.de
GNU General Public License v3.0
12 stars 15 forks source link

Implement 'Jump by seconds' forward/backward in video #822

Closed ToukL closed 4 months ago

ToukL commented 4 months ago

Splitted from #821

Implements a "jump by seconds" settings (settable by user) discussed here #262

mzur commented 4 months ago

While looking at your other PR, I see now why you changed the icons for the next/previous video. I still think the old icons should keep their function, otherwise users will be confused.

Maybe we can use fa-caret-square-right (or left) for the "jump by frame" buttons?

ToukL commented 4 months ago

It works well, thank you! I have a few comments on the interface:

  • The next/previous video buttons should still belong to the same btn-group than the "play" button.
  • The next/previous buttons should still have the fa-step-backward or fa-step-forward icons because these are the icons the image annotation tool uses, too. Personally, I like the chevron icons better for next/previous, but the old icons were there forever and users are used to them so we should stick to them, too.
  • Users should be able to set the jump step to 0 to disable this feature. If it is 0, the jump buttons should be hidden, too. This should be mentioned in the description and the manual (see below)
  • There should be a keyboard shortcut for the jump buttons. Maybe Ctrl+left/right arrow?
  • The feature should also be mentioned in the manual article.

Thank you for the feedback. Yes I suspected that the change of button would not be accepted in the official instance, I will rollback to using fa-step-backward and fa-step-forward for changing videos and use the fa-caret-square-right for the frame as you suggested. But do you think we can keep the left/right arrow shortcut for the prev/next frame ? I feel that if we manage to implement that feature it would be more intuitive to use keyboard arrows to change frames and maybe use another shortcut for changing videos ? Maybe shift+left/right arrow ? I can leave the old shortcut for this PR and do the change in the other one that deals with frame navigation.

I'll do all the other changes as suggested. Thanks again !

mzur commented 4 months ago

I can leave the old shortcut for this PR and do the change in the other one that deals with frame navigation.

Please do. I'll start my review of your other PR now and we can deal with this question there.

mzur commented 4 months ago

While I reviewed you other PR, something else came to my attention. Rather than setting currentTime directly in videoPlayback, you should emit an event that calls the seek() method of the videoContainer. This will show an appropriate loading state. Also, you can then disable the jump by seconds button while this.seeking===true in the videoContainer. The this.seeking state is also available in the videoScreen component and in videoPlayback.

mzur commented 4 months ago

And another thing when you implement the keyboard shortcuts: I always add the shortcut in bold text to the title attribute of the buttons. I use this tool for that and choose the "Math sans bold" text.

ToukL commented 4 months ago

While I reviewed you other PR, something else came to my attention. Rather than setting currentTime directly in videoPlayback, you should emit an event that calls the seek() method of the videoContainer. This will show an appropriate loading state. Also, you can then disable the jump by seconds button while this.seeking===true in the videoContainer. The this.seeking state is also available in the videoScreen component and in videoPlayback.

Sure I will do that too !