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

feat: add option to minimize other windows when switching #2290

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JonyEpsilon
Copy link

This PR adds a feature to minimise other windows, both as an intentional action from the switching window, and as a configurable default action when completing a switch.

I'm not sure you'll want to keep adding features to AltTab until it becomes all things to all people, so I won't be offended if you'd prefer not to add this functionality :-) There do look to be a few requests for something similar: #1530, #1902, #2086 . These FRs asked for the ability to hide other apps on switching, but that feels like quite a heavy action on MacOS - hiding all windows across all spaces - whereas minimising other windows can be implemented in a way that just applies to the set of windows that the current AltTab action is applying to, whether that be a single space or all spaces.

I've tried to change the existing logic as little as possible, as my impression is that it's probably been polished in the face of a thousand edge cases! I'm not sure the implementation is as clean as it could be, and we could happily streamline it if you're happy to risk changing the logic more (in particular, the thing that could be cleaned up, I think, is how the "action" to either focus the selected window, or minimise the other windows is triggered inside ATShortcut when the triggerPhase is up).

"holdShortcut3": { App.app.focusTarget() },
"holdShortcut4": { App.app.focusTarget() },
"holdShortcut5": { App.app.focusTarget() },
"holdShortcut": { App.app.handleHold(0) },
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure about the name of this function, but I can't think of a better one :-)

static func minimizeOtherWindows() {
Windows.list.enumerated().forEach {
if $0.offset != focusedWindowIndex && !$0.element.isMinimized && !$0.element.isWindowlessApp && $0.element.shouldShowTheUser {
$0.element.minDemin()
Copy link
Owner

Choose a reason for hiding this comment

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

You check shouldShowTheUser here. Is it what the user would expect though? Let's imagine I use the command+` shortcut. It shows me only windows of the active app. When I set it to "minimize other windows" on release, I think I expect that every other window on my screen will minimize, except the one I just focused, no? Here it will only minimize the other windows of the active app.

Same logic will keep some windows on-screen, depending on the shortcut filtereing logic behind shouldShowTheUser.

Another point: I wonder if people will expect this single-window mode to be per-Space and/or per-Screen. I'm not sure users expect windows in every Space and on every Screen to minimize when they go for this type of workflow. Maybe they would like to have a "main window" per Screen?

What do you think?

@@ -28,6 +28,7 @@ class Preferences {
"minDeminWindowShortcut": "M",
"quitAppShortcut": "Q",
"hideShowAppShortcut": "H",
"minimizeOtherWindowsShortcut": "1",
Copy link
Owner

Choose a reason for hiding this comment

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

I think adding the option to minimize other windows is not adding much noise in the existing dropdown "on release:". However, as a secondary shortcut like quit/hide/etc, I think it's too niche to have such a prominent UI in the preferences. This shortcut 1 would also conflict with #210. I think the feature is a bit too niche to have a dedicated shortcut as such.

@lwouis
Copy link
Owner

lwouis commented Feb 3, 2023

Hi @JonyEpsilon 👋

This PR adds a feature to minimise other windows, both as an intentional action from the switching window, and as a configurable default action when completing a switch.

I'm not sure you'll want to keep adding features to AltTab until it becomes all things to all people, so I won't be offended if you'd prefer not to add this functionality :-) There do look to be a few requests for something similar: #1530, #1902, #2086 .

I think it's ok to add yet another preference, if it's in the existing dropdown "On release:". I think it's a dropdown very few people touch in the first place, so it's ok to cram another power-user option there. Regarding adding a new secondary shortcut, I think it's too niche to warrant the added complexity though.

These FRs asked for the ability to hide other apps on switching, but that feels like quite a heavy action on MacOS - hiding all windows across all spaces - whereas minimising other windows can be implemented in a way that just applies to the set of windows that the current AltTab action is applying to, whether that be a single space or all spaces.

I made a comment in the PR to discuss the topic of which windows should be affected by the trigger.

I will add, regarding other people wanting to hide windows instead of minimizing, that I think if we add this feature, we could as well add yet another dropdown option to hide instead of minimizing.

I've tried to change the existing logic as little as possible, as my impression is that it's probably been polished in the face of a thousand edge cases! I'm not sure the implementation is as clean as it could be, and we could happily streamline it if you're happy to risk changing the logic more (in particular, the thing that could be cleaned up, I think, is how the "action" to either focus the selected window, or minimise the other windows is triggered inside ATShortcut when the triggerPhase is up).

You're very kind. I think the codebase is a bit messy, as a result of the million complexities and tricks we do to get macOS to comply to basic functionality (e.g. there is no public API to manipulate Spaces, so we use private APIs/SPIs and side-effects to detect things).

The code in this PR is of very good quality, in my eyes. I think if you're ok to make a few changes from my suggestions, I can merge this PR and release soon after 👍

@JonyEpsilon
Copy link
Author

Hey, just a very quick comment to say that the above all sounds reasonable, and I'll update the PR when I have a chance - been a bit busy lately.

I might need some advice on how to select the proper set of windows to minimise, but I should think about it myself first!

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.

2 participants