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.

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

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

Merged
merged 1 commit into from Jun 29, 2020

Conversation

tzz
Copy link
Collaborator

@tzz tzz commented Jun 22, 2020

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!

@jwiegley jwiegley merged commit b9f1fe6 into jwiegley:master Jun 29, 2020
@rprimus
Copy link

rprimus commented Jun 30, 2020

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
Copy link
Contributor

wyuenho commented Jul 7, 2020

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

@link0ff
Copy link

link0ff commented Jul 9, 2020

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

tzz pushed a commit to tzz/use-package that referenced this pull request Jul 12, 2020
tzz added a commit to tzz/use-package that referenced this pull request Jul 12, 2020
@tzz
Copy link
Collaborator Author

tzz commented Jul 12, 2020

@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
Copy link
Collaborator Author

tzz commented Jul 12, 2020

@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
Copy link

link0ff commented Jul 12, 2020

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.

zzamboni added a commit to zzamboni/dot-emacs that referenced this pull request Aug 9, 2020
After jwiegley/use-package#850, did the
following to eliminate duplicate variable definitions in custom.el and
init.el:

- Removed from custom.el all variables that are configured in init.org
- Changed all uses of customize-set-variable to custom-set-variables.
@a13
Copy link

a13 commented Aug 19, 2020

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

@tzz @jwiegley

@a13
Copy link

a13 commented Aug 19, 2020

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
Copy link
Contributor

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
Copy link
Collaborator Author

tzz commented Aug 22, 2020

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
Copy link

a13 commented Aug 24, 2020

Those two are not exclusive.

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

@link0ff
Copy link

link0ff commented Sep 7, 2020

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
Copy link

link0ff commented Sep 9, 2020

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
Copy link
Collaborator Author

tzz commented Nov 8, 2020

@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
Copy link

link0ff commented Nov 8, 2020

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.

CeleritasCelery pushed a commit to CeleritasCelery/use-package that referenced this pull request May 17, 2021
CeleritasCelery pushed a commit to CeleritasCelery/use-package that referenced this pull request May 17, 2021
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Nov 24, 2022
use-package-core.el: use the Emacs set-default function to avoid saving :custom vars twice
GitHub-reference: jwiegley/use-package#850
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Nov 24, 2022
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Nov 24, 2022
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Nov 29, 2022
use-package-core.el: use the Emacs set-default function to avoid saving :custom vars twice
GitHub-reference: jwiegley/use-package#850
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Nov 29, 2022
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants