Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: LSP navigation updates diffs #538

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

delabere
Copy link

@delabere delabere commented Apr 20, 2024

Describe what this PR does / why we need it

When you navigate using LSP or a command like gf while reviewing a change using the use_local_fs option, you lose the diff, even if the file you navigated to was part of the change.

This change allows you to navigate around a PR's changed files using your LSP

Screen.Recording.2024-04-20.at.19.05.43.mov

Does this pull request fix one issue?

Fixes #535

Describe how you did it

Review layouts keep track of files, these are the files that you see populated in the files pane after opening a review.
After entering a buffer, if the filepath is not octo://..., I assume that it is a local file and I check whether the file is part of the layout's file's. If it is then I update the review to that file using review.layout:set_file(file)

Describe how to verify it

You need to use use_local_fs and review a PR that has multiple files, where you can navigate between those files via LSP.

Special notes for reviews

There is some unintended behaviour with this change. When you navigate to the right buffer in the diff you see this message:

image

It doesn't happen if I comment out review.layout:set_file(file) in my change, so I assume that the side-effects of this are triggering more autocomands

@delabere delabere marked this pull request as ready for review April 20, 2024 18:07
@delabere delabere changed the title function to allow refresh of layout with file under cursor after navi… Feature: LSP navigation updates diffs Apr 20, 2024
@jondkinney
Copy link

Seems like this would be a good change! Will this be included in a future release sometime soon?

@delabere
Copy link
Author

delabere commented Sep 5, 2024

@pwntester 👋 just giving this one a nudge

@pwntester
Copy link
Owner

@delabere took a look some time ago and could not figure out why its popping that autocmd error. Will take another look since merging it in this state will results in issues complaining about the error.

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.

When using use_local_fs = true, go to definition doesn't open a diff buffer
3 participants