Update resume state when you press back when watching movie

1hitsong commented 3 months ago

Changes

Updates resume point for movie when you return to Movie Detail view from Video Player
Updates poster image with new resume point data using transition to prevent flicker

Issues

Fixes #1647

jellyfin-bot commented 3 months ago

This pull request has merge conflicts. Please resolve the conflicts so the PR can be reviewed. Thanks!

1hitsong commented 3 months ago

So, what's our path forward on this one? It feels like cewert and I are currently at an impasse on both the code and refresh approach.

anthonylavado commented 3 months ago

To make the most informed comment, I would need to test this PR and really dig in to the code. As it stands from a quick overview, it looks like the update is written, and might only need some possible comments as it is.

As it is an enhancement, would it not be possible to merge this and then discuss code standards later? If it turns out there is more to address, it could continue to be enhanced at a later point.

There have been a lot of changes across the client apps and client app teams to enhance the speed/velocity of releases, we shouldn't block that further.

cewert commented 3 months ago

Sorry for lack of response. Feel free to dismiss my review if needed.

As it is an enhancement, would it not be possible to merge this and then discuss code standards later? If it turns out there is more to address, it could continue to be enhanced at a later point.

This is tagged as bugfix which means after it's merged it will be in prod for the next bugfix release. I think that's where the disconnect is coming from because I was reviewing this PR as if it were being pushed to prod after merging. We wouldn't be able to enhance it until the next minor release.

To recap my thoughts:

Adding a short circuit to the update function just to speed it up is an enhancement and not part of the bug fix. The goal of the PR can be accomplished without hardcoding what movie data gets updated and what doesn't.
I strongly disagree with implementing new coding standards in a bugfix without any previous discussion with the team. The enum stuff isn't necessary to accomplish the goals of the PR.
There should be a standard way for views to deal with state. We used isFirstRun in previous bugfixes and I believe we should use it here too. If the team decides enums are better, then I think that should be a separate enhancement PR that converts all views to use enums isntead of isFirstRun.

1hitsong commented 3 months ago

I too see this as a bug fix, not as an enhancement.

anthonylavado commented 3 months ago

My apologies, I was using "enhancement" in the general sense to mean "improvement", and not in the context of any programming or project management methodology.

jellyfin-bot commented 2 months ago

This pull request has merge conflicts. Please resolve the conflicts so the PR can be reviewed. Thanks!

cewert commented 2 months ago

Closing in favor of #1771

jellyfin / jellyfin-roku

Update resume state when you press back when watching movie #1751

Changes

Issues