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

Implement DnD reordering #181

Merged
merged 41 commits into from
Nov 11, 2023
Merged

Implement DnD reordering #181

merged 41 commits into from
Nov 11, 2023

Conversation

leolost2605
Copy link
Member

@leolost2605 leolost2605 commented Jul 26, 2023

Pretty much replicates the plank remove and reorder experience 1 : 1

@leolost2605 leolost2605 requested a review from a team July 26, 2023 18:38
Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

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

For some reason I can remove unpined apps by dragging into nothing.

@lenemter
Copy link
Member

And I think the Launcher object being a drop target is wrong, we need to insert a separate drop target object in between the launchers and expand them on hover, like the Plank does.

src/Launcher.vala Outdated Show resolved Hide resolved
@leolost2605
Copy link
Member Author

leolost2605 commented Jul 30, 2023

Unpinned apps can't be removed anymore.
Also I added animations when reordering and the poof animation. The reordering and removing experience should therefore be almost the same as with plank now.
Currently there is a bug in GTK4 < 4.8 that CSS animations sometimes just don't play that is noticeable when reordering but that's fixed with this PR and I tested it with 4.10 and it works really good there. I'm not very experienced with CSS so it's a very basic animation but IMO it looks pretty similar to the plank one. Still if somebody has ideas to improve it they're very welcome.
Admittedly the poof animation seems like a bit of a hacky solution but it works and I had only the plank svg to work with 🤷. Also the poof animation now is the same size as the icons whereas in plank it was a bit bigger, I'm not sure which way to go here?

@lenemter

This comment was marked as off-topic.

@leolost2605
Copy link
Member Author

Hmm just removing an app nothing else?

@lenemter
Copy link
Member

@leolost2605 yep

@leolost2605
Copy link
Member Author

leolost2605 commented Jul 30, 2023

@lenemter that is strange, cause I don't get these glitches...
I'm not very familiar with how gtk draws stuff but is it possible that different gala or gtk versions do something like that?
Also do you get the poof animation?

src/Launcher.vala Outdated Show resolved Hide resolved
src/Launcher.vala Outdated Show resolved Hide resolved
src/Launcher.vala Outdated Show resolved Hide resolved
src/Launcher.vala Outdated Show resolved Hide resolved
src/Launcher.vala Outdated Show resolved Hide resolved
src/Launcher.vala Outdated Show resolved Hide resolved
src/MainWindow.vala Outdated Show resolved Hide resolved
src/PoofPopover.vala Outdated Show resolved Hide resolved
src/MainWindow.vala Outdated Show resolved Hide resolved
src/MainWindow.vala Outdated Show resolved Hide resolved
@lenemter
Copy link
Member

lenemter commented Jul 30, 2023

I'm not very familiar with how gtk draws stuff but is it possible that different gala or gtk versions do something like that?

TBH I'm not familiar with this either. I remember seeing similar glitches in Sideload app and the cause was that window didn't smoothly change its size. It was easy to fix thanks to Gtk.Stack.interpolate_size, but since here's no stack used, I don't know if that can be the source of issues.

Also do you get the poof animation?

Oh yes, I do. It's a bit small though.

@leolost2605
Copy link
Member Author

leolost2605 commented Jul 30, 2023

I no longer encounter glitches, but I can resize the dock now :)

Ok the last commit adds decorated=false which fixes all glitches for me and keeps the resizable=false. I hope that might also fix yours?

Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

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

Sorry it took so long to review. I left inline comments on things I personally don't like

src/Launcher.vala Outdated Show resolved Hide resolved
src/MainWindow.vala Outdated Show resolved Hide resolved
src/MainWindow.vala Outdated Show resolved Hide resolved
src/MainWindow.vala Show resolved Hide resolved
Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

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

I tried this branch in Wayland session and it's totally broken, you can't remove an app using dragging, and when dragging there is no icon at all. This is definitely some Wayland issues, so worth asking @tintou

@danirabbit
Copy link
Member

@lenemter Would you be okay with merging for now since it's working in X and then we can keep iterating since we're such early days here? It would be nice to keep momentum going to replace plank ASAP imo

@danirabbit
Copy link
Member

@leolost2605 can you resolve conflicts here please? :)

@leolost2605 leolost2605 marked this pull request as draft November 11, 2023 12:58
@leolost2605 leolost2605 marked this pull request as ready for review November 11, 2023 14:00
@leolost2605
Copy link
Member Author

@danirabbit done!

Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

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

This is something we have to revisit later but it's good enough to start testing the dock. Thanks for working on this <3

@leolost2605 leolost2605 enabled auto-merge (squash) November 11, 2023 14:16
@leolost2605 leolost2605 merged commit 47c02f8 into main Nov 11, 2023
4 checks passed
@leolost2605 leolost2605 deleted the dnd-reordering branch November 11, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants