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

[draft][summon-workspace] summon workspace works on non-focused monitors #606

Conversation

raisjn
Copy link

@raisjn raisjn commented Oct 20, 2024

Description

If the workspace has an assignedMonitor, it pulls that workspace onto its assigned monitor but the focus is retained on the current monitor. If the workspace is visible, nothing happens and focus stays on the current monitor

use case

You may keep a terminal on one monitor and want to cycle what is visible on the other monitor while staying in the terminal. This fixes the issue of rapidly cycling workspaces causing the monitor focus to shift. When wanting to focus a monitor, focus-monitor is used instead.

Test Plan

[1,2,3] are main, [8,9,0] are secondary, [U,Z] have no assigned monitor

  • [1, 0*] - summon 2 -> [2, 0*] (summons 2 to pinned monitor)
  • [1, 0*] - summon U -> [1, U*] (summons U to current monitor)
  • [U, 0*] - summon U -> [U, 0*] (nothing happens because 0 is pinned)
  • [U, Z*] - summon U -> [Z, U*] (swap workspaces)

Notes

  • feel free to edit the commit
  • feel free to ignore this commit, this is just how i configured the behavior for myself

@nikitabobko
Copy link
Owner

If the workspace has an assignedMonitor, it pulls that workspace onto its assigned monitor but the focus is retained on the current monitor

It's yet unclear to me if it should be the default behavior. At least, it should be configurable to add possibility to focus the "pinned" workspace

alt-1 = 'summon-workspace 1 --reveal-if-force-assigned' # just make it visible, as you suggest
alt-1 = 'summon-workspace 1 --focus-if-force-assigned' # focus it

How does Xmonad behave? Does Xmonad even have a notion of force assigned workspaces?

Right now, I lean towards: it should be an error by default (as it is now), and the error message should mention --reveal-if-force-assigned and --focus-if-force-assigned flags to make the flags more discoverable.

[U, 0*] - summon U -> [U, 0*] (nothing happens because 0 is pinned)

Thanks for bringing this example! I didn't think about it. Alternatively, it could work this way: [U, 0*] - summon U -> [1, U*]

But given: "workspaces try to swap if both of them are visible" mantra, the feature interaction of workspace-to-monitor-force-assignment and summon-workspace becomes tricky, I don't have an answer myself yet


And sorry to mention it, but the quality of this PR is subpar too. I've not yet tried to understand the code you wrote, but the PRs should at least keep the CI green

@nikitabobko
Copy link
Owner

I don't have an answer myself yet

Maybe it should be a separate flag --swap-if-both-are-visible (the flag should be incompatible with --focus-if-force-assigned/--reveal-if-force-assigned)

Your 3rd example got me thinking that existing workspace-to-monitor-force-assignment feature makes it impossible to make AeroSpace behave like Xmonad by default.

@raisjn raisjn changed the title [summon-workspace] summon workspace works on non-focused monitors [draft][summon-workspace] summon workspace works on non-focused monitors Oct 21, 2024
@raisjn raisjn closed this Oct 21, 2024
@raisjn
Copy link
Author

raisjn commented Oct 21, 2024

Your 3rd example got me thinking that existing workspace-to-monitor-force-assignment feature makes it impossible to make AeroSpace behave like Xmonad by default.

Yes, that is what I have been thinking about and why I implemented it this way - I've never used Xmonad with pinned screens. I came up with this solution because it feels closest to how I was using Xmonad. I was trying to accomplish: "when using workspace switching, it should never move the mouse off the current monitor".

And sorry to mention it, but the quality of this PR is subpar too. I've not yet tried to understand the code you wrote, but the PRs should at least keep the CI green

No problem - I should have marked this draft - my intent is to start a conversation + demonstrate the behavior I am using. I will push my changes to a fork in the future if I need to demonstrate any code.

A hanging semicolon and some syntax ("let onMonitor = onMonitor") caused the linter to fail in CI. I ran tests locally and they pass (using swift test), but the scripts in your repo don't work for me. They silently exit during the source ./script/setup.sh step - this is a point of friction which is making it difficult to create builds, install dependencies, automatically run tests, generate docs and run the linters. I'm not sure if it is because I'm running zsh or some other reason.

@nikitabobko
Copy link
Owner

nikitabobko commented Oct 21, 2024

No problem - I should have marked this draft - my intent is to start a conversation + demonstrate the behavior I am using. I will push my changes to a fork in the future if I need to demonstrate any code.

Thanks for understanding, and sorry if I sounded too harsh. If someody wants to show a snippet, it's better to attach it as patch in the issue comment, or give a link to the commit in their fork. PRs create an impression that the person expects the commit to be merged, and I expect PRs to be polished to an acceptable degree. Red CI is a big red flag that the person didn't even run tests.

but the scripts in your repo don't work for me. They silently exit during the source ./script/setup.sh step

Thanks for mentioning! The failures should no longer be silent in main branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-rejected Pull Request is rejected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants