Hi,
I was just reviewing the list of user options you plan to deprecate, and
was disappointed to see the deprecation of 'modus-themes-inhibit-reload'
in ab3f31a91f3674fa70018c3cd84f070fccccc85c. Having contributed the
code a while back, I might be biased, but I still consider it very
useful and think it would be a pity to have the user options removed.
Forgive me for saying this, but I believe the feature has been
under-appreciated and it was an unfortunate mistake to have it disabled
by default (which I believe was due to a mistaken judgement by Gustavo
Barros).
If possible, I'd like to convince you to keep the option in the package,
but for that to work I'd have to know why you decided to not only
deprecate, but also remove it (which is a thing I don't quite
understand, usually you deprecate a option/function and remove it at
some later point to give people a chance to adapt their configuration
without it breaking). If you are willing to discuss this, or perhaps
even improve on what we had, then I'd be more than willing to help!
> From: Philip Kaludercic <philipk@posteo.net>> Date: Sat, 10 Dec 2022 13:39:38 +0000>> Hi,
Hello Philip,
> I was just reviewing the list of user options you plan to deprecate, and> was disappointed to see the deprecation of 'modus-themes-inhibit-reload'> in ab3f31a91f3674fa70018c3cd84f070fccccc85c. Having contributed the> code a while back, I might be biased, but I still consider it very> useful and think it would be a pity to have the user options removed.
Sorry about that. Note that the files are not final. I still have
incomplete changes that I have not pushed and others I need to review.
For example, 'modus-themes-org-blocks' is partially broken right now and
I want to see what to do with it. 'modus-themes-completions' is
untested and not perfect. Also, the user option about the org-agenda
may be worth keeping, but otherwise simplifying.
> Forgive me for saying this, but I believe the feature has been> under-appreciated and it was an unfortunate mistake to have it disabled> by default (which I believe was due to a mistaken judgement by Gustavo> Barros).
I no longer remember the discussion, except that there was some minor
tension. Back then I was not using customize myself, though now I am
gradually introducing 'setopt' everywhere. I like how seamless it is
and how it mirrors the 'setq' format.
I admit that I underappreciated and underutilised this feature. I do
see its added value though. Enabling it by default is worth it.
> If possible, I'd like to convince you to keep the option in the package,> but for that to work I'd have to know why you decided to not only> deprecate, but also remove it (which is a thing I don't quite> understand, usually you deprecate a option/function and remove it at> some later point to give people a chance to adapt their configuration> without it breaking). If you are willing to discuss this, or perhaps> even improve on what we had, then I'd be more than willing to help!
There is no need to convince me, as I am fine with keeping it. I simply
performed multiple drastic changes all at once, but still need to
double-check everything. Though yes, if nobody would ask about this I
could miss it.
As for improving it, I remember there were some initial problems with it
(recursion?) but they were fixed long ago. So I don't know if it needs
anything else. I will thus bring it back, subject to a review.
[ NOTE: The same is true for packages. I removed lots of them because
they were using deprecated faces, but I want to gradually bring some
back. I did it with a few already, though there probably are more. ]
All the best,
Prot
--
Protesilaos Stavrou
https://protesilaos.com
> From: Protesilaos Stavrou <info@protesilaos.com>> Date: Sat, 10 Dec 2022 15:58:24 +0200> [... 31 lines elided]> I admit that I underappreciated and underutilised this feature. I do> see its added value though. Enabling it by default is worth it.
Since I am on the "mea culpa" flow, a few days ago I realised that
'custom-safe-themes' can be set to non-nil, which removes the problem I
had with 'theme-choose-variant'. The command now works fine. Sorry for
not noticing this before! (The only addition I would like to see to the
core command is the functionality of "specify two themes to
toggle/choose", so that the user can switch between, say, modus-operandi
and ef-dark.)
Anyhow, my point is that we can revisit the point of deprecating
modus-themes-toggle.
--
Protesilaos Stavrou
https://protesilaos.com
Protesilaos Stavrou <info@protesilaos.com> writes:
>> From: Protesilaos Stavrou <info@protesilaos.com>>> Date: Sat, 10 Dec 2022 15:58:24 +0200>>> [... 47 lines elided]>>> As for improving it, I remember there were some initial problems with it>> (recursion?) but they were fixed long ago. So I don't know if it needs>> anything else. I will thus bring it back, subject to a review.>> Hello again Philip,
Hi, sorry for the late response,
> I have made the preparations to reinstate this feature. I think this is> the right time to (i) rename it, and (ii) enable it by default. I> tweaked it a bit to work with the two extra themes that we have (those> complete the set).
Sounds great, thank you for reconsidering.
> Diff below. Since you were the original author, can I add you as the> author of this change?>>> doc/modus-themes.org | 35 ++++++++++++++++++++++++++> modus-themes.el | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++-> 2 files changed, 105 insertions(+), 1 deletion(-)>> diff --git a/doc/modus-themes.org b/doc/modus-themes.org> index d4464c9..5a7d8af 100644> --- a/doc/modus-themes.org> +++ b/doc/modus-themes.org> @@ -472,6 +472,7 @@ * Customization Options> modus-themes-subtle-line-numbers nil> modus-themes-deuteranopia t> modus-themes-variable-pitch-ui nil> + modus-themes-custom-auto-reload t> > modus-themes-fringes nil ; {nil,'subtle,'intense}> > @@ -529,6 +530,40 @@ * Customization Options> (t . (1.1))))> #+end_src> > +** Option for inhibiting theme reload> +:properties:> +:alt_title: Custom reload theme> +:description: Toggle auto-reload of the theme when setting custom variables> +:custom_id: h:9001527a-4e2c-43e0-98e8-3ef72d770639> +:end:> +#+vindex: modus-themes-custom-auto-reload> +> +[ Revised as part of {{{development-version}}}. It used to be named> + ~modus-themes-inhibit-reload~. ]> +> +Brief: Toggle reloading of the active theme when an option is changed> +through the Custom UI.> +> +Symbol: ~modus-themes-custom-auto-reload~ (=boolean= type)> +> +Possible values:> +> +1. ~nil~> +2. ~t~ (default)> +> +All theme user options take effect when a theme is loaded. Any> +subsequent changes require the theme to be reloaded.> +> +When this variable has a non-nil value, any change made via the Custom> +UI or related functions such as ~customize-set-variable~ and ~setopt~> +(Emacs 29), will trigger a reload automatically.> +> +With a nil value, changes to user options have no further> +consequences. The user must manually reload the theme.> +> +Regardless of this option, the active theme must be reloaded for changes> +to user options to take effect ([[#h:3f3c3728-1b34-437d-9d0c-b110f5b161a9][Enable and load]]).
Maybe this should be clarified, because I first read it as negating the
entire point of the user option. I believe you are trying to clarify
that when the option is enabled, the theme is
"automatically"/"implicitly" reloaded, right?
> ** Option for red-green color deficiency or deuteranopia> :properties:> :alt_title: Deuteranopia style> diff --git a/modus-themes.el b/modus-themes.el> index 3e4b4df..720b56a 100644> --- a/modus-themes.el> +++ b/modus-themes.el> @@ -291,6 +291,44 @@ (make-obsolete-variable 'modus-themes-box-button-pressed nil "4.0.0")> > ;;;; Customization variables> > +(define-obsolete-variable-alias> + 'modus-themes-inhibit-reload> + 'modus-themes-custom-auto-reload> + "4.0.0")
Isn't adding this alias "dangerous", because now if someone sets
`modus-themes-inhibit-reload' to t (I know of people who follow your
example and set variables to their default values to protect against
future changes), they would unintentionally be setting
`modus-themes-custom-auto-reload' to t which would have the opposite of
the intended effect? Shouldn't we just mark
`modus-themes-inhibit-reload' as deprecated and mention
`modus-themes-custom-auto-reload' in the reason?
(define-obsolete-variable-alias uses defvaralias, and the docstrings
says "Aliased variables always have the same value; setting one sets the
other.")
> +(defcustom modus-themes-custom-auto-reload t> + "Automatically reload theme after setting options with Customize.> +> +All theme user options take effect when a theme is loaded. Any> +subsequent changes require the theme to be reloaded.> +> +When this variable has a non-nil value, any change made via the> +Custom UI or related functions such as `customize-set-variable'> +and `setopt' (Emacs 29), will trigger a reload automatically.> +> +With a nil value, changes to user options have no further> +consequences. The user must manually reload the theme."> + :group 'modus-themes> + :package-version '(modus-themes . "4.0.0")> + :version "30.1"> + :type 'boolean> + :link '(info-link "(modus-themes) Custom reload theme"))> +> +(defun modus-themes--set-option (sym val)> + "Custom setter for theme related user options.> +Will set SYM to VAL, and reload the current theme, unless> +`modus-themes-custom-auto-reload' is non-nil."> + (set-default sym val)> + (when (or modus-themes-custom-auto-reload> + ;; Check if a theme is being loaded, in which case we> + ;; don't want to reload a theme if the setter is> + ;; invoked. `custom--inhibit-theme-enable' is set to nil> + ;; by `enable-theme'.> + (bound-and-true-p custom--inhibit-theme-enable))> + (when-let* ((modus-themes-custom-auto-reload t)> + (theme (modus-themes--current-theme)))> + (modus-themes-load-theme theme))))> +> (defconst modus-themes-items> '( modus-operandi modus-vivendi> modus-operandi-tinted modus-vivendi-tinted)> @@ -321,6 +359,8 @@ (defcustom modus-themes-to-toggle '(modus-operandi modus-vivendi)> modus-themes-items))))> :package-version '(modus-themes . "4.0.0")> :version "30.1"> + :set #'modus-themes--set-option> + :initialize #'custom-initialize-default> :group 'modus-themes)
BTW, we could simplify this by making use of the fact that all user
options of a group are stored under the `custom-group' symbol property
for the symbol designating the group name:
(get 'modus-themes 'custom-group)
=> ((modus-themes-faces custom-group)
(modus-themes-inhibit-reload custom-variable)
(modus-themes-operandi-color-overrides custom-variable)
(modus-themes-vivendi-color-overrides custom-variable)
;; ...
)
At the end of the file, we could then check all options in a group,
modify the respective properties (:set puts `custom-set' and :initialize
is directly invoked, see `custom-declare-variable'). Optionally this
could be wrapped in a function and invoked. If you wish to avoid
modifying all user options without involving some heuristics, the
respective options would be placed in a "modus-themes-appearance"-like
group, which doesn't even sound that bad unrelated to this change.
Protesilaos Stavrou <info@protesilaos.com> writes:
>> From: Protesilaos Stavrou <info@protesilaos.com>>> Date: Sat, 10 Dec 2022 15:58:24 +0200>>> [... 31 lines elided]>>> I admit that I underappreciated and underutilised this feature. I do>> see its added value though. Enabling it by default is worth it.>> Since I am on the "mea culpa" flow, a few days ago I realised that> 'custom-safe-themes' can be set to non-nil, which removes the problem I> had with 'theme-choose-variant'.
The default value of `custom-safe-themes' is not arbitrary, custom
themes carry the burden of not just being colour schemes in Lisp, which
is something a lot of users aren't conscious of. Promoting the usage of
"disabling" `custom-safe-themes' is something I would be careful of
doing.
> The command now works fine. Sorry for> not noticing this before! (The only addition I would like to see to the> core command is the functionality of "specify two themes to> toggle/choose", so that the user can switch between, say, modus-operandi> and ef-dark.)
Right, that was my intention (I have bound the key to C-c t). I still
get the warnings that a theme might be unsafe when playing with
ef-themes from time to time, but I consider this a fair point.
What could be done, but I don't know if it should be done, is to have
all ef-themes mark each other as safe as soon as one of them has been
enabled (since the main logic is shared anyway and each "theme" is just
an instanciation of a macro).
> Anyhow, my point is that we can revisit the point of deprecating> modus-themes-toggle.
> From: Philip Kaludercic <philipk@posteo.net>> Date: Sun, 11 Dec 2022 22:02:38 +0000>> Protesilaos Stavrou <info@protesilaos.com> writes:>>>> From: Protesilaos Stavrou <info@protesilaos.com>>>> Date: Sat, 10 Dec 2022 15:58:24 +0200>>>>> [... 47 lines elided]>>>>> As for improving it, I remember there were some initial problems with it>>> (recursion?) but they were fixed long ago. So I don't know if it needs>>> anything else. I will thus bring it back, subject to a review.>>>> Hello again Philip,>> Hi, sorry for the late response,
No worries!
> [... 62 lines elided]>> +Regardless of this option, the active theme must be reloaded for changes>> +to user options to take effect ([[#h:3f3c3728-1b34-437d-9d0c-b110f5b161a9][Enable and load]]).>> Maybe this should be clarified, because I first read it as negating> the entire point of the user option. I believe you are trying to> clarify that when the option is enabled, the theme is> "automatically"/"implicitly" reloaded, right?
Indeed. This is a leftover from before. I will rephrase it.
>> ** Option for red-green color deficiency or deuteranopia>> :properties:>> :alt_title: Deuteranopia style>> diff --git a/modus-themes.el b/modus-themes.el>> index 3e4b4df..720b56a 100644>> --- a/modus-themes.el>> +++ b/modus-themes.el>> @@ -291,6 +291,44 @@ (make-obsolete-variable 'modus-themes-box-button-pressed nil "4.0.0")>> >> ;;;; Customization variables>> >> +(define-obsolete-variable-alias>> + 'modus-themes-inhibit-reload>> + 'modus-themes-custom-auto-reload>> + "4.0.0")>> Isn't adding this alias "dangerous", because now if someone sets> `modus-themes-inhibit-reload' to t (I know of people who follow your> example and set variables to their default values to protect against> future changes), they would unintentionally be setting> `modus-themes-custom-auto-reload' to t which would have the opposite of> the intended effect? Shouldn't we just mark> `modus-themes-inhibit-reload' as deprecated and mention> `modus-themes-custom-auto-reload' in the reason?>> (define-obsolete-variable-alias uses defvaralias, and the docstrings> says "Aliased variables always have the same value; setting one sets the> other.")
Oh right! We do as you say.
> [... 44 lines elided]> BTW, we could simplify this by making use of the fact that all user> options of a group are stored under the `custom-group' symbol property> for the symbol designating the group name:>> (get 'modus-themes 'custom-group)> => ((modus-themes-faces custom-group)> (modus-themes-inhibit-reload custom-variable)> (modus-themes-operandi-color-overrides custom-variable)> (modus-themes-vivendi-color-overrides custom-variable)> ;; ...> )>> At the end of the file, we could then check all options in a group,> modify the respective properties (:set puts `custom-set' and :initialize> is directly invoked, see `custom-declare-variable'). Optionally this> could be wrapped in a function and invoked. If you wish to avoid> modifying all user options without involving some heuristics, the> respective options would be placed in a "modus-themes-appearance"-like> group, which doesn't even sound that bad unrelated to this change.
I am fine with this. I will now add the change, simply bringing back
the feature with the tweaks we covered, and then we can simplify it
further.
--
Protesilaos Stavrou
https://protesilaos.com
> From: Protesilaos Stavrou <info@protesilaos.com>> Date: Mon, 12 Dec 2022 05:20:25 +0200> [... 78 lines elided]>> At the end of the file, we could then check all options in a group,>> modify the respective properties (:set puts `custom-set' and :initialize>> is directly invoked, see `custom-declare-variable'). Optionally this>> could be wrapped in a function and invoked. If you wish to avoid>> modifying all user options without involving some heuristics, the>> respective options would be placed in a "modus-themes-appearance"-like>> group, which doesn't even sound that bad unrelated to this change.>> I am fine with this. I will now add the change, simply bringing back> the feature with the tweaks we covered, and then we can simplify it> further.
I pushed the change earlier and enabled the feature by default. Though
I now encountered a problem with 'setopt'. Before, I was using it with
modus-operandi and only with one variable at a time. But trying the
following with modus-vivendi has a noticeable delay, as it flashes in a
white background:
(setopt modus-themes-mixed-fonts t
modus-themes-variable-pitch-ui t
modus-themes-italic-constructs t
modus-themes-bold-constructs t
modus-themes-prompts '(background bold italic)
modus-themes-mode-line '(3d accented)
modus-themes-fringes 'intense
modus-themes-deuteranopia t
modus-themes-headings nil)
(setopt modus-themes-mixed-fonts nil
modus-themes-variable-pitch-ui nil
modus-themes-italic-constructs nil
modus-themes-bold-constructs nil
modus-themes-prompts nil
modus-themes-mode-line nil
modus-themes-fringes nil
modus-themes-deuteranopia nil
modus-themes-headings nil)
This shows that the theme is being reloaded for each variable. Whereas
we would only want to do it once. I probably did something wrong with
'modus-themes--set-option', but I am looking into it.
--
Protesilaos Stavrou
https://protesilaos.com
Protesilaos Stavrou <info@protesilaos.com> writes:
>> From: Protesilaos Stavrou <info@protesilaos.com>>> Date: Mon, 12 Dec 2022 05:20:25 +0200>>> [... 78 lines elided]>>>> At the end of the file, we could then check all options in a group,>>> modify the respective properties (:set puts `custom-set' and>>> :initialize>>> is directly invoked, see `custom-declare-variable'). Optionally this>>> could be wrapped in a function and invoked. If you wish to avoid>>> modifying all user options without involving some heuristics, the>>> respective options would be placed in a "modus-themes-appearance"-like>>> group, which doesn't even sound that bad unrelated to this change.>>>> I am fine with this. I will now add the change, simply bringing back>> the feature with the tweaks we covered, and then we can simplify it>> further.>> I pushed the change earlier and enabled the feature by default. Though> I now encountered a problem with 'setopt'. Before, I was using it with> modus-operandi and only with one variable at a time. But trying the> following with modus-vivendi has a noticeable delay, as it flashes in a> white background:>> (setopt modus-themes-mixed-fonts t> modus-themes-variable-pitch-ui t> modus-themes-italic-constructs t> modus-themes-bold-constructs t> modus-themes-prompts '(background bold italic)> modus-themes-mode-line '(3d accented)> modus-themes-fringes 'intense> modus-themes-deuteranopia t> modus-themes-headings nil)>> (setopt modus-themes-mixed-fonts nil> modus-themes-variable-pitch-ui nil> modus-themes-italic-constructs nil> modus-themes-bold-constructs nil> modus-themes-prompts nil> modus-themes-mode-line nil> modus-themes-fringes nil> modus-themes-deuteranopia nil> modus-themes-headings nil)>> This shows that the theme is being reloaded for each variable. Whereas> we would only want to do it once. I probably did something wrong with> 'modus-themes--set-option', but I am looking into it.
I don't think that there is anything wrong wit
`modus-themes--set-option', that is the expected behaviour of a custom
setter. The way I see it, we have a few options:
1. Accept this overhead and advise setting user options programmatically
before loading the theme.
2. Adding some ugly hacks to detect how the custom setter is being
invoked, and anticipate if the theme should be reloaded or not.
3. Change the behaviour of the hook, e.g. to defer reloading to the end
of an init file and avoid reloading the theme if it has already been
loaded.
4. Be more clever about reloading and only change what an options
affects. For this to be done properly, it would require some
annotations. I can take a look at how the code looks like right now,
and perhaps propose a macro that would make this easier.
I think that 4. would be the best strategy, if the effort is reasonable.
But perhaps all of this shouldn't be theme-specific anyway, and it would
be better to move the work into the core again, specifying some generic
way to associate user options with a theme.
> From: Philip Kaludercic <philipk@posteo.net>> Date: Sun, 11 Dec 2022 22:07:03 +0000> [... 21 lines elided]>> The command now works fine. Sorry for>> not noticing this before! (The only addition I would like to see to the>> core command is the functionality of "specify two themes to>> toggle/choose", so that the user can switch between, say, modus-operandi>> and ef-dark.)>> Right, that was my intention (I have bound the key to C-c t). I still> get the warnings that a theme might be unsafe when playing with> ef-themes from time to time, but I consider this a fair point.>> What could be done, but I don't know if it should be done, is to have> all ef-themes mark each other as safe as soon as one of them has been> enabled (since the main logic is shared anyway and each "theme" is just> an instanciation of a macro).
The documentation of 'custom-face-themes' states that:
This variable cannot be set in a Custom theme.
Maybe we can work around it, though a clean solution is preferable for
the long-term.
--
Protesilaos Stavrou
https://protesilaos.com
Protesilaos Stavrou <info@protesilaos.com> writes:
>> From: Philip Kaludercic <philipk@posteo.net>>> Date: Sun, 11 Dec 2022 22:07:03 +0000>>> [... 21 lines elided]>>>> The command now works fine. Sorry for>>> not noticing this before! (The only addition I would like to see to the>>> core command is the functionality of "specify two themes to>>> toggle/choose", so that the user can switch between, say, modus-operandi>>> and ef-dark.)>>>> Right, that was my intention (I have bound the key to C-c t). I still>> get the warnings that a theme might be unsafe when playing with>> ef-themes from time to time, but I consider this a fair point.>>>> What could be done, but I don't know if it should be done, is to have>> all ef-themes mark each other as safe as soon as one of them has been>> enabled (since the main logic is shared anyway and each "theme" is just>> an instanciation of a macro).>> The documentation of 'custom-face-themes' states that:
I am guessing you mean `custom-safe-themes'?
> This variable cannot be set in a Custom theme.
Does this include a direct manipulation of the user option written in
Lisp?
> Maybe we can work around it, though a clean solution is preferable for> the long-term.
Perhaps `custom-theme-load-confirm' could check if any theme from the
family has been marked as safe?
> From: Philip Kaludercic <philipk@posteo.net>> Date: Mon, 12 Dec 2022 20:15:33 +0000> [... 47 lines elided]>> This shows that the theme is being reloaded for each variable. Whereas>> we would only want to do it once. I probably did something wrong with>> 'modus-themes--set-option', but I am looking into it.>> I don't think that there is anything wrong wit> `modus-themes--set-option', that is the expected behaviour of a custom> setter. The way I see it, we have a few options:>> 1. Accept this overhead and advise setting user options programmatically> before loading the theme.>> 2. Adding some ugly hacks to detect how the custom setter is being> invoked, and anticipate if the theme should be reloaded or not.>> 3. Change the behaviour of the hook, e.g. to defer reloading to the end> of an init file and avoid reloading the theme if it has already been> loaded.>> 4. Be more clever about reloading and only change what an options> affects. For this to be done properly, it would require some> annotations. I can take a look at how the code looks like right now,> and perhaps propose a macro that would make this easier.>> I think that 4. would be the best strategy, if the effort is reasonable.>> But perhaps all of this shouldn't be theme-specific anyway, and it would> be better to move the work into the core again, specifying some generic> way to associate user options with a theme.
Yes, this seems like it belongs in emacs.git. If we can have something
in the modus-themes, it will make it possible to provide the feature
for earlier versions of Emacs.
--
Protesilaos Stavrou
https://protesilaos.com
> From: Philip Kaludercic <philipk@posteo.net>> Date: Tue, 13 Dec 2022 15:40:47 +0000> [... 23 lines elided]>> The documentation of 'custom-face-themes' states that:>> I am guessing you mean `custom-safe-themes'?
Yes, sorry. A funny mistake of mine!
>> This variable cannot be set in a Custom theme.>> Does this include a direct manipulation of the user option written in> Lisp?
I did not check the code and did not put this to the test. Sorry!
>> Maybe we can work around it, though a clean solution is preferable for>> the long-term.>> Perhaps `custom-theme-load-confirm' could check if any theme from the> family has been marked as safe?
This makes sense. Many theme packages contain multiple items.
--
Protesilaos Stavrou
https://protesilaos.com