Improve or remove file saving on F5 in the editor

[soliton-]

Describe the desired feature

I don't see how it's any useful and it has weird behavior of saving all open scenarios/maps under unexpected paths.

https://github.com/wesnoth/wesnoth/blob/e67a91caf22429c56cd2012e324cd8ee212b5481/src/editor/controller/editor_controller.cpp#L804-L808

https://github.com/wesnoth/wesnoth/blob/e67a91caf22429c56cd2012e324cd8ee212b5481/src/editor/map/context_manager.cpp#L807-L832

[doofus-01]

Is this the "F5" reload feature? That's very useful for terrain graphics development, please don't remove that.

[soliton-]

Yes, but in the editor not from the title screen.

If it makes sense in the editor as well then we can certainly keep it. Do you know what the map/scenario saving is for?

[babaissarkar]

The F5 is also useful for testing custom Time Schedules.

[doofus-01]

Right, it's useful in the editor, not just the title sceen. I'm not sure if the file-saving was well thought out, but even that can be useful to quickly reload a test map.

[babaissarkar]

I'm not sure if the file-saving was well thought out, but even that can be useful to quickly reload a test map.

A possible solution is showing the user a Save As dialog for unsaved maps instead of saving to an autogenerated path that the user will have trouble guessing. Saving is useful, otherwise when reloading the user might lose their changes.

[CelticMinstrel]

I don't think it should save just because you pressed F5. If the reload could lose your changes though, then that's another bug. Ideally (and this might be quite tricky) a failure while reloading should revert the game config to its pre-reload-attempt state.

[doofus-01]

I've certainly been in situations where I needed to rapid-fire F5-reload in the editor (usually because I was empirically bumbling toward some terrain-graphics layering or layout setting), and an extra pop-up save dialog would have been pretty annoying.

If there's a good reason to remove it, I won't resign in protest, but the auto-save-on-F5 feature isn't a useless bug. If there is a way to preserve it, I would appreciate it.

Thanks

[babaissarkar]

Changes in 3164ce8984ba280064dc4fb466d804941d921aab: Removes usage of editor_default_dir preference. Temporary maps/scenarios are now saved inside filesystem::get_legacy_editor_dir() (that is, USERDATA/editor), under maps or scenarios folder as applicable. The filename is now tmp_scenario_i or tmp_map_i instead of window_i.

Everything works as it previously did, that is, you can do whatever modification you want to do and press F5 to reload it. No annoying dialogs in the middle.

@Pentarctagon I removed the usage of the preference, so should be possible to remove it now. @soliton- @doofus-01 Are these changes acceptable?

[soliton-]

I've certainly been in situations where I needed to rapid-fire F5-reload in the editor (usually because I was empirically bumbling toward some terrain-graphics layering or layout setting), and an extra pop-up save dialog would have been pretty annoying.

There would be at most a save dialog popup once. None if you load a map with a filename.

If there's a good reason to remove it, I won't resign in protest, but the auto-save-on-F5 feature isn't a useless bug. If there is a way to preserve it, I would appreciate it.

Can you please explain what you would want it preserved for? There is no issue with saving an existing map/scenario should that be necessary but what's the point of saving all windows regardless under a new name?

[soliton-]

Changes in 3164ce8: Removes usage of editor_default_dir preference. Temporary maps/scenarios are now saved inside filesystem::get_legacy_editor_dir() (that is, USERDATA/editor), under maps or scenarios folder as applicable. The filename is now tmp_scenario_i or tmp_map_i instead of window_i.

Everything works as it previously did, that is, you can do whatever modification you want to do and press F5 to reload it. No annoying dialogs in the middle.

That makes a bit more sense but I wouldn't save windows with maps/scenarios that already have a proper name under a different name. At most save new maps without a name if that is necessary.

The other thing is how is the user supposed to know about this? Those automatically saved maps/scenarios are not automatically loaded anywhere.

[babaissarkar]

That makes a bit more sense but I wouldn't save windows with maps/scenarios that already have a proper name under a different name. At most save new maps without a name if that is necessary.

Only new maps that are not already saved anywhere are saved in the temporary file. If it has a valid save location anywhere, it gets saved to there instead. That's the purpose of this check: (the second one that checks name, name is actually the place where it is saved.) I think I should change name to save_path or something. https://github.com/wesnoth/wesnoth/blob/e67a91caf22429c56cd2012e324cd8ee212b5481/src/editor/map/context_manager.cpp#L813-L815

The other thing is how is the user supposed to know about this? Those automatically saved maps/scenarios are not automatically loaded anywhere.

Maybe we can show the file's path in the window titlebar (it currently shows the filename only)? (Ideally, if a statusbar gets added someday, we might show a message there)

[soliton-]

Only new maps that are not already saved anywhere are saved in the temporary file. If it has a valid save location anywhere, it gets saved to there instead. That's the purpose of this check: (the second one that checks name, name is actually the place where it is saved.) I think I should change name to save_path or something.

Ok, I missed that. A better variable name would be nice.

Maybe we can show the file's path in the window titlebar (it currently shows the filename only)? (Ideally, if a statusbar gets added someday, we might show a message there)

So that they can see the names the unnamed maps/scenarios got? Not sure how helpful that is. If we expect that all windows stay open why do we save at all?

[babaissarkar]

So that they can see the names the unnamed maps/scenarios got? Not sure how helpful that is. If we expect that all windows stay open why do we save at all?

Any better ideas?

The other thing is how is the user supposed to know about this? Those automatically saved maps/scenarios are not automatically loaded anywhere.

I though you wanted the user to know the location where the temporary files are beings saved. Can you explain it in more detail? Maybe I misunderstood something.

Ok, I missed that. A better variable name would be nice.

Updated the variable name. (aa2b04a5cceafc4044b888ff3e8273ed38035c15)

[soliton-]

The simplest thing would be to not save at all. It seems like no one knows why the saving is done in the first place so best would be to clear that up first.

So I tried reloading with a syntax error. That is certainly not handled well. I get the syntax error presented twice and then wesnoth exits. So if we do not improve that then this saving makes sense.

Plain maps are saved in the editor dir and for scenarios the .cfg file is also saved there and the .map in the maps dir of the selected addon.

The chance that a user figures this out to find their lost maps/scenarios is quite low though. Ideally on start of the editor it would at least show a notice when it finds files of the form window_<number>... in the editor dir.

To recap:

• Ideally no termination on errors loading WML simply don't replace the currently loaded WML and remove this saving code.
• Second best keep the saving and notify the user on next editor (or even wesnoth) start where to find the files.

[babaissarkar]

The simplest thing would be to not save at all. It seems like no one knows why the saving is done in the first place so best would be to clear that up first.

Unsaved files are saved to a temporary file, then the map/scenario gets loaded from those files. Reloading those without saving will be very tricky, as much of the responsible code is written assuming those are loaded from a file (and not from memory). I'd much prefer sticking with the simpler solution of loading them from a cached file.

• Ideally no termination on errors loading WML simply don't replace the currently loaded WML and remove this saving code.
• Second best keep the saving and notify the user on next editor (or even wesnoth) start where to find the files.

I think those are doable. To paraphrase, if there are temporary cached files, the editor can show a notice and then the user decides whether to load those or not. Also, while reloading, if there is a syntax error, then the changes are simply lost and not loaded.

Plain maps are saved in the editor dir and for scenarios the .cfg file is also saved there and the .map in the maps dir of the selected addon.

I assume that's on master branch, and not #8903.

[soliton-]

Using actual temp files that are just an implementation detail, handled transparently to the user would be a valid solution IMO. Regardless if wesnoth might terminate on reload or not. Just if wesnoth can terminate on reload then it needs a notice.

Yes, I'm talking about current mainline code not any PR.

[Pentarctagon]

I'm not sure I follow what the issue is. Is it that it saves these temp files out at all, or is it that it puts the files in a weird place?

[CelticMinstrel]

Only new maps that are not already saved anywhere are saved in the temporary file. If it has a valid save location anywhere, it gets saved to there instead.

Isn't that even worse than I thought? It's overwriting the previous version of the map without telling you. If it does auto-save, then it shouldn't ever overwrite the file that was loaded. (It would be a different story if it was an auto-save thing from preferences that the user has explicitly enabled that they know will be overwriting the original file at regular intervals.)

So I tried reloading with a syntax error. That is certainly not handled well. I get the syntax error presented twice and then wesnoth exits. So if we do not improve that then this saving makes sense.

Yes, handling this badly is probably the main reason for the auto-save. It's preventing data loss caused by a different bug.

[babaissarkar]

afe0cff5341c5b73f9a4e2aee91765ad62537aeb removes the dependency on temporary save files. F5 now directly reloads from memory. (Work in progress, since I haven't managed to recover the undo/redo stack yet. Plus there are annoying save dialogs shown after reload which I also need to disable.)

[soliton-]

Only new maps that are not already saved anywhere are saved in the temporary file. If it has a valid save location anywhere, it gets saved to there instead.

Isn't that even worse than I thought? It's overwriting the previous version of the map without telling you. If it does auto-save, then it shouldn't ever overwrite the file that was loaded. (It would be a different story if it was an auto-save thing from preferences that the user has explicitly enabled that they know will be overwriting the original file at regular intervals.)

First thing I tried was editing a scenario outside of wesnoth to see if the reload would pick up the changes. With the saving going on that cannot work but IMO that is a reasonable expectation.

We also don't auto-save when leaving the editor. We ask the user.

[soliton-]

I'm not sure I follow what the issue is. Is it that it saves these temp files out at all, or is it that it puts the files in a weird place?

Both. In principle there is no need to save at all. If saving is done though the user has to have an idea that it's happening or it's useless.

[babaissarkar]

As of 44553ba840491e13b6018c9e551f8b80d0d39d7a, the undo/redo stack is preserved.

[babaissarkar]

@doofus-01 Will it be possible for you to build the latest commit in #8903 and see if F5 behaves as you would expect it to be? (For example, if terrains reload correctly).