Ubiquity plugins review

[Michael Terry]

On Tue, 2009-08-25 at 15:32 +0100, Evan Dandrea wrote:
> Having a ordering system codified in a prefix of the filenames strikes
> me as making the tree look a bit more complicated than it is.
> Personally, I would add a WEIGHT field alongside BEFORE and AFTER, but
> I'm fine with the way you've approached the problem.

WEIGHT is a good idea.  With the idea of fixing only what we need to for
FF, I'm thinking we could leave it as-is, and add WEIGHT later.  It can
co-exist with filename sorting (for plugins with WEIGHT, use that, else
use filename as a stringified WEIGHT maybe -- or convert the first parts
to int).  Or honestly, since I have to touch the branch anyway to fix
the bugs below, I could convert now.  Probably not that much of a
change.

> I think we need to do a better job of documenting the code.  For
> example, it's not immediately clear to me why, in the main loop, the
> dbfilter needs to be run in an idle handler.

Good point.  At times, I was lax in documentation.

The particular reason for the idle handler was that dbfilters expect to
run their nested loop inside an existing main loop.  This is handled
implicitly in trunk by the kicked off command output processing
happening by the time a main loop is entered*.  But with 'fake
dbfilters' (i.e. dbfilters without an actual run script), they want to
start a nested loop immediately.  So if we don't start() a dbfilter in a
main loop, we'll end up with two sequential main loops rather than
nested ones.  Hence the idle loop kickoff.

* I'm not sure if this was always an unlikely race condition or if the
details of how debconffilters work guaranteed that the main loop would
be entered by the time they processed output.

> There's a slight typo at the bottom of the console_setup and timezone
> plugins.  It should be InstallPlugin.install(self, target, ...)
> 
> import usersetup_apply is missing from scripts/install.py and
> self.get_language needs to be replaced with self.locale.split('_')[0]
> in kde_ui.

Oh, goodness; good catch.  Last minute changes by me.  I can fix today.
Thanks a lot for reviewing it.

-mt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/ubuntu-installer/attachments/20090825/716f9a3d/attachment.pgp