Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug with Emacs 29 setopt does not correctly honor modus defcustom types #118

Closed
shipmints opened this issue Sep 7, 2024 · 12 comments
Closed

Bug with Emacs 29 setopt does not correctly honor modus defcustom types #118

shipmints opened this issue Sep 7, 2024 · 12 comments

Comments

@shipmints
Copy link

Not sure if this is an issue with modus defcustom definitions or with the implementation of setopt itself. This is FYI as more people might start using setopt and you do briefly mention setopt in the modus documentation. If setopt is broken, it might be good to report a bug before Emacs 30 goes to press. Or this could just be my misunderstanding.

Emacs 29.3, and the most recent ELPA modus-themes-20240905...

The literal example from the docstring works

  (setopt modus-themes-headings
          (quote ((1 . (variable-pitch 1.5))
                  (2 . (1.3))
                  (agenda-date . (1.3))
                  (agenda-structure . (variable-pitch light 1.8))
                  (t . (1.1)))))

Amended with level 3 works

  (setopt modus-themes-headings
          '((1 . (variable-pitch 1.5))
            (2 . (1.3))
            (3 . (variable-pitch 1.8))
            (agenda-date . (1.3))
            (agenda-structure . (variable-pitch light 1.8))
            (t . (1.1))))

But breaks with overline added

  (setopt modus-themes-headings
          '((1 . (variable-pitch 1.5))
            (2 . (1.3))
            (3 . (overline variable-pitch 1.8))
            (agenda-date . (1.3))
            (agenda-structure . (variable-pitch light 1.8))
            (t . (1.1))))
⛔ Warning (emacs): Value '((1 variable-pitch 1.5) (2 1.3) (3 overline variable-pitch 1.8) (agenda-date 1.3) (agenda-structure variable-pitch light 1.8) (t 1.1))' does not match type (alist :options ((0 #1=(set :tag Properties :greedy t (const :tag Proportionately spaced font (variable-pitch) variable-pitch) (choice :tag Font weight (must be supported by the typeface) (const :tag Unspecified (use whatever the default is) nil) (const :tag Thin thin) (const :tag Ultra-light ultralight) (const :tag Extra-light extralight) (const :tag Light light) (const :tag Semi-light semilight) (const :tag Regular regular) (const :tag Medium medium) (const :tag Semi-bold semibold) (const :tag Bold bold) (const :tag Extra-bold extrabold) (const :tag Ultra-bold ultrabold)) (radio :tag Height (float :tag Floating point to adjust height by) (cons :tag Cons cell of `(height . FLOAT)' (const :tag The `height' key (constant) height) (float :tag Floating point))))) (1 #1#) (2 #1#) (3 #1#) (4 #1#) (5 #1#) (6 #1#) (7 #1#) (8 #1#) (t #1#) (agenda-date #1#) (agenda-structure #1#)) :key-type symbol :value-type #1#)

Similarly, this works

  (setopt modus-themes-completions
          '(
            (matches . (extrabold underline))
            (selection . (semibold italic))
            )))

But breaks when text-also added

  (setopt modus-themes-completions
          '(
            (matches . (extrabold underline))
            (selection . (semibold italic text-also))
            ))
⛔ Warning (emacs): Value '((matches extrabold underline) (selection semibold italic text-also))' does not match type (set (cons :tag Matches (const matches) (set :tag Style of matches :greedy t #1=(choice :tag Font weight (must be supported by the typeface) (const :tag Unspecified (use whatever the default is) nil) (const :tag Thin thin) (const :tag Ultra-light ultralight) (const :tag Extra-light extralight) (const :tag Light light) (const :tag Semi-light semilight) (const :tag Regular regular) (const :tag Medium medium) (const :tag Semi-bold semibold) (const :tag Bold bold) (const :tag Extra-bold extrabold) (const :tag Ultra-bold ultrabold)) (const :tag Italic font (oblique or slanted forms) italic) (const :tag Underline underline))) (cons :tag Selection (const selection) (set :tag Style of selection :greedy t #1# (const :tag Italic font (oblique or slanted forms) italic) (const :tag Underline underline))) (cons :tag Fallback for both matches and selection (const t) (set :tag Style of both matches and selection :greedy t #1# (const :tag Italic font (oblique or slanted forms) italic) (const :tag Underline underline))))
@protesilaos
Copy link
Owner

I have encounted this issue with practically everything I have tested. setopt does not seem to honour the :type. But I did not look into it further.

Does this issue happen if you try to modify those variables through the Custom user interface?

@shipmints
Copy link
Author

I use only programmatic elisp for better control and easier version controlling. There might be a simple adjustment to allow the custom infrastructure to work with modus-themes-completion. In the end, setopt just warns but still does what you tell it.

@protesilaos
Copy link
Owner

protesilaos commented Oct 19, 2024 via email

@alphapapa
Copy link

According to the info manual for setopt:

This works the same as ‘setq’, but if the variable has any special setter functions, they will be run automatically when using ‘setopt’. You can also use ‘setopt’ on other, non-customizable variables, but this is less efficient than using ‘setq’.

It would be nice if a warning were given for a value that doesn't match the expected type, but I guess that isn't implemented yet. Might be worth mentioning on emacs-devel.

@shipmints
Copy link
Author

Debated at length https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73098

@shipmints
Copy link
Author

I am trying this now and it does not give me any error: (custom-set-variables '(modus-themes-to-toggle "hello")) Meanwhile, the :type of that user option does not allow a string or non-nil value: :type `(choice (const :tag "No toggle" nil) (list :tag "Pick two themes to toggle between" (choice :tag "Theme one of two" ,@(mapcar (lambda (theme) (list 'const theme)) modus-themes-items)) (choice :tag "Theme two of two" ,@(mapcar (lambda (theme) (list 'const theme)) modus-themes-items)))) So I guess those are only meant for the Custom UI.

I think you might have meant (customize-set-variable which I believe uses the full customize mechanical infrastructure.

setopt is implemented on top of the customize type checking infrastructure but I do not know how robust that is vs. a set of convenience helpers to drive the customize GUI.

The question for your modus users, Prot, is that since you have setters on your options, should they have a way to call the same function manually after running their setq's?

@protesilaos
Copy link
Owner

protesilaos commented Oct 19, 2024 via email

@shipmints
Copy link
Author

If the only purpose of the setters is to force theme reloading, then I'd say not needed programmatically.

@protesilaos
Copy link
Owner

protesilaos commented Oct 20, 2024 via email

@shipmints
Copy link
Author

If a setter has no practical value for a particular user-settable option, I'd say remove it. Leaving it in might cause people who read the code itself (vs. just the documentation) to believe they need to do something special vs. merely setq. I've adopted the habit of reading the code for every user option so I can see precisely what's going on/intended vs. documentation. I would have been fooled by an erroneous setter.

@protesilaos
Copy link
Owner

protesilaos commented Oct 20, 2024 via email

protesilaos added a commit that referenced this issue Oct 20, 2024
@protesilaos
Remove some needless :set functions
613f953 
Our :set function is designed to reload the theme, with the intent to
apply whatever change to the visuals. But this is not relevant for
user options that have no direct effect on the appearance of the
themes.

This is done as part of a discussion with shipmints in issue 118:
<#118>.
@protesilaos
Copy link
Owner

I just removed some of the :set that are not relevant. Most are still there though. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@alphapapa @protesilaos @shipmints