Skip to content

Conversation

@BugsSindFeatures
Copy link
Contributor

@BugsSindFeatures BugsSindFeatures commented Oct 26, 2024

Description

Added code to keep the current time of a selfhosted video when switching to questions and back. This was resolved by saving the currenttime and currentvideoID as a cookie and comparing if the saved ID is the same as the ID of the playing Video before setting the time of the video to the stored value.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Translation updates (fix/improve or add new translations)

Fixes Bootstrap-Academy/Bootstrap-Academy#109

My Bootstrap Academy username: R4bbit

Copy link
Contributor

@Lauritz-Tieste Lauritz-Tieste left a comment

Choose a reason for hiding this comment

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

I have noticed that a few errors have sneaked into your code. 🧐

  1. you wrote: if (currentVideoTime < 0) ~than save videoID~
    This must be the other way around. You have to replace the less-than sign with a greater-than sign.

  2. you called the function removeItem. In my opinion, this function does not exist. And if it did exist, shouldn't there be brackets?

I have written your code a little cleaner and renamed a few variables. Could you perhaps adopt these changes and, above all, understand them? I am looking forward to your opinion. 😊

(unfortunately I don't have the rights to edit your PR directly :-)

Changes

// ...
<video
  v-else-if="activeLecture.type == 'mp4'"
  :poster="course.image"
  controls
  playsInline
  title="video player"
  allowfullscreen
  controlsList="nodownload"
  oncontextmenu="return false;"
  :key="`video-${activeLecture.id}`"
  @timeupdate="onTimeUpdate(activeLecture.id, $event)"
  @loadstart="onVideoLoad(activeLecture.id, $event)"
>
// ...
// ...
function onTimeUpdate(videoID: string, event: any) {
  const videoCookie = useCookie("currentVideo");
  const timeCookie = useCookie("currentVideoTime");

  const currentVideoTime = event.target.currentTime;
  timeCookie.value = currentVideoTime;

  if (currentVideoTime > 0) videoCookie.value = videoID;
}

function onVideoLoad(videoID: string, event: any) {
  const videoCookie = useCookie("currentVideo");
  const timeCookie = useCookie("currentVideoTime");

  if (!videoCookie || videoCookie.value == "" || !timeCookie || timeCookie.value == "") return;

  if (videoCookie.value !== videoID) {
    videoCookie.value = "";
    timeCookie.value = "";
    return;
  }

  event.target.currentTime = timeCookie.value;
}

return { videoSRC, refSource, onTimeUpdate, onVideoLoad };
// ...

@BugsSindFeatures
Copy link
Contributor Author

Thank you for the suggestions. :)

  1. if (currentVideoTime > 1) ~than save videoID~ was originally to reduce console logs for testing purposes and i forgot to change that.

  2. I used currentvideotime.removeItem because Visual Studio Code suggested this as an option and somehow the code worked.

Your Code looks far more readable and I couldn't find any problems with it, so I adopted it.

Lauritz-Tieste
Lauritz-Tieste previously approved these changes Oct 27, 2024
Copy link
Contributor

@Lauritz-Tieste Lauritz-Tieste left a comment

Choose a reason for hiding this comment

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

I don't have the rights to merge your code, but @Defelo can.

@TheMorpheus407
Copy link
Contributor

It looks like the issue is with the logic in the onVideoLoad function. Specifically, when you load a new video, the currentTime of the video element is immediately set to the value in timeCookie, which may still be holding the time from the previously played video if it was saved just before the new video loaded.

Here’s what happens in your code:

  1. When a video is playing, onTimeUpdate continuously updates timeCookie with the current time.
  2. When you switch to a new video, onVideoLoad immediately sets the currentTime based on the timeCookie, which still has the previous video's time stored in it.

Solution

To avoid this, you can reset the timeCookie when a new video is selected before it starts updating the currentTime. Here’s how:

  1. Reset timeCookie to 0 or an empty string in onVideoLoad if the videoID doesn’t match videoCookie.
  2. This ensures that the new video starts at 0 if it’s different from the previous video.

Updated Code

function onVideoLoad(videoID: string, event: any) {
  const videoCookie = useCookie("currentVideo");
  const timeCookie = useCookie("currentVideoTime");

  if (!videoCookie || videoCookie.value === "" || !timeCookie || timeCookie.value === "") return;

  if (videoCookie.value !== videoID) {
    // Reset the time cookie to start the new video from the beginning
    timeCookie.value = 0;
    videoCookie.value = videoID;
  } else {
    // Set the current video time to the saved cookie value
    event.target.currentTime = timeCookie.value;
  }
}

This adjustment should ensure that only if the video ID matches the previously saved video, it will use the currentVideoTime value. Otherwise, it will start fresh for any new video.

@BugsSindFeatures
Copy link
Contributor Author

BugsSindFeatures commented Oct 30, 2024

Well, this was a mistake, I'll correct that.

@BugsSindFeatures
Copy link
Contributor Author

Thanks for the suggestions and explanations. I have implemented the updated Code, but I have accidentally force-synced the develop branch with the official develop branch, which lead to this chaos here, sorry. :(

@TheMorpheus407
Copy link
Contributor

hm, that didnt fix it sadly. Everything works, except for one thing:
So the issue is the following:
I am watching eg video 1 and I am at 2:45.
I do NOT click pause, I just click on the next video, say video 2
Video 2 will start at 2:45

@BugsSindFeatures
Copy link
Contributor Author

I think the problem is because i forgot to restore the "key" line. I hope that fixed it now.

Copy link
Contributor

@Lauritz-Tieste Lauritz-Tieste left a comment

Choose a reason for hiding this comment

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

Please first make sure that all checks were successful, which is not the case here.

Lauritz-Tieste
Lauritz-Tieste previously approved these changes Oct 31, 2024
Copy link
Contributor

@Lauritz-Tieste Lauritz-Tieste left a comment

Choose a reason for hiding this comment

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

Ready for final review from @Defelo

@Lauritz-Tieste Lauritz-Tieste requested a review from Defelo October 31, 2024 19:01
@github-actions
Copy link

Preview deployed to https://e1927485.academy-preview.pages.dev (total size: 14M)

Copy link
Contributor

@TheMorpheus407 TheMorpheus407 left a comment

Choose a reason for hiding this comment

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

looks good, tests went well, code approved from my end :)

@Defelo Defelo merged commit 9c32221 into Bootstrap-Academy:develop Nov 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keep the time of a video when switching to questions and back

4 participants