buresdv / Cork

A fast GUI for Homebrew written in SwiftUI
https://corkmac.app
2.33k stars 132 forks source link

Taps section of the sidebar doesn't animate #183

Open buresdv opened 1 year ago

buresdv commented 1 year ago

Bug Report

Mandatory Information

Describe the bug When adding or removing taps, the addition/removal animation doesn't play in the Taps section of the sidebar

To Reproduce Add or remove a tap. There will be no animation.

Expected Behavior The animation doesn't play

Screenshots https://github-production-user-asset-6210df.s3.amazonaws.com/22037369/272531402-0d5b1ffe-59dc-4379-8f8d-bdaf345cd075.mov

System and Cork Information:

Optional Information

Additional context N/A

Checklist

boscojwho commented 1 year ago

@buresdv Is the animation logic in a private branch? Also animation works for Installed Formulae and Installed Casks, right?

buresdv commented 1 year ago

@boscojwho the entire code is here. You can find the animation logic here: https://github.com/buresdv/Cork/blob/2fc2f3a0ad65cfc2311e8f9bfe23f1ce60bb3fd0/Cork/Views/Taps/Add%20Tap%20View.swift#L146

And yes, the animation works fine for Installed Formulae and Installed Casks. Thank you for looking into it!

boscojwho commented 1 year ago

Took a quick look at repro:

https://github.com/buresdv/Cork/assets/2549615/acaf1d46-d3b6-49a7-a262-b7ca23c9939a

boscojwho commented 1 year ago

"Remove" tap animates with the same buggy behaviour as "Add" if triggered using button in Detail view (see end of video below):

https://github.com/buresdv/Cork/assets/2549615/a7cae938-c62f-40b2-a754-a5e4d6ec7318

boscojwho commented 1 year ago

func removeTap(name: String, availableTaps: AvailableTaps, appState: AppState, shouldApplyUninstallSpinnerToRelevantItemInSidebar: Bool = false)

boscojwho commented 1 year ago

Seems like remove formulae also doesn't animate at all:

https://github.com/buresdv/Cork/assets/2549615/94523b72-2920-4eb3-bda3-627071315222

boscojwho commented 1 year ago

Update: Added repo that reproduces behaviour (https://github.com/boscojwho/sidebar-list-add-remove-row-mac)

Interesting, I made a test macOS project to see whether it's system behaviour/bug, and I was able to repro the behaviour here: https://gist.github.com/boscojwho/6bdf2df26750602262d23754ea90bcaa

buresdv commented 1 year ago

Thanks a lot for the writeup

"Remove" tap animates with the same buggy behaviour as "Add" if triggered using button in Detail view (see end of video below):

Did you try this on main? This was a bug in 2fc2f3a0ad65cfc2311e8f9bfe23f1ce60bb3fd0 and I think I fixed it on main.

Update: Added repo that reproduces behaviour (https://github.com/boscojwho/sidebar-list-add-remove-row-mac)

I can reproduce on Somona 14.0, as well as Ventura.

Not sure if system bug (I rarely write actual Mac apps, lol) or app bug (I used the same basic logic as in Cork).

I wouldn't be surprised if it was a system bug, macOS is an afterthought for the SwiftUI team 😭 If you could change the logic, what would you change?

boscojwho commented 1 year ago
boscojwho commented 1 year ago

@buresdv So, if we use a ScrollView + LazyVStack setup in the sidebar, row insert/removal animations work properly (see video):

https://github.com/buresdv/Cork/assets/2549615/b9040f9c-86b0-47d7-acec-3bcbf47d6f64

buresdv commented 1 year ago

@boscojwho Okay, that ScrollView solution looks great in regards to the animations. Like you said, it just needs some extra styling

boscojwho commented 1 year ago

Yea, I can take a look and also switch out the current setup with this new one if you want.

buresdv commented 1 year ago

Yea, I can take a look and also switch out the current setup with this new one if you want.

That'd be great if you could give it a go, just make sure the searching still works properly and that the sections can be collapsed. Thank you!

boscojwho commented 1 year ago
boscojwho commented 1 year ago

I'm trying to figure out if .searchable works with a ScrollView+LazyVStack in split view sidebar on macOS (would really suck if it doesn't). Other than that, I got the setup to behave and look like a sidebar List.

https://github.com/buresdv/Cork/assets/2549615/0536abfc-b14a-40ba-9499-d4fa90f2dfcf

boscojwho commented 1 year ago

Unfortunate, looks like .searchable() doesn't play well with non-List views on macOS (the same setup works on iPhone and iPad with .sidebar placement):

https://github.com/buresdv/Cork/assets/2549615/220dfdc9-19c4-4fe1-a76a-381e2e9bdac5

buresdv commented 1 year ago

Man

I wish Apple paid SwiftUI on the Mac 30% of the attention it gets on iPlatforms

buresdv commented 7 months ago

Moved to "Waiting for Apple to Fix" since there doesn't seem to be anything we can do

SilverMarcs commented 5 months ago

.listStyle(.inset) with .scrollContentBackground(.hidden) should render animations correctly. And you can retain all the built-in functionality of List.

Of course this is far from perfect since it probably requires other tweaks to get list sections and to separators to show in the desired manner,