#8762 closed patch
Refactoring of the getSavePath method
| Reported by: | SF/david_corrales | Owned by: | sev- |
|---|---|---|---|
| Priority: | low | Component: | --Other-- |
| Version: | 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)
Change History (14)
by , 17 years ago
| Attachment: | getsavepath.patch added |
|---|
comment:1 by , 17 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.
comment:2 by , 17 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 , 17 years ago
| Owner: | set to |
|---|
comment:4 by , 17 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 , 17 years ago
| Owner: | changed from to |
|---|
comment:6 by , 17 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 , 17 years ago
| Owner: | changed from to |
|---|
comment:8 by , 17 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 , 17 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → closed |
comment:10 by , 17 years ago
noticed it after the branching, so committed to both trunk and branch. Thank you
comment:11 by , 6 years ago
| Component: | → --Other-- |
|---|
Initial version