Eli Zaretskii writes: >> Cc: 57639@debbugs.gnu.org >> From: Philip Kaludercic >> Date: Sat, 17 Sep 2022 18:13:39 +0000 >> >> +@findex theme-choose-variant >> + Some themes have variants (most often these are light and dark >> +pairs). You can switch between these by typing @kbd{M-x >> +theme-choose-variant}. Note that this only works if only one theme is >> +active. If a theme has only one alternative, it will toggle >> +automatically. If there are more of them, it will query which one to >> +use. > > This description is confusing, I needed to read it several times > before I understood what you were trying to say. The main problem is > that the "Note" sentence doesn't belong, and it interrupts the logic > of the description. Here's my suggestion for saying it more clearly: > > Some themes have variants (most often just two: light and dark). > You can switch to another variant with @kbd{M-x > theme-choose-variant}. If the currently active theme has only one > other variant, it will be selected; if there are more variants, the > command will prompt you which one to switch to. > > Note that @code{theme-choose-variant} only works if a single theme > is active. I prefer your phrasing and have adapted it in my next iteration of the patch below. > (Btw, what happens if more than one theme is active and the user > invokes theme-choose-variant? should this be described?) The current patch doesn't support this, but I've considered that a custom theme could clarify that it is a colour scheme in the new property list, and toggling would then not assume that there is only one active theme but only one active colour scheme. >> +Themes*} buffer. The remaining arguments @var{properties} is used >> +pass a property list with theme attributes. ^^^^^^^ > > "are used", in plural. Fixed. >> + :brightness 'light > > Should we use :background-mode instead of :brightness, for consistency > with frame-background-mode? I chose "brightness" because I was looking at terms used in colour theory and everyday language[0], and considered describing properties such as "hue", "saturation", "contrast", etc. but that would probably be overkill. Consistency is probably preferable, so I'll also make this change. [0] https://en.wikipedia.org/wiki/Color_term#In_natural_languages >> +(defun theme-choose-variant (&optional no-confirm no-enable) >> + "Toggle the current active theme by enabling its dual pair. > > "Toggle ... by enabling"? "Dual pair"? Can this sentence be > rephrased to be more clear? >> +The current theme will be immediately disabled before the dual >> +theme has been enabled. > > Likewise here: "dual theme" doesn't explain itself, and seems to be > inaccurate, given the description in the manual. You are right, the documentation was outdated. See below. >> If THEME is not active an error will be >> +raised. > > Passive tense alert! > >> If theme is nil For NO-CONFIRM and NO-ENABLE, see >> +`load-theme'." > > Something's missing here or wrong with the punctuation? The sentence was cut-off, so I just removed everything from "It" to "nil". >> + ((length> custom-enabled-themes 1) >> + (user-error "More than one theme active, cannot unambiguously toggle"))) > > Wouldn't it be better to prompt for the theme in this case? That would be another possibility. I guess it could be combined with the above proposal. >> + (let* ((theme (car custom-enabled-themes)) >> + (family (plist-get (get theme 'theme-properties) :family))) >> + (unless family >> + (error "`%s' is not part of a family" theme)) > > "Family"? this terminology was never mentioned in the manual or the > doc string. How about > > Theme `%s' does not have any variants > > instead? Strictly speaking that error message would be wrong at this point, because we cannot say if a theme has no variants if it is not part of a family. This is because variants of a theme are all those that are part of the same family. I think it would be better to clarify this in the documentation. > Thanks.