Skip to content
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

Delete button for saveloadwindow, make this window less error prone (and some more) #5674

Conversation

Max5377
Copy link
Contributor

@Max5377 Max5377 commented Nov 26, 2023

Adds delete button for saveloadwindow to allow user to delete any save in user\\Documents\\Pioneer\\savefiles\\ folder.
For this, adds new static method Game::DeleteSave(const std::string& filename) and two new methods in FileSystemFS class:

  1. FileSystemFS::RemoveFile(const std::string& relativePath), to remove file in the FileSystemFS::m_root + relativePath. Deletion is restricted to only delete regular files;
  2. FileSystemFS::IsChildOfRoot(const std::string& path) to check if given path is in the FileSystemFS::m_root.
    Makes saveloadwindow less error prone, especially when trying to save/load. Will show MessageBox with a text about why save/load failed.
    Before saving will check filename with new FileSystem::IsValidFilename(const std::string& filename) if filename has forbidden OS characters.
    Other changes include optimization for Game::CanLoadGame (just tries to open file to check it's existence) and Game::SaveGame (tries to open file first, if failed, immediatlly throw exception), new MessageBox type and little refactoring for LuaGame.cpp.
    Demonstration (Linux VM, similar functionality in Windows):
Delete.Button.mp4

Update:

  1. saveloadwindow now shows MessageBox with confirmation if user really wants to delete save. Next it will show another MessageBox, that tells if save deleted successfully or not;
  2. Some optimization changes for FileSystemFS::IsChildOfRoot(const std::string& path).

@Max5377 Max5377 force-pushed the saveloadwindow-delete-btn-make-less-error-prone branch 2 times, most recently from 1d602e4 to 6aaee7b Compare November 26, 2023 11:50
@bszlrd
Copy link
Contributor

bszlrd commented Nov 26, 2023

Nice!
It might be useful to have some kind of confirmation for deleting to avoid any accidental deletes I think.

@Max5377
Copy link
Contributor Author

Max5377 commented Nov 26, 2023

@nozmajner It has, just not used for now. I want to show MessageBox with the error if something goes wrong, but if I show MessageBox with confirmation first, MessageBox with the error will be just not shown. Added comment in saveloadwindow.lua about it:

-- XXX Not used for now, because lui.COULD_NOT_DELETE_SAVE has no time to show itself
-- do to creation of it on top of lui.DELETE_SAVE_CONFIRMATION which deletes itself
-- by the time of showing
local function showDeleteConfirmation()

Edit: correct me if I wrong.
Edit2: workaround for that was actually easy. Going to do push later.

@Max5377 Max5377 force-pushed the saveloadwindow-delete-btn-make-less-error-prone branch 3 times, most recently from 350bc77 to 49975c1 Compare November 26, 2023 20:28
@Max5377
Copy link
Contributor Author

Max5377 commented Nov 26, 2023

Hope this is last, missed some copypaste error since last time. Sorry for a lot of pushes.

@Web-eWorks
Copy link
Member

Rather than displaying another popup that the user has to click through telling them that deleting a save succeeded, I'd recommend simply drawing the status message right-aligned on the same line as the Save: text. That would put it directly above the Save and Delete buttons where the user expects to see it.

Generally, you want to only interrupt the user's flow to ask them if they're doing a potentially-destructive operation by accident - informational notices should not interrupt the player. If the save or delete operation failed, that can be a valid case for displaying a popup notification, but deleting a savefile shouldn't take three clicks.

@Max5377
Copy link
Contributor Author

Max5377 commented Nov 28, 2023

@Web-eWorks

DeleteSuccessfulText.mp4

@Web-eWorks
Copy link
Member

Looks good! I'd recommend using the next smaller font size breakpoint (with ui.withFont(<font>, function() ... end)) and adding some spacing between the text and the Load buttons, but otherwise I think it will work.

@Max5377
Copy link
Contributor Author

Max5377 commented Dec 4, 2023

@Web-eWorks
Changed font size to small and added padding between Load button and Save deleted successfully.
Also added new pigui lib ui-timer to create timers that work from internal ImGui timer. For example, can be used to show something for a limited amount of time (like with Save deleted successfully).
Don't really know if it's good for a solution, but will change it if needed.

@Max5377 Max5377 force-pushed the saveloadwindow-delete-btn-make-less-error-prone branch from 2be61d4 to 6d23dae Compare December 4, 2023 17:06
@Max5377 Max5377 force-pushed the saveloadwindow-delete-btn-make-less-error-prone branch from 6d23dae to 98586c3 Compare December 4, 2023 17:52
@Web-eWorks Web-eWorks self-requested a review January 7, 2024 17:38
Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally reviewing this now that I have the time available, thank you for the patience.

Code looks good, the new methods in FileSystemPosix/FileSystemWin32 could probably receive another pass at some point for optimization and cleanup (for example, the forbidden characters list on unix is definitely missing a few characters), but overall I think it's fine.

Tested on Linux, if anyone wants to give this a test on Windows before I merge, let me know in the next 24 hours or so.

Adds new message box "OK_CANCEL" with two buttons "OK" and "Cancel". Pressing "OK" will call function specified in "callback" parameter. As the side change, all types of message box will be positioned at the center of the screen.
1. FileSystem::IsValidFilename to check if filename doesn't have forbidden OS characters;
2. FileSourceFS::IsChildOfRoot to check if given path is inside FileSourceFS::m_root;
3. FileSourceFS::RemoveFile to remove file from passed relative path. During method execution, relative path will transform to m_root + relativePath. Deletion is restricted to only delete regular files.
1. All variables that are not going to change their value are marked constant;
2. Adds "return luaL_error", like specified in "TODO's";
3. All instances where "return 0" was called after "luaL_error" changed simply to "return luaL_error(...)".
Adds new delete button for saveLoadWindow, which enables user to delete any save in the user\\Documents\\savefiles\\ folder.
Makes saveLoadWindow and all functions related to saves less error prone. To name a few;
1. Before saving, savefile name will be checked for forbidden symbols (eg. for Windows: '\', ' " ', '\0');
2. All instances where JoinPathBelow is used is wrapped around try-catch block, because it can throw "std::invalid_argument" exception;
3. Game.SaveGame and Game.LoadGame is pcall'ed to catch any errors that can happen in them.
MessageBox will be shown to user to tell, what gone wrong during saving/loading the game.
Adds new static method Game::DeleteSave and lua variation l_game_delete_save.
Allow getting time from internal ImGui timer from lua.
Adds new pigui lib "ui-timer" that allows creation of timer that checks timer from internal ImGui timer, rather then Game.time.
Additional changes to saveloadgame.lua to use new ui-timer functionality.
@Max5377 Max5377 force-pushed the saveloadwindow-delete-btn-make-less-error-prone branch from 98586c3 to 8cb8c65 Compare January 13, 2024 09:52
@Web-eWorks Web-eWorks merged commit 44772ec into pioneerspacesim:master Jan 14, 2024
6 checks passed
@Max5377 Max5377 deleted the saveloadwindow-delete-btn-make-less-error-prone branch January 14, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants