Merge lp:~mterry/ubiquity/plugins into lp:ubiquity

This is phase 1 of my plugins work. I'm 3/4 of the way there, I think. The basic infrastructure is there, I've fully converted the first four pages of ubiquity to plugins (intro, language, location, keyboard) and half-converted the mythbuntu frontend to a series of plugins. The other ubiquity pages I've modified just enough to still work in the new scheme.

Slightly outdated spec about this work here: https://wiki.ubuntu.com/Ubiquity/Plugins . I intend to update that to match what this tree has shortly (tomorrow), but I wanted to get this merge request in, and maybe some eyeballs, before I finished that.

All the frontends should work with the new plugins. A plugin has to explicitly support a given frontend, so this let us much more elegantly handle the case of some frontends (notably debconf) supporting a different set of components than the others. All in all, hopefully 'elegant' is the watchword.

There should be no user-visible changes from this branch.

In general I like this approach. The branch is much too complicated for me to be able to do a line-by-line, so I'm just going to make some general comments and trust that you've done industrial-strength testing. In particular, noting your locale changes, I strongly recommend testing in Portuguese (both Portugal and Brazil) and Chinese.

Any particular reason that po/ubiquity.pot is removed in this branch?

I'd go for /usr/lib/ubiquity/plugins/ rather than /usr/lib/ubiquity/plugins.d/. The reason for this is that, normally, foo.d directories are a split-out analogue of a corresponding single file (see e.g. /etc/apt/sources.list and /etc/apt/sources.list.d/). In this case, there's no corresponding single file, and it would make more sense to drop the ".d".

There are some delicate dependencies in scripts/install.py which I'm not entirely sure you've preserved. The main one that comes to mind is that configure_timezone (scripts/install.py) may require network access for NTP syncing. I don't think configure_network has delicate dependencies of its own so you can probably just run it earlier.

Please always put a blank line between method definitions, even if the methods are short.

I'm not sure I like the get_ui design. Wouldn't it be simpler to just have attributes on the PageGtk etc. instances directly? Then the simple case would just be something like:

  class PageGtk:
      optional_widgets = 'stepMigrationAssistant'

... while in more complicated cases you could initialise attributes in __init__. You can then use hasattr() to find out if a page instance has a particular attribute, if need be.

Unless I'm missing something subtle, you should omit blank __init__ methods from page classes.

Could PageGtk (etc.) instances inherit from something, so that you have a debug method similar to that in FilteredCommand? That would be better than printing directly to sys.stderr in places.

"if type(widget_list).__name__ != 'list'" etc. - what's wrong with isinstance?

> Any particular reason that po/ubiquity.pot is removed in this branch?

No, that must have been an accident from a po/real-po merge.

> I'd go for /usr/lib/ubiquity/plugins/

OK

> There are some delicate dependencies in scripts/install.py which I'm not
> entirely sure you've preserved. The main one that comes to mind is that
> configure_timezone (scripts/install.py) may require network access for NTP
> syncing. I don't think configure_network has delicate dependencies of its own
> so you can probably just run it earlier.

Good point. Plugins should probably go near the end, since nothing can really depend on them anyway.

> Please always put a blank line between method definitions, even if the methods
> are short.

OK

> I'm not sure I like the get_ui design. Wouldn't it be simpler to just have
> attributes on the PageGtk etc. instances directly?

I didn't like doing that, because I didn't want to collide with future extensions. That is, I don't want a PageGTK to define a convenience member blarg today, if we may start using blarg as part of the API tomorrow. I suppose that's an argument for versioning.

However, it's not like I really avoided that problem with the current design. Right now, there are 3 interactions -- plugin->frontend-methods-and-data (Controller), frontend->plugin-methods (just calls the method on the PageGtk), and frontend->plugin-data (get_ui()). So we already have the collision problem with methods on the PageGtk.

We could fix this by a reverse Controller, putting methods pointers in get_ui() as well, namespacing API PageGtk members, or namespacing non-API PageGtk members.

I think I probably like namespacing non-API members the best and getting rid of get_ui(), as you suggest. Then official recommendations to plugin writers would be that all convenience members should start with an underscore.

> Unless I'm missing something subtle, you should omit blank __init__ methods
> from page classes.
>
> Could PageGtk (etc.) instances inherit from something, so that you have a
> debug method similar to that in FilteredCommand? That would be better than
> printing directly to sys.stderr in places.

OK

> "if type(widget_list).__name__ != 'list'" etc. - what's wrong with isinstance?

Nothing, now that you say it; I used isinstance elsewhere. Not sure what made me be so clunky there.

On Tue, Sep 01, 2009 at 01:58:08PM -0000, Michael Terry wrote:
> Colin Watson wrote:
> > There are some delicate dependencies in scripts/install.py which I'm not
> > entirely sure you've preserved. The main one that comes to mind is that
> > configure_timezone (scripts/install.py) may require network access for NTP
> > syncing. I don't think configure_network has delicate dependencies of its own
> > so you can probably just run it earlier.
>
> Good point. Plugins should probably go near the end, since nothing
> can really depend on them anyway.

configure_locales, at least, probably needs to run early.

> I think I probably like namespacing non-API members the best and
> getting rid of get_ui(), as you suggest. Then official
> recommendations to plugin writers would be that all convenience
> members should start with an underscore.

This sounds sensible to me.

> configure_locales, at least, probably needs to run early.

I just moved network and apt ahead of plugins. Within the plugins, they are configured in the same order they are presented (which isn't necessarily a hard and fast rule -- if we had some page that needed a different ordering, it could be broken into two plugins, one for the UI-side and one for the install-side).

> > I think I probably like namespacing non-API members the best and
> > getting rid of get_ui(), as you suggest. Then official
> > recommendations to plugin writers would be that all convenience
> > members should start with an underscore.
>
> This sounds sensible to me.

OK, your suggested changes and the namespacing are now in my branch. But as we briefly discussed on IRC, this isn't urgent and is too risky for Alpha 5. But after A5, here's hoping!