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.

Sign up for GitHub

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

Jump to bottom

Add group argument for LspGotoCommand #2031

Merged
merged 10 commits into from
Sep 10, 2022
Merged

Add group argument for LspGotoCommand #2031

merged 10 commits into from
Sep 10, 2022

Conversation

ninth-dev
Copy link
Contributor

This PR will allow the user to pass a group for Goto commands.

When the group is specified:

  • The result will always open in the group.
  • If there's more than one result, it will focus to the group and open the LocationPicker.

Relates to: #2022

rchl
rchl reviewed Aug 24, 2022
View reviewed changes
plugin/core/open.py Outdated Show resolved Hide resolved
rchl
rchl reviewed Aug 24, 2022
View reviewed changes
plugin/core/open.py Outdated Show resolved Hide resolved
@ninth-dev
Copy link
Contributor Author

Thanks for feedback and suggestions!

jwortmann
jwortmann reviewed Aug 24, 2022
View reviewed changes
Copy link
Member

@jwortmann jwortmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the group argument is ignored in

LSP/plugin/core/sessions.py

Line 1500 in ee8fc6e

def _open_uri_with_plugin_async(
.

plugin/core/open.py Outdated
Comment on lines 19 to 25
if specified_group > -1:
return False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the file is already open in the specified group? Then it should return True I guess. That is what the former opens_in_desired_group variable did.

plugin/core/open.py Outdated
Comment on lines 21 to 22
open_side_by_side = bool(flags & (sublime.ADD_TO_SELECTION | sublime.REPLACE_MRU))
if open_side_by_side:
Copy link
Member

@jwortmann jwortmann Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be put directly into the if-condition without initializing a new variable for it. (Maybe with a comment which describes what is currently used as the variable name)

plugin/core/open.py Outdated
Comment on lines 24 to 25
file_in_active_group = open_file_group == active_group
if file_in_active_group:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

plugin/core/open.py Outdated
file_in_active_group = open_file_group == active_group
if file_in_active_group:
return True
# Jump to the file if sublime.FORCE_GROUP is set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be "... is not set", right?

@ninth-dev
Copy link
Contributor Author

ninth-dev commented Aug 25, 2022 •
edited
Loading

It looks like the group argument is ignored in

LSP/plugin/core/sessions.py

Line 1500 in ee8fc6e

def _open_uri_with_plugin_async(

.

Hi @jwortmann, could you please elaborate what is required to change here ?

@jwortmann
Copy link
Member

jwortmann commented Aug 25, 2022 •
edited
Loading

Hi @jwortmann, could you please elaborate what is required to change here ?

The "Goto" commands call open_location_async, which calls Session.open_location_async, which calls Session.open_uri_async, which might call Session._open_uri_with_plugin_async (for some special URI schemes like deno:). All these functions pass the group argument, but it is not used in the last function. Therein, the Window.new_file API function is called, which doesn't accept a group argument, but instead uses the currently active group to create the new tab in. But if the server response is only a single location, the location picker isn't used, so the self._window.focus_group(group) isn't invoked in that case.

I think it should work if you do the same at

LSP/plugin/core/sessions.py

Lines 1515 to 1516 in ee8fc6e

def open_scratch_buffer(title: str, content: str, syntax: str) -> None:
v = self.window.new_file(syntax=syntax, flags=flags)

right before the call to new_file (possibly only if group > -1).

@ninth-dev
Copy link
Contributor Author

Hi @jwortmann, could you please elaborate what is required to change here ?

The "Goto" commands call open_location_async, which calls Session.open_location_async, which calls Session.open_uri_async, which might call Session._open_uri_with_plugin_async (for some special URI schemes like deno:). All these functions pass the group argument, but it is not used in the last function. Therein, the Window.new_file API function is called, which doesn't accept a group argument, but instead uses the currently active group to create the new tab in. But if the server response is only a single location, the location picker isn't used, so the self._window.focus_group(group) isn't invoked in that case.

I think it should work if you do the same at

LSP/plugin/core/sessions.py

Lines 1515 to 1516 in ee8fc6e

def open_scratch_buffer(title: str, content: str, syntax: str) -> None:
v = self.window.new_file(syntax=syntax, flags=flags)

right before the call to new_file (possibly only if group > -1).

Thanks for detailed explanation, I've added the change.

@rchl
Copy link
Member

rchl commented Aug 29, 2022

Is there particular reason why lsp_symbol_references doesn't support group?

rchl
rchl reviewed Aug 29, 2022
View reviewed changes
plugin/goto.py Show resolved Hide resolved
rchl
rchl reviewed Aug 29, 2022
View reviewed changes
docs/src/features.md Outdated Show resolved Hide resolved
rchl
rchl reviewed Aug 29, 2022
View reviewed changes
plugin/core/open.py Outdated Show resolved Hide resolved
@ninth-dev
Copy link
Contributor Author

Is there particular reason why lsp_symbol_references doesn't support group?

Hi @rchl, no particular reason.

This was my first PR to the repo and was trying to keep it small and contained :)

I can follow up with another PR to add group support for lsp_symbol_reference.

jwortmann
jwortmann reviewed Aug 30, 2022
View reviewed changes
plugin/core/open.py Outdated Show resolved Hide resolved
@ninth-dev ninth-dev requested review from rchl and jwortmann and removed request for rchl and jwortmann September 8, 2022 05:14
rchl
rchl reviewed Sep 8, 2022
View reviewed changes
Copy link
Member

@rchl rchl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some pending comments that I forgot to send. Sorry.

plugin/core/open.py Outdated Show resolved Hide resolved
plugin/core/open.py Outdated Show resolved Hide resolved
plugin/core/open.py Outdated Show resolved Hide resolved
plugin/locationpicker.py Show resolved Hide resolved
@ninth-dev
Copy link
Contributor Author

I had some pending comments that I forgot to send. Sorry.

No worries! I've removed the redundant comments and ensure the sheet is closed when escape is pressed.

@rchl rchl merged commit 575b790 into sublimelsp:main Sep 10, 2022
@rchl
Copy link
Member

rchl commented Sep 10, 2022

Thanks.

rchl added a commit that referenced this pull request Sep 11, 2022
@rchl
Merge branch 'main' into feat/diag-annotations
29bfefc 
* main:
  Add group argument for LspGotoCommand (#2031)
  Add context menu entry in log panel for toggling lines limit (#2047)
  Automatically hide the diagnostics panel on save (#2037)
  Add support for triggerKind in code action requests (#2042)
  Custom context menu in log panel and "Clear log panel" item (#2045)
  Add icons and isPreferred support for code actions (#2040)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwortmann jwortmann jwortmann left review comments

@rchl rchl rchl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ninth-dev @jwortmann @rchl