X Tutup
Skip to content

Support showing content ratings for TmdbProvider#705

Merged
LagradOst merged 12 commits intorecloudstream:masterfrom
Luna712:content-ratings
Dec 9, 2023
Merged

Support showing content ratings for TmdbProvider#705
LagradOst merged 12 commits intorecloudstream:masterfrom
Luna712:content-ratings

Conversation

@Luna712
Copy link
Contributor

@Luna712 Luna712 commented Oct 21, 2023

Resolves #613

Copy link
Contributor

@Blatzar Blatzar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't mind me, just spreading the niceness of the built in kotlin functions 🤤

Nothing you need to change

@Luna712 Luna712 requested a review from Blatzar October 23, 2023 19:39
@KingLucius
Copy link
Contributor

Thanks for this PR, I need this so much

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 24, 2023

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.

@KingLucius
Copy link
Contributor

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.
Good luck 👍

@Luna712 Luna712 mentioned this pull request Oct 24, 2023
3 tasks
@Luna712
Copy link
Contributor Author

Luna712 commented Oct 24, 2023

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. Good luck 👍

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?

@KingLucius
Copy link
Contributor

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. Good luck 👍

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.

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 24, 2023

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.

@fire-light42
Copy link
Collaborator

image

What updating mainapi does to a mf

@fire-light42
Copy link
Collaborator

So this probably wont get merged until a new stable release to not fuck with the current stable

@CranberrySoup
Copy link
Contributor

Make another constructor with identical parameters to the old one and it will work 👍

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 24, 2023

Make another constructor with identical parameters to the old one and it will work 👍

What do you mean?

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 24, 2023

So this probably wont get merged until a new stable release to not fuck with the current stable

@fire-light42

When do you think a new stable release will be?

@CranberrySoup
Copy link
Contributor

Make another constructor with identical parameters to the old one and it will work 👍

What do you mean?

Example: https://github.com/recloudstream/cloudstream/blob/master/app/src/main/java/com/lagradost/cloudstream3/utils/ExtractorApi.kt#L412-L426

If the old constructor also remains it will be backwards compatible

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 25, 2023

image

What updating mainapi does to a mf

@fire-light42

It should now maintain backwards compatibility

@CranberrySoup thanks a lot for the suggestion

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 25, 2023

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...

@fire-light42
Copy link
Collaborator

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

@Luna712
Copy link
Contributor Author

Luna712 commented Oct 25, 2023

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?

Copy link
Contributor

@Blatzar Blatzar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just needs testing to make sure the constructors are working 🙏

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 3, 2023

@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.

@Blatzar
Copy link
Contributor

Blatzar commented Nov 21, 2023

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 🤷
Having it say "Original Language: English" does not conform with how all the other data is displayed.

What would be really cool, but a lot harder to implement would be reviews.

@Blatzar
Copy link
Contributor

Blatzar commented Nov 21, 2023

@fire-light42

@Luna712
Copy link
Contributor Author

Luna712 commented Dec 9, 2023

@fire-light42 just bumping this... any update?

@fire-light42
Copy link
Collaborator

fire-light42 commented Dec 9, 2023

@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

@Luna712
Copy link
Contributor Author

Luna712 commented Dec 9, 2023

@fire-light42 just bumping this... any update?

Uhhh, we will see when stable drops

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.

@LagradOst
Copy link
Contributor

Yes, a stable is needed

@LagradOst LagradOst merged commit 3c152e0 into recloudstream:master Dec 9, 2023
@fire-light42
Copy link
Collaborator

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.

@Luna712
Copy link
Contributor Author

Luna712 commented Dec 10, 2023

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.

@fire-light42

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.

@Luna712 Luna712 deleted the content-ratings branch November 19, 2025 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for displaying content rating

6 participants

X Tutup