Support showing content ratings for TmdbProvider#705
Support showing content ratings for TmdbProvider#705LagradOst merged 12 commits intorecloudstream:masterfrom
Conversation
6db749b to
1c4c61a
Compare
1c4c61a to
69a4b3c
Compare
Blatzar
left a comment
There was a problem hiding this comment.
don't mind me, just spreading the niceness of the built in kotlin functions 🤤
Nothing you need to change
app/src/main/java/com/lagradost/cloudstream3/metaproviders/TmdbProvider.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lagradost/cloudstream3/metaproviders/TmdbProvider.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/lagradost/cloudstream3/ui/result/ResultFragmentPhone.kt
Show resolved
Hide resolved
|
Thanks for this PR, I need this so much |
You're very welcome. I have PRs ready for extensions to implement support to some like SoraStream as well but we'll see how far it goes as it will definitely need to be implemented in extensions for support, but there is a concern with that since LoadResponse.contentRating doesn't already exist if you add it before CS is updated it will break things. I have no idea how to handle that backwards compatibility so was thinking maybe after this is merged, wait a month or so before doing extensions or something like that. Maybe make a new stable release of CS also so it can be added there as well. |
Tmdbprovider needs some love indeed 👍, you have to be carefull when modifying the mainapi and always think about back compatability. |
Indeed, and thanks. I'm trying to think of anything else that might be good in the future to just add the variables now so at least we don't have to worry about the variables in the future again, but I can't think of anything else right now, can you? |
I can't think of something particular right now, i will check the tmdb apis what they are providing. |
|
I think maybe original language could be useful as it could 1) display the original language or even potentially allow filtering by it in the future using a language setting but not sure. |
|
So this probably wont get merged until a new stable release to not fuck with the current stable |
|
Make another constructor with identical parameters to the old one and it will work 👍 |
What do you mean? |
When do you think a new stable release will be? |
If the old constructor also remains it will be backwards compatible |
It should now maintain backwards compatibility @CranberrySoup thanks a lot for the suggestion |
|
I'm wondering if we should add descriptors for content ratings as well, this might allow some things in the future to use, I have quite a bit of ideas we could do to Tmdbprovider but the way MainApi works, not being very extendable makes it difficult as well... |
The problem is that the devs are using the wrong constructor. they should be using newTvSeriesSearchResponse that is built for this exact reason or the several other ones to construct it, but some devs use the built in constructor and this causes this issue |
Personally, I think that support to do it that way should be deprecated, and eventually removed. It causes a headache with CS development, and it isn't the way it should be done unless there is a real reason it has to be done the other way? |
…into content_ratings # Conflicts: # app/src/main/java/com/lagradost/cloudstream3/MainAPI.kt
Blatzar
left a comment
There was a problem hiding this comment.
LGTM just needs testing to make sure the constructors are working 🙏
|
@Blatzar thoughts on me adding original language to TmdbProvider also (in a separate PR maybe)? Maybe also content rating descriptors. Mostly so we don't have to change MainAPI again for this, at the very least, even if we don't add it to TmdbProvider itself just yet, maybe just to MainAPI constructors so we don't have to worry about breaking extensions again later if/when we add to TmdbProvider? Also if there is anything else we might want to add to TmdbProvider later on, I was thinking adding now might not be a bad idea since changing MainAPI has so many potential complications... Just a thought, content rating was always the most important one for me personally to get added anyway, so this is fine if nothing else. |
…into content_ratings
|
Only problem with original language I could see is that it would be confusing for extensions with another language dubbed. Then people might confuse that for the dub language 🤷 What would be really cool, but a lot harder to implement would be reviews. |
|
@fire-light42 just bumping this... any update? |
Uhhh, we will see when stable drops. Will ask lag and this will be merged before he releases it |
The point of this PR and backwards compatibility now is to support pre release also, so it can be done in stages, merge this, then stable, then update extensions to support it. But if you prefer to wait I am totally okay with that as well. Thank you! Backwards compatibility should probably be kept for at least one stable iteration anyway, maybe removed in the next stable update after the upcoming one but tnats up to you all also. I appreciate the response. |
|
Yes, a stable is needed |
|
Your mitigation for backwards compatibility was nice, however as suspected this also caused forwards compatibility issues where the providers updated but everyone on a lower version of the app cant use them. This is why we dont touch MainAPI unless we know that stable and prerelease users dont get screwed over for even for the most simple change. |
I wonder if we can add some sort of 'version' on MainAPI, that can be pulled and checked by extensions to allow them to maintain forward and backward compatibility in the future, using it would break it one last time and require updating, but it would be a way to prevent it again in the future I think. If not that, then I think something should be done to handle this and make it more extendable in the future... EDIT: now that I think of it this may not actually work anyway. I'm not certain though. |


Resolves #613