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

common-tweaks layer #4250

Closed

Conversation

StreakyCobra
Copy link
Contributor

Allow users to easily enable/disable non-trivial tweaks.

This layer has no effect until some tweaks are enable through variables:

#+begin_src emacs-lisp
(setq-default dotspacemacs-configuration-layers '(common-tweaks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing a set of parens here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

@TheBB
Copy link
Contributor

TheBB commented Dec 22, 2015

Is there any reason why we need a complicated macro and not just a bunch of conditionals?

@StreakyCobra
Copy link
Contributor Author

Is there any reason why we need a complicated macro and not just a bunch of conditionals?

Hum… It's not really a complicated one (except maybe the loader trick). Advantages are:

  • It clearly separates different parts of the tweaks (Functions, tweaks, loading the code at the right moment, description)
  • All functions and lines related to a tweak are grouped in the same entity, so it's easier to:
    • Keep the related code grouped together
    • Delete a tweak without forgetting some parts/functions of the tweak
    • Temporary disabling all related code with a flag :disable t in one go
  • Avoid boilerplate code, like:
    • Defining a variable with the same name as the tweak
    • Print a message about enabled tweaks when --debug-init is passed
    • Checking the variable for activating the tweak or not
    • Synchronizing the documentation of the tweak with the documentation of the variable
  • It will be easier to add a general functionality about tweak in only one place (whatever it is).

The macro is not really difficult to understand and quite straightforward (just grouping all parts with a (progn …, if we omit the :loader trick). You just need the :tweak property to be set, all other being optional and should be used only if needed.

I see many advantages for the code structure and its cleanness, without big drawbacks. Of course if it is really a problem, it's not difficult for me to change this.

@StreakyCobra StreakyCobra changed the title common-tweaks layer [WIP] common-tweaks layer Jan 3, 2016
@StreakyCobra StreakyCobra force-pushed the feature/common-tweaks branch from c89cfd2 to 8927a8e Compare January 10, 2016 13:08
@StreakyCobra
Copy link
Contributor Author

Let me know here if there are other tweaks that can be useful to spacemacs users.

@StreakyCobra StreakyCobra changed the title [WIP] common-tweaks layer common-tweaks layer Jan 10, 2016
@StreakyCobra StreakyCobra force-pushed the feature/common-tweaks branch from 37fc5ba to b3577ed Compare January 11, 2016 20:58
@justbur
Copy link
Contributor

justbur commented Jan 11, 2016

For counsel-find-file

(setq counsel-find-file-ignore-regexp "\\(?:\\`[#.]\\)\\|\\(?:[#~]\\'\\)")

Ignores any file or directory starting with a dot (plus a couple of more) unless the user starts the search string with a dot.

@StreakyCobra
Copy link
Contributor Author

@justbur Thanks, done 👍

@StreakyCobra StreakyCobra changed the title common-tweaks layer [WIP] common-tweaks layer Jan 11, 2016
@StreakyCobra StreakyCobra changed the title [WIP] common-tweaks layer common-tweaks layer Jan 11, 2016
@StreakyCobra
Copy link
Contributor Author

To WIP or not to WIP, that is the question… I still have improvements to do, but I suppose it's ready to merge/review to have first users feedback.

@fbergroth
Copy link
Contributor

Not sure if it belongs here, but I'd like to see

  • restore windows after quitting ediff
  • quit ediff without prompting

@StreakyCobra
Copy link
Contributor Author

It may be. Do you already have snippets?

@fbergroth
Copy link
Contributor

Not any good ones :-)

@fbergroth
Copy link
Contributor

@StreakyCobra
For restoring window layout:

(add-hook 'ediff-after-quit-hook-internal 'winner-undo)

Slightly modified your attempt for quitting ediff without prompt:

(defun ct//no-confirm (origfunc &rest args)
  (cl-letf (((symbol-function 'y-or-n-p) (lambda (&rest args) t)))
    (apply origfunc args)))

(advice-add 'ediff-quit :around 'ct//no-confirm)

@StreakyCobra
Copy link
Contributor Author

Thx, I'll add this later this week :-)
On Jan 11, 2016 11:13 PM, "Fredrik Bergroth" notifications@github.com
wrote:

@StreakyCobra https://github.com/StreakyCobra
For restoring window layout:

(add-hook 'ediff-after-quit-hook-internal 'winner-undo)

Slightly modified your example for quitting ediff without prompt:

(defun ct//no-confirm (origfunc &rest args)
(cl-letf (((symbol-function 'y-or-n-p) (lambda (&rest args) t)))
(apply origfunc args)))

(advice-add 'ediff-quit :around 'ct//no-confirm)


Reply to this email directly or view it on GitHub
#4250 (comment).

@TheBB
Copy link
Contributor

TheBB commented Jan 11, 2016

(add-hook 'ediff-after-quit-hook-internal 'winner-undo)

This at least should be a default, and in any case is not nontrivial.

@nixmaniack
Copy link
Contributor

Yes, restoration of windows after ediff quit should be a default!

@StreakyCobra
Copy link
Contributor Author

This at least should be a default, and in any case is not nontrivial.

Agreed.

We also have to discuss what belongs to common-tweaks and what belongs to the layers.

@sooheon
Copy link

sooheon commented Apr 19, 2016

Everything from 115 to 122 here should be a tweak, imo: https://github.com/syl20bnr/spacemacs/blob/develop/layers/+distribution/spacemacs-bootstrap/packages.el#L115

There is some discussion about the >gv tweaks in #5273, and I think the J/K move line tweaks fall under the same umbrella.

@StreakyCobra
Copy link
Contributor Author

@sooheon The link referencing the precise commit so we can find the specified lines if the file change later:

(define-key evil-visual-state-map "J" (concat ":m '>+1" (kbd "RET") "gv=gv"))
(define-key evil-visual-state-map "K" (concat ":m '<-2" (kbd "RET") "gv=gv"))
(evil-ex-define-cmd "enew" 'spacemacs/new-empty-buffer)
(define-key evil-normal-state-map (kbd "K") 'spacemacs/evil-smart-doc-lookup)
(define-key evil-normal-state-map (kbd "gd") 'spacemacs/evil-smart-goto-definition)


Concerning the status of this PR, I'm not sure if it should be its own layer like it I'm proposing here, or if the tweaks should be proposed in the corresponding layer directly. I remember someone pointed this out and it was discussed, but I can't find out where :/

The idea was that if we propose the tweaks directly in the layers, then people just have to read the README of the corresponding layer and already know how to customize it. With a common-tweaks layer people have first to know about its existence, and then knows there are some tweaks regarding a given functionality before being able to use it.

@fbergroth
Copy link
Contributor

I remember someone pointed this out and it was discussed, but I can't find out where :/

@StreakyCobra The link referencing the precise message 😄
https://gitter.im/syl20bnr/spacemacs?at=56942381d739f50a3602f788

@StreakyCobra
Copy link
Contributor Author

@fbergroth 👍 Nice thanks :-)

@sooheon
Copy link

sooheon commented Apr 19, 2016

Hm, especially given that now even spacemacs itself is broken up into layers, it makes even more sense.

@d12frosted
Copy link
Contributor

@syl20bnr Is it really in Merging... state? 😸

@syl20bnr
Copy link
Owner

I'm still thinking about it but I lean toward dispatching the tweaks where they belongs. If a tweak is worthy it should be an option in its corresponding layer IMO.

@StreakyCobra
Copy link
Contributor Author

☝️ Agreed

@syl20bnr syl20bnr removed their assignment May 22, 2017
@github-actions
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please let us know if this PR is still need merging!

@github-actions github-actions bot added the stale marked as a stale issue/pr (usually by a bot) label Sep 30, 2020
@github-actions github-actions bot closed this Dec 29, 2020
@StreakyCobra StreakyCobra deleted the feature/common-tweaks branch November 29, 2022 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Layer stale marked as a stale issue/pr (usually by a bot)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants