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.

Sign up for GitHub

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

Jump to bottom

Implement video player in Vite branch #1878

Merged
merged 8 commits into from
Feb 28, 2023
Merged

Implement video player in Vite branch #1878

merged 8 commits into from
Feb 28, 2023

Conversation

ThibaultNocchi
Copy link
Member

@ThibaultNocchi ThibaultNocchi commented Jan 31, 2023 •
edited
Loading

  • Remove unneeded logs
  • Remove unneeded CSS styles and classes
  • Skim over the code with a clear mind

@ThibaultNocchi ThibaultNocchi marked this pull request as draft January 31, 2023 12:04
@jellyfin-bot jellyfin-bot added the vue Pull requests that edit or add Vue files label Jan 31, 2023
ferferga
ferferga requested changes Jan 31, 2023
View reviewed changes
Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

Aside from all these (quick) comments. Why you created your custom components instead of using the templates I set here and here

I don't see why OsdPlayer must be a component, I don't see them being reusable at all, given that the mini player OSD is really different to the fullscreen video player and I'd rather not have another component filled with v-if and such.

I'm even in favor to break everything as much as possible, and make individual components for the play/pause, back, forward, repeat and shuffle buttons. Yes, they're really simple, but having them reimplemented multiple times is error and inconsistency prone.

frontend/src/components/Playback/PlayerElement.vue Outdated Show resolved Hide resolved
frontend/src/plugins/vuetify.ts Outdated Show resolved Hide resolved
frontend/src/store/playbackManager.ts Outdated Show resolved Hide resolved
frontend/src/store/playbackManager.ts Outdated Show resolved Hide resolved
frontend/src/store/playbackManager.ts Outdated
@@ -793,7 +815,7 @@ class PlaybackManagerStore {
!isNil(this.currentItem.Id) &&
!isNil(remote.auth.currentUser)
) {
await this._reportPlaybackStopped(this.currentItem.Id);
this._reportPlaybackStopped(this.currentItem.Id);
Copy link
Member

Choose a reason for hiding this comment

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

await is needed, otherwise the promise won't work Why this change? The timeout already lets us be async without touching the original function.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you await the reportPlayback, then when you close the player there's a non null time where nothing happens (cause it's waiting for the API call to end) before the players closes.

We could just return the promise sent by the reportPlayback then.

frontend/src/components/Playback/PlayerElement.vue Outdated Show resolved Hide resolved
frontend/src/components/Playback/PlayerElement.vue Outdated Show resolved Hide resolved
@ThibaultNocchi
Copy link
Member Author

  1. Completely missed the video player page, I'll move everything to it.
  2. I think the OSD logic being separated avoids us having a big video page script, and it being a component doesn't change its v-if needs (or not). So I don't really agree that's it can't be split, even if it won't be reused. The player element logic + template is already complicated, even if short, that we don't need to add the OSD logic, style and template to it.

@ThibaultNocchi ThibaultNocchi force-pushed the feat/video-player branch 12 times, most recently from 0b85f15 to a03eb41 Compare February 6, 2023 07:41
@jellyfin-bot jellyfin-bot added the merge conflict Something has merge conflicts label Feb 6, 2023
@ThibaultNocchi ThibaultNocchi force-pushed the feat/video-player branch 2 times, most recently from fd211d0 to 641d7e9 Compare February 6, 2023 18:40
@jellyfin-bot jellyfin-bot removed the merge conflict Something has merge conflicts label Feb 6, 2023
@ThibaultNocchi ThibaultNocchi force-pushed the feat/video-player branch 7 times, most recently from 24328fa to d3a106f Compare February 7, 2023 11:50
@jellyfin-bot jellyfin-bot removed the merge conflict Something has merge conflicts label Feb 23, 2023
@ThibaultNocchi ThibaultNocchi force-pushed the feat/video-player branch 2 times, most recently from 48eb2ea to 060f889 Compare February 23, 2023 22:32
@ThibaultNocchi ThibaultNocchi marked this pull request as ready for review February 23, 2023 22:32
@jellyfin-bot jellyfin-bot added the merge conflict Something has merge conflicts label Feb 23, 2023
@jellyfin-bot jellyfin-bot removed the merge conflict Something has merge conflicts label Feb 23, 2023
@sonarcloud
Copy link

sonarcloud bot commented Feb 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jellyfin-bot
Copy link

Cloudflare Pages deployment

Latest commit d750a76
Status ✅ Deployed!
Preview URL https://9aff9275.jf-vue.pages.dev
Type 🔀 Preview

View build logs
View bot logs

@ferferga ferferga merged commit ec6b581 into vite Feb 28, 2023
@ferferga ferferga deleted the feat/video-player branch February 28, 2023 23:03
@ferferga ferferga added this to the Vue 3 milestone Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aweebs aweebs aweebs left review comments

@ferferga ferferga ferferga approved these changes

Assignees
No one assigned
Labels
vue Pull requests that edit or add Vue files
Projects
None yet
Milestone
Vue 3
Development

Successfully merging this pull request may close these issues.
4 participants
@ThibaultNocchi @jellyfin-bot @aweebs @ferferga