Add setting to save modified files after applying a refactoring

jwortmann commented 2 months ago

Closes #2371

Edit: the following info is outdated, see https://github.com/sublimelsp/LSP/pull/2433#issuecomment-2016549517 for the updated version of this PR.

This would allow to add a key binding for the rename command like

[
    {
        "keys": ["f2"],
        "command": "lsp_symbol_rename",
        "args": {"preserve_tabs": true},
        "context": [{"key": "lsp.session_with_capability", "operand": "renameProvider"}]
    },
]

and with this new "preserve_tabs" argument enabled, after applying the edits it will automatically

save affected files which were already open in the window, but didn't have unsaved changes before the rename modifications
save and close files which were not already open in the window
don't save affected files if they already had unsaved changes

Questions:

Should it be enabled by default? I think I'd prefer this to be opt-in, in order to keep the current behavior, and also enabling this argument has the disadvantage that modifications to files currently not open in the window could be unnoticed by the user.
Maybe there should be another option to keep the modified unopened files open as new tabs in the window.
Would a new setting be better, instead of the command argument? With the argument you can't easily modify the menu items in the main and context menu.
Name of the argument - my first ideas were "quiet" (hm, not really accurate) and "preserve_tab_states" (too verbose?)

netlify[bot] commented 2 months ago

Deploy Preview for sublime-lsp ready!

Name	Link
Latest commit	572638eb988f6f782c5625b98dc68a2727dbfdc8
Latest deploy log	https://app.netlify.com/sites/sublime-lsp/deploys/66243073b1c9850008e90e45
Deploy Preview	https://deploy-preview-2433--sublime-lsp.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

predragnikolic commented 2 months ago

@jwortmann thank you!

Let's jump in the feedback.

So I did a little research with how VS Code behaves.

I know that customization is good to have. I also see a benefit if one thing can work only in one way.

VS Code does keep it simple regarding the logic for renaming.

Here is how VS Code behaves when renaming symbols.

Let's say we have two files:

_| foo.ts |____________ # imagine these are tabs :) 
export const foo = 369 

_|bar.ts|_________________
import { foo } from '.foo'

CASE 1

If we have foo.ts open and rename foo to baz. VS code will preform the symbol rename in foo.ts and bar.ts and preform a file save of foo.ts and bar.ts. NOTE, VS didn't open the bar.ts file.

_| foo.ts |____________
export const baz = 369

CASE 2

If we have foo.ts and bar.ts open and rename foo to baz, VS code will preform the symbol rename in foo.ts and bar.ts and preform a file save of foo.ts and bar.ts.

_| foo.ts |_bar.ts_|_____
export const baz = 369

CASE 3

If we have foo.ts and bar.ts open and bar.ts has unsaved changes, when we rename foo to baz VS code will preform the symbol rename in foo.ts and bar.ts and preform a file save of foo.ts and bar.ts, although bar.ts had unsaved changes!

to illustrate... let's say we modified bar.ts and we didn't press save

_|_foo.ts_| bar.ts* |_____
import { foo } from '.foo'

+ // this was added, and we didn't save

If we go back to the foo.ts and preform a rename in foo.ts,

_| foo.ts |_bar.ts*_|_____
export const foo = 369

In VS code will end up with both files being saved.

_| foo.ts |_bar.ts_|_____
export const baz = 369

In all 3 cases, VS Code behaves the same.

I think that LSP for ST could behave the same, and avoid having arguments in the commands.

There are of course downsides. My biggest concern is a bug in pyls I had a couple of years ago, I have renamed the str type, and the LS did renamed all str on my whole laptop, in python core libraries and such... it wasn't nice :D LSP opened countless tabs. it took me 1h to close them all and to undo the changes. If we were to implemet logic to not open the modified files, I would not know that I accidental modified code in core libraries.

Luckily now, we do show a popup to that says "Rename 55 occurrences in 60 files.", so we are safe in some way... and we have a preview edits that you have improved in your last PR. So I think we kind guards against this... Also LS can prevent this in textDocument/prepareRename to throw "You can not rename this element"

All in all, to sum up: It is nice to have options, I would lean on how VS Code behaves for this one.

WDYT?

predragnikolic commented 2 months ago

If we implement as VS Code here would be the answers:

save affected files which were already open in the window, but didn't have unsaved changes before the rename modifications

We will save all files that have workspace edits, and don't care if the file had modifications or not.

save and close files which were not already open in the window

Yes, we would open, save and close the files that were not already open in the window.

don't save affected files if they already had unsaved changes

We would save the file, regardless if it had unsaved changes.

Should it be enabled by default? I think I'd prefer this to be opt-in, in order to keep the current behavior, and also enabling this argument has the disadvantage that modifications to files currently not open in the window could be unnoticed by the user.

We would only have one default behavior and no options....

files currently not open in the window could be unnoticed by the user.

1+ Maybe open files that are not part of the workspace? I would like no additional logic... but I do not know what is sane here.

Maybe there should be another option to keep the modified unopened files open as new tabs in the window.

If we implement as VS Code, we would not need such option.

Would a new setting be better, instead of the command argument? With the argument you can't easily modify the menu items in the main and context menu. Name of the argument - my first ideas were "quiet" (hm, not really accurate) and "preserve_tab_states" (too verbose?)

If we implement as VS Code, I do not think we would need a new setting.

jwortmann commented 2 months ago

Thanks for the feedback and testing how it works in VS Code!

So the only difference is case 3, where VS Code saves even the files with previously unsaved changes. I must say that from my point of view the behavior I implemented here is better and what I would expect as a user. If I had an unsaved file open, then I don't want the editor to save it by its own when I do a rename operation. For example sometimes I intentionally leave files with some experimental or temporary changes unsaved, so I can simply close the file (without saving) and reopen it, if it turns out that the changes were wrong and I'd like to revert them.

In all 3 cases, VS Code behaves the same.

Well, it depends on what exactly is meant with "behaves the same". VS Code behaves the same in the way that it always saves all files. But this implementation behaves the same in all 3 cases in the way that the original file state is always preserved. So both are consistent in their own way 😉

We would only have one default behavior and no options....

Hm... I could imagine that some users wouldn't like the new behavior, especially that unopened files are immediately closed again. So I would expect some complaints if this will be forced upon all users.

Maybe open files that are not part of the workspace?

Interesting idea. But I think in general there should never be files affected by a WorkspaceEdit (name speaks for itself) that are outside of the workspace. I would think that the case you experienced where str was renamed also in other core libraries etc. must have been a bug of the language server. So I don't think that this is really necessary.

jwortmann commented 2 months ago

I noticed that VS Code has this setting:

  // Controls if files that were part of a refactoring are saved automatically
  "files.refactoring.autoSave": true,

If set to false it doesn't save automatically, and previously unopened files are kept open (unsaved) in the editor, similar to what we currently do.

Something else I noticed is that with this PR, unopened files will be visible in the tab bar for a very short amount of time, before they are immediately closed again. I assume this happens because window.open_file() used with the promise implementation runs async and doesn't block the UI rendering. Of course this is done intentionally, but in this case VS Code behaves a bit nicer without the short tab flickering. But it's probably not a big drawback, and I guess we don't want to create a blocking wrapper around open_file for this. However, another aspect I noticed, and which seems wrong to me, is that for these files (when I tested it) we send didChange either after didClose or not at all. I'm unsure if this could cause problems.

LDAP commented 2 months ago

+1 for the behavior of VS Code (even as new default). It happens quite often that I miss to save all other tabs. Though, I can imagine some users like the current behavior, so maybe we can keep it with a setting similar to the autosave setting in VS Code.

jwortmann commented 2 months ago

Then I would probably add a new setting, but with multiple options:

// Controls if files that were part of a refactoring are saved automatically:
// "always" - save all affected files
// "preserve" - only save files that didn't have any unsaved changes beforehand
// "preserve_opened" - only save files that didn't have any unsaved changes beforehand
//                     and were already open in the editor
// "never" - never save files automatically
"refactoring_auto_save": "never",

This would apply to all workspace edits, i.e. also to code actions and to manually triggered commands. So if you choose "always", it means that your file is saved automatically after applying a code action, even if it only affects the active file. Perhaps we should pass the code action kind further to the function and only save if the kind is "refactor" (I think this is what VS Code does).

predragnikolic commented 4 weeks ago

Thanks, this is nice

sublimelsp / LSP