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 keybindings to switch to ws from all monitors #645

Merged

Conversation

Lythenas
Copy link
Collaborator

@Lythenas Lythenas commented Oct 25, 2023

Fixes #635

Adds two new keybindings:

  • Switch to workspace below (ws from all monitors)
  • Switch to workspace above (ws from all monitors)

by default they have no keys bound.

They work the same as "Switch to workspace below/above (ws only from current monitor)" (renamed from just "Switch to workspace below/above"). Except the user can switch to all workspaces (except the once currently active on another monitor).

Please test this.

@Lythenas
Copy link
Collaborator Author

The animation at the beginning (i.e. when starting the switching) is a tiny bit strange if there are workspaces that were previously on other monitors. I think they ease into position from the other monitor. But it was a bit hard to tell in the VM. I will test tomorrow on my real device and create a recording.

@Lythenas Lythenas added the testing-requested Testing by users is requested for this PR/branch/issue label Oct 25, 2023
@jtaala
Copy link
Collaborator

jtaala commented Oct 25, 2023

Sounds good. For animation stuff, it's often helpful to set the animation-time to something big (like 5.00 or 6.00) - makes it much easier to see what's going on.

@jtaala
Copy link
Collaborator

jtaala commented Oct 26, 2023

Hey @Lythenas - this looks good to me! Tested and didn't see/notice any weird animation stuff too.

Good job!

tiling.js Outdated Show resolved Hide resolved
@Lythenas
Copy link
Collaborator Author

I slowed down the animation and to me it looks like if the workspace was previously on that monitor or is empty. The workspace is positioned below the current workspace, when the animation begins. If the workspace was previously on another monitor it is positioned kind of behind the current workspace. But it is also moved to the correct position and this is barely noticeable on normal animation speed.

To be honest I'm not sure how to fix that. And I'm fine with the current state.

@Lythenas Lythenas marked this pull request as ready for review October 27, 2023 17:08
@Lythenas Lythenas requested a review from jtaala October 27, 2023 17:09
without actually setting the space.monitor to it.  I.e. only set the
monitor to a space if it's actually selected.
Copy link
Collaborator

@jtaala jtaala left a comment

Choose a reason for hiding this comment

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

Hey @Lythenas,

I just made a couple of tweaks.

Main one is not actually setting the space.monitor until the space is actually selected. In other words, just switching (in sequence switching view) shouldn't actually set the monitor to that space - in case users have both sets of keybinds set, and might get confused why their space "moved" to another monitor when they didn't actually select the space on that monitor. That's the theory anyway - let me know if that makes sense and what you think.

I also see the animation - I think it looks cool, and in the very least is a small visual signal that this space was on the previous monitor.

I don't mind it at all. We could probably dig deep to try change that but I don't think it's worth it tbh.

Approving this.

@jtaala
Copy link
Collaborator

jtaala commented Oct 28, 2023

Oh, now for the (maybe not fun?) part. Can you throw a small section in the README.md re this stuff if you're happy with the code?

@Lythenas
Copy link
Collaborator Author

Lythenas commented Oct 28, 2023

Makes sense. I also noticed this but thought people probably won't use both keybindings. But this better.

Ah totally forgot about the readme. I will write something.

@Lythenas
Copy link
Collaborator Author

Lythenas commented Oct 28, 2023

I added something to the "The workspace stack & monitors" section in the readme. I hope it is understandable.

@jtaala feel free to merge if this is ok. It says it needs an additional approval, but I can't approve my own PR.

@jtaala jtaala added the merging soon Label for PRs that are planned to be merged soon (usually within the next week) label Oct 28, 2023
@jtaala
Copy link
Collaborator

jtaala commented Oct 28, 2023

Hey @Lythenas,

Just approved. Looks good to me. Can you give merging a go? (you should be able to merge - unless I messed the settings up).

Jay.

@jtaala jtaala removed the testing-requested Testing by users is requested for this PR/branch/issue label Oct 28, 2023
@Lythenas Lythenas merged commit a2c2c08 into paperwm:develop Oct 28, 2023
@Lythenas Lythenas deleted the switch-to-workspace-from-all-monitors branch October 28, 2023 15:35
jtaala added a commit that referenced this pull request Nov 14, 2023
Gnome 44 port of #645

> Fixes #635
> 
> Adds two new keybindings:
> 
>     Switch to workspace below (ws from all monitors)
>     Switch to workspace above (ws from all monitors)
> 
> by default they have no keys bound.
> 
> They work the same as "Switch to workspace below/above (ws only from
current monitor)" (renamed from just "Switch to workspace below/above").
Except the user can switch to all workspaces (except the once currently
active on another monitor).
> 
> Please test this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merging soon Label for PRs that are planned to be merged soon (usually within the next week)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants