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

Add option for goto-line-preview-ensure-line-number-mode #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

waymondo
Copy link

@waymondo waymondo commented Mar 5, 2019

This can be the symbol of a line-number-mode that should always be
enabled for the duration of goto-line-preview. This is useful if you
are in a buffer that is not showing line numbers but would like to see
then when you invoke goto-line-preview.

Modes known to work are display-line-numbers-mode, linum-mode,
and nlinum-mode. The default value is nil.

Connect to #2

This can be the symbol of a line-number-mode that should always be
enabled for the duration of `goto-line-preview`. This is useful if you
are in a buffer that is not showing line numbers but would like to see
then when you invoke `goto-line-preview`.

Modes known to work are `display-line-numbers-mode`, `linum-mode`,
and `nlinum-mode`. The default value is `nil`.
@purcell
Copy link
Contributor

purcell commented Mar 6, 2019

This is only necessary for linum and nlinum, because the advice approach mentioned in #10 handles display-line-numbers-mode nicely. However, linum and nlinum are effectively obsolete, so I question the wisdom of adding code here to support them. It's all very special-case-y, and it makes the code less clear. Personally I'd just add a README example for display-line-numbers and stop there.

@jcs090218
Copy link
Member

jcs090218 commented Mar 6, 2019

@waymondo Hello, thanks for contributing to this project. As you guys may know I have no experience to display-line-numbers-mode or nlinum, plus my personal config just has the line number constantly showing. Hopefully you guys wouldn't mind if I give opinions to this PR.

I have read the code and much worry about the increased of code complexity, and I do agree with @purcell this is a special case. However, I do like to make this package as much compatible to other line number mode. Does #10 advice approach resolve #2 to all the line number modes? If it does, I would just prefer to have this documented?

@purcell
Copy link
Contributor

purcell commented Mar 6, 2019

Does #10 advice approach resolve #2 to all the line number modes? If it does, I would just prefer to have this documented?

Yes, you could use advice for the legacy line number modes (linum, nlinum) too, but the advice would be slightly more complicated because it's not sufficient to dynamically bind a variable. So it might look something like this (untest):

(defun with-linum (f &rest args)
  (let ((initial-linum linum-mode))
    (linum-mode 1)
    (unwind-protect
        (apply f args)
      (unless initial-linum
        (linum-mode -1)))))

(advice-add 'goto-line-preview :around #'with-linum)

@waymondo
Copy link
Author

waymondo commented Mar 6, 2019

Yes, basically this PR is assembling @purcell's logic above in a way that also works if the interactive function has not been loaded yet, and for the most popular line number packages.

Since the default is nil, the behavior is not considered unless set, so I don't see what the harm is in merging. At the end of the day, this PR is really just embedding the idea of this behavior into the source code, instead of only adding it to the readme.

Feel free to close if you think the readme is a better place for this.

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.

3 participants