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

WIP: Add quick-actions #94

Closed
wants to merge 1 commit into from
Closed

Conversation

rbnis
Copy link
Contributor

@rbnis rbnis commented Dec 27, 2019

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.
Ideas on how to achieve this are more than welcome
Works now :)
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:

  • Close windows (w)
  • Quit application (q)
  • Snap window left (arrow left)
  • Snap window right (arrow right)
  • Enter full screen (f)
  • Minimize Window (m)

Closes #9

Copy link
Owner

@lwouis lwouis left a 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/logic/Keyboard.swift Outdated Show resolved Hide resolved
alt-tab-macos/logic/TrackedWindow.swift Outdated Show resolved Hide resolved
alt-tab-macos/logic/TrackedWindow.swift Show resolved Hide resolved
Comment on lines 109 to 110
// slight delay needed to give the window time to close before updating the thumbnails
DispatchQueue.main.asyncAfter(deadline: DispatchTime.now() + .milliseconds(75), execute: {
Copy link
Owner

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 :)

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'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 🚀

alt-tab-macos/ui/Application.swift Outdated Show resolved Hide resolved
@rbnis rbnis force-pushed the quick-actions-wip branch 3 times, most recently from 0b6332b to 16b7ce0 Compare December 30, 2019 21:26
@rbnis rbnis closed this Jan 3, 2020
@rbnis
Copy link
Contributor Author

rbnis commented Jan 3, 2020

Sorry, messed something up while pushing a change to the Observer class. Ignore the close und reopening of the PR.
For closing windows I now use a kvo observer which works better in this situation. But tbh I'm not happy how the observer class looks at the moment, I think it's real messy. Any Ideas how this could be refactored? Especially with the 2 different observer types it currently combines.

Copy link
Owner

@lwouis lwouis left a 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:

image

image

  • 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 being m is pretty hard to do with 1 hand; you may want to consider another one

@rbnis
Copy link
Contributor Author

rbnis commented Jan 5, 2020

No problem, it's holiday season in Germany too :)

Your observations seam fitting, dealing with quitting apps is a lot worse than the windows.
But for the first bug I am not sure what's happening, the panel shouldn't close except when releasing the modifier.
And the switching of spaces is the focus event wich is triggered when releasing the modifier. This is annoying if one only wants to close a window / app. Maybe we could dismiss the focus event until another window is selected?
As for the Bug with partial rendered Thumbnails, I have an idea how to work around this and will later look into it :)

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 :)

@lwouis
Copy link
Owner

lwouis commented Jan 5, 2020

But for the first bug I am not sure what's happening, the panel shouldn't close except when releasing the modifier.

Are you able to reproduce that behavior of losing focus when quitting apps on your side?

Maybe we could dismiss the focus event until another window is selected?

Well I think we should fix the root cause, then we would not have this issue of focusing like that.

As for the Bug with partial rendered Thumbnails, I have an idea how to work around this and will later look into it :)

I updated #110 with some info regarding the OS APIs that get us the screenshots. You may find it interesting/related.

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.

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 AXUIElements of every single window, without resorting to private API black magic like is currently done, when focusing a window from another space for instance.

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.

@rbnis
Copy link
Contributor Author

rbnis commented Jan 5, 2020

Are you able to reproduce that behavior of losing focus when quitting apps on your side?

Unfortunately not. Under what circumstances did this issue occur? Was the app you were trying to quit minimized, fullscreen, on the current space?

Well I think we should fix the root cause, then we would not have this issue of focusing like that.

I totally agree. When there is a bug which closes the thumbnails panel, this should be fixed before resorting to workarounds.

I updated #110 with some info regarding the OS APIs that get us the screenshots. You may find it interesting/related.

This is really nice to know. In this case I thought of just removing the windows from the TrackedWindows. Since we get notified when the Application is closed it should be be safe to do so here.

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.

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 AXOberserver. For example is it triggered when you close a window, but not necessarily wen quitting an application. This is the reason I use KVO for application quitting.

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 sleep 10 && exit is a good test for windows that take a long time.
And for closing windows / apps on different spaces I mostly use AltTab itself since that's the thing I'm currently interested in. But here sleep 10 && exit could come in handy too.
But I like the AppleScript way, since it seems a bit more controlled :D

@lwouis
Copy link
Owner

lwouis commented Jan 8, 2020

@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

@lwouis lwouis mentioned this pull request Jan 8, 2020
@lwouis
Copy link
Owner

lwouis commented Apr 7, 2020

Hi @Robinhuett! I was wondering if now that the master branch is stable and contains everything needed to get to a nice and easy implementation of the ticket, you would be interested in implementing it.

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! :)

@rbnis
Copy link
Contributor Author

rbnis commented Apr 19, 2020

Hi @lwouis
I'm sorry for the late response. To be honest I haven't looked at the code for a while so I'm not really up to date on the most recent changes.
I can definitely take another stab at this, if you haven't already started.

Comment on lines +97 to +100
if isFullScreen() {
AXUIElementSetAttributeValue(self, AXAttributeKey.fullScreen.rawValue as CFString, 0 as CFTypeRef)
return
}
Copy link
Owner

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! 👍

@lwouis
Copy link
Owner

lwouis commented Apr 19, 2020

@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.

@lwouis
Copy link
Owner

lwouis commented Apr 19, 2020

Closing as it was subsumed by #251. Thanks again for your contribution, especially the private API for fullscreen windows! Really cool stuff!

@lwouis lwouis closed this Apr 19, 2020
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.

Add quick-actions on thumbnails
2 participants