rasmuslos / AmpFin

Native Jellyfin music player for iOS & iPadOS
Other
107 stars 10 forks source link

feat: iPad Support v2 and Mac Catalyst Support #28

Closed gnattu closed 3 months ago

gnattu commented 3 months ago

This is a follow-up of #27, which addressed the outstanding UI issue presented.

To avoid layout issues, we should refrain from calculating static sizes and setting them as absolute frame sizes using UIKit-derived numbers. These numbers are not safe to use with SwiftUI and can cause layout issues, especially on iPad where the window size is not a constant value.

This represents a paradigm shift where we calculate dimensions ourselves in UIKit or on the Web, but SwiftUI prefers to "negotiate" with it and let the framework itself determine the best size most of the time.

Fixes #21

gnattu commented 3 months ago

Updated to support Mac Catalyst, also introduced some Mac Catalyst's specific workarounds. I currently identified two:

The current toggle animation for the NowPlayingView appears to be too complicated for large screens, causing performance issues that SwiftUI/CoreAnimation cannot reliably handle. Framerate drops and even hanging in the middle of the animation can occur on iPad and Mac. Simplifying the animation will improve performance, so I will try to address this.

rasmuslos commented 3 months ago

First of all, thank you for your work!

Most of it looks good, although I will not publish the Mac app in the App Store, I would like until the app is fully written in SwiftUI / AppKit as a native target, rather than a SwiftUI Multiplatform app. But side loading on the Mac is quite easy, so there will be downloads on GitHub.

I would like to merge this onto a separate branch for now to make some additional changes:

(For future reference): tvOS/ ... macOS/ ...


This would also allow for platform specific code for highly platform specific components, like the now playing bar / sheet. I am the first to admit that the new animation makes them a unmaintainable mess of spaghetti code, so I think it would be best to reuse components like `Queue` / `Lyrics` / etc. but using different containers. And while your solution of using `ViewThatFits` for things like the album header is really clever I think it would be better to just write one for iOS and another one for iPadOS.
- In essence: write platform specific components for
  - the now playing view / bar
  - album header
  - playlist header
  - artist page (or just the header we will see)
  - maybe replace the track**list** with a table

And i noticed you hid the toolbar in the same views they are defined on, but this is actually not necessary, you can hide the toolbar wherever you like.

Also while looking at Apple Music on the iPad I noticed that the apple engineers could also not be bothered to create a custom now playing animation for the iPad and just used `fullscreenCover` (or the UIKit equivalent). It can be dragged (not possible in SwiftUI) but this is about it. I think its quite funny that the Apple Music engineers seem to have a similar option about the now playing bar animation as I do, to at least don't want to port it to iPadOS...
rasmuslos commented 3 months ago

Also i will give you credit for your work (as well as the libraries AmpFin uses) in an acknowledgments section in the settings app, it will be added before the next release. If you have not bought AmpFin from the AppStore yet I can also give you a code so that you can get it for free.

gnattu commented 3 months ago

And while your solution of using ViewThatFits for things like the album header is really clever I think it would be better to just write one for iOS and another one for iPadOS.

This will end up with iPadOS just using my current version. The view size is changeable on iPadOS(and macOS), so that you will have to provide multiple layouts for this view to accommodate for different view sizes. The same also applies to other view like the nowplaying view, but that one could use different container to resolve the issue we are having with animations so it might be beneficial to do.

rasmuslos commented 3 months ago

And while your solution of using ViewThatFits for things like the album header is really clever I think it would be better to just write one for iOS and another one for iPadOS.

This will end up with iPadOS just using my current version. The view size is changeable on iPadOS(and macOS), so that you will have to provide multiple layouts for this view to accommodate for different view sizes. The same also applies to other view like the nowplaying view, but that one could use different container to resolve the issue we are having with animations so it might be beneficial to do.

You are right about that, if the app is in split view it should adapt to it

rasmuslos commented 3 months ago

I still want to restructure the project, I did a the same with ShelfPlayer a while back (https://github.com/rasmuslos/ShelfPlayer/commit/2dd9609dfa9b2644480582ae8745f68593ff8af5) and this seems like good opportunity to restructure AmpFin but using ViewThatFits over #if is probably the way to go. But I am not sure about the NavigationSplitView. Apple Music just displays something that is more or less the iOS app in split over, but I am not sure if this can be archived using just ViewThatFits...

rasmuslos commented 3 months ago

Again, thanks a lot for this!

rasmuslos commented 3 months ago

This is what the custom now playing bar and container now look like:

https://github.com/rasmuslos/AmpFin/assets/49640416/9ecaee3f-eda2-4bc9-9138-76983ea75c71

There are some really stupid hacks involved to get the now playing bar to resize properly (you cannot attach it to the detail column of the NavigationSplitView because the NavigationStack in there is purely cosmetic and doesn't to anything) but I think it turned out pretty good.

gnattu commented 3 months ago

you cannot attach it to the detail column of the NavigationSplitView because the NavigationStack in there is purely cosmetic and doesn't to anything

Actually it requires very special handling but it can work, let me see if it could be better.

gnattu commented 3 months ago

Well this implementation still uses a lot of UIScreen.main.bounds which should not be used at all even in compact views as iPad will report this value vastly different from the actual view size and can cause a lot of layout problems especially in split view. (Yes, and iPad can enter compact mode when the view size is narrow enough).

On macOS, this value is very mysterious which does not even equal to the actual screen size so the position of the nowplaying bar and nowplaying view will be very wrong.

I'will fixing this first then look for a less hacky way to place our nowplaying bar.

rasmuslos commented 3 months ago
Bildschirmfoto 2024-04-10 um 09 33 17

The one on the left is the designed for iPad app, the one on the right the Mac Catalyst one. I think it is better to just use the designed for iPad version because it looks virtually the same and has less bugs that need fixing. I plan on making a dedicated macOS app using a native target SwiftUI / AppKit, so this is only a temporary solution.

I will also look over the uses of UIScreen myself, I already found some places where it can be replaced.

rasmuslos commented 3 months ago

I'will fixing this first then look for a less hacky way to place our nowplaying bar.

the problem is that you cannot attach anything to the detail column. It just does not work. For some reason it does on macOS, but on iPadOS the NavigationStack inside the detail column get replaced upon navigation, even though it shouldn't...

gnattu commented 3 months ago

I will also look over the uses of UIScreen myself, I already found some places where it can be replaced.

Save your time as I already fixed it.

I also have a very interesting find for the NavigationSplitView, and I will submit a new PR.

gnattu commented 3 months ago

I think it is better to just use the designed for iPad version because it looks virtually the same and has less bugs that need fixing.

The designed for iPad one will not be available for Intel Macs and it is much more harder to side-load, where the catalyst one you can just click to open the app. And no, use a designed for iPad Version will not save too much in debugging because the components breaks in the Mac Catalyst will be very likely to break in the Designed for iPad version as well, like the Menus.

rasmuslos commented 3 months ago

I think it is better to just use the designed for iPad version because it looks virtually the same and has less bugs that need fixing.

The designed for iPad one will not be available for Intel Macs and it is much more harder to side-load, where the catalyst one you can just click to open the app. And no, use a designed for iPad Version will not save too much in debugging because the components breaks in the Mac Catalyst will be very likely to break in the Designed for iPad version as well, like the Menus.

The toolbar is working, but the menus are not. I did not know that you cannot side load designed for iPad apps... Then catalyst it is.

gnattu commented 3 months ago

I did not know that you cannot side load designed for iPad apps...

If you have to there are ways, but it is just as hard as sideloading iOS apps then, because it is fundamentally an iOS app, and Apple does not want Apple Silicon Macs act like jail-broken iPads.

The toolbar is working, but the menus are not.

Which one though? Menus do require extra handling on macOS and that's why I hade specific macros defined for it. Some menus require long-press to show will require a right click on Mac Catalyst.

rasmuslos commented 3 months ago

Which one though? Menus do require extra handling on macOS and that's why I hade specific macros defined for it. Some menus require long-press to show will require a right click on Mac Catalyst.

The right click menus and the menus that require the label fix work.

Just FYI you don't have to create a @State var alwaysOn = true, you can just use .constant(true) as the binding parameter.