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

Add ability to reorder workspaces #464

Merged
merged 23 commits into from
Jul 16, 2019
Merged

Add ability to reorder workspaces #464

merged 23 commits into from
Jul 16, 2019

Conversation

donadigo
Copy link
Contributor

@donadigo donadigo commented Feb 8, 2019

Fixes #259.
Depends on #566 and https://gitlab.gnome.org/GNOME/mutter/merge_requests/670.
Depends on a distro-patch for mutter (https://gist.github.com/donadigo/fb99b5a9e8a092c76c4d6f03cc7ef930) that is already in the repos.

This adds the ability to drag a workspace and reorder it to a different place. This required some changes in core components to workaround the absence of a native method in libmutter to do this.

Some changes in the codebase:

  • DragDropAction now can be initialized with both source and destination flags and supports those. This was done to have IconGroup as a source (DND to different place) and as a destination (DND a window to an icon group).

  • remove_group_in_place is used to remove an IconGroup without having the IconGroupContainer reallocate and change it's position.

  • The delay of WorkspaceInsertThumb is now dynamic, this is because dragging an IconGroup has a delay of 0 (the space is immediately made for the widget) and the standard (when dragging a window icon to make a new workspace)

  • Methods like the new move_workspace_to_index require freezing all animations in the multitasking view to stop having unnecessary animations when reordering workspaces. MultitaskingView now has static methods to change animation state.
    We need to wait until https://gitlab.gnome.org/GNOME/mutter/merge_requests/670 hits our repos, then, none of the previous solutions will be required.
    This was merged as well.

@donadigo
Copy link
Contributor Author

donadigo commented Feb 10, 2019

This is now ready to review. @elementary/ux

@cassidyjames
Copy link
Contributor

@donadigo from a brief UX review, this looks really good and works as I'd expect! I'll continue using it and would like to hear from some people more familiar with the codebase, but I'm 👍

@donadigo donadigo requested a review from ricotz March 12, 2019 01:18
@Blast-City
Copy link

This is such a great feature.

Will it have a keyboard shortcut? Ctrl+Super+Alt+Left/Right?

@peteruithoven
Copy link
Collaborator

One first tiny note. The workspace box, with the icons, what you drag seem completely transparent, which looks a bit weird when dragging them over other workspaces:
Screenshot from 2019-07-02 02 08 04@2x
Maybe the background of those boxes should be semi-transparent or opaque?

@donadigo
Copy link
Contributor Author

donadigo commented Jul 2, 2019

@peteruithoven fixed.

@danirabbit
Copy link
Member

danirabbit commented Jul 2, 2019

It seems like there a couple of unnecessary animations, but otherwise this is a great functional improvement.

Open a few workspaces with one app on each of them. Open Multitasking view. :

  • Grab the current workspace and drag it ever so slightly to the left or right (like 6px). The workspace to your left will jump to the position of your current workspace and then quickly animate back to its original position. It probably shouldn't move until you drag past it.
  • Switch the order of any two workspaces. You'll notice that when you drop an app, it appears to be in a folder with the other app momentarily before it animates back to full size with a big "pop"

@donadigo
Copy link
Contributor Author

donadigo commented Jul 2, 2019

@danrabbit I've tried to fix 2. but it's really hard to do this because yet again mutter doesn't let us reorder workspace indexes so what we do instead is move all apps one by one to the next workspace so that we can make room for the new workspace. Moving those apps causes signals to fire and gala reanimates the apps that changed. It would be quite hacky in gala to prevent that.

@peteruithoven
Copy link
Collaborator

I noticed I can easily lose the icons and I can't get them back unless I restart Gala.
Screen record from 2019-07-02 19 40 07

I ran gala with G_MESSAGES_DEBUG=all but seen no warnings or errors.

@danirabbit
Copy link
Member

@donadigo ah damn. Would it be awful to wait to animate for like 100ms or something really tiny? Long enough for the move to occur but short enough that people won't really notice?

@donadigo
Copy link
Contributor Author

donadigo commented Jul 2, 2019

@danrabbit I will try to figure this out, you can see that there are still major problems found.

Copy link
Collaborator

@peteruithoven peteruithoven left a comment

Choose a reason for hiding this comment

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

Loads of questions, few tiny issues.

src/DragDropAction.vala Show resolved Hide resolved
src/Widgets/WorkspaceInsertThumb.vala Show resolved Hide resolved
src/Widgets/WorkspaceInsertThumb.vala Show resolved Hide resolved
src/Widgets/IconGroup.vala Outdated Show resolved Hide resolved
src/Widgets/IconGroup.vala Show resolved Hide resolved
src/Widgets/IconGroupContainer.vala Outdated Show resolved Hide resolved
src/Widgets/MultitaskingView.vala Outdated Show resolved Hide resolved
src/DragDropAction.vala Show resolved Hide resolved
@donadigo
Copy link
Contributor Author

@danrabbit @peteruithoven @cassidyjames all known issues fixed.

@peteruithoven
Copy link
Collaborator

The issue where the issues would disappear is fixed indeed.

The animation from right to left when a workspace is dropped, is a bit weird:
Screen record from 2019-07-14 00 28 35
When you just drag and drop a workspace to the same place it's somewhat less weird, because it's just the items on the right of it.
Screen record from 2019-07-14 00 30 51

Is that something we can improve with the current Mutter API?

@donadigo
Copy link
Contributor Author

@peteruithoven I feel like this should be resolved in another PR, I believe that to make this animation seamless there will be a lot of reworking needed on how IconGroupContainer works internally. I would like to avoid making the diff bigger.

Copy link
Collaborator

@peteruithoven peteruithoven left a comment

Choose a reason for hiding this comment

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

Only noticed one tiny typo

src/Widgets/IconGroupContainer.vala Outdated Show resolved Hide resolved
* Fix moving the container when dropping the IconGroup
@peteruithoven
Copy link
Collaborator

Nice! That fix helps:
Screen record from 2019-07-14 01 50 06

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't figure out how to reorder workspaces
5 participants