Skip to content

fix: Add immediate tag to MediaSources watcher #2205

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

Merged
merged 2 commits into from
Jan 10, 2024
Merged

Conversation

EffakT
Copy link
Contributor

@EffakT EffakT commented Jan 10, 2024

When viewing an episode/item page, the MediaSources watcher does not trigger instantly, causing the Video, Audio, and Subtitles dropdowns to not show until the MediaSources are updated again.
Watchers by default are lazy, when in this instance it should be eager and trigger when it attaches.

Adding the immediate tag makes this watcher eager, resolving the issue.

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@ferferga
Copy link
Member

ferferga commented Jan 10, 2024

@EffakT Sorry for pushing to the branch, but I think it was quicker doing it than entering into the review loophole.

With the migration to Vue 3 and composition API, I discovered that previously we abused watchers way too much for things that can be computed! Computed properties are awesome and paired with setters they're usually underlooked by beginners (like we were at the beginning).

  • They're explicit: you know exactly when and how the property change
  • They're faster (and this). I know that there are a lot of quirks here and then, but as we go, I try to do as many microoptimizations as possible.
  • You can even handle the previous value more easily than with a watch since Vue 3.4: computed((previous) =>

There are a lot of places where we still have this kind of pattern, since we focused past in the day in getting Vuetify and Vue 3 up and running as soon as possible and a lot of refactors and simplifications that previously were impossible due to Options API are now possible.

If you don't want this to be your first and only PR, this is your chance :)
I'll approve but wait for your feedback so you can confirm me that everything works as with the immediate watcher.

PD: This is another one of those remnants of the pre-Vue 3 migration, since before all data was fetched after mount (so the watcher fired afterhand) and now the data is fetched before mount, so the watcher doesn't fire.

@jellyfin-bot
Copy link

Cloudflare Pages deployment

Latest commit 4e34c6f
Status ✅ Deployed!
Preview URL https://jf-vue.pages.dev (https://5e161e93.jf-vue.pages.dev)
Type ⚙️ Production

View build logs
View bot logs

@EffakT
Copy link
Contributor Author

EffakT commented Jan 10, 2024

@ferferga No worries, was just in a bit of a hurry this morning so quickly patched it so it was at least working.
Using computed is absolutely a better idea. Though in the past I've run into issues when I had to use watchers instead of computed due to data structure issues (guessing this is one of the quirks you are talking about).

Looks like this is working as expected now.

@ferferga ferferga merged commit 94c8090 into jellyfin:master Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants