phillco / talon-axkit

Talon macOS accessibility magic!
MIT License
45 stars 12 forks source link

window_action: use appscript to close all windows #55

Closed phillco closed 1 year ago

phillco commented 1 year ago

I noticed that stuff like "window close all" in applications like Finder was really slow and flaky, and would often result in several windows being left open if you had a lot open (probably because they technically treat tabs as windows).

I was poking around with AppleScript and noticed that it's actually much faster and more reliable to just close all of the windows that way, ie with:

tell application "Finder"
    close every window
end tell

It fixes the Finder issue, and even in applications previously behaved well, there is an improvement (all windows close immediately, rather than a cascade of closures as it iterates through them). So let's use that!

I didn't do much to improve the "current window" and "other windows" use cases, although it probably not much work. I just wanted to knock this out quickly.

nriley commented 1 year ago

Looks like a start although I am a bit worried about apps that are not scriptable or have broken scripting interfaces. Closing all windows in an app is not something I do very often, and I didn't realize you implemented this with different syntax to the typical knausj "noun verb" so I'd not been using any of these commands.

I notice you have a long timeout here. Is this because closing is synchronous in some apps which will do things like prompt to save changes? (Note that the timeout is in seconds — maybe you thought it was milliseconds? https://appscript.sourceforge.io/py-appscript/doc/appscript-manual/11_applicationcommands.html) I'd recommend you don't bother waiting for a response to the event more than a couple hundred milliseconds, so this doesn't hang the thread in Talon. You should get a different exception (immediately) if the app’s scripting dictionary doesn't support window closing, for example:

2023-06-19 10:00:00    IO Error closing windows for app Discord using appscript: <class 'AttributeError'> Unknown property, element or command: 'windows'; falling back to accessibility
phillco commented 1 year ago

The 10s was pretty arbitrary. I only included it because appscript included a timeout by default, and I was worried about the possibility of it hanging indefinitely. I can set it to something lower like 3-5s, although I think it can genuinely take a while for an application to close all of its windows if there are a lot of them and it's not the most performant UI system (for example, intellij).

Do you know if this timeout is for just sending the message, or for the entire action to run? If it's the former, then only waiting for 300ms make sense; if it's the latter, I think it's reasonable to wait a few seconds if it really needs that time to finish the work.

I'm guessing if we hit the timeout, it won't interrupt the process of closing the windows (?), but it will cause this code to try to proceed to accessibility, which is not necessary.

I didn't realize you implemented this with different syntax to the typical knausj "noun verb" so I'd not been using any of these commands.

Ah, I didn't realize the versions in the repo were verb-noun, I thought I had checked in noun-verb versions. I can add those.

I personally find these pretty useful!

nriley commented 1 year ago

I think you can set the timeout very short. You don't care about a successful response or subsequent actions, only a failed response to allow you to fall back to accessibility. Yes it should not interrupt the process of closing the windows!

phillco commented 1 year ago

That's fine; I'm just wondering if there's a case where the scripting call would succeed, but legitimately take a few seconds, and we might time out before it finishes.

nriley commented 1 year ago

The timeout is only on the client side. The server (application) isn't even aware of the timeout — so nothing to worry about.

phillco commented 1 year ago

Got it. So something like 0.3?

nriley commented 1 year ago

Yup! Just enough to ensure that you can get an error response — should be fine, particularly when the fallback will still work!

phillco commented 1 year ago

Done. I also refactored slightly and made the AttributeError silent, since we expect plenty of applications not to implement scripting (like Discord, which you noticed), which is fine.