Enhancements for the rename panel #2428
Conversation
I am using OpenBox, |
Thanks, then it seems to work as expected. I've also considered to remove the dialog entirely and always show the preview panel, because then it's still only 1 button click to apply the changes. But in the dialog, the "Replace" button is preselected (at least on Windows), so you only need to hit Enter. And this is not possible in the preview panel, so I rejected this idea.
Yes, it was basically meant as a note that there is the possibility of the preview not being 100% accurate. But it's probably not so relevant in practice. To fix it, I think we would need to iterate through each character in the string to check if it's not a single UTF-16 character (like an emoji), but I don't think that would be worth it just for the preview. We could maybe reconsider if users start to complain. Update: it's fixed now. |
Maybe track the relevant view.change_count()? |
Hm... but here the change count of all affected views is relevant, i.e. if the content of any of the views changes, we want to reject the whole rename operation alltogether, and not only ignore the edits for the changed view(s). This is probably not so trivial to implement. I've tried to add the version information to each of the documents in the WorkspaceEdit like this expanddiff --git a/plugin/core/edit.py b/plugin/core/edit.py
index 48e509df..2633f5af 100644
--- a/plugin/core/edit.py
+++ b/plugin/core/edit.py
@@ -4,13 +4,14 @@ from .protocol import TextEdit
from .protocol import UINT_MAX
from .protocol import WorkspaceEdit
from .typing import List, Dict, Optional, Tuple
+from .url import parse_uri
import sublime
WorkspaceChanges = Dict[str, Tuple[List[TextEdit], Optional[int]]]
-def parse_workspace_edit(workspace_edit: WorkspaceEdit) -> WorkspaceChanges:
+def parse_workspace_edit(workspace_edit: WorkspaceEdit, window: Optional[sublime.Window] = None) -> WorkspaceChanges:
changes = {} # type: WorkspaceChanges
document_changes = workspace_edit.get('documentChanges')
if isinstance(document_changes, list):
@@ -22,16 +23,29 @@ def parse_workspace_edit(workspace_edit: WorkspaceEdit) -> WorkspaceChanges:
text_document = document_change["textDocument"]
uri = text_document['uri']
version = text_document.get('version')
+ if window and version is None:
+ version = _current_view_version(window, uri)
edits = document_change.get('edits')
changes.setdefault(uri, ([], version))[0].extend(edits)
else:
raw_changes = workspace_edit.get('changes')
if isinstance(raw_changes, dict):
for uri, edits in raw_changes.items():
- changes[uri] = (edits, None)
+ version = None
+ if window:
+ version = _current_view_version(window, uri)
+ changes[uri] = (edits, version)
return changes
+def _current_view_version(window: sublime.Window, uri: str) -> Optional[int]:
+ scheme, filepath = parse_uri(uri)
+ if scheme == 'file':
+ view = window.find_open_file(filepath)
+ return view.change_count() if view else 0
+ return None
+
+
def parse_range(range: Position) -> Tuple[int, int]:
return range['line'], min(UINT_MAX, range['character'])
diff --git a/plugin/rename.py b/plugin/rename.py
index d50f9d26..16822a9f 100644
--- a/plugin/rename.py
+++ b/plugin/rename.py
@@ -175,7 +175,7 @@ class LspSymbolRenameCommand(LspTextCommand):
def _on_rename_result_async(self, session: Session, response: Optional[WorkspaceEdit]) -> None:
if not response:
return session.window.status_message('Nothing to rename')
- changes = parse_workspace_edit(response)
+ changes = parse_workspace_edit(response, self.view.window())
file_count = len(changes.keys())
if file_count == 1:
session.apply_parsed_workspace_edits(changes)but it's useless because it will still apply the edits to the views where the version is matching. This means that the code will be broken, because only some of the symbols were renamed. So this looks like it would need a few bigger changes/refactorings and is probably better suitable for another PR. And to be fair, the same problem is also present in the current code, because the dialog is not a modal dialog (at least on Windows). If you make changes to an affected view before clicking on "Replace" in the dialog, you still end up with messed up edits. |
|
@rchl I wonder if you want to take a look too / have any opinions about it, or can we merge this? I have a few other ideas and refactorings on top of this PR in mind (e.g. convert |
|
Hey. Would like to check it out but I'm busy with life at the moment. Will try checking it out soon. |
|
No problem, we can leave the PR open for a while 👍 |
✅ Deploy Preview for sublime-lsp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| def is_range_response(result: PrepareRenameResult) -> TypeGuard[Range]: | ||
| return 'start' in result | ||
|
|
||
|
|
||
| def utf16_to_code_points(s: str, col: int) -> int: |
There was a problem hiding this comment.
maybe the naming could be a bit more clear that it's about position like utf_16_pos_to_code_point_pos?
There was a problem hiding this comment.
I don't really feel utf_16_pos_to_code_point_pos. Could we keep the current name? I have reworded the docstring a little bit, and I think it's clear from the docstring and from usage what the function does.
Btw, the parameter is named col because that's the name given in the View.text_point_utf16 method.
There was a problem hiding this comment.
The name to me strongly suggests something that feels like charset conversion which I think is kinda confusing.
Can we do utf16_offset_to_code_point_offset? Or if we want to match ST more then utf16_point_to_code_point?
There was a problem hiding this comment.
I'm willing to give up if you insist. I'm after all nit picking a bit.
There was a problem hiding this comment.
Well, I'd say "code points" already implies that it's about the position, because there is no character encoding named "code points". If we want to have the word "position" in the name, then I would suggest utf16_to_utf32_position.
If we want to mach ST closely, then it should probably be code_point_utf16 or even the same name text_point_utf16 (but the latter would be confusing I guess). The only difference between the builtin and this function here is that the builtin counts from the beginning of the view's content, while this one takes the string as an explicit parameter, and it always uses clamp=True.
So I would prefer one of these:
utf16_to_code_points(current)utf16_to_utf32_positioncode_point_utf16
There was a problem hiding this comment.
kinda like utf16_to_utf32_position but choose yourself whether to rename it or not later
Currently the "Dry Run" functionality for a multi-file rename operation is a bit awkward to use, because after you have looked at the rename locations which are shown in the panel, you need to run the rename command again, including typing of the new name, to actually do the rename. This PR adds "Apply" and "Discard" buttons into the panel to save a bit of time. In theory there could be wrong changes applied if you edit the view(s) first, before clicking the "Apply" button (because the document version is not mandatory in the LSP edits), but in that case I'd consider it to be the user's fault (or does anyone have an idea how to prevent it?). The buttons will automatically hide after they are clicked.
Changes are now displayed in the panel using Sublime's inline diff feature. Perhaps it could be reused later to add a preview for other "refactor" code actions.
Small drawback: the diff rendering requires more vertical space (an empty line after each line is needed because otherwise ST diff rendering would combine the lines into a single block). But is it really a drawback? I think it's okay and the diff rendering is useful because it shows what will change on the line (admittedly the changes are obvious for a simple rename, but still...)
I've also adjusted the dialog labels:
Before:
After:
Could someone on Mac/Linux confirm that passing the
titleparameter doesn't throw an error - I assume that it is simply ignored on Mac/Linux (?)