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

Detach video params from selected #533

Merged
merged 3 commits into from
Oct 13, 2023

Conversation

unclekingpin
Copy link
Contributor

@unclekingpin unclekingpin commented Oct 10, 2023

While integrating the the latest stremio-video into stremio-web, in order to start sending videoParams to subtitles addons, i found an issue with how stremio-core and stremio-web worked together.

  1. In stremio-web we reload stremio-video on each change of player.selected.
  2. Reloading stremio-video results in fetching the videoParams from the stremio-server.
  3. Updating the videoParams prop from stremio-video causes a new Load action for the Player model to be dispatched.
  4. Dispatching a new Load action to the Player model causes player.selected to change.
  5. And it all ends in an infinete loop.

The only way to integrate all parts together is by detaching the videoParams from the player's selected prop. (or a massive refactor down to the useCoreModel hook and potentially every route of stremio-web)

This change is a breaking change for stremio-android-tv. It needs to send this new PlayerAction::VideoParamsUpdated instead of new Load action once video params are ready. @TheBeastLT Does that look like a simple change or it will cause massive refactor on your side?

The functionality can be tested in stremio-web gh pages on this url: https://stremio.github.io/stremio-web/update-stremio-video/#/player/eJwVysENgDAIAMBdGKB8fHUbrAQbSyAFjYlxd+O974FzDqiwZ3pUxEibJFzETAaT9yjNFOWi1W5U3jphkPrg+G87ivoC7wel8Bou/https%3A%2F%2Fv3-cinemeta.strem.io%2Fmanifest.json/https%3A%2F%2Fv3-cinemeta.strem.io%2Fmanifest.json/movie/tt420/tt420
which load some stock video with an arbitrary tt prefixed id
In the network tab you can see the opensubtitles request with the correct params.

...

The PR is based on this commit because there are some changes in development branch which are not compatible with core-web's deveopment branch.

CI fails for some unrelated reason

@TheBeastLT
Copy link
Contributor

On Android TV side it should be easy enough change.

Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

Fix looks good to me.

I have 1 question regarding the code:
Do we actually need an Option for the new Action, seems to me you would only call this Action once the video params have changed and never with None?

@unclekingpin unclekingpin force-pushed the detach-video-params-from-selected branch from acc136f to 63f4cc8 Compare October 11, 2023 20:44
@unclekingpin
Copy link
Contributor Author

unclekingpin commented Oct 11, 2023

Fix looks good to me.

I have 1 question regarding the code: Do we actually need an Option for the new Action, seems to me you would only call this Action once the video params have changed and never with None?

There is a usecase for this, it can happen if you change the streaming server during playback, if you start casting to a chromecast or if you toggle a boolean query param forceTranscoding because in those cases we reload the video and eventually rerequest the videoParams

@elpiel
Copy link
Member

elpiel commented Oct 12, 2023

The only request I see for Opensubtitles is this one:
https://opensubtitles.strem.io/stremio/v1/q.json?b=eyJpZCI6MSwianNvbnJwYyI6IjIuMCIsIm1ldGhvZCI6InN1YnRpdGxlcy5maW5kIiwicGFyYW1zIjpbbnVsbCx7InF1ZXJ5Ijp7Iml0ZW1IYXNoIjoidHQ0MjAiLCJ2aWRlb0hhc2giOiIxZDI0YzhlNzM3MjUxYzdkIiwidmlkZW9TaXplIjo2NzMwNDg2fX1dfQ==

Is this using the VideoParam hash?

@elpiel elpiel merged commit dbf4bbc into development Oct 13, 2023
1 check passed
@elpiel elpiel deleted the detach-video-params-from-selected branch October 13, 2023 08:13
@elpiel elpiel mentioned this pull request Nov 7, 2023
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.

3 participants