goto-line-preview icon indicating copy to clipboard operation
goto-line-preview copied to clipboard

Add option for `goto-line-preview-ensure-line-number-mode`

Open waymondo opened this issue 6 years ago • 4 comments

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

waymondo avatar Mar 05 '19 23:03 waymondo

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.

purcell avatar Mar 06 '19 01:03 purcell

@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?

jcs090218 avatar Mar 06 '19 03:03 jcs090218

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)

purcell avatar Mar 06 '19 03:03 purcell

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.

waymondo avatar Mar 06 '19 04:03 waymondo