rasmuslos / AmpFin

Native Jellyfin music player for iOS & iPadOS
Other
154 stars 11 forks source link

fix: don't use UIScreen for iPad #32

Closed gnattu closed 5 months ago

gnattu commented 5 months ago

Most of the statically calculated frame sizes can be omitted, allowing SwiftUI to negotiate them by itself.

The most interesting part is handling the NavigationSplitView and our NowPlayingBar.

Firstly, all modifiers will only apply to the root view in the NavigationStack, which is not suitable for the NowPlayingBar unless we want to manually add this thing to each and every view in the stack.

Secondly, the NavigationSplitView is wrapped around UIKit and is not SwiftUI native. This means its state change will occur after the view change, causing issues because we want to know if the sidebar is visible or not to calculate the padding for the NowPlayingBar. However, that status will not change until all animations are finished. Instead, we have to observe the origin of the sidebar and use that position to calculate the padding.

rasmuslos commented 5 months ago

How does this look?

https://github.com/rasmuslos/AmpFin/assets/49640416/67812365-f406-4e52-b9c3-8384e7cb180f

gnattu commented 5 months ago

How does this look?

This looks better, very close to what the Apple Music App looks like. I think this could be the final version.

rasmuslos commented 5 months ago

How does this look?

This looks better, very close to what the Apple Music App looks like. I think this could be the final version.

Great!

rasmuslos commented 5 months ago

When you open the app and start a track without opening the sidebar once in portrait mode the now playing bar does something weird:

Simulator Screenshot - iPad Pro (12 9-inch) (6th generation) (16GB) - 2024-04-10 at 18 52 02

I am pretty sure this is related to the offset, could you look into that?

rasmuslos commented 5 months ago

This is what I came up with, the problem is that the sidebar seems to be the only content when the app starts in portrait mode, so the offset gets set to the width of the screen. I (again) used UIScreen to mitigate this, but maybe there is a more elegant solution?

https://github.com/rasmuslos/AmpFin/commit/7b37759c37ad3b847f415de39852bcba8e1fdad9

gnattu commented 5 months ago

This is what I came up with, the problem is that the sidebar seems to be the only content when the app starts in portrait mode, so the offset gets set to the width of the screen. I (again) used UIScreen to mitigate this, but maybe there is a more elegant solution?

7b37759

This will only happen when the App is first start and the width of the sidebar is initialized before the actual rendering of it for performance reasons(visible views get priority).

This event will have an unusually high width, which is quite abnormal considering that Apple adheres to a platform-defined standard width for the sidebar. This width remains fixed on the platform for a layout and will not change unless the user explicitly requests it. However, even if a specific size is required, it may not have any effect on certain platforms like macOS, as AppKit does not provide such functionality. I think a sane fixed value will be enough. Currently on both iPadOS and macOS, this standard width is 320px, and we can just use a larger value like 400px just in case Apple changed their mind, but Apple is very unlikely to pick more than that. So I think a check for if the width exceeds 400px will be enough and is actually more robust than comparing it with the UIScreen width because UIScreen provided value sometimes can be mysterious on iPad.

For example:

if (reader.size.width < 400) {
    NotificationCenter.default.post(name: .init("b"), object: reader.frame(in: .global).origin.x + reader.size.width)
}
gnattu commented 5 months ago

Actual example why UIScreen should not be used:

image

Yes, this can occur even in landscape mode if your window size in the split view is precisely "fit", you can trigger this and the size reported is still smaller than UIScreen's bounds.

When comparing with a sane threshold like 400 then this won't happen. Because the app is going to use the compact mode and no more sidebar if the window size is that narrow.

rasmuslos commented 5 months ago

Fixed! Thanks.

I want to add on more thing, a way to tell the loadMore function that the user is searching for something so that content that is not yet loaded can also be searched, but this should be easy. After that this should be production ready. You seem to have side loaded this, but if I want I can add you to a TestFlight. And again, if you want a promo code, say the word.

There is some testing left to do, I will try this on my physical device a bit, but the update should be released soon!

Again, thanks a lot for your work!