Skip to content

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