Add prompt to save scripts on quit [4.0]

[ator-dev]

Fix #55026

[Paulb23]

Seems okay, couple comments inline, though I do wonder about ordering, whether we should save the scenes first, then the scripts.

Also whether we need to do something similar for the shader editor.

[ator-dev]

whether we should save the scenes first, then the scripts

I think it makes the most sense to save scripts before scenes, partly because it seems intuitive to me to save the 'resources' before the scenes which utilise them, but mainly due to the option to save each scene individually. If you're not paying much attention (e.g. simply pressing enter for every 'save scene' dialog) you could just accidentally save all scripts without even knowing what the unsaved scripts were. I'm happy to discuss this further though.

something similar for the shader editor

Good point - I think this is a good idea, but should I implement it in a future PR instead? I'd like to keep this one fairly small, especially since I'm now making some wider changes to allow script discarding.

I believe your suggestion goes hand in hand with handling of built-in scripts, which are slightly weird because saving them necessarily saves their scene, and vice-versa. When I make a PR for this I might simply add text to warn the user of this, or maybe implement something to mitigate this effect.

[Paulb23]

I don't think there's necessarily a right order as it could be argued that changing a scene involves it's resources. I was thinking here if we could avoid additional popups. Though thinking about it, that would only apply to built-in scripts?

[Paulb23]

Do we need to call this at all here, as when the editor exits it's called automatically?

[ator-dev]

Good point, thank you - I forgot about that, I'll test it with the line removed.

[ator-dev]

I was a bit vague by saying 'resources' - I meant specifically external scripts, and in my local version built-in scripts are included in the popup with a message informing the user that they will be saved when their scenes are.

I'm not sure what you mean about avoiding additional popups - I only add one popup asking to save all external scripts and listing them, and I don't think this can (or should) be avoided. Could you clarify this for me please?

[ator-dev]

By the way, I've been having a lot of trouble discarding scripts, which is why I haven't updated this PR with my updated version yet.

Part of my message to the contributors chat on the subject:

I've been trying for over a week to make a method to discard a script (without closing the tab), for use in my save-or-discard-scripts prompt. This has turned out to be incredibly difficult; I've tried:

• Set text_editor's text to text_file's text
  Problem: the text_file resource is updated with the unsaved text whenever you switch away from its tab, so only works for currently open tab.

• Unref the script resource, then set_edited_resource to one newly loaded (current approach)
  Problem: again the cached one is retrieved, which contains the unsaved text and not the saved version.

• Destroy the tab, then recreate and put in the same place with the same history
  Problem: horrible and brittle whichever way you approach it.

• A bunch of variations of the above and other brief ideas which turned out to be infeasible.

... is it possible to load a resource without retrieving it from the cache? I'm using ResourceLoader::load() but it doesn't seem to have an option not to do that.

@KoBeWi helped me out with ResourceLoader, but none of the other options worked and it seems this PR is related: #63224 (thanks to @dzil123)

[ator-dev]

Start of another conversation relating to usability of the popup: https://chat.godotengine.org/channel/devel?msg=BABJmMxA8mX5sgFEu

[ator-dev]

Current state (Don't Save still not functional):

[Paulb23]

For popups I was thinking if we save the scene first that will in turn save the scrips, so we could avoid having this popup appear under that use case.

For GDScript looks like we are dependent on #63224 or do you mean TextFiles and C# are also not working?

As for usability we have this discussion from a while back #21959

[ator-dev]

For popups I was thinking if we save the scene first that will in turn save the scrips, so we could avoid having this popup appear under that use case.

Isn't the purpose of this to show the user which script changes will be saved so that they can change their mind? For example, they might have temporarily changed or deleted some code and forgotten about it. Do you think it's more important that scripts are simply always saved on quit (i.e. when scenes are or with a dedicated prompt otherwise)? Not criticising, I think I just misunderstood your intention.

do you mean TextFiles and C# are also not working?

I added some behaviour for TextFiles, but haven't tested with either them or C# yet. At the moment I'm focusing on GDScript which I can't seem to discard; I agree we're dependent on that PR although a workaround could be added, like the 'do not save' flag I used before.

That usability discussion is interesting, I think it's a shame it was never picked up. Maybe some broader work is needed which would integrate improved script-saving behaviour with a more comprehensive scene-saving UI?

[Paulb23]

I guess it would depend if you see scenes as entirely separate entities to scripts. As a change in a script could be considered a change in the scene. Unless the script is not part of any scene. In which case it would be shown as a popup here. But I released that would only really apply to built-in scripts. So this approach does make more sense for the common usecase.

Personally not a fan of permanent temporary workarounds, would much rather wait for that other bug to get fixed first.

Yeah to get usability right would probably require some broader development in some way, shape, or form. I think for now what we have is good enough. Could chuck it in an ItemList if we're worried about users having that many unsaved scripts at a single time.

[ator-dev]

That's what I was thinking of, yeah - to me external scripts being saved when scenes are (which are entirely separate files that have references to scripts) is confusing to the user and could result in data loss if they messed around with their scripts in testing.

I agree, I don't want to add 'temporary' statefulness that will probably interact weirdly with other things and confuse contributors. The PR can probably wait until scripts have a nice way of being discarded.

I might look into using an ItemList - it's probably not a big deal but could improve readability, especially for large numbers of scripts. Definitely the current development is 'good enough' though and provides a good philosophy on which to build future work.

[KoBeWi]

I'm thinking about implementing a generic solution for all editor plugins:
godotengine/godot-proposals#2153 (comment)
It would handle scripts, shaders and everything.

[akien-mga]

Superseded by #67503.