search

kodi-svtplay / xbmc-svtplay

Unofficial addon for watching SVT Play on Kodi
GNU General Public License v3.0
66 stars 34 forks source link

Prefix item title with season title if condtions met #264

Closed bohmandan closed 3 years ago

bohmandan commented 4 years ago

Some samples of the before - after outcome. The greatest benefit is displayed in the last example, where episodes is displayed from multiple seasons.

! Note item.positionInSeason is used to determine if it is any "value" for the end-user to set season title. For example, for the show "Greta Gris", the season title was simply "Kortisar" which made no sense prefixing the episode titles with.

btw, could you please add topic "hacktoberfest"? 🙂 🙏 "Maintainers of the repository can add the "hacktoberfest" topic to their repository if they wish to participate." https://hacktoberfest.digitalocean.com/

I also fixed an issue with episodes roaming around without a show title under Genres, see commit https://github.com/nilzen/xbmc-svtplay/pull/264/commits/ece239c5e917a091e045d459a12210fdd7759451 and screenshots 007 - Before and 008 - After.

Tests works fine, no changes/additions to tests however.

001 - Before 001-before 002 - After 002-after

003 - Before 003-before 004 - After 004-after

005 - Before 005-before 006 - After 006-after

linqcan commented 4 years ago

Thanks for contributing!

I like the rationale for this change but I'd prefer the solution to not just be an override of the title string from the API, instead I would like to see:

The reason for this is that I would have anything related to UI (how titles etc are presented) in the svtplay module instead of being decided by the API module.

Let me know if you have any questions!

bohmandan commented 4 years ago

Sure thing! Update to come...

bohmandan commented 4 years ago

Ok updated. The screenshots above still apply. I have verified and I dont find any category/channels etc that looks strange now.

(I also looked into adding season title in for example "Popular" list but it seems the value is not available for that call, from SVT Play.)

bohmandan commented 3 years ago

Good job!

I have some comments however as after reviewing the code I believe only VideoItem should have the attribute.

Also, please run the tests if you haven't. My "ocular compilation" tells me that this PR doesn't run for genres for example (?).

Ok! Some more commits is in place. The tests works fine. I could not see that this would apply for Genres. (If you open up a serie from a Genre however, it works fine). But I have however fixed another issue with episodes directly under a Genre (Långkörare) that I found while testing - see screenshots.

I have not added any test case for my alterations, not sure if that is something you would want? It seems the tests is kept a bit less detailed then that.

linqcan commented 3 years ago

Except my comment about removing the "genre commit" I think it looks good. Well done! Thank you for spending time improving the addon! 🙏

bohmandan commented 3 years ago

Ok! commit removed, good reason as you mentioned.