Better errors when determining detection method #62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rather than just saying that we _couldn’t_ determine the detection method, this indicates why and makes some suggestions. Probably even longer errors with concrete steps to remedy each case would be helpful. This includes an alternative approach to the first bullet point in LionyxML#61 – changing where we suggest that `auto-dark-allow-osascript` should be enabled (although this does not change the error that suggests it in the wrong place).
sellout
commented
Sep 25, 2024
| 'applescript) | ||
| (auto-dark-allow-osascript | ||
| 'osascript) | ||
| (t "You are on a Darwin (Mac OS) system, but Emacs does not have AppleScript enabled and `auto-dark-allow-osascript` is nil. Either rebuild Emacs with AppleScript support or set `auto-dark-allow-osascript` to t"))) |
There was a problem hiding this comment.
Just noticed a bug here – this doesn’t call error, it just returns a string.
sellout
added a commit
to sellout/auto-dark-emacs
that referenced
this pull request
Oct 17, 2024
Previously, failing to “determine a viable theme detection mechanism” would error, preventing the rest of `auto-dark-mode` setup from running. This is now a warning, and you are effectively left in “manual” mode. This is technically a fix for LionyxML#66, but doesn’t address all the related changes there. Those will be addressed in LionyxML#62. It also partially addresses LionyxML#73 by adding `auto-dark-toggle-appearance`.
LionyxML
pushed a commit
that referenced
this pull request
Oct 18, 2024
* Set up build & test infrastructure This has a few layers: 1. Eldev, for Emacs package management; 2. Nix, for coördination (e.g., running multiple test configurations, building against multiple Emacs distributions); and 3. garnix, for CI. Each should work without the ones after it, so it is somewhat modular. E.g., garnix could be replaced with GitHub workflows (but at the cost of more configuration). * Appease the linters There are a couple things in here that should be changed: - don’t use `fboundp` on every invocation - don’t set `fill-column` to 167 * Add a test suite There are three pieces to this - “standard” unit tests (currently fairly minimal) - initialization tests that check how various styles of user init behave - a library for simulating the Emacs init process * Pre-load themes & update state when vars set Pre-loading themes helps shift `custom-safe-theme` interactions to when the user sets the variable instead of during initialization or mode change, but this isn’t a full fix for #64, since it doesn’t address the conundrum of `custom-safe-themes` being set after `auto-dark-mode` is enabled. Updating the state when the variables are set fixes the “it seems to only work after one dark mode toggle” issue (which especially crops up when variables are customized after the mode is enabled). Users of the timer likely don’t notice this, as it only takes five seconds for Auto-Dark to fix the state, but the pub/sub detection methods wouldn’t otherwise update until the next mode change. * Set themes even if detection mechanism fails Previously, failing to “determine a viable theme detection mechanism” would error, preventing the rest of `auto-dark-mode` setup from running. This is now a warning, and you are effectively left in “manual” mode. This is technically a fix for #66, but doesn’t address all the related changes there. Those will be addressed in #62. It also partially addresses #73 by adding `auto-dark-toggle-appearance`. * Defer setting old variables Since the old `auto-dark-dark/light-theme` variables have defaults, a traditional configuration (enabling Auto-Dark before customizing vars) can lead to a `default` → `auto-dark-dark/light-theme` → `auto-dark-theme` “flicker” sequence during Emacs initialization. This avoids that by deferring initialization of the old variables until `after-init-mode`, so they only affect the display if both `auto-dark-themes` and `custom-enabled-themes` are `nil`. The consequence is that users of the old variables may have a slightly longer delay until the initial Auto-Dark theme appears. (And also that Auto-Dark has a bit more defensive code to ensure it doesn’t try to set themes before enough is initialized.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rather than just saying that we couldn’t determine the detection method, this indicates why and makes some suggestions. Probably even longer errors with concrete steps to remedy each case would be helpful.
This includes an alternative approach to the first bullet point in #61 – changing where we suggest that
auto-dark-allow-osascriptshould be enabled (although this does not change the error that suggests it in the wrong place).All of the logic here should be identical, it just splits it into multiple conditionals to avoid duplication and identify different failure modes.