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

feat: add player MarkVideoAsWatched action #750

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

tymmesyde
Copy link
Member

No description provided.

@kKaskak kKaskak self-requested a review December 10, 2024 11:49
src/models/player.rs Show resolved Hide resolved
src/models/player.rs Show resolved Hide resolved
@@ -1052,10 +1077,7 @@ fn library_item_update<E: Env + 'static>(
meta_request: Some(meta_request),
..
}) => {
let library_item = library_item
.as_ref()
.filter(|library_item| library_item.id == meta_request.path.id)
Copy link
Member

@elpiel elpiel Dec 10, 2024

Choose a reason for hiding this comment

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

  • Before removing this we need to know why it was here in the first place and if this case still holds.

Please elaborate

Copy link
Member

Choose a reason for hiding this comment

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

Taking a second look after discussing the flow with Tim, I think this should be fine.
THe LibraryItem form the LIbrary will be the most up-to-date library item as we send any update to be updated from the Ctx

Copy link
Contributor

Choose a reason for hiding this comment

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

for the first 90 seconds of the Player lifecycle a library_item exists only in the Player, but not in ctx.library. (if you watch a movie for the first time ever)
All changes made to that temp library_item during those 90 seconds are gonna be lost. (in case youve marked video as watched)

Copy link
Contributor

@nklhtv nklhtv Dec 11, 2024

Choose a reason for hiding this comment

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

also self.push_library_item_time should be reset on Unload i think.
Perhaps you get library item from the ctx.library every time bc you dont reset push_library_item_time, which causes the library_item to be persisted even on the first TimeChanged action.
This is a case which i think we wanted to avoid, watching less than 90 seconds of stream should not end up in your library(only as temp library_item)

Copy link
Member

Choose a reason for hiding this comment

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

And "only as temp" meaning "existing only in the player until those 90 seconds end"?

At which point it will place the library in your Library Bucket.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the library_item exits in the player for the first 90 seconds (or until you dispatch some action that triggers UpdateLibraryItem, like markvideoaswatch etc)
but with this change it recreates library_item from meta_item every time, until its not actually persisted in the library bucket

src/models/player.rs Show resolved Hide resolved
@elpiel
Copy link
Member

elpiel commented Dec 11, 2024

✔️ Tested and works with stremio-web build.

@kKaskak
Copy link
Member

kKaskak commented Dec 11, 2024

Tested and works well in the web app ✅

@elpiel elpiel merged commit ef64789 into development Dec 11, 2024
5 checks passed
@elpiel elpiel deleted the feat/player-mark-video-as-watched branch December 20, 2024 06:59
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.

4 participants