-
-
Notifications
You must be signed in to change notification settings - Fork 305
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
Prioritize backdrop image for cinematic background #893
Prioritize backdrop image for cinematic background #893
Conversation
I don't seem to have permission to label this PR. If there is a way to do it, please let me know! |
Thank you for taking a look at this, but I don't like the idea of another flag that just propagates throughout the app for only a single view usage. While the I'd be fine with the following workaround in // TODO: fix once Poster image sources are changed
private func backdropImageSources(from item: Item?) -> [ImageSource] {
(item?.landscapePosterImageSources(maxWidth: UIScreen.main.bounds.width, single: false) ?? [])
.filter { $0.url?.absoluteString.contains("Backdrop") ?? false }
}
var body: some View {
RotateContentView(proxy: proxy)
.onChange(of: viewModel.currentItem) { newItem in
proxy.update {
ImageView(backdropImageSources(from: newItem))
.placeholder {
Color.clear
}
.failure {
Color.clear
}
}
}
} Obviously it's a workaround observing the URL but at least it's contained. |
I completely understand the concern with the original solution. I did consider one similar to what you proposed; however, in the case that there is no backdrop photo, then no photo will show because we removed all the other fallback images from the array. I was hoping to not make this change an immediate liability, so I tried to dig into the app a bit more tonight. I have committed some new code that I think is much better. It does seem best to not continue to bloat the landscape images function on Base+Poster because it really seems to be designed for the poster itself and not for the cinematic background. To me it makes sense to require of whoever conforms to Poster to define their cinematic photos separately. With that being said, I don't know what all the logic should be on the cinematic poster function. Most specifically, I don't know if this: I appreciate you working with me and being patient as I try to learn the project better. Do let me know if you would rather go a different direction with this. I'll be happy to adjust as needed. Thanks! |
I'm actually fine with that, because we should only show backdrop images in this view.
It does not, that should only be used for the landscape posters for episodes, at least by description for now. I may remove this in the future and just have episodes use their backdrop image. However I still have to think about this since sometimes episode backdrops aren't the same quality as the series backdrops in my experience... I will have to see again how Apple TV does this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm overall fine with this approach for now, as I may change how posters work overall and I think that this is good to have while I work on that as a reminder.
Since other types don't need/use the cinematic view, I would rather define the default conformance on Poster
:
extension Poster {
func cinematicPosterImageSources() -> [ImageSource] {
[]
}
}
and the conformances can be removed from BaseItemPerson
and ChapterInfo.FullInfo
.
Thanks again for the great feedback! This all makes a lot of sense. I have updated the code to reflect your requested changes. Everything looks good in my testing. |
This is a solution for #853.
The ImageView selects the first source from the array. This change allows the caller to get the backdrop image at the front of the array so that it will be displayed if it exists.