Add before/after hooks for goto-line-preview

[manuel-uberti]

These hooks let the user set up something like:

(setq goto-line-preview-before-hook (lambda () (display-line-numbers-mode))
      goto-line-preview-after-hook (lambda () (display-line-numbers-mode -1)))

This is an attempt at fixing #2

I hope @purcell and @waymondo can share their opinions on this. Happy to fix anything that doesn't work as expected, of course.

[jcs090218]

@manuel-uberti Hi, sorry for the late response, because I have never used display-line-numbers-mode or nlinum. I definitely need help from @purcell and @waymondo, I will remain here and see what you guys came up with.

[manuel-uberti]

@jcs090218 no worries at all. I just like this package a lot and want to add what I think could be a nice customisation. Let's wait for them. :)

[waymondo]

The only use case that this doesn't work seamlessly in terms of yours (and my) example is the following:

If you have a line-numbering mode enabled for some modes but not all, your example would always turn off the line numbering mode after using the goto-line-preview function, even in modes where you want it. This can be solved with some extra setup in these hooks and a local defvar/defcustom to white/blacklist certain modes, or advice wrapping the function that lexically binds the previous state of the line numbering mode.

In the end, I think hooks are a common convention for emacs customization and I'm fine doing this extra setup locally in my config, so it gets a 👍 from me.

[manuel-uberti]

Thank you. If we can improve this PR and make the configuration on the user's side easier, I'm happy to do it. Just need some guidance.

[waymondo]

I think this PR is good to merge as is and if I get some time in the next few days, I can open another with the behavior I outlined here: #2 (comment)

[manuel-uberti]

Great, thank you. :)

[purcell]

The robust hook version looks like this:

(setq
 goto-line-preview-before-hook (lambda ()
                                 (setq-local my/prev-line-number-state display-line-numbers-mode)
                                 (display-line-numbers-mode))
 goto-line-preview-after-hook (lambda () (unless my/prev-line-number-state
                                      (display-line-numbers-mode -1))))

while the advice version is arguably much neater:

(defun with-display-line-numbers (f &rest args)
  (let ((display-line-numbers t))
    (apply f args)))

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

I kinda feel like we should just include an example for the latter in the README & Commentary, and skip merging this PR...?

[manuel-uberti]

It's OK to me if we go with the advice. ☺️

[waymondo]

I like the advice implementation and would be fine personally doing it this way since i'm using display-line-numbers-mode.

I did just open #11 as a way to simulate this behavior with alternate line numbering modes though.

[jcs090218]

I personally like the hook method. Of course to resolve #2, advice approach would be much neater. I think the before/after hooks make the package more configurable? So people can choose to do stuff right before/after the command. And still, if the advice approach already resolved the issue here, I guess is no need to merge this PR? Since this package only serve this small purpose, I'm trying not to add code complexity here.

[waymondo]

I can't think of a personal use case other than this behavior we are talking about, but I can see some other user wanting to dim/hide other windows, or a different unknown idea.

If you want to keep the complexity down here, adding empty hooks is a good way to let users add their own expansive ideas in their own .emacs.d without bloating this package.

[manuel-uberti]

@jcs090218 what about merging this (to give the chance to customise what happens before and after anyway, besides line numbering), and mention the advice solution in the README?

[jcs090218]

@jcs090218 what about merging this (to give the chance to customise what happens before and after anyway, besides line numbering), and mention the 'advice` solution in the README?

Yeah, this is what I was thinking right now.

Edit: I realized advice could also do :after, :before and :around. Not quit sure if I need to add hook anymore... hmm.... I guess it would be no harm to add the hook methods?

[manuel-uberti]

As you prefer, really. I usually don't use advice too much, but if in this particular case it makes everything simpler you can leave out the hooks, update the README with advice examples, and close this PR.

[jcs090218]

Since this is no harm to the package itself, I guess giving more options should be okay.

[roshanshariff]

For future reference, here is a version of @purcell's display-line-numbers-mode advice that also works with goto-line-preview-relative:

(defun with-display-line-numbers (f &rest args)
  (let ((display-line-numbers t)
        (display-line-numbers-offset
         (if goto-line-preview--relative-p (- (line-number-at-pos)) 0)))
    (apply f args)))

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

I'm not sure where would be a good place to document this sort of thing.

[jcs090218]

I think it would be fine if we document under Usage and above Contribution just for customization or prefer configuration, etc.