Skip to content

Add rename text input handler#367

Merged
tomv564 merged 3 commits intosublimelsp:masterfrom
predragnikolic:add-new-rename-text-input-handler
Jul 11, 2018
Merged

Add rename text input handler#367
tomv564 merged 3 commits intosublimelsp:masterfrom
predragnikolic:add-new-rename-text-input-handler

Conversation

@predragnikolic
Copy link
Member

@predragnikolic predragnikolic commented Jul 3, 2018

Ask for new symbol name though the command palette, instead of the window input panel. #206

output

There is a comment I left for you to have a look.

Can you please tell me, what the purpose of the event argument in the run method and when do you pass it by to the command?

class LspSymbolRenameCommand(LspTextCommand):
    #...
    def run(self, edit, new_name, event=None):
        pos = get_position(self.view, event)
        params = get_document_position(self.view, pos)

Also Tom, is there something you would like to change? :)

@predragnikolic predragnikolic changed the title Add rename text input handler #206 Add rename text input handler Jul 3, 2018
@coveralls
Copy link

coveralls commented Jul 3, 2018

Pull Request Test Coverage Report for Build 795

  • 0 of 24 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 17.158%

Changes Missing Coverage Covered Lines Changed/Added Lines %
plugin/rename.py 0 24 0.0%
Totals Coverage Status
Change from base Build 789: 0.3%
Covered Lines: 466
Relevant Lines: 2716

💛 - Coveralls

Copy link
Contributor

@tomv564 tomv564 left a comment

Choose a reason for hiding this comment

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

Awesome work, great demo also!
I guess users need a recent-ish Sublime Text build (7 May 2018) - if it turns out this build is not available to a lot of our users then we could do a feature detection.

Copy link
Contributor

@tomv564 tomv564 left a comment

Choose a reason for hiding this comment

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

The event parameter is provided when the command is triggered by mouse click, for example when right-clicking on a symbol.

Did you test the rename from the context menu after your changes?

Also, I like to make it clear to the user when commands are not supported by LSP.
You added Rename to the panel so it will appear to work even when the server does not support it?

@rwols
Copy link
Member

rwols commented Jul 6, 2018

You could also choose to change the minimum required build number in your package control submission.

@predragnikolic
Copy link
Member Author

predragnikolic commented Jul 7, 2018

Thanks for explaining :)

Did you test the rename from the context menu after your changes?

Sorry, I didn't, but I fixed it.
And now renaming works in all three cases, when it is triggered through the:

  • keyboard shortcut
  • context menu
  • command palette

You added Rename to the panel so it will appear to work even when the server does not support it?

The command must be present in the command palette for this to work (see second paragraph), but Sublime won't show the command in the command palette if the is_enabled returns False, so the command will only be visible if the user can actually rename the symbol.

we could do a feature detection?

I will let you decide if you want a feature detection :)

Note: In the python language server, if I rename a symbol, and try to rename it again, it wont take affect. But if I rename the symbol, save the file, and then rename it again, it works.
In your typescript language server, everything works great, I can rename the symbol as much as I want, the above problem only affects pyls and that problem existed even before this PR.

@tomv564 tomv564 merged commit b771c47 into sublimelsp:master Jul 11, 2018
@predragnikolic predragnikolic deleted the add-new-rename-text-input-handler branch September 7, 2018 08:11
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.

4 participants