tide-references: make entire line clickable

[sandersn]

Note that the presentation doesn't change; only the symbol in question is highlighted, but you can now click anywhere in the line to jump to the reference location.

I made this change for two reasons:

1. I am bad at clicking things, especially small ones.
2. I use hiwin to grey out non-active windows, which cancels tide-reference's highlights when it's not active.

However, I think this change will benefit everybody by making the click target bigger.

[lddubeau]

I've also been annoyed at having to move to the identifier I searched for in order to be able to navigate to its occurrence. I pretty much always forget that it is not grep and always first just hit enter on the line before remembering that I have to move to the specific occurrence. So I'm in favor of the general idea of making *tide-references* less strict for navigation.

However, the proposed modification does not work well if the symbol that was searched for appears multiple times on the same line. I tried searching references to db in a project that has return db.transaction("rw", db.packs, db.xmlfiles, () => {. The behavior pre-PR is that I have to go to one of the hits and when I hit enter on it, then I'm taken to that specific hit. post-PR the new behavior is that no matter where I hit enter, I'm taken to the last hit on the line. It is confusing if a user has focused on one specific hit, to be taken to another hit and have to move again to get to the correct hit.

And the proposed modification messes up navigation in *tide-references*. n and p are supposed to move from hit to hit, but after applying the PR they move from line to line.

Here's an idea, off the top of my head. I don't know how well it would work when it comes down to the nitty-gritty but maybe the tide-goto-reference function could compute a column number to jump to by taking the column number at which point is when the user hits enter, adjusted to take into account the line numbers that are added at the beginning of each line in the *tide-references* buffer. This way, the user would land in a position that corresponds to where point was when the user hit enter.

(Tangential note: we really should have formal tests for this in the test suite.)

[sandersn]

@lddubeau I like your idea. Two questions: Should we collapse multiple hits into one line? Do you think it's all right for 'n' and 'p' to just move from line to line in that case?

[josteink]

What has always annoyed me with tide-references is how it doesn't work like occur or grep, much like you complain about.

I agree there's reasons for why things are like they currently are, like multiple hits/references on the same line.

That said... If we are to change this to work like occur or grep, we should do it properly: Not just cosmetically.

Right now occur or grep lets you globally navigate between hits by using next-error or previous-error, much like compilation mode. These are very useful bindings, and I have them mapped to functions keys globally.

If we are to change this behaviour to be more in line with how other "common" Emacs-functions work, it would really be great to have it work similarly at the core, not just at the appearance-level.

[ananthakumaran]

If we are to change this behaviour to be more in line with how other "common" Emacs-functions work, it would really be great to have it work similarly at the core, not just at the appearance-level.

We discussed about the UI change in the past. If somebody interested in spending time on this area, probably look at adding support for Xref. Emacs comes with a default UI and other frameworks could hook into xref and customize like https://github.com/brotzeit/helm-xref, https://github.com/alexmurray/ivy-xref

[lddubeau]

@sandersn Regarding your second question, with what I have in mind n and p would not change from their current behavior. Regarding your first question, I'm not sure what you are asking there. I'm going to describe what I have in mind in other words because I think @josteink and I are not on the same page as to what is being proposed here. If nothing I'm writing answers your question, I'm going to need details about what you're asking.

The change I proposed in response to the initial PR would not include any cosmetic change: the *tide-references* buffer would look exactly like it does now. It moreover would not change anything to existing user interactions that are already supported (so n and p would work the same as they do now). The only difference is that if someone goes into *tide-references* and hits the enter key somewhere else than right inside a hit, Emacs would follow the reference to the corresponding position into the source file references. (What I mean by "corresponding position" is that if searched for foo and the *tide-references* buffer has a line 23: const bar = baz(foo) + moo(foo) and I hit enter in *tide-references* while point is on the + symbol, then I should get onto the + symbol into the source file that contained the hit.)

The above was my initial "off the top of my head" idea. I can see arguments for some amendments to that idea. For instance, a case could be made that although *tide-references* should respond to enter being hit anywhere in a line, it should not jump between hits but should instead jump to the closest hit.

@josteink I agree that *tide-references* should harmonize as much as possible with what we get with grep or similar tools. It seems to me what @sandersn initially proposed and the change I proposed in response to the PR is one step towards that goal. When you are in a *grep* buffer, you can hit enter everywhere and jump to a reasonable position. Right now, you cannot do the same in *tide-reference*. If what I'm proposing is implemented, then *tide-references* will be handling enter better than *grep* because *grep* mishandles multiple hits on the same line. Using the same example I gave in an earlier comment, if I M-x grep-find the string db and hit enter on the 3rd instance of db in the line *grep* takes me right onto the 1st instance! In my view, that's a bug.

I realize the proposal here does nothing with regards to addressing bindings to next-error, etc.. If @sandersn wants to address that too, that's great. But I don't think @sandersn needs to address that in this PR. And I don't see anything proposed here as somehow impeding the changes you mentioned.

[josteink]

When you are in a grep buffer, you can hit enter everywhere and jump to a reasonable position. Right now, you cannot do the same in tide-reference

Agreed. And I think that should be fixed too.

If what I'm proposing is implemented, then tide-references will be handling enter better than grep because grep mishandles multiple hits on the same line.

Which is good.

I realize the proposal here does nothing with regards to addressing bindings to next-error

Yeah. That's just me wishing. I don't strictly see it as a requirement, but if we could get it working while redoing this porting of the code, I just thought that would be nice, because then it would be even more in line with how grep, occur and similar work in Emacs.

If it can't be done or nobody wants to do it, I'm not going to stall this PR over that,

[elibarzilay]

I ran into this annoyance too, and some more; and @lddubeau's idea seemed appealing so I implemented it. It doesn't change n/p, and doesn't implement a next-error thing, and it doesn't change any of the existing code, but it works fine in a way that sucks less:

(defun tide-goto-reference-line ()
  "Jump to reference location in the file."
  (interactive)
  (-when-let* ((bol  (line-beginning-position))
               (line (1+ (next-single-property-change bol 'face))) ; after the ":"
               (col  (max 0 (- (point) line)))
               (ref  (next-single-property-change bol 'tide-reference))
               (ref* (get-text-property ref 'tide-reference)))
    (tide-jump-to-filespan ref* nil t)
    (goto-char (+ (line-beginning-position) col))))

Making this a proper PR shouldn't take too much: make it put text properties that are more reliable than relying on where the face changes (to find the text BOL point), or even better -- make the line number displayed without being actual text so it's not counted as contents of the refs buffer (and you can't navigate into it).

[I also ran into more related annoyces: a bug in p (use it from inside an identifier to see it), the lack of TAB binding, and not cycling matches, so I fixed the bug and made another function that cycles through the refs to bind to TAB.]

[josteink]

Making this a proper PR shouldn't take too much: make it put text properties that are more reliable than relying on where the face changes (to find the text BOL point), or even better -- make the line number displayed without being actual text so it's not counted as contents of the refs buffer (and you can't navigate into it).

Please do! 😃