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

Play button and resume #605

Merged
merged 3 commits into from
Jan 26, 2021
Merged

Play button and resume #605

merged 3 commits into from
Jan 26, 2021

Conversation

heyhippari
Copy link
Contributor

(I forgot to open a PR for this one and the branch has been sitting there for a few days 😆 )

This allows resuming if an item is already in progress, and adds a "Play from the beginning" entry to the item menu, which shows up when you can resume an item.

@heyhippari heyhippari mentioned this pull request Jan 19, 2021
16 tasks
@codecov-io
Copy link

Codecov Report

Merging #605 (0e886ae) into master (1382ba4) will increase coverage by 4.91%.
The diff coverage is 2.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
+ Coverage    5.29%   10.20%   +4.91%     
==========================================
  Files         104      106       +2     
  Lines        2851     2929      +78     
  Branches      428      447      +19     
==========================================
+ Hits          151      299     +148     
+ Misses       2693     2614      -79     
- Partials        7       16       +9     
Impacted Files Coverage Δ
components/Item/Card.vue 0.00% <ø> (ø)
components/Item/ItemMenu.vue 0.00% <0.00%> (ø)
components/Item/PlayButton.vue 0.00% <0.00%> (ø)
components/Layout/HomeHeader/HomeHeaderItems.vue 0.00% <ø> (ø)
components/Players/PlayerManager.vue 0.00% <0.00%> (ø)
components/Players/VideoPlayer.vue 0.00% <0.00%> (ø)
mixins/itemHelper.ts 0.00% <0.00%> (ø)
pages/genre/_itemId/index.vue 0.00% <ø> (ø)
pages/item/_itemId/index.vue 0.00% <ø> (ø)
utils/playbackUtils.ts 9.09% <ø> (+9.09%) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1382ba4...0e886ae. Read the comment docs.

Copy link
Contributor

@camc314 camc314 left a comment

Choose a reason for hiding this comment

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

I seem unable to resume anything. The video interface just ends up as a black screen. I have checked the source url and it works.

pages/genre/_itemId/index.vue Outdated Show resolved Hide resolved
@heyhippari
Copy link
Contributor Author

I seem unable to resume anything. The video interface just ends up as a black screen. I have checked the source url and it works.

This is linked to streams being unseekable on master, I believe. Since you can't seek in the file, it'll start from the beginning even with the proper position passed to Shaka.

@camc314
Copy link
Contributor

camc314 commented Jan 19, 2021

I seem unable to resume anything. The video interface just ends up as a black screen. I have checked the source url and it works.

This is linked to streams being unseekable on master, I believe. Since you can't seek in the file, it'll start from the beginning even with the proper position passed to Shaka.

Ah, that would make sense, I'll roll back to RC2 and see whether it works with that.

I'm still getting the same results, playing from the beginning works fine, resuming results in a black screen

@heyhippari
Copy link
Contributor Author

I'll roll back to RC2 and see whether it works with that.

The webm issue is on our end, since web doesn't have the issue :p It's likely due to some change in the profile. Ours is very different compared to jf-web, which it shouldn't be :/

@@ -239,28 +238,30 @@ export default Vue.extend({
this.setLastProgressUpdate({ progress: new Date().getTime() });
break;
case 'playbackManager/SET_CURRENT_TIME': {
const now = new Date().getTime();
if (state.playbackManager.status === PlaybackStatus.playing) {
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for reporting progress. Technically, if we're paused, stopped or in error, there is no progress to report.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are also setting the current time in play() now, so before playback starts (This is needed to tell Shaka where to start in the stream).

So this also prevents an error when starting playback, due to playback not being started yet when this is called the first time.


this.setLastProgressUpdate({ progress: new Date().getTime() });
this.setLastProgressUpdate({ progress: new Date().getTime() });
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use now here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but that's not really the scope of this PR.

Copy link
Member

@ferferga ferferga Jan 23, 2021

Choose a reason for hiding this comment

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

@MrTimscampi I don't understand why it's outside the scope. You're already declaring const now in line 242 (as I mentioned above), why not use that instead? You're getting time two times from Date() when you already have it in a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is all untouched code though, it just has an indentation change :)

@ferferga ferferga added the merge conflict Something has merge conflicts label Jan 24, 2021
@heyhippari heyhippari removed the merge conflict Something has merge conflicts label Jan 26, 2021
@sonarcloud
Copy link

sonarcloud bot commented Jan 26, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@heyhippari heyhippari merged commit 332b232 into master Jan 26, 2021
@heyhippari heyhippari deleted the play-button-and-resume branch January 26, 2021 13:44
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.

None yet

4 participants