-
-
Notifications
You must be signed in to change notification settings - Fork 330
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
base: master
Are you sure you want to change the base?
Conversation
"holdShortcut3": { App.app.focusTarget() }, | ||
"holdShortcut4": { App.app.focusTarget() }, | ||
"holdShortcut5": { App.app.focusTarget() }, | ||
"holdShortcut": { App.app.handleHold(0) }, |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
Hi @JonyEpsilon 👋
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.
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.
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 👍 |
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! |
796a3c3
to
a67d123
Compare
7d7d9cf
to
8abb9b4
Compare
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 thetriggerPhase
isup
).