zed icon indicating copy to clipboard operation
zed copied to clipboard

Fix cmd+click find all references fallback not working in Vim mode

Open Congyuwang opened this issue 1 year ago • 6 comments

Exclude go-to-definition links returned by LSP that points to the current cursor position. This fixes #10392 . Related PR #9243 .

The previous implementation first performs go-to-definition, and if the selected text covers the clicked position, it figures out that it is already clicking on a definition, and should instead look for references.

However, the selected range does not necessarily cover the clicked position after clicking on a definition, as in VIM mode.

After the PR, if cmd+click on definitions, the definition links would be an empty list, so no go-to-definition is performed, and find-all-references is performed instead.

Release Notes:

  • Fixed #10392 , now cmd+clicking to find all references works in vim mode.

Congyuwang avatar Apr 17 '24 14:04 Congyuwang

We require contributors to sign our Contributor License Agreement, and we don't have @Congyuwang on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

cla-bot[bot] avatar Apr 17 '24 14:04 cla-bot[bot]

@cla-bot check

Congyuwang avatar Apr 17 '24 14:04 Congyuwang

The cla-bot has been summoned, and re-checked this pull request!

cla-bot[bot] avatar Apr 17 '24 14:04 cla-bot[bot]

If I'm understanding this feature correctly, we had this functionality previously and reverted it due to both issues in the implementation and not being satisfied with the UX.

I think we'll need to evaluate this closely to make sure the behavior is exactly what we want.

Here is the other PR: #6924 And the PR that reverted it: #10094

Apologies if the two are unrelated, they just read as very similar to me.

Not really, this is quite different from #6924. #6924 changes how findAllReferences behaves, which this PR does not touch.

I think this PR only tries to achieve the same thing as #9243 (not reverted), but with a slightly different logic, so that it also works in VIM mode.

Congyuwang avatar Apr 17 '24 15:04 Congyuwang

@Congyuwang Thanks for this, and sorry for the slow reply, it took a while to get familiar with the codepaths here.

Two changes needed to get this merged:

  • Currently we filter links before showing them, however given that clicking on the definition does do something, it should be underlined. I think the best way to do this would be to remove the filter from show_link_definition and do it in cmd_click_reveal_task before passing the links in to navigate_to_hover_links.
  • As exclude_link_to_position is only used by the hover links code, it should not be defined on Project, but in hover_links.rs. Secondly, as you are taking a Model<Buffer> as the first argument, you don't need to pass the snapshot, instead pass cx and then you can get one with let snapshot = buffer.read(cx).snapshot(cx).

Let me know if that works out, or if there are further complexities I missed. Happy to pair on this if you'd like https://calendly.com/conradirwin/pairing

ConradIrwin avatar Apr 25 '24 02:04 ConradIrwin

@ConradIrwin Great. I see what you mean. It seems to work after the changes above, and the definition underlines are back. Also, Project is not touched now.

Congyuwang avatar Apr 25 '24 04:04 Congyuwang