paperwm / PaperWM

Tiled scrollable window management for Gnome Shell
GNU General Public License v3.0
2.73k stars 122 forks source link

Spaces: Add sequential workspace switching and window move using sliding wall animation #115

Closed goodwillcoding closed 4 years ago

goodwillcoding commented 5 years ago

Some notes: I avoided using switchWorkspaces leaving exclusively to non PaperWM uses as it stands now.

Please review and offer feedback. I intend to document and comment function some more as well, so do not pull PR yet (@hedning @olejorgenb)

hedning commented 5 years ago

:+1: I'd like to support this feature, but I think it needs its own animation. The carousel simply doesn't make much sense when navigating sequentially.

goodwillcoding commented 5 years ago

What are you thinking for animation? Showing slightly smaller clones but have them slide? Add arrows?

My original thinking was

  1. Both next/prev and sequenctial are workspace switching
  2. Reluctance to introduce more code then needed
  3. Gnome shell standard switcher I think is mostly unusable cause no previews (one used by switchWorkspaces)

That said in my work flow I rarely cycle thru all of them using Super+Above_Tab mru style so I might not be the best example since I mentally do not diffentiate between 2 types of switches but others might.

hedning commented 5 years ago

Gnome shell standard switcher I think is mostly unusable cause no previews

Yeah, it also won't integrate with the per-workspace background. And since we keep a full clone of all the workspaces contained in a single actor it's pretty straightforward to make a another animation. We also don't want to trigger focus until navigation is done, which is again difficult to achieve with the native switcher.

Showing slightly smaller clones but have them slide

Probably something like that. In general I think navigating through a fixed order vs. navigating through an mru . And in case of fixed order there's a pretty obvious way to visualize it as you note. Making page_up/page_down navigate sequentially seems like a good default.

Now, I think multimonitor needs some thought. Having one order which is shared between monitors isn't trivial (eg. workspaces that are visible on one monitor will leave gaps). The first solution that comes to mind is limiting sequential navigation to workspaces that are attached to the current monitor: https://github.com/paperwm/PaperWM/blob/393365ed05e71194bdbdd2595486334960ff58cb/tiling.js#L946

goodwillcoding commented 5 years ago

@hedning okie everything you said and the code review comments make sense to me. I am going to do the monitor changes and work on slide style animation (might take some time, never done it before).

As a side note after your explanation I now understand why animation is a stack now and can not unsee it anyore ... and sequential space navigation using a stack does feel odd to me now ... lol.

hedning commented 5 years ago

why animation is a stack

Hehe, yeah, I basically stole the idea from android's most recently used window stack.

Some stuff that might be useful:

Happy to answer any questions you might have :)

goodwillcoding commented 4 years ago

@hedning ... sorry it took so long, I was learning gnome shell extension development. I reimplemented the sequential navigation to your feedback and rebased my branch. When you get a chance can you please check it out.

All your feedback was implemented as far as I can tell, including restriction to monitor, but I am still testing multimonitor setup (I guess I need to test with dynamic space? [I usually use static ones]? right?)

Couple outstanding things.

  1. I am not really liking InPreviewMode but ecmascript is not supporting enums, so meh
  2. I am going to look into tweak clone placement so upper workspace is partially visible, assuming this approach work
  3. Super-Above did not seem to be an issue, it actually works well between switcing sequential and stack modes nicely
hedning commented 4 years ago

No worries, the code base isn't really that pretty so big props to getting this done :)

Review incoming.

goodwillcoding commented 4 years ago

@hedning when you get a chance can you please check out 2e28be0. I improved the sequential animation positioning to show a small part of either next, previous or bothe next and previous worskpace which is a tad more intuitive and matches other switchers I have seen. There is a small cost of dropping scale from 0.9 to 0.825 (and for padding I use 4% of workspace height) which I also applied to the stack animation to match since it's possible to start with one and switch to another.

See it in action Peek 2019-11-07 23-29

hedning commented 4 years ago

I also applied to the stack animation to match since it's possible to start with one and switch to another.

Might be better to make the mode «stuck», so redirect any select to switch if we're in sequential. Not entirely sure, but going between sequential and stack doesn't seem very intuitive or useful to me. (If we land on them being used together then select and switch needs to set inPreview, otherwise the finishing animation won't work correctl (ie. animateToSpace won't know what to do).

goodwillcoding commented 4 years ago

I also applied to the stack animation to match since it's possible to start with one and switch to another.

Might be better to make the mode «stuck», so redirect any select to switch if we're in sequential. Not entirely sure, but going between sequential and stack doesn't seem very intuitive or useful to me. (If we land on them being used together then select and switch needs to set inPreview, otherwise the finishing animation won't work correctl (ie. animateToSpace won't know what to do).

Fixed in 0790409, now it's no longer possible to switch to stack when sequetial mode is on and vice versus.

goodwillcoding commented 4 years ago

@hedning renamed the navigation methods close to what you suggested -> d18e1d3

hedning commented 4 years ago

Pushed a few fixes. In particular animateToSpace will now animate the whole sequence.

I'm happy with how things work/look now at least :) Just need to squash some commits and fixup commit messages, so they describe what's been done. Also keep the description to one line and not too long.

One remaining «issue» is animations being reset when releasing Super, though it's not a huge deal and will probably require quite a bit of refactoring (probably need to reparent the spaces into a single container that can be moved and scaled together). I'd say we don't bother with that for now, and rather land this first.

goodwillcoding commented 4 years ago

@hedning when you say squash some commits do you wantr to do all 21 commits both of us done into 1 or didi you mean you wanted to squash some of your more specific?

goodwillcoding commented 4 years ago

@hedning I have a 21 commit squashed on stand by with following message, let me know if it works for you

    Add sequential workspace switching and window move using sliding wall animation

    Move Super+PageUp/PageDown and Ctrl+Super+PageUp/PageDown from workspace stack
    to sequential workspace. Rename selectSpace to selectStackSpace. Rework
    animation meachanism to accomodate stack and sequential.
hedning commented 4 years ago

Took care of squashing/rebasing.

goodwillcoding commented 4 years ago

@hedning anything else you need before merging this? (I take it you do not want to squash all the commits into 1)

goodwillcoding commented 4 years ago

Also have this pending for the screenshot of sequential animation: https://github.com/paperwm/media/pull/1

hedning commented 4 years ago

Nope, think it's fine, will merge soon if I don't find any bugs etc. (should probably double check multiple monitors, only checked with that minimally).

goodwillcoding commented 4 years ago

@hedning works well on 3.28 multiple monitors. One monitor only has 1 space though (is that correct). I use 4 spaces on 2 dual monitors.

hedning commented 4 years ago

@hedning works well on 3.28 multiple monitors. One monitor only has 1 space though (is that correct). I use 4 spaces on 2 dual monitors.

Yeah, since monitors can have different dimensions we don't want to move spaces without reason. Now that means we also want an easier way to move a space between monitors (eg. something like examples.keybindings.moveSpaceToMonitor).

hedning commented 4 years ago

We should probably show empty workspaces on all monitors. Will take a look in the future.