search

jwiegley / use-package

A use-package declaration for simplifying your .emacs
https://jwiegley.github.io/use-package
GNU General Public License v3.0
4.42k stars 260 forks source link

use-package-core.el: use the Emacs set-default function to avoid saving :custom vars twice #850

Closed tzz closed 4 years ago

tzz commented 4 years ago

This was discussed in #517 and I CC-ed @jwiegley on the emacs-devel discussion.

The change will ensure that :custom vars are not also saved to custom.el.

In my testing it worked correctly, I don't see custom.el entries for those variables anymore. Testing is welcome!

rprimus commented 4 years ago

Tue Jun 30 11:14:51 BST 2020

HI,

customize-set-variable is still mentioned in the docstring for the macro use-package. It is still used in use-package-test/:custom-1.

:custom          Call `customize-set-variable' with each variable definition.
wyuenho commented 4 years ago

@tzz This broke the test. In addition, you need to fix the docstring too. Please fix them.

link0ff commented 4 years ago

@tzz :custom-face needs a similar fix where custom-set-faces should be replaced with face-spec-set.

tzz commented 4 years ago

@tzz This broke the test. In addition, you need to fix the docstring too. Please fix them.

Thank you, and apologies for the late update. Please see #855.

tzz commented 4 years ago

@tzz :custom-face needs a similar fix where custom-set-faces should be replaced with face-spec-set.

@link0ff thank you for the suggestion. I don't know the Emacs face code at all, and looking at custom-theme-set-faces I'm concerned it's fairly complex in the interactions with themes.

link0ff commented 4 years ago

looking at custom-theme-set-faces I'm concerned it's fairly complex in the interactions with themes.

Actually you don't need all the complexity of custom-theme-set-faces, like you don't need all the complexity of customize-set-variable. Exactly like you replaced customize-set-variable with just set-default that in terms of the Customization UI means that the value is changed outside of the Customization UI, the same way it should be possible to replace custom-set-faces with just face-spec-set with the meaning that the face is set outside of the Customization UI.

a13 commented 4 years ago

This change had broken old :custom behavior in a lot of ways, see #856 and #861 in particular.

@tzz @jwiegley

a13 commented 4 years ago

The change will ensure that :custom vars are not also saved to custom.el.

why do you use custom.el at all, if you already use :custom keyword?

akhramov commented 4 years ago

Sadly this breaks my workflows. Not to say the change should be rolled back, yet something needs to be figured.

I'll follow up.

tzz commented 4 years ago

This change had broken old :custom behavior in a lot of ways, see #856 and #861 in particular.

I will take a look. Code fixes welcome to speed up the process.

why do you use custom.el at all, if you already use :custom keyword?

Those two are not exclusive.

a13 commented 4 years ago

Those two are not exclusive.

of course, I'm wondering /why/ one may want to use both

link0ff commented 4 years ago

Unfortunately, set-default is not an adequate replacement of customize-set-variable because when a command calls custom-reevaluate-setting, it reverts such user setting to the default value, not to the value set by the user with set-default.

And there are many such commands in Emacs that use custom-reevaluate-setting to revert variables to customized values.

link0ff commented 4 years ago

See also https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg00306.html So custom should be fixed to save package customizations to the same place where these customizations were loaded, i.e. in the :custom keyword of use-package.

tzz commented 4 years ago

@link0ff please see #881 for an attempt to resolve this issue with regards to custom variables. I think a similar fix for custom faces is possible but am not sure, considering this is the filter applied to them:

      (when (or (and spec (eq (nth 0 spec) 'user))
            comment
            (and (null spec) (get symbol 'saved-face)))

What do you think?

Thanks to everyone who chimed in, and apologies for the late followup.

link0ff commented 4 years ago

Thanks, it seems this is the right fix for now (later custom.el could be improved to save customizations to the same place where they were loaded, but this is much larger redesign of custom.el).

Regarding the custom faces: like you put theme-value in your patch, perhaps for faces it should work when you put the same way its counterpart symbol theme-face.