Add: map autosaves #6056
Add: map autosaves #6056
Conversation
|
Hey there! Thanks for helping Mudlet improve. 🌟 Test versionsYou can directly test the changes here:
No need to install anything - just unzip and run. |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Yep, fair concern - like profile autosaves, I made it write to one file ever only, so old autosaves are automatically replaced. It won't interfere with manual saves either. |
|
oh right, derp. I'm gonna have to drink more coffee. |
|
We do still need to add running |
There was a problem hiding this comment.
You might need to check having the map being flagged as dirty in the T2DMap mouse event handlers as somewhere in there represents where map label and room movements (at least) are concluded/committed - in further review I think I have seen some of this being done but I have not worked out if every case is being covered.
TBH The fact that various map and not map classes can dip into each others contents directly rather than going through a uniform interface (perhaps the TMap class) does make this harder than it might otherwise be - but then that is why I raised #4569 in the first place.
src/TRoom.cpp
Outdated
| } | ||
| mpRoomDB->mpMap->mUnsavedMap = true; |
There was a problem hiding this comment.
Needs to be moved up into each of the if () and else if () { code blocks. So setting/removing an exit weight for a non-existent exit does not dirty the map. OTOH the validity for setting an exit weight is not having the given exit being checked here... 😟
| @@ -325,8 +325,8 @@ class mudlet : public QMainWindow, public Ui::main_window | |||
| const QMap<QByteArray, QString>& getEncodingNamesMap() const { return mEncodingNameMap; } | |||
| void refreshTabBar(); | |||
| void updateDiscordNamedIcon(); | |||
| // Has to be public as it needs to be called from dlgConnectionProfiles class: | |||
There was a problem hiding this comment.
Not sure it is helpful to remove this comment - it gives advice about something that might not be obvious. IMHO.
There was a problem hiding this comment.
A quick investigation will fix it.
Co-authored-by: Stephen Lyons <slysven@virginmedia.com>
…nto add-map-autosave
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
| @@ -495,6 +496,8 @@ Host::Host(int port, const QString& hostname, const QString& login, const QStrin | |||
| auto entry = i.next(); | |||
| profileShortcuts.insert(entry, new QKeySequence(*mudlet::self()->mShortcutsManager->getSequence(entry))); | |||
| } | |||
|
|
|||
| startMapAutosave(); | |||
There was a problem hiding this comment.
Is this the most appropriate point to initiate autosaving the map?
For a new profile (not a copy of an existing one) there won't necessarily be a map yet and for an existing one IIRC a map may not get loaded until the "Map" toolbar button is clicked for the first time if a mapper is not instantiated by a script/package...
Looking at the calls to new dlgMapper(...) they happen in:
TMainConsole::createMapper(...): 2 alternatives one for the Main console and one for when it is in a sub-console)Host::createMapper(...)
Oh! That is messy/confusing - there are MORE calls to TMap::restore(...) than that - ah, that might explain why the map is sometimes loaded multiple times on start-up - something to investigate and fix I suppose.
Also, in some extreme, pathological, cases it can take a long time to load and audit a map on a slow machine with a big map (I was testing something on a Windows machine with a debug build {so automatically slower} with that humongous >2M room map recently and that took nearly an hour to finish the loading!) Might it not be better to make startMapAutosave() idempotent and to call it after each call to TMap::audit()` - so it only starts once a map has been successfully loaded and audited - ah, that won't include the case where the user is starting from scratch.
Okay, how about changing the lines that do mpMap->mUnsavedMap = true to call a method that both sets the flag AND starts the autosave timer if it hasn't been started before?
What about the cases after TMap::mapClear() gets called - should that stop the timer (and also run the timer code) so that the (to be/just) deleted map is saved in the automatic backup until a new map overwrites it?
There was a problem hiding this comment.
@SlySven it's important to focus on keeping things as simple as possible instead of complicated! If you find any practical issues with autosaves, I'm happy to fix them, we can't afford to spend time and energy thinking of theoretical problems and then solving those theoretical problems.
Brief overview of PR changes/additions
Add a map autosave feature to prevent large losses of work when Mudlet/computer shut down unexpectedly.
Motivation for adding to Mudlet
Better player experience.
Other info (issues closed, discussion etc)
Closes #2121.
Works just like profile autosave which has been very successful since its introduction - autosaves the map every 2min if any changes have been done to it. Hopefully this won't be an issue for enormous maps - if it is, we'll need to look into making the save process hog the main thread less.
Adding this revealed that the internal mapper API is messy - ideally the 'mDirtyMap' flag should never be set within TLuaInterpreter.cpp!