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

Add "start at:" option to share menu #943

Merged
merged 13 commits into from
Sep 26, 2023

Conversation

owi92
Copy link
Member

@owi92 owi92 commented Sep 20, 2023

Closes #191

  • This adds a "Start at: {time}" timepicker to the direct link on the Video details page and both share options of the video page's share menu.

  • Picking a time will append said time as a search query to the shared link.

  • The linked video will start at the chosen time.

  • Users can also manually add a time to a link in their browser by appending ?t=2h2m20s (or any other combination of hours, minutes and seconds like ?t=2m or ?t=1h20s or ?t=20s etc). You can try it out with https://pr943.tobira.opencast.org/v/GjDqXFlukkH?t=20s (or any other video).

  • There is unfortunately one visual issue with the time picker in chrome, where the rightmost number is slightly cut off. I don't know what causes this yet.

Bildschirmfoto 2023-09-20 um 23 11 22
  • As always, visuals and wording (and I also the places where this is included) are open for discussion and feedback is appreciated.

@github-actions github-actions bot temporarily deployed to test-deployment-pr943 September 20, 2023 22:20 Destroyed
@owi92 owi92 added the changelog:user User facing changes label Sep 20, 2023
@owi92
Copy link
Member Author

owi92 commented Sep 20, 2023

Just noticed an issue with the timestamp in the share menu. Will fix asap.

@github-actions github-actions bot temporarily deployed to test-deployment-pr943 September 20, 2023 22:51 Destroyed
@oas777
Copy link
Collaborator

oas777 commented Sep 21, 2023

Ole, if this lets me manually enter the time information, we're on the wrong track from my perspective: The timestamped sharing should be synchronized with the player so that the user can choose whether to add that timed information or not. Ideally, one could also change that information. Cf. https://tinyurl.com/279ecb8t under "Media".

@owi92
Copy link
Member Author

owi92 commented Sep 21, 2023

Oh ok, well the manual part is also possible at https://tinyurl.com/279ecb8t, and at least the time picker on https://pr943.tobira.opencast.org/~manage/videos/GjDqXFlukkH needs the be operated manually.
But I see your point about the synchronized part which makes sense for our share menu.

@dagraf
Copy link
Collaborator

dagraf commented Sep 21, 2023

I agree with @oas777, that the timestamped sharing should be synchronized with the player. Here two screenshots how YouTube and SWITCHtube is offering this feature. I would prefer the solution of SWITCHtube or ETH-videoportal.

YouTube:

  • Here the timestamp is taken from the moment the user clicks on the "Share" button.
Bildschirmfoto 2023-09-21 um 10 54 32

SWITCHtube:

  • Here the timestamp is taken at the moment the option "Wiedergabe an aktueller Position starten" gets activated. But it does NOT get constantly actualized when the video remains playing. But when de- and reactivated, the time gets acutalized.
Bildschirmfoto 2023-09-21 um 10 51 23

@LukasKalbertodt
Copy link
Member

I also think we want a checkbox whether to include the timestamp. And that the time should be synchronized to the player. Well, when the share menu is opened, the current player time should be taken. Whether it needs to update continuously if the player if playing... not sure about that.

But I also think it should be editable (as it is now). If the user wants to manually set a time. That's also useful for the share thing in the "my videos" section.

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This already implements two things at once! It also closes #678.

As usual, some comments, but nothing special. The main problem is the UI for picking the timestamp.

frontend/src/ui/player/Paella.tsx Outdated Show resolved Hide resolved
frontend/src/util/index.ts Outdated Show resolved Hide resolved
frontend/src/i18n/locales/de.yaml Show resolved Hide resolved
frontend/src/routes/Video.tsx Outdated Show resolved Hide resolved
frontend/src/routes/Video.tsx Outdated Show resolved Hide resolved
frontend/src/routes/Video.tsx Outdated Show resolved Hide resolved
frontend/src/routes/manage/Video/Details.tsx Outdated Show resolved Hide resolved
frontend/src/routes/manage/Video/Details.tsx Outdated Show resolved Hide resolved
@owi92 owi92 changed the title Timestamp for videos Add "start at:" option to share menu Sep 23, 2023
Before this, the urls were build using the
exact url of the page, meaning it would include
any search query. This fixes this by removing
existing time queries before appending a new one.
With this, the current state (whether it's loaded)
and certain methods like getting and setting the
current play time of the video can be accessed by
using the player ref.
@github-actions github-actions bot temporarily deployed to test-deployment-pr943 September 24, 2023 20:07 Destroyed
@owi92
Copy link
Member Author

owi92 commented Sep 24, 2023

@oas777, @dagraf: I have updated this to be synchronized with the player. The current solution is closer to youtube as I don't think the time should be continuously updated when the video is still playing.
I'm also not sure whether the time should really be updated on un- and rechecking the checkbox. There are a couple of reasons against that in my opinion:

  • As a user, I would expect the timestamp to be "frozen" once I open the share menu. I'd also imagine users would generally pause the video anyway before sharing, and not wait for the "perfect moment" while the share menu is open and the video is still playing.
  • We also have the "embed" option, which uses its own checkbox. So a user might open the share menu, see the timestamp in the "main" share option, switch to the "embed" option and check the checkbox there only to see a new value to share. I'd expect the timestamp to be the same for both options. Especially since we also allow manual inputs.

But if one or both of you still feel strongly that this should behave closer to SWITCHtube or the ETH portal, like David mentioned in his comment, we can do that of course.

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current behavior is what we want. Only taking the current video position when the share menu is opened.

A few more things:

  • This time input needs to be disabled for livestreams and upcoming livestreams as it doesn't make sense there (right?)
  • I would add more (vertical) space between the time input line and the resulting link input.
  • I can input 3 then add a minus before it. The textbox resets to 0, but the link contains the -3.

frontend/src/routes/Video.tsx Outdated Show resolved Hide resolved
frontend/src/routes/manage/Video/Details.tsx Outdated Show resolved Hide resolved
frontend/src/ui/Input.tsx Outdated Show resolved Hide resolved
frontend/src/ui/Input.tsx Show resolved Hide resolved
frontend/src/ui/Input.tsx Outdated Show resolved Hide resolved
@owi92
Copy link
Member Author

owi92 commented Sep 25, 2023

I would still prefer for the time to continue with the video playing, but given I cannot pause the video without exiting the dialogue, I don't see how this could work

I suppose we could "freeze" the time once the checkbox gets activated.

Do the time figures have a different font (size) than "Starten bei"?

They do indeed. That's an oversight on my end.

I'm not a fan of the boxes for the time figures.

We will need some form of indication that the time figures are editable, at the very least for accessibility considerations.
But I kind of agree that it doesn't look too good this way - and it is also the reason for the "jump cutting" (which exists so there won't be any gap between number and unit when the thing is deactivated). I'll try to find a better solution.

@oas777
Copy link
Collaborator

oas777 commented Sep 25, 2023

Apologies, Ole, my (now deleted) answer did not fully take into account what you wrote yesterday and now you've already replied... I will continue in response to that latest comment of yours.

@owi92
Copy link
Member Author

owi92 commented Sep 25, 2023

Oh sorry, I didn't notice you deleted your answer

@oas777
Copy link
Collaborator

oas777 commented Sep 25, 2023

My fault entirely. Let's stick with time synch as it is. Given you don't have the same interaction situation is was referring to in our current video portal, we can assume users will wait for the "right moment" in the video, then switch to "Share". And yes, let's see an alternative to the boxes please.

@github-actions github-actions bot temporarily deployed to test-deployment-pr943 September 25, 2023 15:52 Destroyed
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how it looks and that you can use tab to switch to the next field. I think though that most people would use arrow keys to do that, so I fear you have to implement that as well :/

And a completely different thing I noticed: the embed route doesn't work anymore, due to the player context thingy. You have to add the context provider in routes/Embed.tsx too. FYI: You can just open the link from the embed code in the browser.

frontend/src/ui/Input.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to test-deployment-pr943 September 25, 2023 19:49 Destroyed
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks and works really well now! I found two bugs still unfortunately.

  • The QR code doesn't contain the time parameter. Should be simple to fix.
  • The embed route with time parameter doesn't seem to work. Paella shows it's playing the video, but I don't see any video. No idea what's going on there :/
    EDIT: Uhm, it's weirder. Sometimes it works. If I wait for a long time before pressing play it works, if I press play immediately it doesn't. Wait... and the audio always plays, but what I meant is that the video is just grey in those cases. Wat.

@github-actions github-actions bot temporarily deployed to test-deployment-pr943 September 26, 2023 09:47 Destroyed
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like all comments here (also by the others) were resolved. Thus merging. We can always re-adjust later.

@LukasKalbertodt LukasKalbertodt merged commit 6d6e9ed into elan-ev:master Sep 26, 2023
2 checks passed
@oas777
Copy link
Collaborator

oas777 commented Sep 27, 2023

FWIW, this is the syntax our video portal is using currently:
https://video.ethz.ch/events/opencast/2023/berlin/69fa69d5-d976-4e22-9b82-0460d3fa7e04.html?time=3m2s

Which is different from Tobira:
https://tobira.opencast.org/lectures/d-infk/2019/autumn/252-0856-00L/v/EJic0mSes6f?t=02s

YouTube has
https://youtu.be/mvoFemftA34?t=10,

Vimeo
https://vimeo.com/30698649#t=34s,

so we know who is the odd one out.

Probably have to accept time-stamped links from the old video portal will not work in the new one. Unless you tell me there's an easy way out.

@LukasKalbertodt
Copy link
Member

YouTube also has the 3m2s format. But if there are no units, it just treats it as seconds.

We will have to see regarding your old video portal. With what we just discussed there are multiple such "old links need to work" issues. Might be worth it just adding a tiny service in front of Tobira that just rewrites old links to new links. But again, I think we can talk about this separately.

@owi92
Copy link
Member Author

owi92 commented Sep 27, 2023

If this is just about the time parameter, I think we can easiliy change the t part of the url to time or even allow both versions. That's what opencast is doing anyway.

@oas777
Copy link
Collaborator

oas777 commented Sep 27, 2023

Thanks, Ole, if you could allow both that would be great.

Slightly related: What was the reason for the "/v" in

https://tobira.opencast.org/lectures/d-infk/2019/autumn/252-0856-00L/v/EJic0mSes6f?

Because that would be

https://video.ethz.ch/lectures/d-infk/2019/autumn/252-0856-00L/1603eb14-81aa-4286-b076-8d39b632dcae

at our end.

@LukasKalbertodt
Copy link
Member

Slightly related: What was the reason for the "/v" in

To avoid ambiguitities/name clashes. What if you want to create a page with a path the same as a video ID. Unlikely but this is a cleaner system. But did you also notice that your video portal uses Opencast IDs (the long ones) and Tobira uses its own IDs (shorter).

As I already said: I am pretty sure we want some extra service in front of Tobira to rewrite old video portal links. We can't change Tobira such that all old links from your video portal still work. It also wouldn't make sense to put such backwards-compatibility things into a new project that is for the whole community. I also wouldn't change the time parameter name now because of this. We might still change it in the future, but we shouldn't because of the old video portal. t seems to be more common with big video platforms.

@oas777
Copy link
Collaborator

oas777 commented Sep 28, 2023

Let me get this straight: You added this just in case someone was to create a page called "EJic0mSes6f"?

@oas777
Copy link
Collaborator

oas777 commented Sep 28, 2023

PS: Agreed, I put this on the list of issues in the context of an ETH migration.

@LukasKalbertodt
Copy link
Member

Let me get this straight: You added this just in case someone was to create a page called "EJic0mSes6f"?

Any 11 character alphanumeric string is a valid Tobira video ID (e.g. "vorlesungen", "jahresvideo" and "leadership2"). While most IDs will not clash with anything anyone ever wants to type, this is still a possibility. I don't want someone running into this rare issue 4 years from now, thinking "what a stupid software" (rightly so). But maybe more importantly: we might change the Tobira ID format in the future. And we might very well add additional things other than /v/, e.g. /s/ for series or /p/ for playlists. This gives us flexibility down the line.

@owi92 owi92 deleted the timestamp-for-videos branch March 4, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GET parameter to specify a timestamp for a video (e.g. t=3m12s)
4 participants