Preserve comment and set synthetic theme

[tzz]

This attempts to resolve #861 and #856 after #850 was merged as follows:

• revert to old customize-set-variable call instead of set-default
• add a synthetic theme property that's enough to prevent the customized variable from getting saved, which was the original goal of #850
• the synthetic theme property is not enough to add the customized variable to the custom-set-faces portion of the customize save file
• this follows advice from @link0ff and @evelineraine and others in the above referenced PRs and bug reports for which I'm very grateful
• this should restore the minor-mode custom set behavior
• tests may need fixing

[evelineraine]

I'm glad my input was helpful!

Since you decided to go with synthetic theme, perhaps the following approach (less ad-hoc than setting theme-value per symbol) would make sense, if you don't find it too intrusive.

1. Declare our synthetic theme somewhere early in use-package.el and immediately enable it (it's empty for now, so no side-effects on any variable):

   (deftheme use-package)
   (enable-theme 'use-package)

2. Replace customize-set-variable and put calls with this one call:

   (let ((custom--inhibit-theme-enable nil))
   (custom-theme-set-variables 'use-package
                 `(,variable ,value nil () ,comment)))

   • This makes value the highest priority value for variable of all enabled themes and immediately sets it.

If done this way, 'use-package is a legit Custom theme and seems to behave like any normal theme loaded with (load-theme 'theme).

Advantages:

• Doesn't overwrite, but adds to theme-value property along with other themes specifying value for the variable
  • User can determine which variable is set from where: use-package, or custom.el or, say, 'leuven-theme
• Potentially more reliable safeguard from having the value written to custom.el
  • The value now is in fact set by a proper theme, and can never be mistaken for user-modified (while a fake value of theme-value might if custom.el code changes)
• Customization can be disabled and previous values restored (set by another theme or standard value
  • (disable-theme 'use-package) to kill all customization from use-package or per-variable using custom-theme-reset-variables
• value expressions are staged and will only be evaluated when corresponding defcustoms run
  • Expressions can read variable's standard value (e.g. append 'id to default value of org-modules)
  • Expressions can use any features from :require key in defcustom, any (require)s the defining surrounding module uses and, if :set-after specified in defcustom, use values of those variables
• variable's value is void up until defcustom is evaluated and specified :initialize procedure is called
  • Variable is never in inconsistent state: it's either void or set by the proper initializer

Disadvantages:

• Can't set defvar variables this way
• Relies on using private variable of custom.el
  • Although, if some major improvements are done to Customize, a better way to do the same might get introduces

If something like this becomes the default :custom behavior, I am probably not going to need my custom init-custom.el package at all. Which would be great.

[tzz]

@evelineraine I think your approach is nice and clean. I've updated the code and will update the tests shortly.

I have a concern that mixing custom variables with themes will be confusing and may surprise users. But it doesn't seem like a big risk, compared to the benefits of functioning code and satisfying user expectations.

[tzz]

@jwiegley : I think this is ready for your review. The custom theme approach seems to integrate best with what Emacs considers "not my business to save" but still behaves like custom variables otherwise.

(Aside: fix-expansion kept aborting for me (inserting an empty list as the second argument of match-expansion) so it was hard to fix the tests. I ended up doing it manually with this (which is what fix-expansion does internally). Would you like a documentation update for future test writers, since currently there's no docs on how that works and it's very convenient?

(insert ?\n ?\` (pp-to-string (macroexpand-1 `(use-package foo :custom (foo bar)))))

Sorry for the long note, but this may have blocked other people from contributing...)

[gusbrs]

Hi @tzz , this is a late reply, but I had been thinking about this approach of defining a custom-theme for use-package, and I just thought of a potential pitfall which is perhaps worth bringing to the attention of this thread.

Regardless of what a "theme" means technically in Emacs, I'd say it is common for users to associate them with color themes, and to think of them as "something I can ditch at will". So it is not uncommon to find advice on the web to do such things as:

(dolist (theme custom-enabled-themes)
  (disable-theme theme))

(That's from https://stackoverflow.com/a/62534045)

And there's not even need to follow a random advice on the web. If one counsel user, for example, chooses to run counsel-load-theme, counsel will do it for them:

(defun counsel-load-theme-action (x)
  "Disable current themes and load theme X."
  (condition-case nil
      (progn
        (mapc #'disable-theme custom-enabled-themes)
        (load-theme (intern x) t)
        (when (fboundp 'powerline-reset)
          (powerline-reset)))
    (error "Problem loading theme %s" x)))

Correct me if I'm wrong but, if I understand it, using a use-package theme such as the one of the PR, if a user happens to run such a "catch all" command to disable themes, the customizations made with use-package will also be dropped alongside any other enabled theme. Is that correct?

We may argue whether this "disable all themes" is good practice but, if I understood it correctly, wouldn't this approach of handling use-package's customizations by means of a theme result in a good deal of noise?

Anyway, again, just a thought I believed was worth bringing to the attention of this thread. If I'm missing something on how the approach is supposed to work, which might well be the case, fell free to dismiss.

[tzz]

Correct me if I'm wrong but, if I understand it, using a use-package theme such as the one of the PR, if a user happens to run such a "catch all" command to disable themes, the customizations made with use-package will also be dropped alongside any other enabled theme. Is that correct?

I think so - this is both an advantage and a disadvantage of the theme approach.

I think disabling all themes is uncommon enough that we could even ignore it, but perhaps it's better to have some prevention in place.

Here are my suggestions, what do you think?

1. advise disable-theme to print a warning
2. advise disable-theme to error out on our special theme
3. advise disable-theme to ask y/n on our special theme
4. advise disable-theme to skip our special theme
5. advise custom-theme-enabled-p to pretend our special theme is not enabled (this may cause other problems)

I think I like (1) non-interactively, and (3) best interactively.

Anyway, again, just a thought I believed was worth bringing to the attention of this thread. If I'm missing something on how the approach is supposed to work, which might well be the case, fell free to dismiss.

Not at all, I really appreciate the suggestion, I didn't know this was a common practice and learned a lot :)

[gusbrs]

Hi @tzz thank you for the careful consideration to this issue.

Since you asked, I'll be glad to share some thoughts. But I think I'm probably way less experienced than required to judge this issue. I'm just an user that was aware of this PR because I was bitten by the issue it is trying to solve and happened to be discussing themes this week and thus connected these particular dots. So please bear that in mind.

I think disabling all themes is uncommon enough that we could even ignore it, but perhaps it's better to have some prevention in place.

Indeed, I don't have any real data on how common this practice is. I've seen it around, that's really all I can say. But it does address an uncovered corner of the theme infrastructure. There is really no way for a "color theme" to declare it is one such kind of theme, or to know if a set of themes is mutually exclusive. To be a little more general, there is no way to handle the relation of different themes one to another. All we know is that the user theme gets precedence over any other themes, and that the precedence of other themes will depend on the order which they were enabled.

Here are my suggestions, what do you think?

1. advise disable-theme to print a warning
2. advise disable-theme to error out on our special theme
3. advise disable-theme to ask y/n on our special theme
4. advise disable-theme to skip our special theme
5. advise custom-theme-enabled-p to pretend our special theme is not enabled (this may cause other problems)

I think I like (1) non-interactively, and (3) best interactively.

It is a thorough list, but I must say I'm not particularly enthusiastic about them. They feel hacky, somewhat ad hoc. Of course, there are trade-offs involved, and it is a matter of choosing where to take the bite, but still.

But assuming this perspective, I'd say there are conceptually two ways to go. Either treat the use-package theme to be more akin to the user theme, and provide that it's handled in similar fashion (not included in custom-enabled-themes, precedence etc). Or consider the use-package theme "not special", in which case a warning or y/n question, or even nothing, would be granted.

But, if we are considering interventions at this level, why not approach the problem from another direction? If I recall correctly, the original issue to solve was the duplication of the variables (once in :custom but also appearing in custom-set-variables as generated by the customization infrastructure). So, for example, why not retain customize-set-variable as the wrapper for :custom but advise whatever generates the custom-set-variables block to ignore a variable if it includes the "Customized with use-package" comment?

Or, another example, give another stab at the attempt at https://github.com/jwiegley/use-package/pull/850, but ensuring that whatever customize-set-variables did, besides registering it in customize, is done so that they are really equivalent in effect (and get minor modes to work).

Depending on the way that is chosen, perhaps it is a case to discuss this more broadly (e.g. emacs-devel). Whatever there is of "cavalier" approach regarding theme disabling is there because there is an uncovered spot in the infrastructure, so that some discussion might bring some change in that area. And the trade-offs involved are "collective". To use the economics jargon, there are externalities involved, and negative ones at that (I'm not claiming they are large, I really don't know that). True, use-package may hold the position that the PR is technically correct, which it is, and leave the adjustment to be dealt with in decentralized fashion for those who employ "dubious" procedures in that area. use-package may choose to absorb at least part of the adjustment in a centralized way, which may work until the next relevant "non-color theme" decides to step in. Either way, the issue is not just one of the inner-workings of use-package but how it relates to other packages and users' practices.

Again, these are just some thoughts of some user who never handled a package of his own. So take them with the due grain of salt.

And I do trust you, John, and other contributors here are way more equipped to judge, and will certainly find a good way around this.

Thanks again!

[tzz]

Indeed, I don't have any real data on how common this practice is. I've seen it around, that's really all I can say. But it does address an uncovered corner of the theme infrastructure. There is really no way for a "color theme" to declare it is one such kind of theme, or to know if a set of themes is mutually exclusive. To be a little more general, there is no way to handle the relation of different themes one to another. All we know is that the user theme gets precedence over any other themes, and that the precedence of other themes will depend on the order which they were enabled.

OK. But that's not a problem we can solve here. Even if we got emacs-devel buy-in and Someone(TM) wrote the necessary code, it would be many years before we could just use that facility. So we have to work within those constraints.

But assuming this perspective, I'd say there are conceptually two ways to go. Either treat the use-package theme to be more akin to the user theme, and provide that it's handled in similar fashion (not included in custom-enabled-themes, precedence etc). Or consider the use-package theme "not special", in which case a warning or y/n question, or even nothing, would be granted.

It's really a second user theme. Is there any code we can advise in order to make Emacs believe that?

In parallel, maybe there's Emacs core code that can support that in the future, but that would be an emacs-devel discussion. I'm happy to support a proposal from you or others along those lines - but again, that won't help us with this PR directly.

But, if we are considering interventions at this level, why not approach the problem from another direction? If I recall correctly, the original issue to solve was the duplication of the variables (once in :custom but also appearing in custom-set-variables as generated by the customization infrastructure). So, for example, why not retain customize-set-variable as the wrapper for :custom but advise whatever generates the custom-set-variables block to ignore a variable if it includes the "Customized with use-package" comment?

Or, another example, give another stab at the attempt at #850, but ensuring that whatever customize-set-variables did, besides registering it in customize, is done so that they are really equivalent in effect (and get minor modes to work).

I think the theme approach is more likely to do what users expect, and results in less complexity in the use-package code, than the alternatives above. I considered that carefully after @evelineraine and others proposed the theme approach.

[gusbrs]

I think the theme approach is more likely to do what users expect, and results in less complexity in the use-package code, than the alternatives above. I considered that carefully after @evelineraine and others proposed the theme approach.

Hi Ted. As I've admitted previously, this issue is beyond my league. I shared my thoughts upon your request, and I hope they've been somehow useful. But I understand, and am ready to agree with the points you've made. My initial comment was just meant to bring the issue to the thread's attention and, at this point, it's been taken into consideration. That's all I could hope for. So, the only thing I can add is: Thank you!

[terlar]

This change broke use-package for people compiling and only requiring use-package at compile time, e.g. as mentioned in the README using:

(eval-when-compile
  (require 'use-package))

I think some extra code needs to be added to make this theme available without requiring use-package at runtime.

With this change you will see things like:

Error (use-package): no-littering/:catch: Unknown theme ‘use-package’

[terlar]

Another thing to note is that quite a lot of people are using code like:

(defun switch-theme (theme)
  "Disable all themes and switch to THEME if it is not enabled."
  (unless (eq theme (car custom-enabled-themes))
      (mapc #'disable-theme custom-enabled-themes)
      (when theme
        (if (custom-theme-p theme)
            (enable-theme theme)
          (load-theme theme :no-confirm))

E.g. both consult and counsel do this:

• https://github.com/minad/consult/blob/main/consult.el#L1933-L1961
• https://github.com/abo-abo/swiper/blob/master/counsel.el#L1056-L1076

I don't think this is good and probably these packages should be fixed, I am running multiple themes already. But I think it will be quite unexpected for people to see this kind unexpected of side-effects for their usage of use-package. If anyone use one of these two functions suddenly all their :custom config will be undone.

Personally I would never want that to happen, and I am considering to stop using :custom keyword if this will be the case. Could it perhaps be possible to have a flag to use the old way of setting customizations?

Personally I don't even track/persist the custom-file so many of the mentioned issues are moot for me. I am using:

(setq custom-file (expand-file-name "custom.el" temporary-file-directory))

Edit: I read through the discussion and see that you are aware of the practice to disable themes. I have also done the test and can confirm that running the counsel or consult function drops the use-package theme and undones all customizations.

[terlar]

Not sure if this will show up in your notifications since it is merged (at least for me it doesn't show up among my current ongoing issues). So a gentle ping @jwiegley and @tzz just to make sure you are aware. Let me know if I should create a new issue instead.

[lbolla]

This bit me today. I had a similar function as described in https://github.com/jwiegley/use-package/pull/881#issuecomment-756104975 I had to change it to ignore "use-package" theme:

(defun my/switch-theme (theme)
  "Disable active themes and load THEME."
  (interactive (list (intern (completing-read "Theme: "
                                              (->> (custom-available-themes)
                                                   (-map #'symbol-name))))))
  (-map-when (lambda (x) (not (eq x 'use-package))) #'disable-theme custom-enabled-themes)
  (load-theme theme 'no-confirm))

This is very surprising behavior, IMO.

[tzz]

Continued in #899

Please open issues and reference this PR if there are new issues caused by it.