siku2 / script.service.sponsorblock

Kodi add-on for SponsorBlock
MIT License
124 stars 14 forks source link

refactor: restructure multiple addon support #49

Closed SethFalco closed 1 year ago

SethFalco commented 1 year ago

If you're happy with this, please test it on the YouTube app first before merging! This makes substantial changes and may introduce regressions! I've only tested it against Invidious!


In my original PR adding Invidious support, I left an error in which I've been tackling. The error is due to failing to parse the video init notification coming from the plugin.video.invidious, since it doesn't have the same schema as plugin.video.youtube.

See:

Rather than simply work around the notification, I thought it would be worth restructuring the project so that it's easier to maintain support for multiple addons.

What we had before was suitable for a minimal integration with Invidious, but we already have mismatching file names to functionality with Invidious code living in youtube_api.py. This will continue to get more messy as we have more addon specific behavior, or want to support more addons like plugin.video.tubed.

See:

This proposes that we create a new directory called apis with models, abstractions, and implementations that with hold addon specific implementation details in the respective file.

What this PR solves:

Chores done on the side:


I appreciate this is quite a radical change, so critiques are welcome! It may also be that further restructuring would help, for example if there is more integration-agnostic behavior in youtube_api.py. I'm not familiar enough with everything it's doing to know what can be pulled out, though.

SethFalco commented 1 year ago

Rebased, and made a small change.

AbstractApi#parse_notification_payload can now return None, and I added a check in Monitor#onNotification to exit early if the NotificationPayload is None.

It's negligible, but simplifies integration code a little. Using the tuple to knowingly store (None, None) is redundant anyway. This change also makes it the integration feel marginally simpler imo.

i.e. return None is more intuitive than return NotificationPayload(None, None), which one would have to copy from elsewhere or read the docs for, if they can't support preloading or just don't know how to populate it.

Welcome to critique this change. :+1:


PS: I'll update my sample implementation on tickets regarding plugin.video.tubed to match the new interface when this is merged.