Simplify AppleScript handling #59
Conversation
Fix some incorrect indentation and try to keep lines within 80 columns. This only affects indentation and line breaks in expressions. It doesn’t make any changes that should affect execution at all (not even introducing escaped newlines into strings to shorten the line length). This change is purely cosmetic, but should make some overly-enthusiastic editor configurations (like mine) happier.
This unifies the three separate copies of the AppleScript used to query the system’s mode. Each of the three approaches has some changes - `ns-do-applescript` uses AppleScript’s built-in boolean coercion to integer, since it doesn’t support AppleScript booleans; - `mac-do-applescript` _seems_ to return the command output as a string, so this avoids the extra conversion to an AppleScript string and returns the boolean directly; and - `osascript` now uses `process-lines` instead of `shell-command-to-string` which avoids shell invocation, the need to handle any shell escaping in the arguments, and the need to trim the result (which is now a list of output lines).
|
I tested both the |
This makes a few changes, somewhat independent, but all related: * defaults `auto-dark-allow-osascript` to true[^1]; * falls back to `osascript` on-demand if neither `ns-do-applescript` nor `mac-do-applescript` is available[^2]; and * removes `osascript` as a separate `auto-dark-detection-method`[^3]. These can be easily separated depending on which, if any, you think are improvements. [^1]: I don’t know why this is even a variable. If it’s not allowed, then there is no way to use Auto-Dark on systems that require it, and the failure mode is opaque (“Could not determine a viable theme detection mechanism!”). [^2]: If this change isn’t made, then the error message “Try setting `auto-dark-allow-osascript` to t” doesn’t make sense, as it’s only reported in a location where the value of `auto-dark-allow-osascript` has no effect. The alternative is to change the error message to something like “`auto-dark-detection-method` indicates that this Emacs build has AppleScript support, but none could be found. Either rebuild Emacs with AppleScript support or change the detection method and set `auto-dark-allow-osascript`”. [^3]: With LionyxML#59, the two `fboundp` checks added before falling back to `osascript` should be more than outweighed by the removal of shell invocation.
This makes a few changes, somewhat independent, but all related: * defaults `auto-dark-allow-osascript` to true[^1]; * falls back to `osascript` on-demand if neither `ns-do-applescript` nor `mac-do-applescript` is available[^2]; and * removes `osascript` as a separate `auto-dark-detection-method`[^3]. These can be easily separated depending on which, if any, you think are improvements. [^1]: I don’t know why this is even a variable. If it’s not allowed, then there is no way to use Auto-Dark on systems that require it, and the failure mode is opaque (“Could not determine a viable theme detection mechanism!”). [^2]: If this change isn’t made, then the error message “Try setting `auto-dark-allow-osascript` to t” doesn’t make sense, as it’s only reported in a location where the value of `auto-dark-allow-osascript` has no effect. The alternative is to change the error message to something like “`auto-dark-detection-method` indicates that this Emacs build has AppleScript support, but none could be found. Either rebuild Emacs with AppleScript support or change the detection method and set `auto-dark-allow-osascript`”. [^3]: With LionyxML#59, the two `fboundp` checks added before falling back to `osascript` should be more than outweighed by the removal of shell invocation.
It comes from https://github.com/railwaycat/homebrew-emacsmacport this brew formulae does things a bit differently. |
|
If something like #74 gets in, I’d update this to include system & emacs-dist specific tests. E.g., testing |
|
I just discovered there’s an additional way to check the appearance on Emacs-macport: (pcase (plist-get :appearance (mac-application-state))
("NSAppearanceAqua" 'light)
("NSAppearanceDarkAqua" 'dark)
(appearance
(lwarn 'auto-dark :warning "Unknown system appearance “%s”" appearance)
nil))This avoids AppleScript, so I think it’s both faster than Wondering if you had already seen this and ruled it out for some reason. |
|
this PR would solve another problem: by using the shell for |
Now that #74 has been merged into the development branch, I’ll update this with tests and I can also test the macport version, so this can probably get in shortly. |
The lines are shorter, so we can get it closer to a reasonable value.
This error occurs mostly when Emacs doesn’t have System Event permissions, but it also pops up in other cases sometimes (LionyxML#58). This doesn’t fix the issue, but downgrades it from an error to a warning, and tries to improve the messaging.
sellout
force-pushed
the
unify-applescript
branch
from
8a9024d to
b18c17a
Compare
This unifies the three separate copies of the AppleScript used to query the system’s mode.
Each of the three approaches has some changes
ns-do-applescriptuses AppleScript’s built-in boolean coercion to integer, since it doesn’t support AppleScript booleans;mac-do-applescriptseems to return the command output as a string, so this avoids the extra conversion to an AppleScript string and returns the boolean directly; andosascriptnow usesprocess-linesinstead ofshell-command-to-stringwhich avoids shell invocation, the need to handle any shell escaping in the arguments, and the need to trim the result (which is now a list of output lines).