commit be43819de2b1183d2f739c87de436d27ba025935
parent 58017952bc0d396fba561332331cb7038be17788
Author: Daniel D’Aquino <daniel@daquino.me>
Date: Sat, 2 Nov 2024 10:34:43 -0700
Update and refactor ImageCarousel fill handling
Previously, the ImageCarousel needed to directly set a video size
binding to the video player view, in order to communicate and listen to
video size changes, and it used that to calculate the carousel image fill.
However, in the new video coordination architecture, the video size is
not owned by the ImageCarousel, but instead it is owned by the video
player itself, which in turn is owned by the video coordinator.
Therefore, this is incompatible with several logic elements of
ImageCarousel.
This commit updates the image carousel to integrate with the new video
coordinator architecture, and it also refactors the image fill logic
almost completely — with a focus on reducing stateful behavior by
carefully employing some state management patterns.
Furthermore, the new CarouselModel was heavily documented to explain its
design decisions and inner workings.
Note: There used to be some caching on the ImageFill calculations, but
after using this new refactored version without caching, I have not
noticed any noticeable performance regressions, so I have decided not to
add them back — applying Occam's razor
Changelog-Changed: Improved image carousel image fill behavior
Closes: https://github.com/damus-io/damus/issues/2458
Signed-off-by: Daniel D’Aquino <daniel@daquino.me>
Diffstat:
2 files changed, 212 insertions(+), 104 deletions(-)
diff --git a/damus/Components/ImageCarousel.swift b/damus/Components/ImageCarousel.swift
@@ -7,6 +7,7 @@
import SwiftUI
import Kingfisher
+import Combine
// TODO: all this ShareSheet complexity can be replaced with ShareLink once we update to iOS 16
struct ShareSheet: UIViewControllerRepresentable {
@@ -95,64 +96,203 @@ enum ImageShape {
}
}
+/// The `CarouselModel` helps `ImageCarousel` with some state management logic, keeping track of media sizes, and the ideal display size
+///
+/// This model is necessary because the state management logic required to keep track of media sizes for each one of the carousel items,
+/// and the ideal display size at each moment is not a trivial task.
+///
+/// The rules for the media fill are as follows:
+/// 1. The media item should generally have a width that completely fills the width of its parent view
+/// 2. The height of the carousel should be adjusted accordingly
+/// 3. The only exception to rules 1 and 2 is when the total height would be 20% larger than the height of the device
+/// 4. If none of the above can be computed (e.g. due to missing information), default to a reasonable height, where the media item will fit into.
+///
+/// ## Usage notes
+///
+/// The view is has the following state management responsibilities:
+/// 1. Watching the size of the images (via the `.observe_image_size` modifier)
+/// 2. Notifying this class of geometry reader changes, by setting `geo_size`
+///
+/// ## Implementation notes
+///
+/// This class is organized in a way to reduce stateful behavior and the transiency bugs it can cause.
+///
+/// This is accomplished through the following pattern:
+/// 1. The `current_item_fill` is a published property so that any updates instantly re-render the view
+/// 2. However, `current_item_fill` has a mathematical dependency on other members of this class
+/// 3. Therefore, the members on which the fill property depends on all have `didSet` observers that will cause the `current_item_fill` to be recalculated and published.
+///
+/// This pattern helps ensure that the state is always consistent and that the view is always up-to-date.
+///
+/// This class is marked as `@MainActor` since most of its properties are published and should be accessed from the main thread to avoid inconsistent SwiftUI state during renders
+@MainActor
class CarouselModel: ObservableObject {
- var current_url: URL?
- var fillHeight: CGFloat
- var maxHeight: CGFloat
- var firstImageHeight: CGFloat?
+ // MARK: Immutable object attributes
+ // These are some attributes that are not expected to change throughout the lifecycle of this object
+ // These should not be modified after initialization to avoid state inconsistency
+
+ /// The state of the app
+ let damus_state: DamusState
+ /// All urls in the carousel
+ let urls: [MediaUrl]
+ /// The default fill height for the carousel, if we cannot calculate a more appropriate height
+ /// **Usage note:** Default to this when `current_item_fill` is nil
+ let default_fill_height: CGFloat
+ /// The maximum height for any carousel item
+ let max_height: CGFloat
+
+
+ // MARK: Miscellaneous
+
+ /// Holds items that allows us to cancel video size observers during de-initialization
+ private var all_cancellables: [AnyCancellable] = []
+
+
+ // MARK: State management properties
+ /// Properties relevant to state management.
+ /// These should be made into computed/functional properties when possible to avoid stateful behavior
+ /// When that is not possible (e.g. when dealing with an observed published property), establish its mathematical dependencies,
+ /// and use `didSet` observers to ensure that the state is always re-computed when necessary.
- @Published var open_sheet: Bool
- @Published var selectedIndex: Int
- @Published var video_size: CGSize?
- @Published var image_fill: ImageFill?
+ /// Stores information about the size of each media item in `urls`.
+ /// **Usage note:** The view is responsible for setting the size of image urls
+ var media_size_information: [URL: CGSize] {
+ didSet {
+ guard let current_url else { return }
+ // Upon updating information, update the carousel fill size if the size for the current url has changed
+ if oldValue[current_url] != media_size_information[current_url] {
+ self.refresh_current_item_fill()
+ }
+ }
+ }
+ /// Stores information about the geometry reader
+ /// **Usage note:** The view is responsible for setting this value
+ var geo_size: CGSize? {
+ didSet { self.refresh_current_item_fill() }
+ }
+ /// The index of the currently selected item
+ /// **Usage note:** The view is responsible for setting this value
+ @Published var selectedIndex: Int {
+ didSet { self.refresh_current_item_fill() }
+ }
+ /// The current fill for the media item.
+ /// **Usage note:** This property is read-only and should not be set directly. Update `selectedIndex` to update the current item being viewed.
+ var current_url: URL? {
+ return urls[safe: selectedIndex]?.url
+ }
+ /// Holds the ideal fill dimensions for the current item.
+ /// **Usage note:** This property is automatically updated when other properties are set, and should not be set directly
+ /// **Implementation note:** This property is mathematically dependent on geo_size, media_size_information, and `selectedIndex`,
+ /// and is automatically updated upon changes to these properties.
+ @Published private(set) var current_item_fill: ImageFill?
+
+
+ // MARK: Initialization and de-initialization
- init(image_fill: ImageFill?) {
- self.current_url = nil
- self.fillHeight = 350
- self.maxHeight = UIScreen.main.bounds.height * 1.2 // 1.2
- self.firstImageHeight = nil
- self.open_sheet = false
+ /// Initializes the `CarouselModel` with the given `DamusState` and `MediaUrl` array
+ init(damus_state: DamusState, urls: [MediaUrl]) {
+ // Immutable object attributes
+ self.damus_state = damus_state
+ self.urls = urls
+ self.default_fill_height = 350
+ self.max_height = UIScreen.main.bounds.height * 1.2 // 1.2
+
+ // State management properties
self.selectedIndex = 0
- self.video_size = nil
- self.image_fill = image_fill
+ self.current_item_fill = nil
+ self.geo_size = nil
+ self.media_size_information = [:]
+
+ // Setup the rest of the state management logic
+ self.observe_video_sizes()
+ Task {
+ self.refresh_current_item_fill()
+ }
+ }
+
+ /// This private function observes the video sizes for all videos
+ private func observe_video_sizes() {
+ for media_url in urls {
+ switch media_url {
+ case .video(let url):
+ let video_player = damus_state.video.get_player(for: url)
+ if let video_size = video_player.video_size {
+ self.media_size_information[url] = video_size // Set the initial size if available
+ }
+ let observer_cancellable = video_player.$video_size.sink(receiveValue: { new_size in
+ self.media_size_information[url] = new_size // Update the size when it changes
+ })
+ all_cancellables.append(observer_cancellable) // Store the cancellable to cancel it later
+ case .image(_):
+ break; // Observing an image size needs to be done on the view directly, through the `.observe_image_size` modifier
+ }
+ }
+ }
+
+ deinit {
+ for cancellable_item in all_cancellables {
+ cancellable_item.cancel()
+ }
+ }
+
+ // MARK: State management and logic
+
+ /// This function refreshes the current item fill based on the current state of the model
+ /// **Usage note:** This is private, do not call this directly from outside the class.
+ /// **Implementation note:** This should be called using `didSet` observers on properties that affect the fill
+ private func refresh_current_item_fill() {
+ if let current_url,
+ let item_size = self.media_size_information[current_url],
+ let geo_size {
+ self.current_item_fill = ImageFill.calculate_image_fill(
+ geo_size: geo_size,
+ img_size: item_size,
+ maxHeight: self.max_height,
+ fillHeight: self.default_fill_height
+ )
+ }
+ else {
+ self.current_item_fill = nil // Not enough information to compute the proper fill. Default to nil
+ }
}
}
// MARK: - Image Carousel
+
+/// A carousel that displays images and videos
+///
+/// ## Implementation notes
+///
+/// - State management logic is mostly handled by `CarouselModel`, as it is complex, and becomes difficult to manage in a view
+///
@MainActor
struct ImageCarousel<Content: View>: View {
- var urls: [MediaUrl]
-
+ /// The event id of the note that this carousel is displaying
let evid: NoteId
-
- let state: DamusState
+ /// The model that holds information and state of this carousel
+ /// This is observed to update the view when the model changes
@ObservedObject var model: CarouselModel
let content: ((_ dismiss: @escaping (() -> Void)) -> Content)?
init(state: DamusState, evid: NoteId, urls: [MediaUrl]) {
- self.urls = urls
self.evid = evid
- self.state = state
- let media_model = state.events.get_cache_data(evid).media_metadata_model
- self._model = ObservedObject(initialValue: CarouselModel(image_fill: media_model.fill))
+ self._model = ObservedObject(initialValue: CarouselModel(damus_state: state, urls: urls))
self.content = nil
}
init(state: DamusState, evid: NoteId, urls: [MediaUrl], @ViewBuilder content: @escaping (_ dismiss: @escaping (() -> Void)) -> Content) {
- self.urls = urls
self.evid = evid
- self.state = state
- let media_model = state.events.get_cache_data(evid).media_metadata_model
- self._model = ObservedObject(initialValue: CarouselModel(image_fill: media_model.fill))
+ self._model = ObservedObject(initialValue: CarouselModel(damus_state: state, urls: urls))
self.content = content
}
var filling: Bool {
- model.image_fill?.filling == true
+ model.current_item_fill?.filling == true
}
var height: CGFloat {
- model.firstImageHeight ?? model.image_fill?.height ?? model.fillHeight
+ // Use the calculated fill height if available, otherwise use the default fill height
+ model.current_item_fill?.height ?? model.default_fill_height
}
func Placeholder(url: URL, geo_size: CGSize, num_urls: Int) -> some View {
@@ -160,7 +300,7 @@ struct ImageCarousel<Content: View>: View {
if num_urls > 1 {
// jb55: quick hack since carousel with multiple images looks horrible with blurhash background
Color.clear
- } else if let meta = state.events.lookup_img_metadata(url: url),
+ } else if let meta = model.damus_state.events.lookup_img_metadata(url: url),
case .processed(let blurhash) = meta.state {
Image(uiImage: blurhash)
.resizable()
@@ -177,24 +317,17 @@ struct ImageCarousel<Content: View>: View {
case .image(let url):
Img(geo: geo, url: url, index: index)
.onTapGesture {
- model.open_sheet = true
+ present(full_screen_item: .full_screen_carousel(urls: model.urls, selectedIndex: $model.selectedIndex))
}
case .video(let url):
- DamusVideoPlayerView(url: url, coordinator: state.video, style: .preview(on_tap: { model.open_sheet = true }))
- .onChange(of: model.video_size) { size in
- guard let size else { return }
-
- let fill = ImageFill.calculate_image_fill(geo_size: geo.size, img_size: size, maxHeight: model.maxHeight, fillHeight: model.fillHeight)
-
- print("video_size changed \(size)")
- if self.model.image_fill == nil {
- print("video_size firstImageHeight \(fill.height)")
- self.model.firstImageHeight = fill.height
- state.events.get_cache_data(evid).media_metadata_model.fill = fill
- }
-
- self.model.image_fill = fill
- }
+ let video_model = model.damus_state.video.get_player(for: url)
+ DamusVideoPlayerView(
+ model: video_model,
+ coordinator: model.damus_state.video,
+ style: .preview(on_tap: {
+ present(full_screen_item: .full_screen_carousel(urls: model.urls, selectedIndex: $model.selectedIndex))
+ })
+ )
}
}
}
@@ -203,31 +336,18 @@ struct ImageCarousel<Content: View>: View {
KFAnimatedImage(url)
.callbackQueue(.dispatch(.global(qos:.background)))
.backgroundDecode(true)
- .imageContext(.note, disable_animation: state.settings.disable_animation)
+ .imageContext(.note, disable_animation: model.damus_state.settings.disable_animation)
.image_fade(duration: 0.25)
.cancelOnDisappear(true)
.configure { view in
view.framePreloadCount = 3
}
- .imageFill(for: geo.size, max: model.maxHeight, fill: model.fillHeight) { fill in
- state.events.get_cache_data(evid).media_metadata_model.fill = fill
- // blur hash can be discarded when we have the url
- // NOTE: this is the wrong place for this... we need to remove
- // it when the image is loaded in memory. This may happen
- // earlier than this (by the preloader, etc)
- DispatchQueue.main.asyncAfter(deadline: .now() + 1.0) {
- state.events.lookup_img_metadata(url: url)?.state = .not_needed
- }
- self.model.image_fill = fill
- if index == 0 {
- self.model.firstImageHeight = fill.height
- //maxHeight = firstImageHeight ?? maxHeight
- } else {
- //maxHeight = firstImageHeight ?? fill.height
- }
- }
+ .observe_image_size(size_changed: { size in
+ // Observe the image size to update the model when the size changes, so we can calculate the fill
+ model.media_size_information[url] = size
+ })
.background {
- Placeholder(url: url, geo_size: geo.size, num_urls: urls.count)
+ Placeholder(url: url, geo_size: geo.size, num_urls: model.urls.count)
}
.aspectRatio(contentMode: filling ? .fill : .fit)
.kfClickable()
@@ -242,25 +362,19 @@ struct ImageCarousel<Content: View>: View {
var Medias: some View {
TabView(selection: $model.selectedIndex) {
- ForEach(urls.indices, id: \.self) { index in
+ ForEach(model.urls.indices, id: \.self) { index in
GeometryReader { geo in
- Media(geo: geo, url: urls[index], index: index)
+ Media(geo: geo, url: model.urls[index], index: index)
+ .onChange(of: geo.size, perform: { new_size in
+ model.geo_size = new_size
+ })
+ .onAppear {
+ model.geo_size = geo.size
+ }
}
}
}
.tabViewStyle(PageTabViewStyle(indexDisplayMode: .never))
- .fullScreenCover(isPresented: $model.open_sheet) {
- if let content {
- FullScreenCarouselView<Content>(video_coordinator: state.video, urls: urls, settings: state.settings, selectedIndex: $model.selectedIndex) {
- content({ // Dismiss closure
- model.open_sheet = false
- })
- }
- }
- else {
- FullScreenCarouselView<AnyView>(video_coordinator: state.video, urls: urls, settings: state.settings, selectedIndex: $model.selectedIndex)
- }
- }
.frame(height: height)
.onChange(of: model.selectedIndex) { value in
model.selectedIndex = value
@@ -278,8 +392,8 @@ struct ImageCarousel<Content: View>: View {
}
- if urls.count > 1 {
- PageControlView(currentPage: $model.selectedIndex, numberOfPages: urls.count)
+ if model.urls.count > 1 {
+ PageControlView(currentPage: $model.selectedIndex, numberOfPages: model.urls.count)
.frame(maxWidth: 0, maxHeight: 0)
.padding(.top, 5)
}
@@ -287,27 +401,6 @@ struct ImageCarousel<Content: View>: View {
}
}
-// MARK: - Image Modifier
-extension KFOptionSetter {
- /// Sets a block to get image size
- ///
- /// - Parameter block: The block which is used to read the image object.
- /// - Returns: `Self` value after read size
- public func imageFill(for size: CGSize, max: CGFloat, fill: CGFloat, block: @escaping (ImageFill) throws -> Void) -> Self {
- let modifier = AnyImageModifier { image -> KFCrossPlatformImage in
- let img_size = image.size
- let geo_size = size
- let fill = ImageFill.calculate_image_fill(geo_size: geo_size, img_size: img_size, maxHeight: max, fillHeight: fill)
- DispatchQueue.main.async { [block, fill] in
- try? block(fill)
- }
- return image
- }
- options.imageModifier = modifier
- return self
- }
-}
-
public struct ImageFill {
let filling: Bool?
@@ -344,4 +437,3 @@ struct ImageCarousel_Previews: PreviewProvider {
.environmentObject(OrientationTracker())
}
}
-
diff --git a/damus/Util/Extensions/KFOptionSetter+.swift b/damus/Util/Extensions/KFOptionSetter+.swift
@@ -58,6 +58,22 @@ extension KFOptionSetter {
return self
}
+
+ /// This allows you to observe the size of the image, and get a callback when the size changes
+ /// This is useful for when you need to layout views based on the size of the image
+ /// - Parameter size_changed: A callback that will be called when the size of the image changes
+ /// - Returns: The same KFOptionSetter instance
+ func observe_image_size(size_changed: @escaping (CGSize) -> Void) -> Self {
+ let modifier = AnyImageModifier { image -> KFCrossPlatformImage in
+ let image_size = image.size
+ DispatchQueue.main.async { [size_changed, image_size] in
+ size_changed(image_size)
+ }
+ return image
+ }
+ options.imageModifier = modifier
+ return self
+ }
}
let MAX_FILE_SIZE = 20_971_520 // 20MiB
