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.
- 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().
- 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
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.
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 05/06: removed isValid() and added the hidden parameter to getChildren().
Common::File Redesign proposal
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().
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)
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)