rasmuslos / AmpFin

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

feat: add iPad Support #27

Closed gnattu closed 6 months ago

gnattu commented 6 months ago

This PR added iPad support and the required layout adjustments for it. Additionally, it fixed the problem where some buttons could only be tapped on the text, which is particularly annoying on larger screens like the iPad.

Furthermore, this PR addressed the performance issue on large libraries with more than 10K albums by implementing lazy loading for albums and tracks instead of loading all of them at once.

Some concerns for the current implementation include:

Fixes #21

rasmuslos commented 6 months ago

Could you split this PR into two? One for the lazy loading (and the hit target fix, it is great, thanks a lot for that!) and one for the iPad support would be great! While I am all for lazy loading I do have some issues with the iPad version.

Some things I noticed with the iPad version:

Simulator Screenshot - iPhone 15 Pro Max - 2024-04-05 at 21 31 22 Simulator Screenshot - iPhone 15 Pro Max - 2024-04-05 at 21 25 03

Simulator Screenshot - iPhone 15 Pro Max - 2024-04-05 at 21 25 52

Simulator Screenshot - iPad Pro (12 9-inch) (6th generation) (16GB) - 2024-04-05 at 21 27 19 Simulator Screenshot - iPad Pro (12 9-inch) (6th generation) (16GB) - 2024-04-05 at 21 27 14

rasmuslos commented 6 months ago

Also albums are way to big on small devices

Simulator Screenshot - iPhone SE (3rd generation) - 2024-04-05 at 21 23 52

gnattu commented 6 months ago

What do you mean by using labels instead of buttons? Can you give an example?

Like this in AlbumsView+Header:

Group {
                                // why not buttons? because swiftui is a piece of shit
                                Label("queue.play", systemImage: "play.fill")
                                    .contentShape(Rectangle())
                                    .onTapGesture {
                                        startPlayback(false)
                                    }
                                Label("queue.shuffle", systemImage: "shuffle")
                                    .contentShape(Rectangle())
                                    .onTapGesture {
                                        startPlayback(true)
                                    }
                            }

The album buttons are to far down next to the album cover

Can you be more specific? If you are taking about the play/shuffle button on the iPad layout, then yes it is far down and needs to be fixed, but I'm not quite sure.

The spacing for albums is not all over the place on different screen sizes. Ideally the padding is the same as the gap between the albums:

I agree the spacing can be improved, but actually it is OK as even Apple themselves does not guarantee the equal-width padding on iPad:

image

I already addressed this in my branch.

Some links behave weird:

The NavigationLink should use plain button style, already fixed.

Also albums are way to big on small devices

Wait, SwiftUI preview lied to me. The scaling is incorrectly calculated on iPhone SEs. Needs to be fixed.

The size and especially the animation of the now playing view behave rather strange

This only happens with albums without a cover. Looking at it.

gnattu commented 6 months ago

The size and especially the animation of the now playing view behave rather strange

I already find the root cause of this thing but I need to come up with a more proper fix later. This is a very very interesting behavior when you attempted to use UIScreen to set bounds and then things happened. The UIScreen bounds is not safe to use on larger screens as they will return mysterious values and that value will not make your view fit to full screen.

What's more, on iPad there will be chances that the view is resized, so it will be smaller than the screen size. I need to find a way that not relies on the UIScreen to get bounds on iPad.

rasmuslos commented 6 months ago

Like this in AlbumsView+Header:

This is because the whole layout has to be contained inside a List (otherwise swipe actions do not work and you cannot really set the size of a list without it breaking your whole layout). Some apple employee decided that it would be funny if buttons contained in the same list row should have stupid it targets so if you clicked on the shuffle button the other one might trigger.

Can you be more specific? If you are taking about the play/shuffle button on the iPad layout, then yes it is far down and needs to be fixed, but I'm not quite sure.

Yes

I agree the spacing can be improved, but actually it is OK as even Apple themselves does not guarantee the equal-width padding on iPad:

I have to use the iPadOS app for a while to get a feel for it but it should be consistent on iOS