[Scummvm-devel] ATTN Porters: SaveFileManager API Adjustments

[Marcus Comstedt]

Johannes Schickel <lordhoto at gmail.com> writes:

> How would engines go on for implementing this? Naively: Use just
> listSavefiles to determine the casing for the specific slot number and
> then use it for openFor*. And this is exactly what you argue against
> because this can lead to bad performance.

No, doing listSavefiles _once_ to get the casing of _all_ saves does
not lead to bad performance.  It's performing the list operation _for
each save_, which is what would need to happen if the functionality is
supposed to exist inside openForReading, that leads to bad performance.


> I am a bit confused by your maths to be honest, openForLoading (with
> system casing) should only take T because it only lists the files
> once?

I included the time for actually reading the contents of the file
(needed to get the display name of the save for the dialog).
Technically, this happens already in openForLoading on the Dreamcast,
but that makes no practical difference.


> But in general what you argue against is exactly what you end up with
> if you leave the case underspecified?

No.  If the engine passes the filenames to openForLoading that it got
from listSavefiles (which is enough for it to work witout specifying
the case mode), the backend can do an open on the lower level
directly, so it's

Once:  list all files (listSavefile)

For each save:  list files until a match is found (internally)
For each save:  read file data from the block location found in the
                previous step

versus (case insensitivity in openForLoading)

Once:  list all files (listSavefile)

For each save:  list files to find the correct case (case
                                        insensitivity emultion)
For each save:  list files until a match is found (internally)
For each save:  read file data from the block location found in the
                previous step

So it's  T(1+2N) versus T(1+3N) like I wrote.


> Let's look at it again. What is our main goal? Be user friendly and
> have a smooth save/load experience for the user.

Yup.  Needlessly making the UI much slower does not work towards that
goal.


> 1) What can affect this experience? Messed up casings on case
> sensitive file systems.

And poor responsiveness of the user interface.  Where can this come
from?  Doing expensive operations needlessly.


> 2) Where can this come from? (In random order:) 1. Users copy over
> save files from friends and ignore case differences.

The user do not have to either care about or ignore the case.  They
just plug in the two memory cards, select the file to copy and the
card to copy it to, and it is copied.


>2. Copying over
> betweens systems which have different cases.

You donÃ't copy files betweensystems on the Dreamcast, you move the
memory card to the new system.


> 3. ScummVM messing up
> cases in previous ScummVM versions (i.e. trying to start a game with a
> different case variation of a target name or the user messing up
> target name case in the Edit Game dialog).

Changing target casing between versions sounds like a bad idea, yes.
We should not do that.


> Result) To be on the safe side, every engine will need to implement
> case insensitive save file operations.

I do not agree with that conclusion.  Using the filenames returned by
listSavegames should work for most engines.


> Let's look at this specific use case of save/load dialogs again
[...]
> As I described above a naive implementation
> would do another listSavefiles to figure out the case to use
> beforehand. This is takes again time T.

No!  There is no need for it to list again, it already got the correct
case from the first list operation.


> Now, an engine can be smart, it can try to cache the save file list to
> prevent another listSavefile for every file opening operation in the
> engine, reducing openFor* again to T instead of 2T (This will bring
> ups back to (N+1)T+ND for the dialogs).
> This work we need to do in every relevant engine and in all places,
> which are all if we want to strive for the best user experience
> possible.

Well, you call it "caching", but it's really just processing the
list.  It needs to go throgh the list one item at a time to populate
its internal represenation of the savegame list. The item _is_ the
filename, so it's right there, no caches needed.


> From 2 we know that we can feature fine performance on case sensitive
> systems when putting effort in this. However, because of 1, this might
> be logic spread across all engines.

If there is really significant logic that would need to be replicated,
it could be put in common or something, but in general it would be
something like

  for(savename : listSavenames)
     if (f = openForLoading(savename))
       addSaveToMenu(savename, f)

> A better idea is to have all this
> logic concentrated in one place: SaveFileManager.

But beacuse of the way the API looks, the SaveFileManager does not
have access to the list generated by listSavefiles, that is owned by
the engine.  The backend could cache the list (in a more proper sense
of the term), but due to the lack of a session concept, it wouldn't
know when to discard said cache (this should happen when the dialog is
completely filled, to prevent stale entries).


  // Marcus