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

Add a configuration to allow the display of the Toast above the mini-player #2555

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions podcasts/Common SwiftUI/Toast/Toast.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ class Toast {
private var window: UIWindow? = nil

/// Display the toast message with the given title and actions
static func show<Style: ToastTheme>(_ title: String, actions: [Action]? = nil, dismissAfter: TimeInterval = 5.0, theme: Style = .defaultTheme) {
static func show<Style: ToastTheme>(_ title: String, actions: [Action]? = nil, dismissAfter: TimeInterval = 5.0, theme: Style = .defaultTheme, aboveMiniPlayer: Bool = false) {
// Hide any active toasts
shared.toastDismissed()

guard let scene = SceneHelper.connectedScene() else { return }

let viewModel = ToastViewModel(coordinator: shared, title: title, actions: actions, dismissTime: dismissAfter)
let viewModel = ToastViewModel(coordinator: shared, title: title, actions: actions, dismissTime: dismissAfter, aboveMiniPlayer: aboveMiniPlayer)
let view = ToastView(viewModel: viewModel, style: theme)
let controller = ThemedHostingController(rootView: view)

Expand Down
2 changes: 1 addition & 1 deletion podcasts/Common SwiftUI/Toast/ToastView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,10 @@ struct ToastView<Style: ToastTheme>: View {
.shadow(color: .black.opacity(0.3), radius: 10)

// Animates the toast in from the bottom of the screen
.miniPlayerSafeAreaInset(multiplier: viewModel.aboveMiniPlayer ? 1.5 : 0)
Copy link
Member

Choose a reason for hiding this comment

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

We'll probably forget this if we change the mini-player and the toast will probably appear incorrectly again. Also, given this is a property I see one of us repeating the same issue

I wonder if there's any way we can:

  1. Detect mini player is visible;
  2. Set this value based on the mini player height/position

However, such a solution still couple this to the mini-player and might require some additional hack code. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The modifier actual ensures that the miniPlayer is taking in account the issue we have is the tabbar, while on the other places the standard safeInset ensure thats the bottom already takes in account the tab bar, on the toast because we are on another Window the tab bar is not taken in account.
Do you see a way to detect the tab bar presence and size from another window?

Copy link
Contributor

Choose a reason for hiding this comment

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

@leandroalonso @SergioEstevao this solution it's similar to what Android team has implemented. In our codebase we have other classes, like the InsetAdjuster, that takes the same argument to adjust the padding.

However, if we want to automatically adjust it without injecting any parameters, we can use the NavigationManager (with some adjustments) as source to get the miniplayer and tabbar access.

Copy link
Contributor

@bjtitus bjtitus Dec 20, 2024

Choose a reason for hiding this comment

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

I think it should be possible to adjust using the MainTabBarController at the ToastWindow level but I wasn't quite able to get the values right:

if windowScene.keyWindow?.rootViewController?.presentedViewController == nil {
    let tabBarController = (windowScene.keyWindow?.rootViewController as? MainTabBarController)
    if let tabBarController, tabBarController.tabBar.isHidden == false {
        let tabBarHeight = tabBarController.tabBar.bounds.height
        controller.additionalSafeAreaInsets = UIEdgeInsets(top: 0, left: 0, bottom: tabBarHeight, right: 0)
    }
}

I was hoping the keyWindow.safeAreaInset would include the tab bar height but it does not seem to right now and adjusting that would probably mess with a bunch of things.

Would this make sense as an alternative? It's still somewhat messy to pull out that MainTabBarController and make sure there's nothing presented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this make sense as an alternative? It's still somewhat messy to pull out that MainTabBarController and make sure there's nothing presented.

@bjtitus The NavigationManager singleton has this information. AppDelegate and the MainTabBarController rely on this. If we want to have an auto position of the Toast I think we can use this as source.
Otherwise my suggestion is to use Sergio solution and align with Android

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielebogo and @leandroalonso I updated the code so that instead of hard-coding the miniplayer inset attribute on toast, I added a inset provider parameter that allows the customization of the inset depending of the screen.
By default it will use the safeAreaInset.bottom but customizations can be made to for example take in account the mini-player.
Give it a look and tell me what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.offset(y: isVisible ? 0 : contentSize.height)
.opacity(isVisible ? 1 : 0)
.animation(ToastConstants.animation, value: isVisible)

// Calculate the view size and inform the view model
.background(GeometryReader(content: { proxy in
Color.clear.onAppear {
Expand Down
4 changes: 3 additions & 1 deletion podcasts/Common SwiftUI/Toast/ToastViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class ToastViewModel: ObservableObject {
let title: String
let actions: [Toast.Action]
let dismissTime: TimeInterval
let aboveMiniPlayer: Bool

deinit {
autoDismissTimer?.invalidate()
Expand All @@ -23,11 +24,12 @@ class ToastViewModel: ObservableObject {
/// When this is true the view should animate out and call `didDismiss`
@Published var didAutoDismiss = false

init(coordinator: ToastDelegate, title: String, actions: [Toast.Action]?, dismissTime: TimeInterval) {
init(coordinator: ToastDelegate, title: String, actions: [Toast.Action]?, dismissTime: TimeInterval, aboveMiniPlayer: Bool = false) {
self.coordinator = coordinator
self.title = title
self.actions = actions ?? []
self.dismissTime = dismissTime
self.aboveMiniPlayer = aboveMiniPlayer
}

// MARK: - Window Methods
Expand Down
11 changes: 8 additions & 3 deletions podcasts/SwiftUI/MiniPlayerPaddingModifier.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,16 @@ import SwiftUI
/// Apply a bottom padding whenever the mini player is visible
public struct MiniPlayerSafeAreaInset: ViewModifier {
@State var isMiniPlayerVisible: Bool = false
let multipler: CGFloat

init(multipler: CGFloat) {
self.multipler = multipler
}

public func body(content: Content) -> some View {
content
.safeAreaInset(edge: .bottom, spacing: 0) {
Color.clear.frame(height: Constants.Values.miniPlayerOffset) // Adjust the bottom inset
Color.clear.frame(height: Constants.Values.miniPlayerOffset * multipler) // Adjust the bottom inset
}
.onAppear {
isMiniPlayerVisible = (PlaybackManager.shared.currentEpisode() != nil)
Expand All @@ -24,7 +29,7 @@ public struct MiniPlayerSafeAreaInset: ViewModifier {

// Create an extension for easier usage
public extension View {
func miniPlayerSafeAreaInset() -> some View {
self.modifier(MiniPlayerSafeAreaInset())
func miniPlayerSafeAreaInset(multiplier: CGFloat = 1) -> some View {
self.modifier(MiniPlayerSafeAreaInset(multipler: multiplier))
}
}
2 changes: 1 addition & 1 deletion podcasts/UpNextViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ class UpNextViewController: UIViewController, UIGestureRecognizerDelegate {
}
let upNextShuffleEnabled = Settings.upNextShuffleEnabled()
if upNextShuffleEnabled {
Toast.show(L10n.upNextShuffleToastMessage)
Toast.show(L10n.upNextShuffleToastMessage, aboveMiniPlayer: self.showingInTab ? true : false)
}
track(.upNextShuffleEnabled, properties: ["value": upNextShuffleEnabled])
}
Expand Down
Loading