Dimillian / IceCubesApp

A SwiftUI Mastodon client
https://apps.apple.com/us/app/ice-cubes-for-mastodon/id6444915884
GNU Affero General Public License v3.0
5.26k stars 487 forks source link

Feature request: Timeline media should be limited to screen height (iPad) #1554

Closed sh95014 closed 9 months ago

sh95014 commented 1 year ago

On a landscape-orientation iPad in particular, portrait images or videos can be as much as two screen heights tall, and often displayed in poor upsampled resolution. It should be a much better general experience to limit the height of images and videos to the screen height.

In the screenshot below, for example, you see barely half of the video (but plenty of artifacts!):

IMG_2D52D3281176-1

sh95014 commented 9 months ago

Height computation is gross, but I think this is already better:

diff --git a/Packages/DesignSystem/Sources/DesignSystem/SceneDelegate.swift b/Packages/DesignSystem/Sources/DesignSystem/SceneDelegate.swift
index 44881c96..c93ba21d 100644
--- a/Packages/DesignSystem/Sources/DesignSystem/SceneDelegate.swift
+++ b/Packages/DesignSystem/Sources/DesignSystem/SceneDelegate.swift
@@ -8,6 +8,10 @@ import UIKit
     window?.bounds.size.width ?? UIScreen.main.bounds.size.width
   }

+  public var windowHeight: CGFloat {
+    window?.bounds.size.height ?? UIScreen.main.bounds.size.height
+  }
+
   public func scene(_ scene: UIScene,
                     willConnectTo _: UISceneSession,
                     options _: UIScene.ConnectionOptions)
diff --git a/Packages/Status/Sources/Status/Row/Subviews/StatusRowMediaPreviewView.swift b/Packages/Status/Sources/Status/Row/Subviews/StatusRowMediaPreviewView.swift
index d4a04b50..c2721550 100644
--- a/Packages/Status/Sources/Status/Row/Subviews/StatusRowMediaPreviewView.swift
+++ b/Packages/Status/Sources/Status/Row/Subviews/StatusRowMediaPreviewView.swift
@@ -68,7 +68,8 @@ public struct StatusRowMediaPreviewView: View {
           imageMaxHeight: imageMaxHeight,
           sensitive: sensitive,
           appLayoutWidth: appLayoutWidth,
-          availableWidth: availableWidth
+          availableWidth: availableWidth,
+          availableHeight: sceneDelegate.windowHeight - 200
         )
         .accessibilityElement(children: .ignore)
         .accessibilityLabel(Self.accessibilityLabel(for: attachments[0]))
@@ -200,6 +201,7 @@ private struct FeaturedImagePreView: View {
   let sensitive: Bool
   let appLayoutWidth: CGFloat
   let availableWidth: CGFloat
+  let availableHeight: CGFloat

   @Environment(\.isSecondaryColumn) private var isSecondaryColumn: Bool
   @Environment(Theme.self) private var theme
@@ -261,7 +263,11 @@ private struct FeaturedImagePreView: View {
     }
     let ratio = newWidth / from.width
     let newHeight = from.height * ratio
-    return .init(width: newWidth, height: newHeight)
+    if newHeight <= availableHeight {
+      return .init(width: newWidth, height: newHeight)
+    }
+    let newWidth = from.width * (availableHeight / from.height)
+    return .init(width: newWidth, height: availableHeight)
   }
 }

@Dimillian I can open a PR if you're willing to take this. Ideally I think the image should be sized such that the entire toot can fit vertically, but SwiftUI doesn't make it easy and I wanted to keep the change small for now.

Note: Might also want to enforce a sanity check for minimum width and height...

sh95014 commented 9 months ago

Also, the existing code will happily blow up a small image and expose ugly artifacts, such as upsizing this 320x274 image more than 7x:

IMG_3F031C6665A5

so I think there should also be a cap (maybe ~2..3x) to how much an image is upsized.

Dimillian commented 9 months ago

I'm all for improvements on this. The current solution is definitely not ideal. A PR we could play with would be good.