BASE: FSNode or DefaultSaveFileManager should provide an overloadable remove file method.

[carlo-bramini]

While working on my WinCE port of SCUMMVM, I got a problem with the use of remove() function from backends\saves\default\default-saves.cpp.
I could easily bypass the trouble by writing a simple wrapper that emulates remove() by calling DeleteFile().
However, by looking the current code at time of writing, you can see this text:

// FIXME: remove does not exist on all systems. If your port fails to
// compile because of this, please let us know (scummvm-devel).
// There is a nicely portable workaround, too: Make this method overloadable.

​https://github.com/scummvm/scummvm/blob/7dba6016361eadc8ae44c91604f0e87081591cdb/backends/saves/default/default-saves.cpp#L208

So, I'm wondering what you will think about adding a "remove" method (or similar, the name itself is not too important...) to Common::FSNode for doing this task. The default will use the usual remove() function from C library, while (for example) the ports for Windows may be able to call the DeleteFile() API.

By looking the code, it seems to me that this would be helpful also for the port for devices running Symbian OS: here there is the problem that "remove" is a macro defined to unlink(), rather than a true function. So, instead of defining SYMBIAN_USE_SYSTEM_REMOVE to undefine the "remove" macro on your needs, you can safely leave it always undefined when you are compiling for this platform and the dedicated code into backends\fs\symbian\symbian-fs.cpp will simply need to use unlink() when implementing this additional method.

This will make the common code easier to read and to maintain, in my opinion.

What do you think?

Sincerely.

[criezy]

I am not sure adding a remove method fo FSNode is a good idea. This would open the door to the possibility to remove any file on the file system if misused in engines for example, and could thus be dangerous. I think keeping the file removal restricted to the save file folder might be a good idea.

And since backends can easily instantiate their own custom SaveFileManager in initBackend() (as is done in the Windows backend for example), a better approach could be to add a protected virtual removeFile method in DefaultSaveFileManager so that you can reimplement it for the WinCE and Symbian backends.

[sev-]

That is correct, we allow read-only support to the game data for security reasons.

Thus, it is by design and will stay as-is. Use SaveFileManager for managing the saves.

[carlo-bramini]

Excuse me, rather than closing the issue, wouldn't it be possible to just change its title and write that "DefaultSaveFileManager" should provide this "removeFile" method for improving porting abstraction?

[criezy]

​https://github.com/scummvm/scummvm/pull/3404

[carlo-bramini]

I'm sorry, perhaps I have understood wrong, but I gave a look to the patch and it is not clear to me how this can avoid the problems with a direct call to remove(). Actually, the call to the function has been just moved to a different point of the file. A wrapper, an hack or something like that is still required for building the file.

[criezy]

Ah, indeed, you are right!
The idea was to allow subclassing the class to use something other than remove, but when you do that you still need to compile the base class. So I will need to go a bit further.

[fedor4ever]

Please, don't touch macro SYMBIAN_USE_SYSTEM_REMOVE. This guard from clash any functions, variables, classes etc with monster comes from system header.

Please.