Skip to content
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

ExtrasItem_Unknown_Role #1034

Closed

Conversation

candry7731
Copy link
Contributor

Fix Actor role that is unknown.

Currenlty when a actor role is unknown the subtitle field is "as"

This changes that feild to be "as Unknown"

Changes
Add if statement
Update tr file from "Unknow" to "as Unknown"

Fixes #1032

@sevenrats
Copy link
Member

I feel strongly that this should just empty the line out. Having a whole cast of "As Unknown" looks really bad imho. Can you help me understand why you think "As Unknown" is the better option?

@candry7731
Copy link
Contributor Author

@sevenrats What do you think about sorting the unknowns to the end of this list? However, I do agree either get rid of the subtext or add unknown.... Either way is fine for me.

#1032 (comment)

@cewert
Copy link
Member

cewert commented Feb 22, 2023

The role is unknown but we still know the person is an actor. How about we make the second line just say "Actor" like it does for Director etc.? That or a blank line would be my vote.

Whatever we decide, consider editing ExtrasRowList.brs instead of ExtrasItem.brs. I think that would be easier. This if statement to be specific https://github.com/jellyfin/jellyfin-roku/blob/unstable/components/extras/ExtrasRowList.brs#L75-L79

If you want to go with "Actor" i think this code would work on line 75
if person.json.type = "Actor" and person.json.Role <> invalid and trim(person.json.Role) <> ""

@sevenrats
Copy link
Member

Without having a good understanding of the types of data that might be missing, I think both of these are risky. I don't see any reason why non-actor roles couldn't be unknown, especially in the case of media before modern rules about credits. I superficially like the idea of sorting the unknowns to the end, but, i think this is solving an issue that is not present. If only a few roles are as "Unknown" then "as Unknown" is not especially "wrong" looking. Sorting them to the end would not solve the problem that does happen, which is when all the roles are unknown and it looks distinctly "wrong." I still think the best option is a blank line.

@cewert
Copy link
Member

cewert commented Feb 22, 2023

@sevenrats I don't understand what you mean by risky? Did you have a look at the code I linked? the second line, at least for the cast and crew section, will only be "as role" when person.json.type = "Actor" and person.json.Role <> invalid. If the person isn't an actor it prints person.json.Type. Role isn't used for non-actors. Candry was talking about showing "unknown" instead of the role. I'm talking about showing the type for actors as well when role is blank

@sevenrats
Copy link
Member

@cewert oh right. Yeah I did not understand this as well as I thought. It also occurred to me after that the gender implications of "Actor" may present a burden as well. Do you have an opinion on this? I just can't escape the elegance of an empty line, in this case.

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.

Add "unkown" to cast role if meta is missing
3 participants