-
Notifications
You must be signed in to change notification settings - Fork 702
feat: support episode alerts using next airing for metapoviders #1678
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
base: master
Are you sure you want to change the base?
Conversation
Good first contribution! While I like the feature the implementation has too much wet code. More of the EpisodeAlertWorker and SubscriptionWorkManager can and should be shared. Repetition leads to inconsistencies and a harder to maintain codebase. For example now only meta-providers gets "Season %d Episode %d released" as opposed to "Episode %d released!". Notification style and content should stay fully consistent between different subscription backends and should only need to be defined once. |
Thank you for the feedback. I'll work on making the suggested improvements. |
@fire-light42 ,could you check the changes made to see if further improvements are needed. I've tested it with trakt and another normal provider and there's consistency to the notification style. |
The code looks very good, but I have not tested it yet. However I do not see any apparent issue with merging it after testing, so is there anything you want to change before that? |
Thanks. I don't plan on changing things currently. If it checks out after testing you can proceed with merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue I found when testing and reading the code is that you only check for nextAiring != null
while scheduleEpisodeAlert also checks for nextAiring.unixTime > now
.
So I would make scheduleEpisodeAlert return a Boolean for if it is valid, or implement the same logic in the if statement. Additionally only nextAiring.unixTime > now
may be too weak of a condition, given that extensions may give random times, therefore something like && nextAiring.unixTime < now + 31_556_926L
(seconds in a year) would be a nice sanity check if some extension mistakenly report Long.MAX_VALUE.
Otherwise you can get java.lang.IllegalArgumentException: The given initial delay is too large and will cause an overflow!
I need a little bit of clarification on the first part especially on making scheduleEpisodeAlert return a Boolean. The reason I used I'll push what I have so far so you can review and correct any mistake. |
My idea was to make scheduleEpisodeAlert return true iff enqueueEpisodeAlertWorker gets called, and false if nextAiring is some null/garbage data. Then that boolean can be used to decide if the process of checking the episode should continue or not. The main issue this solves is that if the first guard passes, but the guard inside enqueueEpisodeAlertWorker does not, then there will be no notification at all. |
Changes to the last-seen episode key used by subscription manager doesn't work properly with meta-providers like trakt since they provide data for a whole season, making the last seen episode static throughout the seasons run(it remains the final episode throughout).
This pr looks to extend subscription manager by using a one-time-worker to schedule notifications for future episodes if loadresponse has a next-airing field which is common with metaproviders like trakt.