User talk:David corrales

Google Summer of Code: Filesystem API

This page holds additional information concerning my proposal for working to redesign the current Filesystem API.

It should be noted that the diagrams don't list all methods (for example, when a class inherits them) for brevity and clarity.

Common::FilesystemNode use cases

This is a list of the use cases for all classes who employ the FilesystemNode. It was created by looking through each file that references fs.h or fs.cpp.

---

1. include "common/fs.h" : 30

---

• backends/plugins/posix/posix-provider.cpp

1) getPlugins(): list all files in a directory using listDir(kListFilesOnly), name(), path().

• backends/plugins/sdl/sdl-provider.cpp

1) getPlugins(): list all files in a directory using listDir(kListFilesOnly), name(), path().

• backends/plugins/win32/win32-provider.cpp

1) getPlugins(): list all files in a directory using listDir(kListFilesOnly), name(), path().

• backends/plugins/dc/dc-provider.cpp

1) getPlugins(): list all files in a directory using listDir(kListFilesOnly), name(), path().

• backends/platform/wince/CELauncherDialog.h

1) automaticScanDirectory(FSNode): internally recurses files and dirs using listDir(kListFilesOnly) and listDir(kListDirectoriesOnly). path().

• backends/fs/abstract-fs.h

-) friend statement. **special case**

• base/main.cpp

1) runGame(): uses isDirectory() for checking the game path.

• base/commandLine.cpp

1) runDetectorTest(): uses listDir(kListAll). Also, there's several command line options (path, soundfont, savepath, themepath) which are paths and have a "//TODO: path is valid" flag.

• common/advancedDetector.h

1) detectAllGames(FSList): calls detectGame().

2) detectGameForEngineCreation(): uses listDir(kListFilesOnly).

3) detectGame(FSList): uses isDirectory(), name() and path().

• common/file.cpp

-) extensive use. **special case**

• common/md5.h

1) md5_file_string(FSNode): checks for isValid() and isDirectory() before calculating the MD5 checksum.

• engines/sword1/sword1.cpp

1) Sword1CheckDirectory(FSList): isDirectory(), name(), listDir(kListFilesOnly).

2) Engine_SWORD1_detectGames(FSList): calls Sword1CheckDirectory().

• engines/sword2/sword2.cpp

1) Engine_SWORD2_detectGames(FSList):isDirectory(), name(), listDir(kListAll).

2) Engine_SWORD2_create(): listDir(kListAll). Calls Engine_SWORD2_detectGames().

• engines/scumm/scumm.cpp

-) removed the include and compilation went fine. Don't see any forward declarations.

• engines/scumm/detection.cpp

1) searchFSNode(FSList, String, FilesystemNode): searches for a node within a list using an iterator and name(). As discussed earlier, this could be moved into the FSNode class.

2) detectLanguage(FSList, byte id): calls searchFSNode(). Also uses isDirectory(), listDir(kListFilesOnly).

3) computeGameSettingsFromMD5(FSList, ...): calls detectLanguage().

4) detectGames(FSList, ...): isDirectory() calls computeGameSettingsFromMD5().

5) Engine_SCUMM_detectGames(FSList): calls detectGames().

6) Engine_SCUMM_create(): creates a FSList using listDir(kListFilesOnly).

• engines/agos/agos.cpp

-) removed the include and compilation went fine. Don't see any forward declarations.

• engines/agi/agi.cpp

-) removed the include and compilation went fine. Don't see any forward declarations.

• engines/agi/agi_v3.cpp

1) detectGame(): creates a FSList using listDir(kListFilesOnly). name().

• engines/sky/sky.cpp

1) Engine_SKY_detectGames(FSList): isDirectory(), name(), path().

• engines/kyra/resource.cpp

1) Resource(): listDir(kListFilesOnly), path(), name(). Note: (uses strings for chars pretty much exclusively)

• engines/lure/lure.cpp

1) Engine_LURE_detectGames(FSList): isDirectory(), name().

• engines/gob/gob.cpp

-) removed the include and compilation went fine. Don't see any forward declarations.

• engines/queen/queen.cpp

1) Engine_QUEEN_detectGames(FSList): isDirectory(), name().

• engines/cine/cine.cpp

-) removed the include and compilation went fine. Don't see any forward declarations.

• gui/themebrowser.h|cpp

1) addDir(...): isValid(), listDir(kListAll), path(). Calls isTheme().

2) isTheme(FilesystemNode, ...): name().

• gui/browser.h|cpp

1) open(): create a FSNode object.

2) handleCommand(...): isDirectory(), getParent().

3) getResult(): returns a FSNode object.

4) updateListing(): path(), listDir(dirs/all), isDirectory(), displayName().

• gui/options.cpp

1) handleCommand(...): create FSNodes, path(). There's a TODO here about the dir being writable.

• gui/launcher.cpp

1) handleCommand(...): create FSNodes, path(). There's a TODO here about checking that the game is in the given directory.

2) addGame(): listDir(kListAll), path().

• gui/massadd.h|cpp

1) MassAddDialog(FSNode): pushes the node in a stack.

2) handleTickle(): listDir(kListAll), path(), is Directory().

---

1. include "fs.h": 2

---

• backends/fs/ds/ds-fs.h|cpp (duplicate include)

Method usage table

The following table is a rough estimate of the usage made by calling classes. It's meant to be a help when deciding what to change and which new methods could help the most.

It should be noted that this table reflects the amount of times a function appears in calling classes, not how many times it's actually performed during execution.

Operation	Calls	Percentage
displayName()	1	1,81%
name()	10	18,18%
path()	10	18,18%
getChild()	0	0%
listDir()	19	34,54%
isDirectory()	12	21,81%
isValid()	2	3,63%
getParent()	1	1,81%
Total	55	100%

Common::File redesign discussion (old)

While analizing the current state of the Common::File class, I've been noticing several important things that I feel, should be discussed. First of all, is the relevance of breaking up this class (like the FilesystemNode) into different implementations for each architecture. This is the primary motivation for this section.

External references

The following table presents the files importing a given header throughout all of ScummVM:

Header File	Imports
file.h	105
fs.h	32
abstract-fs.	11

From this table it's easy to infer that the abstract-fs.h header has no issues. It's only called by the 10 backends plus its cpp file.

The file.h header though, it's called from a -lot- of other files. This means that breaking the current Common::File implementation is like opening a pandora box.

The fs.h doesn't seem as bad, although I haven't had the time to research it's use in each of the modules. Being a wrapper around the AbstractFilesystemNode, it's a lot easier to change the wrapped class, because the classes are designed to treat FilesystemNode as a wrapper.

Update 31/5: the information about FilesystemNode class usage is now available.

Inheritance

Another issue with breaking up Common::File is the Scumm::BaseScummFile class. If the Common::File gets broken in architecture specific implementations, creating an structure like this:

Level 0	Level 1	Level 2
AbstractFile
	BaseScummFile
		ScummFile
		ScummNESFile
	POSIXFile
	WindowsFile
	...File

Carefully looking, you'll see that BaseScummFile and all the architecture dependent implementations are on the same inheritance level! Of course this is not the intended behavior, since you'd want BaseScummFile to work with a backend like POSIXFile. In my second diagram I propose a solution to this issue.

This solution requires the BaseScummFile to use a factory, in order to obtain the correct architecture dependent File implementation. The BaseScummFile would then defer invocations to said backend.

Yet another, less than ideal, solution would be to use #ifdef to inherit the BaseScummFile from the correct backend.

Finally, lets not forget that BaseScummFile uses other tricks to handle the inherited methods from Common::File. Since they are not virtual, the subclasses can create a method with the same signature and still call the base class independently (as done in ScummFile for example). By using the deferred method, this technique could pose problems.

Update 28/03: BaseScummFile could be implemented as a wrapper around Common::File instead of inheriting from it.

Methods affected by different architectures

The methods whose algorithm suffers changes depending on the current backend are only two: fopenNoCase and open. The fopenNoCase changes if the amigaos4, GP32 or palmos are defined. The open one changes only if MACOSX is defined.

There's also methods whose signature gets redefined via the preprocessor using #define's. The architectures using this are three out of ten: Playstation 2, Dreamcast and Symbian32. This raises yet another problem. Since the algorithm doesn't change, to properly do this in subclasses, one would have to retype the whole algorithm -line by line- and only change fopen for say, ps2_fopen. Needless to say, this is not a good solution.

Another option would be to use a pattern like, template for example. And define the algorithm in parts, where each subclass redefines parts as needed. This seems like overkill to me, since most of the backends would use the default implementation. As in a comment inside file.cpp, "such a high price for a simple beautification".

Update 28/03: functions like ps2_open are actually implemented by ScummVM. If Common::File gets redefined, said implementation will be moved to the appropiate subclass of the new File class.

Update 29/03: the ps2sdk does provide fopen, fread, etc... but they get redefined because ScummVM uses inneficient I/O. The redefinitions are then used to -avoid- calling the SDK functions, since the custom ones provide caching.

A possible wrapper

Given that the Common::File class inherits itself from two classes: Common::SeekableReadStream and Common::WriteStream, the wrapper itself would have to handle things like "data >> wrapper", since streams usually overload the >> and << operators. Again, this would add a lot of complexity.

Update 28/03: The operators << and >> and not being currently overloaded by the classes Common::File inherits from.

New methods

Adding new methods to the existing Common::File class wouldn't have an impact, unless it redefines or changes the signature of the old ones. Instead of breaking up said class, it could be cleaned up and extended with new functionality.

New Common::File redesign proposal and implementation

After being involved in the project for more time, I analyzed the current design and created the following proposal to address the Common::File class.

Reading through the cpp file, I discovered that most discrepancies occur in the redefinition of standard I/O manipulation functions (e.g. fopen, ftell, fseek and so on) in 3 platforms: ds, ps2 and symbian. There's some #ifdef's around, but the algorithms are 99% the same for all platforms, so a separate implementation for each architecture is not needed and instead, would result in unnecessary code duplication.

Given this situation the template pattern looked like a really good alternative: create a base file class that implements the algorithm but allows subclasses to choose which method they want for fopen, fclose, etc calls. This is done through protected functions named _fopen, _close, etc.

Additionally, the current Common::File class is mixing responsibilities that go beyond a regular file class. For example, the whole default directory structure (contained in a hash map for quick access) is beyond the scope of a regular file manipulation object. The objective is then to convert Common::File into a wrapper for BaseFile, just like Common:FsNode is around AbstractFilesystemNode.

The responsibilities per function would be:

Function	BaseFile	Common::File
void clearIOFailed	Clear the last I/O error flag	Checks + Call the base class
virtual void close	Close the open file	Checks + Call the base class
bool eof	Whether the seek pointer is at the EOF	Checks + Call the base class
bool eos	Whether the seek pointer is at the EOS	Checks + Call the base class
bool ioFailed	Whether the last I/O failed	Checks + Call the base class
const char *name	Name of the stream	Checks + Call the base class
bool isOpen	Whether the file is open	Checks + Call the base class
virtual bool open	Open a given filepath	Use the default paths to locate the filepath, then call the base class
uint32 pos	Position of the seek pointer within the file	Checks + Call the base class
uint32 read	Reads a chunk from a file	Checks + Call the base class
virtual bool remove	Remove a file	Checks + Call the base class
void seek	Move the seek pointer	Checks + Call the base class
uint32 size	Size of the file	Checks + Call the base class
void addDefaultDirectory	-	Add a default search path
void addDefaultDirectoryRecursive	-	Recursive method of addDefaultDirectory
void resetDefaultDirectories	-	Clear default directories
bool exists	-	Whether the file exists. Uses Common:FSNode

The wrapper will handle the whole default search path functionality plus checks and maybe some caching for the BaseFile object, again, just like Common::FsNode does.

I did not list the write functions here, since a while ago I recall a discussion that implied removing the write abilities from the Common::File class. This of course needs to be defined clearly.

Another issue to be resolved is BaseScummFile inheriting Common::File. Max told me it was possible to code BaseScummFile as a wrapper instead of an inherited class, so that could be a possible solution.

The following diagram shows the class structure:

Current Filesystem API implementation diagrams

Filesystem Node Redesign Progress

Update 01/04: added the FilesystemFactoryMaker class.

Update 05/06: added the new methods exists(), isReadable(), isWritable().

Update 19/06: removed isValid() and added the hidden parameter to getChildren().

Common::File Redesign proposal (old)

Possible new operations

This section describes possible new file/path operations to be added to ScummVM. If the method will be similar to an existing one, it'll be noted in parentheses after the description.

The format used is methodName(param1, ..., paramN) -> ret1, ..., retN: description.

exists(path) -> bool: true if the file exists. Implemented

split(path) -> head, tail: split the pathname path into a pair, (head, tail) where tail is the last pathname component and head is everything leading up to that. (python os.path)

splitDrive(path) -> drive, tail: split the pathname path into a pair (drive, tail) where drive is either a drive specification or the empty string. (python os.path)

getSeparator() -> string: returns the separator used in the current backend. For example, "/" for Unix or "\" for Windows.

isAbsolute(path) -> bool: return True if path is an absolute pathname (begins with a slash). (python os.path)

isReadable(path) -> bool: true if the file can be read. Implemented

isWritable(path) -> bool: true if the file can be written. Implemented

count(dir) -> int: returns the total number of directories and files in the directory. (Qt)

getChildren(list, mode, hidden) -> list: lists the dir, with the given mode and filter. This function is already present. The idea is to augment it with filters. Added a filter to include hidden files

isHidden(path) -> bool: tests whether the file pathname is a hidden file. (Java)

getCanonicalPath(path) -> path: returns the canonical pathname string of the abstract pathname. (Java)

isValid() -> bool: as per Max's instruction, this method was removed from the FilesystemNode class. It wasn't a heavily used method and its semantics weren't clear. It is now possible to use much more rich checks as (exists && isReadable) for example. Removed

Update 05/06: marked some methods as implemented.

Update 19/06: added the isValid() method as removed. Also updated getChildren().

Savefile manager information

Due to request from Max, I've started working on the savefile manager also. Although this isn't technically part of the GSoC project, it's closely related and in need of work.

This section deals with information about the current state of affairs concerning the savefile manager class.

For more information about what needs to be done, one can visit http://wiki.scummvm.org/index.php/User:Fingolfin#TODO or http://wiki.scummvm.org/index.php/TODO#Savegames.

Save game procedure for the Scumm engine

1. Click on the save button. (dialogs.cpp: save())
2. Set the listed files from the list provided by generateSavegameList(). (dialogs.cpp)
   1. Mark the flags of the available savefiles using listSavegames(). (default-saves.cpp)
   2. For the available saves call getSavegameName(index, name). (Reads the user input name from the header) (saveload.cpp)
      1. Call makeSavegameName() to create a default filename.
         1. Creates a filename with "game|type|slot" format. Game is given by _targetName.
      2. Try to open the generated filename with openForLoading(filename).
      3. If 2.2.2 fails, return. Else continue loading the header, etc...
   3. Return the list of user input names from 2.2.
3. Run the dialog and catch an index and a name for the savefile.
4. Request a save file using requestSave() with the given index and file description.

Save file name format strings for all engines

• agi:

sprintf(saveLoadSlot, "%s.%.3d", _targetName.c_str(), num + _firstSlot);

• agos:

if (getGameId() == GID_DIMP) {

sprintf(buf, "dimp.sav");

} else if (getGameType() == GType_PP) {

sprintf(buf, "swampy.sav");

} else if (getGameType() == GType_FF) {

sprintf(buf, "feeble.%.3d", slot);

} else if (getGameType() == GType_SIMON2) {

sprintf(buf, "simon2.%.3d", slot);

} else if (getGameType() == GType_SIMON1) {

sprintf(buf, "simon1.%.3d", slot);

} else if (getGameType() == GType_WW) {

sprintf(buf, "waxworks.%.3d", slot);

} else if (getGameType() == GType_ELVIRA2) {

sprintf(buf, "elvira2.%.3d", slot);

} else if (getGameType() == GType_ELVIRA1) {

sprintf(buf, "elvira1.%.3d", slot);

}

• cine:

snprintf(tmp, 80, "%s.dir", _targetName.c_str());

• gob:

snprintf(slotBase, 3, "%02d", slot);

• kyra:

sprintf(saveLoadSlot, "%s.%.3d", _targetName.c_str(), num + _firstSlot);

• lure:

sprintf(buffer, "lure.%.3d", slotNumber);

• parallaction:

sprintf(filename, "game.%i", slot);

• queen:

strcpy(buf, "queen.asd");

sprintf(buf, "queen.s%02d", slot);

• saga:

sprintf(name, "%s.s%02d", _targetName.c_str(), slotNumber);

• scumm:

sprintf(out, "%s.%c%.2d", _targetName.c_str(), temporary ? 'c' : 's', slot);

• sky:

strcpy(fName, "SKY-VM-CD.ASD");

sprintf(fName, "SKY-VM%03d.ASD", SkyEngine::_systemVars.gameVersion);

• sword1:

_saveFileMan->openForLoading("SAVEGAME.INF");

sprintf(fName, "SAVEGAME.%03d", slot);

• sword2:

snprintf(buf, sizeof(buf), "%s.%.3d", _targetName.c_str(), slotNo);

• touche:

snprintf(dst, len, "%s.%d", _targetName.c_str(), num);

Misc

You might want to keep in minde that a "path" can refer to:

• an existing file (which hence might be writable or readable or both)
• an existing directory (which can be writeable, too, with different meaning)
• a not yet existing but valid path to a file/dir (which is neither readable nor writable, I guess).

Well, that's not really a brilliant observation ; I just wanted to make sure this will be considered in the design process Fingolfin 00:22, 24 March 2007 (CET)

Update 19/06: It is now possible to discern all these cases correctly using exists(), isReadable() and isWritable().

Another thing: look at engines/scumm/plugin.cpp and the searchFSNode() function in there. It can be used to look for a given file(name) within a directory (ignoring case). It's a bit like getChild() only that it ignores the file name case, and is only used to check for the presence of a given file. This is a commonly used function, I think. Might be worse taking this into consideration for the FS API redesign Fingolfin 21:41, 3 April 2007 (CEST)

Update 08/07: The lookupFile() function in fs.h/cpp can be used to supply this functionality. It uses wildcards for matching filenames.