Opened 12 years ago

Closed 12 years ago

Last modified 12 months ago

#8762 closed patch

Refactoring of the getSavePath method

Reported by: SF/david_corrales Owned by: sev-
Priority: low Component: --Other--
Keywords: Cc:
Game:

Description

This patch moves the getSavePath method out of the base class for all save managers. This allows other implementations to define their way to handle the savepaths in a separate manner.
Also, engines now use the error descriptions set by the savemanager.
Finally, a small convenience method popErrorDesc() was added, since it's common to check for the error and then clear it.

Ticket imported from: #1857121. Ticket imported from: patches/867.

Attachments (3)

getsavepath.patch (8.5 KB ) - added by SF/david_corrales 12 years ago.
Initial version
getsavepath2.patch (17.3 KB ) - added by SF/david_corrales 12 years ago.
Revision 2
getsavepath3.patch (17.7 KB ) - added by SF/david_corrales 12 years ago.
Revision 3

Download all attachments as: .zip

Change History (14)

by SF/david_corrales, 12 years ago

Attachment: getsavepath.patch added

Initial version

comment:1 by fingolfin, 12 years ago

A good start.

Now, you still need to deal with backends: Some implement custom savemanagers, and at least one also invokes getSavePath. You will have grep through backends/ for all occurances of getSavePath and deal with those, too.

Furthermore, using popErrorDesc as you do (exclusively, not extra text attached) is not such a good idea, because right now, our savefile managers do not really generate too useful error desc strings. That's something to be fixed, too, BTW -- if you are interested, have a go at that, too. DefaultSaveFileManager::openForSaving produces lots of error desc strings, but openForLoading performs no error checking at all (no "file not found", no "could not read file", no "invalid compression" etc.). And openForSaving is not that good right now either, as it's too sparse in the info it generates -- it does, for example, not mention the filename, nor the path.

So, instead of replacing
displayMessage(0, "Can't write to SAVEGAME.INF in directory '%s'. Device full?", _saveFileMan->getSavePath());

with

displayMessage(0, _saveFileMan->popErrorDesc().c_str());

I would recommend this:

displayMessage(0, "Can't write to SAVEGAME.INF. Device full? (%s)", _saveFileMan->popErrorDesc().c_str());

so the user gets more helpful (?) information.

by SF/david_corrales, 12 years ago

Attachment: getsavepath2.patch added

Revision 2

comment:2 by SF/david_corrales, 12 years ago

I checked the backends and three of them called the getSavePath() method:
- PS2: inherits from the DefaultSaveFileManager so it's all good. Moved the method to being private.
- PalmOS: already inherits from DSFM so there's no problem also.
- DS: switched the inheritance from the SaveFileManager to the DSFM. Looks like it implements all necessary methods, but should be built by someone who can :)

Added path information to the error messages and restored the engine specific error messages. The output should now be more verbose. I'll check the error text later, to avoid duplication. It'd be nice to have a single error message so engines can rely on it, instead of variating the same idea in each one (needs to be a really good one then).

I refactored the path checking code into a method called checkPath(), called by both open and write methods, so they now set the same errors. I know there's code repetition in both of them, but I didn't want to pollute the checkPath() with a bool parameter (for the StdioSaveFile constructor call). Also, it appears that we check only the path we're going to write to, not the file itself. For example, we could get write permissions on the directory, but have a non-writable file with the same name?
File Added: getsavepath2.patch

comment:3 by SF/david_corrales, 12 years ago

Owner: set to SF/david_corrales

comment:4 by fingolfin, 12 years ago

Hrm, no, I don't think PS2 should inherit from DSFM (it doesn't currently, either). Indeed, one should be very careful when doing that. In the case of the PS2 backend, if you actually look at it, it simply provides a dummy getSavePath() impl. So that could simply be removed, no harm done :)

Otherwise, this patch looks pretty good to me. Only question is: Should we add this before 0.11.0, or wait till after the release... I would like to get this off the table, and it should not break anything (TM), but then again, we *are* in a feature freeze. Eugene, what do you say?

comment:5 by fingolfin, 12 years ago

Owner: changed from SF/david_corrales to sev-

comment:6 by fingolfin, 12 years ago

Oh yeah, right: Please get rid of those silly Common::String() casts around string constants -- they are not necessary, just make the code harder to read :)

comment:7 by fingolfin, 12 years ago

Owner: changed from sev- to fingolfin

by SF/david_corrales, 12 years ago

Attachment: getsavepath3.patch added

Revision 3

comment:8 by SF/david_corrales, 12 years ago

The string literals are much clearer now and I also removed the getSavePath from the PS2 backend (it wasn't called anywhere in the file and engines, since they're fixed now).
File Added: getsavepath3.patch

comment:9 by sev-, 12 years ago

Owner: changed from fingolfin to sev-
Status: newclosed

comment:10 by sev-, 12 years ago

noticed it after the branching, so committed to both trunk and branch. Thank you

comment:11 by digitall, 12 months ago

Component: --Other--
Note: See TracTickets for help on using tickets.