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

Fixes #5170 - Set default values only if not already customized #5168

Closed
wants to merge 1 commit into from
Closed

Fixes #5170 - Set default values only if not already customized #5168

wants to merge 1 commit into from

Conversation

opie4624
Copy link
Contributor

Thank you for your pull request. In order for your pull request to be accepted please check the following.

  • I have read the Contribution Guidelines at https://github.com/syl20bnr/spacemacs/tree/develop/CONTRIBUTING.org
  • My pull request is against the develop branch and not master
  • I have rebased my contribution against develop and not master – Changes were made while on the develop branch
  • I have written a descriptive commit message that tells people what I did
  • My pull request has been rebased and squashed into one commit

Failure to check these will result in your pull request being rejected. You may describe your pull request below.

@robbyoconnor
Copy link
Contributor

@opie4624 thanks <3

@opie4624
Copy link
Contributor Author

I think this could stand to see a bit more polish over time. There are lots of defaults that layers override that could stand to have a better "standardized" way to set them that would still honor user's settings via Customize.

Relevant discussion in chat: https://gitter.im/syl20bnr/spacemacs?at=56b391733bd55a660df5688c

@syl20bnr
Copy link
Owner

Sorry but what's the issue you want to fix ? Can you open an issue to describe the problem first ? A PR is not the right place to do this.

@syl20bnr syl20bnr changed the title Set default values only if not already customized Fixes #5170 - Set default values only if not already customized Mar 5, 2016
@syl20bnr syl20bnr self-assigned this Mar 5, 2016
@reactormonk
Copy link

reactormonk commented May 2, 2016

I suggest something along the lines of https://gist.github.com/reactormonk/f4322f4ac1a0dec68413b1ccf44e7c7a

@TheBB
Copy link
Collaborator

TheBB commented May 6, 2016

I think this, or something like it, is what we need to use.

custom-variable-p is unreliable because it doesn't seem to know whether a variable is customizable until its definition has been loaded, and it also returns true regardless of whether it actually has a custom value or not.

(defmacro spacemacs|setq (&rest args)
  (let (result)
    (while args
      (let ((variable (pop args))
            (form (pop args)))
        (push `(or (not (get ',variable 'customized-value))
                   (not (get ',variable 'saved-value))
                   (setq ,variable ,form))
              result)))
    `(progn ,@(reverse result))))

@opie4624
Copy link
Contributor Author

opie4624 commented May 6, 2016

When I was researching this for my org-mode issues, I found a comment buried on the customize page of emacswiki that stated customized-value is set if it has been customized and loaded from .emacs while saved-value is set if the value has been customized within the interactive customize interface during this session. So @TheBB's code is checking the right things.

Also customized-face is set for customized fonts. Again, saved-value is used when fonts are customized interactively in this session. In the code I submitted for the PR, I checked all 3 in the same place since it seemed a nice, clean way to check for customization anywhere in once place.

@TheBB
Copy link
Collaborator

TheBB commented May 6, 2016

Yes, I skipped the face stuff because you wouldn't use a setq macro for that. And I think going with a macro is the right choice here so that you wouldn't have to replicate the checking logic everywhere.

@syl20bnr
Copy link
Owner

Thank you for the PR, I pushed a commit which delays the application of custom settings, they are now applied after user-config.

See commit: e699f18

@syl20bnr syl20bnr closed this Nov 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants