Documentation should be updated to match new zenburn-override-colors-alist customizable functionality

The change in commit 4b3e541 changed the zenburn-override-colors-alist to a customizable variable. This means that the way to override colours as described in the documentation does no longer work:

(defvar zenburn-override-colors-alist
  '(("zenburn-bg+05" . "#282828")
    ("zenburn-bg+1"  . "#2F2F2F")
    ("zenburn-bg+2"  . "#3F3F3F")
    ("zenburn-bg+3"  . "#4F4F4F")))

The same functionality with new implementation could probably be accomplished by using customize-set-variable instead:

(customize-set-variable 'zenburn-override-colors-alist
  '(("zenburn-bg+05" . "#282828")
    ("zenburn-bg+1"  . "#2F2F2F")
    ("zenburn-bg+2"  . "#3F3F3F")
    ("zenburn-bg+3"  . "#4F4F4F")))

Another alternative could be to change the new customize functionality into an additional feature with a slightly different name while still preserving the old defvar configuration option. That way the all the existing users' configurations would still continue to work.

It is wrong to suggest that users call defvar before loading zenburn-theme to begin with; that is the true "bug" here, and was a bug even before 4b3e541721f52dbfa307e2cab3cd682e25987fdd. defvar and defcustom are for package authors to declare variables and assign them a default value if they happen to be void. Users have the customize interface, customize-set-variable, setq, setq-default, etc. through which to set, not declare their preferred settings.

Thank you @juristi for showing how to fix it!

I originally added the defvar customization option and documentation in b057fa5, but I didn't (and still don't!) know enough about elisp to know that it should have been using the defcustom mechanism instead. I was quite confused when my local colours changed back to the defaults recently!

I've proposed https://github.com/bbatsov/zenburn-emacs/pull/306 to at least get the right documentation in the README.

Thanks for the review on the PR, @basil-conto!

I think it would be ideal in the longer run to address this issue by detecting if the user has already set the variable with defvar, and if so, warning them to switch to setq. It may have been a bug to use defvar in the first place, but it was recommended in the docs to people who are now using it and whose configs broke as a result of the change.

Any ideas on how one might make the switch more graceful?

@felixc Thanks for working on this.

I think it would be ideal in the longer run to address this issue by detecting if the user has already set the variable with defvar, and if so, warning them to switch to setq.

FWIW, I think this is outside of the scope of zenburn-emacs specifically, and most Emacs packages generally. This kind of reflection/introspection should be left up to packages which care about the distinction between bound/unbound/set/customised/etc. symbols, such as the built-in custom.el, which provides the macro defcustom.

There are many popular Emacs packages which give sample setup instructions which do not universally work for all users and configurations. That is not the end of the world, because they are just sample instructions. There is a plethora of documentation and Q&A available in the wild which instructs both power- and non-power-users on different ways to specify values and customisations in their initialisation. The onus ultimately lies on them to figure out what they need to put in their initialisation files for their setup to work for them, or at least to ask for help with this.

it was recommended in the docs to people who are now using it and whose configs broke as a result of the change

Here is why I think the effect of the change is far less severe then this issue makes it out to be:

The current defvar-before-load-theme in the README is effectively the same as calling setq-before-load-theme as proposed in #306. The user must be doing something else incorrectly for the defvar to break their configuration.
If the user is doing something else wrong, it is up to them to figure out why and how to fix it, either on their own or by asking for help.
There have been no other reports about this issue.

If there is some other justification for this fear of breaking configs which I am missing, the standard route is to announce the change somewhere and possibly accompany it with a version bump.

Don't forget that zenburn-emacs is a custom theme, not a package, and, as such, should strive to remain as programmatically simple as reasonably possible.

FWIW, I think this is outside of the scope of zenburn-emacs specifically, and most Emacs packages generally. This kind of reflection/introspection should be left up to packages which care about the distinction between bound/unbound/set/customised/etc. symbols, such as the built-in custom.el, which provides the macro defcustom.

I'm not suggesting a general feature for all time, rather a band-aid for a break in the public interface. I don't think this is a general problem that other packages are likely to have, so the fix belongs here (if it is to exist at all).

There are many popular Emacs packages which give sample setup instructions which do not universally work for all users and configurations.

This seems to affect the most basic and standard configuration; it's not about edge cases.

That is not the end of the world, because they are just sample instructions. There is a plethora of documentation and Q&A available in the wild which instructs both power- and non-power-users on different ways to specify values and customisations in their initialisation. The onus ultimately lies on them to figure out what they need to put in their initialisation files for their setup to work for them, or at least to ask for help with this.

I disagree; I think it's a bad thing if the suggested way of using things doesn't work; and if the way that did work in the past is changed out unannounced.

The current defvar-before-load-theme in the README is effectively the same as calling setq-before-load-theme. The user must be doing something else incorrectly for the defvar to break their configuration.

My configuration for this package consisted of literally just the defvar and then loading it, so I believe it's more common than you think, and doesn't require any other misconfiguration.

I f the user is doing something else wrong, it is up to them to figure out why and how to fix it, either on their own or by asking for help.

Making it easier to understand where the problem is is not contrary to this. My suggestion is about getting users to the point where they know what/why changed, so they can either figure out the fix on their own, check available documentation, or ask a good targeted question. I think that's a more valuable use of everyone's time than each user starting from scratch and either spending time on understanding what happened, or having to ask someone else and have people here answer the same question repeatedly.

There have been no other reports about this issue.

I'm not sure how we can count the number of people who came to the repo, saw this issue, fixed it locally, and quietly moved on — or the number of users who just couldn't be bothered to figure out why things broke and just left things as they were.

If there is some other justification for this fear of breaking configs which I am missing, the standard route is to announce the change somewhere and possibly accompany it with a version bump.

Agreed; but in this case the change that broke configs already happened, and I can't see it in any changelog or similar place. That's why this bug was opened: because existing working configurations silently broke.

Don't forget that zenburn-emacs is a custom theme, not a package, and, as such, should strive to remain as programmatically simple as reasonably possible.

I'd argue we should also always strive to be helpful to users and guide them through breaking changes.

I'm not suggesting a general feature for all time, rather a band-aid for a break in the public interface. I don't think this is a general problem that other packages are likely to have, so the fix belongs here (if it is to exist at all).

I have yet to be shown a recipe for how changing zenburn-override-colors-alist from a defvar to a defcustom "broke" the README's suggestion of using defvar before loading the theme. In other words, I disagree with the claim that the sample defvar in the README broke user configurations, because a defvar in that context is effectively the same as calling setq, bar docstring and load-history attachment. Note that the OP does not, and I suspect cannot, provide a reproducible recipe for how the defvar->defcustom transition broke their configuration. When I said that suggesting defvar in the README is a bug in itself, I meant a documentation/idiom shortcoming, not a "this will programmatically break user configs" bug.

This seems to affect the most basic and standard configuration; it's not about edge cases.

I made no mention of edge cases. I am specifically talking about basic, standard, common configurations and unadulterated Elisp.

I think it's a bad thing if the suggested way of using things doesn't work; and if the way that did work in the past is changed out unannounced.

The way that worked in the past still works, as does the suggested way; the latter just happens to be sub-optimal in how idiomatic it is, which was my original point.

My configuration for this package consisted of literally just the defvar and then loading it, so I believe it's more common than you think, and doesn't require any other misconfiguration.

Can you please provide a reproducible recipe, starting from emacs -Q, which shows how defvar-before-load-theme broke your configuration?

Making it easier to understand where the problem is is not contrary to this. My suggestion is about getting users to the point where they know what/why changed, so they can either figure out the fix on their own, check available documentation, or ask a good targeted question. I think that's a more valuable use of everyone's time than each user starting from scratch and either spending time on understanding what happened, or having to ask someone else and have people here answer the same question repeatedly.

I agree that the smarter the system, the better for the user. But introspection by a custom theme to detect a non-issue is completely over-engineering this. Detecting common user-init-file pitfalls such as "calling defvar on an already defined user option doesn't change its value" is an Emacs-wide issue, not one which zenburn-emacs should take on itself to solve. If you have suggestions for such extensions to Emacs, please initiate a discussion on the emacs-devel@gnu.org mailing list or report them as a bug / feature request via M-xreport-emacs-bugRET, so that all packages may one day avail of them in a well-designed, complete, consistent, and standard manner.

I'm not sure how we can count the number of people who came to the repo, saw this issue, fixed it locally, and quietly moved on — or the number of users who just couldn't be bothered to figure out why things broke and just left things as they were.

Again, I have yet to be shown any evidence of how and why users' configurations broke by this defvar->defcustom transition.

Agreed; but in this case the change that broke configs already happened, and I can't see it in any changelog or similar place. That's why this bug was opened: because existing working configurations silently broke.

Again, no-one has actually described how working configurations silently broke.

I'd argue we should also always strive to be helpful to users and guide them through breaking changes.

Sure, which is why #306 is a step in the right direction. It is outside of the scope of a third-party custom theme (a potentially malicious collection of toggleable variable and face values downloaded from the Internet), though, to address any and all common pitfalls that novice Elispers may subject themselves to in their configuration files. Defences against such pitfalls should be added to Emacs proper and its documentation / tutorials / communication media.

Just my $0.02.

Here's a recipe showing the currently documented defvar approach in action:

Start emacs -Q from the top level of the zenburn-emacs repository (i.e. default-directory is "/path/to/zenburn-emacs/").

(let ((custom-theme-directory default-directory))
 (defvar zenburn-override-colors-alist '(("zenburn-bg" . "#000000")))
 (load-theme 'zenburn t))

C-j
Voilà - your background is now black.

M-xemacs-versionRET reports:

GNU Emacs 27.0.50
(build 2, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
of 2018-04-26

Also reproducible on Emacs 24.5.1, 25.2.1, and Emacs 26.1 RC1.

Recipe to reproduce the problem. Use this simple init.el file:

(package-initialize)
(require 'package)
(add-to-list 'package-archives
             '("melpa" . "https://melpa.org/packages/") t)
(package-refresh-contents)
(unless (package-installed-p 'zenburn-theme)
  (package-install 'zenburn-theme))

(defvar zenburn-override-colors-alist
  '(("zenburn-bg" . "#000000")))
(load-theme 'zenburn t)

The background color is not changed.

After digging a bit further into the problem, the missing piece of information was actually that the defvar must come before the (package-initialize) call. I tried the Zenburn theme only after it had switched to customize, so the problem may have existed earlier and the instructions lacking already longer time.

Anyway it's good that the documentation gets an update regarding this. At the time it took a while to figure out why the colours customized in the init file did not work even though I had followed the instructions exactly as given.

After digging a bit further into the problem, the missing piece of information was actually that the defvar must come before the (package-initialize) call.

Right.

I tried the Zenburn theme only after it had switched to customize, so the problem may have existed earlier and the instructions lacking already longer time.

The problem you describe (re: the ordering of package-initialize) is completely unrelated to Customize/defcustom/defvar/setq; it is caused by the addition of the ;;;###autoload cookie to zenburn-override-colors-alist. Even if zenburn-override-colors-alist remained a defvar, your initialisation code would not have worked, because package-initialize goes through your package-user-dir and evaluates all autoloads, and defvar does not change the value of void variables, as mentioned in its doscstring.

It is because of common pitfalls like this that users are recommended to use the Customize interface directly (e.g. via customize-group or customize-variable), rather than rolling out their own Elisp. The best zenburn-emacs can do is recommend a sane setup in its documentation (as per #306; thanks again to @felixc for correcting that); there is no other reliable and non-intrusive way to detect whether users have mistakenly used defvar instead of setq and in what order in the general case, and especially not from zenburn-emacs. Any suggestions for how to improve the handling of such situations should be taken to the emacs-devel or bug-gnu-emacs mailing lists.

bbatsov / zenburn-emacs

Documentation should be updated to match new zenburn-override-colors-alist customizable functionality #302