Support preview a WorkspaceEdit

[testforstephen]

For many refactoring actions (e.g. Move Java file, Rename Java class/package name), it possibly involves updating multiple files, which is especially true for refactoring some big projects. If the VS Code supports preview the changes of a refactoring WorkspaceEdit before applying, this will help the user discover the potential refactor effects in advance and decide whether to continue the operation.

Generally a WorkspaceEdit contains text edits (Insert, Delete) and resource edits (CreateFile, DeleteFile, RenameFile), they are naturally diffs, and very suitable for preview.

[jrieken]

I have merged #88428 which implements a first version of this: 👇

What's implemented

• Refactor preview shows all changes - currently only grouped by file
• Show diff view for each change
• Check/Uncheck individual changes or all changes per file
• Detect concurrent file changes
• Restore previous view state when accepting/discarding changes

What's still missing and/or unclear

• There is no API yet to allow extension to suggest that a preview is necessary, for now the rename box allows the user to chose
• This is currently only enabled for rename as the UX flow for code actions isn't clear yet
• Changes are only grouped by files, e.g there is no support to group by change type like "change in a comment" etc. This will also require new API.
• Changes are "checked" by default. This will require new API
• Show a summary like "Changes for renaming 'foo'", likely also requires new API

[jrieken]

/cc @bobbrow

Wrt to knowing if a preview for rename is needed or not: Does C/CPP know this during the prepareRename-call? We could extend its return-type so that it can signal if a preview is required or optional

[bobbrow]

Very cool. @Colengms should be able to help you answer your question. I don't see an implementation of prepareRename currently.

[Colengms]

Hi @jrieken . Do you mean provideRenameEdits? My understanding is that prepareRename is optional, and used to validate whether the target position of the rename is valid.

Currently, we are using only provideRenameEdits. Could you clarify why we might want to use prepareRename as well?

In our current implementation, if all of the matches found are known to be associated with the selected symbol, we return results from provideRenameEdits immediately. Otherwise, we present UI to allow the user to categorically and individually select what to rename (to opt into things such as matches in comments), and reissue the rename in order to complete it with the selected results.

Are you suggesting we leverage preview instead of our own UI, and make preview required in the scenarios in which we would otherwise have presented our own UI? If so, yes, we would need a way to return that preview is required, but wouldn't that be when returning from provideRenameEdits, as we would discover whether preview is required in the process of scanning for all candidate locations?

[jrieken]

Are you suggesting we leverage preview instead of our own UI, and make preview required in the scenarios in which we would otherwise have presented our own UI? If so, yes, we would need a way to return that preview is required, but wouldn't that be when returning from provideRenameEdits, as we would discover whether preview is required in the process of scanning for all candidate locations?

Yes - the plan is to have one UX that works across different language extensions and across different code actions like rename but also for things like "extract function" etc.

For this and to provide a honest rename input box we need to know upfront if a rename needs a preview or not. That's why I am bringing up prepareRenameEdits. We call that before showing the UX and with some extra information returned by it we can tell the user that a preview will be showing when renaming an element.

[akaroml]

@jdneo will try this on Java projects and provide some feedback.

[jdneo]

Some findings:

1. Depend on the workspaceEdit returned from the Language Server, sometimes the preview panel may contains several changes in one line
   

If the user's screen is not wide enough, some part of the changes cannot be seen in the Refactor Preview panel. But maybe this should be the language server's task to split the workspaceEdit instead of merging them into one? Or maybe VS Code can split the diff according to different lines?

2. If the user just close the preview editor instead of clicking the Discard changes button. He cannot rename from the preview panel anymore
   

[jrieken]

Depend on the workspaceEdit returned from the Language Server, sometimes the preview panel may contains several changes in one line

Yeah, that the design. The list will show each change as computed by the language service. Tbh, I wouldn't make the language service make the change be optimised for the list view as we expect most users to open the diff editor in such cases

If the user just close the preview editor instead of clicking the Discard changes button. He cannot rename from the preview panel anymore

Good find. I will push a fix for that. Thanks

[jrieken]

api ideas

interface WorkspaceEditMetadata {
	label: string
	icon: ThemeIcon | {light: Uri, dark: Uri};
	check: boolean;
}

export class WorkspaceEdit {
	appendTextEdit(uri: Uri, edit: TextEdit, metadata?: WorkspaceEditMetadata):void;
}

• Use metadata#label for grouping, e.g 'changes in strings' or 'inactive'
• Trigger preview when one metadata-object is check = false

[jrieken]

For the next stable release:

• add setting to enable/disable preview per language

[jdneo]

api ideas

interface WorkspaceEditMetadata {
	label: string
	icon: ThemeIcon | {light: Uri, dark: Uri};
	check: boolean;
}

export class WorkspaceEdit {
	appendTextEdit(uri: Uri, edit: TextEdit, metadata?: WorkspaceEditMetadata):void;
}

• Use metadata#label for grouping, e.g 'changes in strings' or 'inactive'
• Trigger preview when one metadata-object is check = false

Hi @jrieken, Could you explain more about this? How will these APIs affect the preview?

[akaroml]

I'm specifically interested in the role of the check field in the WorkspaceEditMetadata. Thanks.

[jrieken]

Sure, these were merely meeting notes and not really fleshed out yet...

We are working on supporting scenarios in which an extension can "classify" changes which won't be applied by default. Cpp already does that (with some custom UX) - the screen capture below shows how there are two candidate changes: one inside a string, one inside an inactive ifdef. The language service can suggest them but has low confidence and therefore human approval is needed.

The api sketch above is to support this scenario, e.g check: boolean would set the state of the checkboxes that we are rendering. The label and icon property can then be rendered inline and/or drive a tree, e.g all changes in strings can become a tree-node all changes in ifdefs become another node etc.

Also, when a change is unchecked then that would be a signal for us to force show the preview. Otherwise we are likely making this depended on a user setting, e.g I have added editor.rename.enablePreview so that user can express their preference.

[jrieken]

@bobbrow fyi - the setting to enable/disable preview is now called editor.rename.enablePreview

[bobbrow]

Thanks @jrieken! For the setting, I read this description in your commit:
Enabe/disable the ability to preview changes before renaming. (also: typo with "Enable")

If it's disabled, does that mean you won't show the preview even if the language server requests it? I'm thinking about what happens after we migrate to use this new feature and remove our language setting and our custom implementation. If the developer disables the preview on their own, will you ever show it? My guess is that vscode could still override the setting if the language server sends a metadata object with check = false. Then the setting will only apply when "regular" WorkspaceEdits are sent in reponse to a rename request?

[jrieken]

f the developer disables the preview on their own, will you ever show it? My guess is that vscode could still override the setting if the language server sends a metadata object with check = false.

Yeah, that's that plan. The setting is your preferred behaviour but if via API uncertainty is expressed (internally I have now called the property needsConfirmation) then we will always show the preview

[jrieken]

@Colengms @jdneo We are planning to finalise the WorkspaceEditMetadata-API for this milestone and it would boost our confidence if you can give some dry-run:

vscode/src/vs/vscode.proposed.d.ts

Lines 1414 to 1452 in 760cb13

	/**
	* Additional data for entries of a workspace edit. Supports to label entries and marks entries
	* as needing confirmation by the user. The editor groups edits with equal labels into tree nodes,
	* for instance all edits labelled with "Changes in Strings" would be a tree node.
	*/
	export interface WorkspaceEditMetadata {
	/**
	* A flag which indicates that user confirmation is needed.
	*/
	needsConfirmation: boolean;
	/**
	* A human-readable string which is rendered prominent.
	*/
	label: string;
	/**
	* A human-readable string which is rendered less prominent in the same line.
	*/
	description?: string;
	/**
	* The icon path or [ThemeIcon](#ThemeIcon) for the edit.
	*/
	iconPath?: Uri | { light: Uri; dark: Uri } | ThemeIcon;
	}
	export interface WorkspaceEdit {
	insert(uri: Uri, position: Position, newText: string, metadata?: WorkspaceEditMetadata): void;
	delete(uri: Uri, range: Range, metadata?: WorkspaceEditMetadata): void;
	replace(uri: Uri, range: Range, newText: string, metadata?: WorkspaceEditMetadata): void;
	createFile(uri: Uri, options?: { overwrite?: boolean, ignoreIfExists?: boolean }, metadata?: WorkspaceEditMetadata): void;
	deleteFile(uri: Uri, options?: { recursive?: boolean, ignoreIfNotExists?: boolean }, metadata?: WorkspaceEditMetadata): void;
	renameFile(oldUri: Uri, newUri: Uri, options?: { overwrite?: boolean, ignoreIfExists?: boolean }, metadata?: WorkspaceEditMetadata): void;
	}

For some tyre-kicking you can use my sample extension that has served us during testing: #89335 (comment)

[Colengms]

Hi @jrieken . I tried it out again with our rename provider. I still have the following feedback:

• The icon we specified is not used. We are specifying { light, dark } and using relative paths. The same works properly with a TreeDataProvider.

• Should groups be sorted? It's unclear what the current sort order is, when sorting by group. We'd like to put our 'confirmed' references at the top of the list. Perhaps we could get by with alphabetic sorting by prefixing our 'confirmed' label with a space.

• Should the group (based on label) have a checkbox? It might be useful to enable/disable entire categories of changes. Currently, if the user knows they want to rename all occurrences, they need to check each file individually (there could be many).

[jrieken]

The icon we specified is not used. We are specifying { light, dark } and using relative paths. The same works properly with a TreeDataProvider.

Understood - I went with the usage of QuickInputButton#iconPath which only supports URI. You should have seen a compile error when passing light, dark as string only... The challenge is that the workspace-edit doesn't get "pulled" and therefore it's harder to massage the path to become extension-absolute uris.

Should groups be sorted? It's unclear what the current sort order is, when sorting by group. We'd like to put our 'confirmed' references at the top of the list.

👍 That was an oversight -> #90664

Should the group (based on label) have a checkbox? It might be useful to enable/disable entire categories of changes. Currently, if the user knows they want to rename all occurrences, they need to check each file individually (there could be many).

We discussed that in UX call and was some push-back on having too many check boxes but I do agree with the use-case. That's also something we can easily tweak after the fact.

Overall, for the API proposal I see a thumbs up, right?

[jrieken]

Suggestion to rename WorkspaceEditMetadata to WorkspaceEditEntryMetadata

[Colengms]

The icon we specified is not used.

I went with the usage of QuickInputButton#iconPath which only supports URI.

@jrieken . Uri's don't seem to be working for me either. The file referred to in the following image is present on disk:

When this happens, I see the following in the console log output (not sure if related) :

I see this error recur as I scroll through the refactor preview list.

Otherwise, the API is looking good to me.

[jrieken]

Understood - I missed the single-icon case, had only the {light, dark}-case implemented... This should finally be working

[jrieken]

To verify:

• make sure the API is in vscode.d.ts and works without enabling proposed API
• make sure refactor preview works