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

Move to using script.module.routing #103

Closed
dagwieers opened this issue Mar 26, 2019 · 7 comments · Fixed by #318
Closed

Move to using script.module.routing #103

dagwieers opened this issue Mar 26, 2019 · 7 comments · Fixed by #318
Labels
enhancement New feature or request question Further information is requested
Milestone

Comments

@dagwieers
Copy link
Collaborator

dagwieers commented Mar 26, 2019

Describe the bug
Yesterday I stumbled on plugin.video.fosdem and I was charmed by how simple and easy the code was to understand. Now I am convinced we ought to be using script.module.routing as well (I added it to the refactoring roadmap).

See https://github.com/jelly/plugin.video.fosdem/blob/master/addon.py#L56

More information related to script.module.routing:

@dagwieers dagwieers added the enhancement New feature or request label Mar 26, 2019
@pietje666
Copy link
Collaborator

Actually when i saw your improvement i wanted to post that i know that this exists. But i never came to implement it. But it is alot better indeed. Just never came to implement it

@dagwieers
Copy link
Collaborator Author

dagwieers commented Mar 26, 2019

It is not bad to take small steps as long as we know where we are heading at.
And only rarely you are in the situation that everyone has the same overall vision.
I feel it is the case in this project. That helps :-)

Just to say that this is not a priority, but maybe at some point it is easier to do this than continue as before.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Apr 1, 2019

So looking into this a bit deeper, if we go along this path it will break every internal URL from the past (so favourites and watched information will break). So this will probably be v2.0.0 material.

We also have to come up with a design for the different routes, I would prefer something more coherent than what we already have. Something like this:

  • / (aka. main-menu)
  • /categories
  • /categories/<category>
  • /channels
  • /channels/<channel>
  • /livetv
  • /livetv/<channel>
  • /favorites (My favorites)
  • /favorites/offline (My soon offline)
  • /favorites/programs (My A-Z listing)
  • /favorites/recent (My recent items)
  • /offline
  • /offline/<page>
  • /play/id/<video_id>
  • /play/video/<video_url>
  • /programs (A-Z listing)
  • /programs/<program> (season or episode listing)
  • /programs/<program>/<season> (season-episode listing)
  • /recent
  • /recent/<page>
  • /tvguide
  • /tvguide/<date>
  • /tvguide/<date>/<channel>
  • /tvguide/yesterday
  • /tvguide/today
  • /tvguide/tomorrow

These entries would be self-sufficient and reuse more generic functionality. And one benefit, these URLs are easy for people to call externally. (e.g. to directly jump to a live channel, or today's TV guide).

In this scenario we also need to foresee exception handling for no-longer available programs etc.

@mediaminister
Copy link
Collaborator

mediaminister commented Jun 11, 2019

Like a said in #300 (comment):
The current play interface needs two parameters for VOD: video_id and publication_id.
So if I follow your syntax you suggested above, this will result in the following urls:

This will work with the current workflow of the add-on:

VOD without web scraping (fast start of playback):
/play/id/<publication_id>/<video_id>
/play/id/pbs-pub-10a01230-3b0b-4921-9936-cfa7e97f7dd0/vid-b133c2ad-d847-4eaa-8158-92fd9f072c8a

VOD with web scraping (slower start of playback):
/play/video/<video_url>
/play/video/https://www.vrt.be/vrtnu/a-z/de-campus-cup/1/campus-cup-s1a9/

LIVE without web scraping (fast start of playback):
/play/id/<video_url>
/play/id/vualto_een_geo

LIVE with web scraping (slower start of playback)
/play/video/<video_url>
/play/video/https://www.vrt.be/vrtnu/kanalen/een/

But, this doesn't really improve the current interface. Asking for video_id and publication_id or a full video_url is still very user unfriendly.

If we completely rethink the add-on workflow, we could make the following work:

/play/<program>/<season>/<episode>
/play/de-campus-cup/1/9

Or a live channel
/play/een

I will investigate this second option.

@dagwieers
Copy link
Collaborator Author

Yes, I would prefer we have a unique sequence for each different input:

  • /play/id/<pub_id>/<vid_id>
  • /play/url/<vid_url>
  • /play/vualto/<vualto_string>

But I prefer the second interface, if that is at all possible. We should make something that we can be sure will work regardless of what backend changes will come in the future. Even if that means translating to/from before/after a call.

@mediaminister
Copy link
Collaborator

mediaminister commented Jun 16, 2019

I investigated the second interface but for a lot of programs I couldn't ensure that the play url's were completely unique. VRT Search api is a real mess, some programs have no or a wrong episode number and I couldn't find a unique correspondence between a vrtnu url and titles or shortdescription variables.

So, I implemented the first interface and made a pull request.

With this new interface, automation fans will have fun:
For instance:

Open a specific menu

curl -X POST -H "content-type:application/json" http://localhost:8080/jsonrpc -d '{"jsonrpc": "2.0", "method": "GUI.ActivateWindow", "params": {"window": "videos", "parameters": [ "plugin://plugin.video.vrt.nu/programs/fiskepark" ]}, "id": "1"}'

Directly play a vrtnu url

curl -X POST -H "content-type:application/json" http://localhost:8080/jsonrpc -d '{"jsonrpc": "2.0", "method": "Player.Open", "params":{"item": {"file" : "plugin://plugin.video.vrt.nu/play/url/https://www.vrt.be/vrtnu/a-z/de-zevende-dag/2019/de-zevende-dag-d20190616/" }}, "id": "1"}'

Directly play a livestream (classic tv zapper)

curl -X POST -H "content-type:application/json" http://localhost:8080/jsonrpc -d '{"jsonrpc": "2.0", "method": "Player.Open", "params":{"item": {"file" : "plugin://plugin.video.vrt.nu/play/id/vualto_canvas_geo" }}, "id": "1"}'

And definitely a lot more.

@dagwieers
Copy link
Collaborator Author

I am genuinely happy we did this, sooner rather than later. But since this breaks existing bookmarks and forgets what was watched we may want to communicate this differently. We have a few options (in order of importance/likeliness):

  1. Communicate beforehand on Facebook, or as part of the changelog/addon description
  2. Provide a one-time popup for people that upgraded the addon (can we intercept/detect this ?)
  3. Add a backward-compatibility layer for existing bookmarks so they keep on working
  4. Rewrite the database to reformat watched items from VRT NU

But the way it looks today, I think we can prepare for a v2.0.0 release, rather than do a v1.10.1 update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
3 participants