Retroplayer - savastate database DeleteSavestate method

NikosSiak commented 4 years ago

Description

Basic implementation of DeleteSavestate method.

Motivation and Context

I am going to apply for the GSoC 2020 for the saved game manager and I found this method that I could write for a first PR. This implementation can be kept as is when the manager is created since the CSavestateUtils::MakePath will probably need a rewrite to return a path that is not next to the rom.

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] Clean up (non-breaking change which removes non-working, unmaintained functionality)
[ ] Improvement (non-breaking change which improves existing functionality)
[X] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that will cause existing functionality to change)
[ ] Cosmetic change (non-breaking change that doesn't touch code)
[ ] None of the above (please explain below)

Checklist:

[X] My code follows the Code Guidelines of this project
[ ] My change requires a change to the documentation, either Doxygen or wiki
[ ] I have updated the documentation accordingly
[X] I have read the Contributing document
[ ] I have added tests to cover my change
[X] All new and existing tests passed

VelocityRa commented 4 years ago

Should be 1 commit imo, and better to prepend it with module/file changed as we do in the codebase. You can look at the file history for examples, in this case I'd name it something like RetroPlayer: CSavestateDatabase savestate deletion implementation

NikosSiak commented 4 years ago

Should be 1 commit imo, and better to prepend it with module/file changed as we do in the codebase. You can look at the file history for examples, in this case I'd name it something like RetroPlayer: CSavestateDatabase savestate deletion implementation

Fixed

garbear commented 4 years ago

Sorry for the delay, I can drop out for a few days at a time due to work/life. Some constructive criticism about commit message: It should explain the "why", not the "what", because the "what" is visible from the diff. CSavestateDatabase::DeleteSavestate() is currently dead code, so I'd like a brief explanation of how it will be used in the future.

NikosSiak commented 4 years ago

Some constructive criticism about commit message: It should explain the "why", not the "what", because the "what" is visible from the diff.

I know but I can't write something more to the "why", it is a delete method to delete the savestate? I though I explained the why in the context

CSavestateDatabase::DeleteSavestate() is currently dead code, so I'd like a brief explanation of how it will be used in the future.

The method will be called from the UI, that is the main goal of the project, the UI will (probably) need to know the path to the save but for now there isn't a method which gets the path of a save. A way to implement this that I can think of is to have an id for each save of the same game and when we call CSavestateUtils::MakePath.

VelocityRa commented 4 years ago

Yeah I wouldn't expect that PR/commit to actually be merged currently, as you said the method is unused.

It's a WIP commit to share the implementation - that code should probably be part of the same commit or at least PR as the UI code that will use it.

garbear / xbmc