stride3d / stride

Stride Game Engine (formerly Xenko)
https://stride3d.net
MIT License
6.45k stars 933 forks source link

UNDO script asset create can lose data if file is unsaved #681

Open jeske opened 4 years ago

jeske commented 4 years ago

Release Type: official 4.0.0.1 beta 2 0926 Version: official 4.0.0.1 beta 2 0926 Platform(s): Windows

Describe the bug

Xenko UNDO has separate contexts for the project assets, and each text-editor, which leads to non-intuitive behavior when trying to use the built-in editor... If focus is accidentally not in the editor, pressing undo can remove the entire file if the last action was to add the script, which is non-intuitive and dangerous if the file is not saved.

To Reproduce Steps to reproduce the behavior:

1) add a script 2) edit the contents of the script (and dont save) 3) click in the solution explorer 4) press undo (ctrl-Z) 5) WITNESS: it removes the script from the project, blowing away edits

Expected behavior

EXPECTED: Xenko to show a warning before removing an unsaved script file asset.

Additional context

Pretty much other editor environment I use (such as visual studio) has a single global UNDO stack.. which means if you press undo, it will remove the last action you did globally in the application, regardless of your focus context. It's very very non-intuitive that in Xenko, if you change the focus and undo, it will undo different things.

In itself, this is not necessarily a problem.. but if you can accidentlaly undo a scrip asset create, that causes the removal of a script asset, without a warning dialog.. that's a problem.

Kryptos-FR commented 4 years ago

Incorrect. The undo/redo stack is global. So modifying a scene, a property or an asset is all part of the same stack.

The only exception is the built-in text editor that you used for code which has its own undo/redo capability that is separated from the global editor one. That is because it is built upon a third-party component. Making those two work together would be very challenging if not impossible. We could also disable the undo/redo feature in the text editor itself, but that would be impractical as well as unexpected.

Also, in all text editors, the undo/redo is local to the current document, so if you have different documents, they all have their own undo/redo text stack.

With all that said, the built-in editor is provided as convenience and it is recommended to edit scripts in an external code editor such as VS Code or Visual Studio.

Finally, your changes are not lost, since you can redo after accidentally deleting the script with the undo and it will come back in its last state (eventually containing the unsaved changes). At that point,, it will prompt you to either restore the script to its last saved state or to its unsaved state.

You can track the list of undo/redo actions under the "edit history" tab usually located on the bottom right corner.

jeske commented 4 years ago

If the editor has it's own undo stack, then the undo stack is not global. By definition. Since if it was global, there would be only one.

I think 90% of this problem could be avoided by just making a warning dialog for undo that will remove an asset that could result in loss of data (like an unsaved script file, because when it disappears one can't save it anymore). As long as the file is still on disk, and is fully saved, then one wouldn't really lose anything.

I see your point about redo.. but in that moment where one is expecting it to undo a keystroke and Xenko eats the whole file and throws it in the trash.. it's more a moment of panic and there is no thought of redo. The thought is "omg why am i using software that will eat my data"

Kryptos-FR commented 4 years ago

I think our current solution is consistent with what any other application is doing. It would be unexpected to have a different behavior. And we don't want to have popups all over the place.

As far as I am concerned, nothing need to be changed, but I'll let others give their opinion on that matter.

sebllll commented 4 years ago

i have also seen my scriptfiles going to nirvana in some cases. a popup before deleting something permanently wouldn't be too much of a clutter in my opinion.

jeske commented 4 years ago

@Kryptos-FR Neither Unity nor Visual Studio will remove an edited script with undo. Read what I wrote below and see if you still feel other applications work this way, and if so, please list a specific application so we can see what you mean.

Visual Studio undo will not remove project level code file additions, ever. You have to delete them explicitly.

Unity's project undo will only undo an asset addition before you finish naming it. Once it has a name, undo will not remove it. Let me describe exactly how Unity works... The moment you add an asset, it highlights the name for you to give it one. While the name is still highlighted, you can hit undo to remove the asset. However, once you finish giving it a name, pressing undo will not remove the asset anymore. You have to delete it. Unity also launches an external editor for text-edits, so it doesn't have this specific issue with confusion over text-edit undo and other undos, but even if it did, the above behavior would stop it from accidentally deleting an asset with undo.

Curiously, Blender's behavior with the text editor vs project level undo does work the way Xenko works. Though I've never noticed this before testing it just now, because it's very uncommon for Blender users to use the text editor. If it was more common, I think this would be an important problem to fix.

Here are some ideas about how to translate this to Xenko: