[Scummvm-devel] GSoC patch for trunk

[David Corrales]

Hey everyone, I created a patch some days ago to merge my work into trunk. I
decided to take the conversation here to give it more exposure from other
devs :)

The following is a reply by Max, which I posted here to further continue the
discussion:

>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".
>
Actually, the factory files used to reside in the same directories as the
FSNode implementations but I moved them out on purpose. I'm working on a
BaseFile class which will use the same factories, so I decided that having a
factory for file objects in the fsnode directory would be misleading. Also,
if any other backend (say, the plugins one) switches to factory-based
instantiation, the factories folder would be the ideal place to put a new
kind of factory into.

For me, it's a matter of clarity. You can check out my branch to see the new
"file" folder and how the factories seem better in their own folder there.

In this regard, I'm waiting for your final word on where to put them :)

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

>* 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".
>
Already corrected this issue. The code does indeed look better.

>* 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? :-)
>
This is another place where I chose a separate file for clarity. I can add
the static method to the abstract-fs-factory.h file if you'd prefer it that
way.

>* common/fs.h: The doxygen comment for FilesystemNode::exists() is
>incorrect (started by /* not /**)
>
Weird, the comment reads "Indicates whether the object referred by this path
exists in the filesystem or not." here, which sounds correct.

>* 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).
>
I added more verbose comments to the methods you said and removed the
references to isValid(). Additionally, I made the lookupFile() method
breadth-first since I believe it fulfills the use case more properly.

>* 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
>
Fixed :D

>
>* 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).
I did these changes on the patch too, at least the ones that compile by
default. Is there a place I can check to see the ENV settings to build all
possible cases (in order to correct any old method call).

I haven't yet committed the corrected patch since there's still things to
discuss. As soon as they're cleared, I'll create the patch and publish it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-devel/attachments/20070813/d6ba7920/attachment.html>