Opened 12 years ago

Closed 12 years ago

Last modified 12 months ago

#8715 closed patch

Merge fsnode-gsoc into trunk

Reported by: SF/david_corrales Owned by: fingolfin
Priority: normal Component: --Other--
Keywords: Cc:
Game:

Description

This patch incorporates all the FSNode changes in the gsoc-fsnode branch back into trunk.

Among the changes are the factories for filesystem objects, new functionality in the abstract filesystem class and fixes for TODO's using these new methods.

I personally tested this patch in Linux and Windows (MinGW). They both build without problems using r28462, but trunk should work as well.

P.D. I've been missing from the IRC channels lately because someone is blocking me any IRC connection =/ I talked to my ISP and they're not blocking any ports so it must be outside. I'm working on a solution.

Ticket imported from: #1768757. Ticket imported from: patches/820.

Attachments (3)

gsoc-fsnode trunk.patch (184.3 KB ) - added by SF/david_corrales 12 years ago.
gsoc-fsnode branch merge into trunk
gsoc-fsnode trunk (rev 1).patch (220.1 KB ) - added by SF/david_corrales 12 years ago.
Revised GSoC-fsnode to trunk patch
gsoc-fsnode patch (sept 18).txt (225.1 KB ) - added by SF/david_corrales 12 years ago.
FSNode patch updated for revision 28941.

Download all attachments as: .zip

Change History (10)

by SF/david_corrales, 12 years ago

Attachment: gsoc-fsnode trunk.patch added

gsoc-fsnode branch merge into trunk

comment:1 by fingolfin, 12 years ago

Owner: set to fingolfin

comment:2 by fingolfin, 12 years ago

Hi David,

some feedback on your patch:

* The "backend/factories" dir is badly named, IMO, as "factories" could be for anything, but you use it for *FS node* factories exclusively. However, instead of renaming this, I propose that you simply merge all of its contents into the existing "backends/fs" dir. So the files in "backends/factories/posix" would end up in "backends/fs/posix".

* Most (all?) of your new source files are missing the mandatory GPL license comment at the start. Should be easy enough to fix, though.

* fs-factory-maker.cpp: It unwise to have a big set of mutually exclusive (!) set of commands/#includes covered in blocks of "#ifdef/#endif". Rather, I'd prefer if this code used "#if/#elif/.../#endif".

* Same file: any good reason to introduce a whole FilesystemFactoryMaker class, just for a single static method in it? This just adds code and fake OO boiler plating, w/o giving any advantage (or does it?). Why not use use a global function, like in the good old C days? :-)

* common/fs.h: The doxygen comment for FilesystemNode::exists() is incorrect (started by /* not /**)

* Same file: could you please update & clarify some doxygen comments, please? You should go through each of them, and fix them. For example, some still refer to methods which no longer exist, like isValid(). Others are rather vague about semanting, e.g. exists() (what does "invalid node" mean, how does one check for one?); isReadable & isWritable (it should be a bit more specific, and point out what these mean for files vs. dirs); for lookupFile(), document whether the search is depth-first or breadth-first (in particular, for the variant searching in multiple dirs).

* dc-fs.cpp: You seem to have a copy&paste bug there, replacing refs to "ronin" by "POSIX":
-/*
- * Implementation of the ScummVM file system API based on ronin.
+/**
+ * Implementation of the ScummVM file system API based on POSIX.

* gui/options.cpp: Small code formatting issue, there is a "{" on a separate line:
+ if(dir.isWritable())
+ {

* common/fs.cpp: more code formatting isues in the lookupFile() method, operator< and possibly others

* Due to recent changes, I had to manually fixup agi/sound.cpp to compile with the patch (trivial changes, though, like name() -> getName()). Same for lure/detection.cpp (note that lure & drascula are only being compiled if you manually enable them when invoking configure).

by SF/david_corrales, 12 years ago

Revised GSoC-fsnode to trunk patch

comment:3 by SF/david_corrales, 12 years ago

File Added: gsoc-fsnode trunk (rev 1).patch

by SF/david_corrales, 12 years ago

FSNode patch updated for revision 28941.

comment:4 by SF/david_corrales, 12 years ago

File Added: gsoc-fsnode patch (sept 18).txt

comment:5 by fingolfin, 12 years ago

Status: newclosed

comment:6 by fingolfin, 12 years ago

Added to HEAD. Thanks, great work!

comment:7 by digitall, 12 months ago

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