-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ask if uses want to overwrite save file when creating save with existing name #1371
Ask if uses want to overwrite save file when creating save with existing name #1371
Conversation
game/ui/general/savemenu.cpp
Outdated
} | ||
} | ||
|
||
void SaveMenu::askUserIfWantToOverrideSavedGame(sp<SaveMetadata> saveMetadata) | ||
{ | ||
auto saveName = saveMetadata->getName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, if something can be const, I think it should be declared as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean in the parameter, like this?
void SaveMenu::askUserIfWantToOverrideSavedGame(const sp<SaveMetadata> saveMetadata)
return !pathExists; | ||
} | ||
|
||
sp<SaveMetadata> SaveManager::getSaveGameIfExists(const UString &name) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to your change, as it's just following what's already there, but I really need to remove the sp<> wrapper here - it's not really a thing that needs shared ownership and arguably a bit unnecessary.
game/ui/general/savemenu.cpp
Outdated
if (parent->Name == newSaveItemId) | ||
{ | ||
if (saveManager.newSaveGame(saveName, currentState)) | ||
auto saveGameMetadata = saveManager.getSaveGameIfExists(saveName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be const?
game/ui/general/savemenu.cpp
Outdated
} | ||
}); | ||
|
||
std::function<void()> onNo = std::function<void()>([this] { clearTextEdit(activeTextEdit); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be auto? The type is already defined in the rhs
game/state/savemanager.cpp
Outdated
|
||
sp<SaveMetadata> SaveManager::getSaveGameIfExists(const UString &name) const | ||
{ | ||
auto saveList = getSaveList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these can be const?
@JonnyH I added some more const values, let me know if I missed some. |
Thanks, looks good. I think c++ really should be "const-by-default", and force you to state if variables are mutable. I just find it makes it much easier to reason about code if you don't have to worry about variables changing :) |
Fix #1347
After discussion in issue thread, I implemented the following logic:
I also encapsulated overwrite messagebox call in
SaveMenu::askUserIfWantToOverrideSavedGame