search

smirgol / plugin.video.crunchyroll

Watch videos from the anime platform Crunchyroll.com on Kodi
GNU Affero General Public License v3.0
49 stars 10 forks source link

feat: Add resume menu #17

Closed lumiru closed 9 months ago

lumiru commented 9 months ago
smirgol commented 9 months ago

Even though you have only 3 translations, which is fine, you still need to add it to the other translation files I think. Just leave the translation empty, like this:

msgctxt "#30047"
msgid "Resume"
msgstr ""
smirgol commented 9 months ago

Sorry about the comments mess, I'm still figuring out how that is properly done in my IDE :)

lumiru commented 9 months ago

Sorry about the comments mess, I'm still figuring out how that is properly done in my IDE :)

Don't worry, I am responsible of that since my commits are messy too. 🤐

I will do my best for the next PRs.

smirgol commented 9 months ago

We might have to take another look at the seasons count. I have some strange readings on some animes, with like Season 56, where there are definitely not that many seasons - I wish there were, but there aren't. :) Ancient Magus Bride with SeriesID GRZXQJJ8Y as an example - there it reads 36 seasons.

Edit: Looks like it's one of those Crunchyroll things. This happens for special seasons, where they have chosen a high season number instead of having an option to leave it blank. The affected seasons are labelled the same way on Crunchy as they are in Kodi now, so there is probably not much we can change.

smirgol commented 9 months ago

All in all it worked good yesterday when I tested it. I like the new "Resume" Option, as it makes it a bit easier to identify updated shows when having a large watchlist, like I do. Just a few things.

  1. I have mixed feelings about the change of the prepended numbering. Somehow I don't really like it, but it's hard to describe why. I've personally never missed the information in the title, which season it is and somehow it doesn't read as nicely as before. That there is no season title might add to that feeling. Maybe it's just the way of formatting that puts me off. I might change that after a merge, not sure yet. Just wanted to point it out.
  2. We definitely need to prepend the season name on the watchlist and the resume menu
  3. I'd move the new resume menu up, right after the watchlist. It makes more sense to me to have them next to each other.

All in all I like the change, it adds something useful to the plugin. Thank you very much for your work! :)

I'll have to see when to merge it, as right now there is a kind of feature freeze, as I wanted to try to merge my changes back to MrKrabat and there are so many changes already. :) Unfortunately, he hasn't replied yet if he is willing and able to continue working on the plugin. Based on that I either merge it with my work and continue the plugin, or create a PR to his repository with the current changes and then add another one for this change.

smirgol commented 9 months ago

This time it's oops for me :) I did want to clean up the branches and this seemed to have closed the PR. I've pushed my dev branches as a new target for the PRs. No idea how I do re-open this one and point it to the new branch - maybe you can also edit this one, but I can't. I either pull your changes later and merge it locally for testing, or you can create a new or update this PR.

smirgol commented 9 months ago

I have now merged your work and pushed it to the staging branches. Thank you for your contribution! :)