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

Move focused window to adjacent Workspace and switch to that Workspace #186

Merged
merged 6 commits into from
Feb 22, 2021

Conversation

dlundgaard
Copy link
Contributor

Hi, first time making a pull request, so please do point out if this PR is not up to standard.

What I'm proposing here is adding functionality to move the focused window to an adjacent workspace and "follow it" in the sense that you switch to the workspace where the focused window now resides.

As such, it is just a "shorthand" for placing a window in the n'th workspace with Alt+Shift+{n} and then switching to it with Alt+{n}.

I added the following key bindings for the above mentioned, for which I could not find any conflicts and I believe are quite intuitive as an extension of the original ones:

  • Alt+Shift+Left Move Focused Window To Previous Workspace And Switch to It
  • Alt+Shift+Right Move Focused Window To Next Workspace And Switch to It

… workspace and switch to it

Also added keybindings for above mentioned:
    Alt+Shift+Left  :: Move Focused Window And Switch To Previous Workspace
    Alt+Shift+Right :: Move Focused Window And Switch To Next Workspace
@josteink
Copy link
Member

I haven't looked too closely at the code, but we just merged this PR here today:

#163

Would you consider that an adequate solution to your needs, or does your PR target other use-cases?

I'm in no way trying to discourage you from contributing, but just trying to be sure we don't merge duplicate functionality.

@dlundgaard
Copy link
Contributor Author

Thanks for your reply, @josteink!

I had a look at #163, and if I'm not mistaken, that one relates to the shuffling/cycling of monitors, whereas this (#186) is meant to streamline moving around the focused window between workspaces on the same monitor.

However, it seems that the key bindings are overlapping, so some new ones will be needed if they are to be included as default. A suggestion would be Alt+Ctrl+Left, Alt+Ctrl+Right, but I would love to hear if you have any convention or guidelines on how to pick them so it stays intuitive.

@josteink
Copy link
Member

To be honest, since I was able to go full Linux on my workstation at work, I'm no longer really using Windows to such an extent that I need/use workspacer anymore.

I just still hang around and do code-review and stuff because I'm very much sympathetic to the goals of the project 😊

So that means @rickbutton or some other actual regular users will have to voice opinions about the ergonomics and preferences for new key-bindings.

Copy link
Member

@dalyIsaac dalyIsaac left a comment

Choose a reason for hiding this comment

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

This functionality seems useful, and the new key-bindings make sense to me.

@@ -16,6 +16,8 @@ public interface IWorkspaceContainer

IWorkspace GetNextWorkspace(IWorkspace currentWorkspace);
IWorkspace GetPreviousWorkspace(IWorkspace currentWorkspace);
int GetNextWorkspaceIndex(IWorkspace currentWorkspace);
int GetPreviousWorkspaceIndex(IWorkspace currentWorkspace);

Copy link
Member

Choose a reason for hiding this comment

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

Since GetNextWorkspaceIndex and GetPreviousWorkspaceIndex are being added, I think it would make sense to expose a GetWorkspaceIndex(IWorkspace workspace) method too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I updated the default key bindings and added GetWorkspaceIndex(IWorkspace workspace) in 5de1d6c.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! Would you be able to use GetWorkspaceIndex in GetNextWorkspaceIndex and GetPreviousWorkspaceIndex too?

dlundgaard and others added 3 commits February 21, 2021 09:13
…t with those recently defined in workspacer#163

added `int GetWorkspaceIndex(IWorkspace workspace)` to the `IWorkspaceContainer` as suggested by @dalyIsaac
…` and `GetPreviousWorkspaceIndex` to reduce code duplication
@josteink josteink merged commit 3278398 into workspacer:master Feb 22, 2021
@josteink
Copy link
Member

LGTM. Thanks for the patch!

Feel free to send more PRs in the future.

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.

3 participants