-
Notifications
You must be signed in to change notification settings - Fork 43
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
Basic zoom functionality for the cutting view #1033
Conversation
Based off of the subtitle view. Implements a very basic zoom functionality, hoping that it's better than nothing. Includes: - A Slider to control the zoom level - A horizontal scrollbar to move the timeline - Drag n' Drop to move the timeline Known issues: - The scrubber/playhead is cut off if placed either at 0 or at the maximum duration. I might have css'd myself into a corner here.
942495b
to
fcc79d0
Compare
This pull request is deployed at test.editor.opencast.org/1033/2023-04-13_14-43-59/ . |
This pull request has conflicts ☹ |
@Arnei could you rebase and fix the conflicts? I'd like to test this. |
This pull request is deployed at test.editor.opencast.org/1033/2023-09-21_14-00-10/ . |
Thank you @Arnei! :) |
This pull request has conflicts ☹ |
This pull request is deployed at test.editor.opencast.org/1033/2023-09-22_14-00-35/ . |
This pull request has conflicts ☹ |
Wrapping with `ScrollContainer` caused the playhead to get cut off, because `ScrollContainer` sets CSS `overflow`. Elevating the relative parent for the playhead above `ScrollContainer` fixes this issue.
This pull request is deployed at test.editor.opencast.org/1033/2023-12-11_14-56-49/ . |
Works now (mostly). 🎉 I found some funny bugs, though:
|
The previous fix for showing the head of scrubber/playhead again caused a number of issues with the position of the scrubber. This undoes the previous change, so that the scrubber binds to the correct parent again, which should undo all the issues. This also includes a stylistic change in moving the timeline a little lower inside its scroll container, to make the top of the scrubber show again.
This pull request is deployed at test.editor.opencast.org/1033/2023-12-12_10-56-44/ . |
Thanks for the quick review. Looks like my workaround was no workaround at all. In that case, the only solution I can think of is moving the elements around a bit, so that the scrubber is completely inside the scroll container (so that it does not get cut off). Otherwise we would have to go with a different approach for zooming altogether, which would be shame since the library we're using for this is otherwise rather nice. |
Looks better now!
Features nice to have:
|
|
I will try to position the scrubber by using a percentage instead of a changing px value. |
Nope, this does not work. The only way I can imagine removing the flickering of the scrubber is separating the elements of the scrollable waveform and the scrubber. The scrubber would have to be positioned separately in a different element. |
0001-Add-zoom-tooltip.patch.txt 0002-Debounce-drag-handler-of-the-scrubber.patch.txt 0003-Fix-fast-zooming-bug.patch.txt 0004-Increase-the-width-of-the-zoom-slider.patch.txt
This pull request is deployed at test.editor.opencast.org/1033/2024-05-22_07-38-02/ . |
@luniki is this a new bug that we already have now or will this bug be added by changing the way we set the max zoom factor? And what do you mean by "shifts in an unexpected way"? |
I have tested the latest changes. Here are the results of my tests for the various open issues:
|
Late to the game with a question: Do the shortcuts work for you? Both me and my colleague failed to activate them. |
The shortcuts for zooming in and out (Shift + Option + z/t or "+"/"-") don't work for me either. However, if I first activate the zoom slider by clicking on it, I can use the "left"/"right" keys to change the zoom level. |
I've tested Any further information like Browser version, System or detailed description/video of what you are doing would be appreciated. |
I tested it on a MacBook Pro (Sonoma 14.5) and Shift + Option + z/t or "+"/"-" didn't work at all in Firefox, Safari, Chrome and Opera Browsers (). |
Oh, so that's an alternative to z/t? They don't work at all at my end. |
I have fixed the drag and drop of the cut marks and changed the timeline zoom maximum depending on the video length in these patches: 0002-Enable-drag-and-drop-of-cut-markers-when-zoomed-in.patch.txt I cannot find a solution for the jumping of the scrubber. I would like to work on a proof of concept for using another library specialized in this. For a first version I would estimate about 2-3h to get an idea on how (and if) this could work. |
Signed-off-by: Arnei <arnewilken@yahoo.de>
Signed-off-by: Arnei <arnewilken@yahoo.de>
This pull request is deployed at test.editor.opencast.org/1033/2024-06-10_10-13-07/ . |
Signed-off-by: Arnei <arnewilken@yahoo.de>
Signed-off-by: Arnei <arnewilken@yahoo.de>
This pull request is deployed at test.editor.opencast.org/1033/2024-06-11_07-04-48/ . |
With lunikis patches we should now be here:
As already elaborated, fixing "jumping of the scrubber when zooming" will require a lot more work and as such we should consider if want to merge this even with this bug present. Also if you try to test "Always 20 sec of waveform in view when max. zoom" with the default "Dual Stream Demo" video, you will notice that the max zoom will not be at 20 seconds. That is because the video is relatively short with only about a minute of playtime, for which 20 seconds don't really make sense. For longer videos "Always 20 sec of waveform in view when max. zoom" will work. |
Also regarding the hotkey issue. Is it only the zoom hotkeys that are not working, or are you having this problem with other hotkeys as well? If it is the latter, I suggest we discuss it in a separate issue. |
We can live with this behaviour for now. I would suggest that we open a separate issue for this cosmetic problem.
I agree with you, it makes sense and is fine for us. And I have tested the latest changes and can confirm that "drag and drop cut marker" and "always 20 sec waveform" work as expected/described. |
Some of the shortcuts for the timeline don't work for me either (Shift + Option + i, Shift + Option + k, Up, Down). So, I'll create a separate issue for that (->#1384). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me and works as expected, exept for the comment below. I experienced the "jumping of the scrubber when zooming" but I see this has already been addressed.
@@ -489,6 +503,14 @@ const setThumbnailHelper = (state: video, id: Track["id"], uri: Track["thumbnail | |||
} | |||
}; | |||
|
|||
const ZOOM_SECONDS_VISIBLE = 20 * 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For videos of 20 seconds length or less, I can not use the zoom-slider at all because the returned value is used as maximum and will always be 1. This might be fine since the zoom level is already very detailed on a short video, but it would probably make sense to have some kind of visual indication / explanation as to why the user can't drag the slider. I was a bit confused at first.
Edit: I meant to highlight the whole timelineZoomMax()
function...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe even just hide the zoom bar completely for short videos < 20sec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hiding is nice. On the other hand, we could always have a minimal "maximum zoom factor" of 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd prefer to have a minimum zoom factor. That way the user always has the same experience and never has to wonder where the zoom slider has gone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the minimal maxZoom factor. This patch takes care of it:
0001-Update-the-minimal-maxZoom-from-1-to-2.patch.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luniki Now the zoom-bar seems to be not correctly shown for short videos. I guess the buttons are not intended?
Tested with the ID-3d-print
video on develop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jduehring I haven't noticed your problems on https://test.editor.opencast.org/1033/2024-06-18_10-33-57/?id=ID-3d-print. My PC displays the zoom slider as it should (tested with Safari, Firefox, Chrome and Opera).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm strange, might have been a local hiccup on my side. Seems to work now. So all in all I have no more complaints and its ready to merge from my side.
This way there is always something to zoom into even with very small videos. Signed-off-by: Arnei <arnewilken@yahoo.de>
This pull request is deployed at test.editor.opencast.org/1033/2024-06-18_10-33-57/ . |
Based off of the subtitle view. Implements a very basic zoom functionality, hoping that it's better than nothing. Includes:
Known issues:
The scrubber/playhead is cut off if placed either at 0 or at the maximum duration. I might have css'd myself into a corner here.Possible future feature extensions:
Resolves #107.
Courtesy of the Opencast 2023 Crowdfunding.