-
-
Notifications
You must be signed in to change notification settings - Fork 345
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
WIP: Add quick-actions #94
Conversation
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.
Great PR overall! I'm impressed at how you put the right code in the right place without effort. That's pretty cool.
The big blocker for merging is the delay to refresh the UI. I pointed out 2 solutions that I think are more robust. Let me know what you think about that, and the other minor comments I made :)
alt-tab-macos/ui/Application.swift
Outdated
// slight delay needed to give the window time to close before updating the thumbnails | ||
DispatchQueue.main.asyncAfter(deadline: DispatchTime.now() + .milliseconds(75), execute: { |
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 afraid this is too fragile. Many windows will take longer to close so it will fail. I think there are way to listen to windows being closed and application being quit. That would be one way to follow the OS work completion.
Another way would be to optimistically update our UI to remove the thumbnail(s), and consider that the OS doing the work is an async task now that will happen eventually (which it should). We could either remove the thumbnails, or mark them somehow like opacity reduced, black, red cross, etc.
I think these are the 2 ways to go: (a) listen to window close events somehow, or (b) update UI optimistically and let the OS deal with execution of the order
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.
Yeah, as I said, im not happy with the delays either.
I'll start with your second suggestion, as this is easily ruled out. Many applications ask you, if you really want to close the window. iTerm with multiple Tabs for example. In this case I think it would be bad UX if we change our UI when the application doesn't.
Now to the first suggestion, which I also thought about. I think this would definitely be the way to go. I briefly looked into AXObserver with which we should be able to do this. Unfortunately I haven't got the notifications working. I'll take another look at this solution since this would be the best.
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.
Update: I got the AXObserver
working. I'm now looking into how I will implement it to be as flexible as possible. Who knows for what it's good in the future :)
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 now using the AXObserver
. So far it seems to work beautifully. I even got closing fullscreen windows to work with it :)
But extensive testing is more than welcome 🚀
0b6332b
to
16b7ce0
Compare
16b7ce0
to
c69123b
Compare
Sorry, messed something up while pushing a change to the Observer class. Ignore the close und reopening of the PR. |
b8476e8
to
5c585be
Compare
5c585be
to
f109ed1
Compare
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.
Hey @Robinhuett! Sorry for the delay in reviewing your PR! Holidays are quite a rare treat here in Japan :p
I tested your PR locally and here are my observations:
-
Closing windows seems to work great 👍
-
Bug: quitting an app closes the thumbnails panel instead of keeping it open and updating it, like when closing windows
-
Bug: closing a fullscreen window refreshes the panel. See these before and after screenshots:
-
Bug: quitting an app by selecting a window on another space successfully quits the app, but switches the user to that window space. I think quitting apps or closing windows should not move the user to any space. Note that closing a window on another space works fine.
-
Overall, I think there are some issues with quitting apps, but closing windows seems ready
-
I haven't reviewed the code yet in depth, but I'm pretty sure it introduces very complex race conditions that are hard to test/reproduce like closing a window that's slow to close. Then what if you close it again while it's closing? what if there is an animation like minimizing another or the same window at the same time? What if you quit an app and close one of its window, or the opposite? etc. Overall we should write code here that is not strict, so that these messy race conditions due to delays/animations don't silently crash the app and the user is like "Wait why isn't AltTab in my menubar anymore? What happened?"
-
A side-note since your PR doesn't implement these at the moment: the shortcut your outlined at the top to snap are already used to navigate between thumbnail (
arrow keys
). Also the shortcut to minimize beingm
is pretty hard to do with 1 hand; you may want to consider another one
No problem, it's holiday season in Germany too :) Your observations seam fitting, dealing with quitting apps is a lot worse than the windows. And yeah, this whole closing windows and quitting apps unfortunately has a lot of potential race conditions. Currently I test a few things when changing something, especially with slowly or not closing windows. But my main goal is to get it working properly. But before any of this ships I plan to test everything as much as I can. I have already noted some edgecases which I want to make sure don't make problems. For the currently not implemented shortcuts, I wanted to get the closing/quitting right first to not litter the commit history too much. I have started with minimizing/fullscreen in another branch. But as I said, one thing at time :) |
Are you able to reproduce that behavior of losing focus when quitting apps on your side?
Well I think we should fix the root cause, then we would not have this issue of focusing like that.
I updated #110 with some info regarding the OS APIs that get us the screenshots. You may find it interesting/related.
I would be really interested in how you simulate/reproduce these corner-cases. I'm currently investigating a big refac of the whole app (i.e. a v3) that would leverage AX observers that build up a state of the system as apps and windows get open/closed, instead of querying the system state on trigger every time. The advantage would be better performance since we process small deltas instead of the whole system every time, but current performance is good I think, so it's eh. The main motivation is to have a cleaner foundation: by using AX observers we may be able to get I just tested locally, and indeed the AX API triggers on a window getting closed on another space. That's encouraging! I used AppleScript to close the window from my current space without visiting the other space. These are the kind of tricks I would love if you could share with me if you have some: easy ways to interact with other space objects, or to simulate a window that's slow to close or don't close, etc. |
Unfortunately not. Under what circumstances did this issue occur? Was the app you were trying to quit minimized, fullscreen, on the current space?
I totally agree. When there is a bug which closes the thumbnails panel, this should be fixed before resorting to workarounds.
This is really nice to know. In this case I thought of just removing the windows from the
The refac sounds interesting. But I'm not sure if al that is needed for 'a simple window switcher'. Also I ran into some limitations while using the As for the testing, one could say I have gone the low budget route. For not closing windows for example I opened 2 Tabs in a iTerm2 Window so it asks me if I really want to quit. Also since I have way to may Tabs open in Safari it's a kinda okayish testcase, even if it's not an edgecase. But iTerm, running |
@Robinhuett I just opened a PR for v3 (#114) which makes it much easier to implement the close window and quit app that you started working on in this PR. I would suggest that either you port your work here on top of my PR, or I'll probably do it some. I think we should be able to implement both quick-actions and #39 with the same code since they are both about listening to the apps/windows in the background, which v3 does already for all apps all the time, and simply refreshing the UI when a change happens. I think it's like 5 lines of code + sending the close and quit message when the shortcut are pressed. Let me know if you want to port your rebase or if you prefer I do it. I think the effort is minimal now that v3 is listening to everything |
Hi @Robinhuett! I was wondering if now that the I'm asking, because in case you are no longer interested or too busy, I have bandwidth to do it now. Please let me know this week! :) |
Hi @lwouis |
if isFullScreen() { | ||
AXUIElementSetAttributeValue(self, AXAttributeKey.fullScreen.rawValue as CFString, 0 as CFTypeRef) | ||
return | ||
} |
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 don't know how you found this private constant, but it works great! 👍
@Robinhuett Oh! I thought you were too busy so I started today. I'm porting the close/quit actions from your PR. I think i'll finish the basic ones. Then for the more advanced things, one of us can do it later. I'm talking about moving left/right, resizing, or displaying UI for mouse triggers. |
Closing as it was subsumed by #251. Thanks again for your contribution, especially the private API for fullscreen windows! Really cool stuff! |
This PR adds quick-actions to quickly manipulate the selected window/application.
Current limitation aside from the progress below is, that windows in fullscreen can't be closed.Works now :)Ideas on how to achieve this are more than welcome
Also im not entirely happy with the delays in./ui/Application.swift
(LOC 110 & 119). Input regarding this is also appreciated.@lwouis: Could you take a look at it, if this is going in the direction you intended? :)
Progress:
w
)q
)arrow left
)arrow right
)f
)m
)Closes #9