Do

Code review comment for lp:~alexlauni/do/package-manager-service

package-manager-service
Merge into trunk

Revision history for this message

Alex Launi (alexlauni) wrote on 2009-06-26:

> review needsfixing
>
> Is this a merge for both PluginManagerService and PackageManagerService?
> If so, could we merge PluginManagerService separately, and then merge
> PackageManagerService after that has landed?

There is already a pending merge for PluginManagerService, this merge depends on it.

> You've added Do.Linux.JoliCloud/Makefile.in to bzr. That's not very
> useful :).

Whoops. Removed.

> In Do.Platform.Linux.JoliCloud/src/Do.Platform/Do.Platform.Linux/Do.Platform.Linux.JoliCloud/PackageManagerService.cs:
>
> I don't much like the magical Daemon property, particularly since you
> need to document it with the comment
> """
> // this call will instantiate the daemon, as well as make sure we also
> got a DBus object
> """
> in Initialize (). Is there any particular reason that Initialise can't
> initialise the daemon explicitly?

I did it this way to avoid the duplication of adding the event, etc. It seems cleaner to me.

> In MaybeStartDaemon (), is it possibly to ask DBus to wait for an owner
> of BusName rather than just sleeping for 5 seconds? I'm not sure if it
> is, but I'd find it cleaner.

I don't think it's possible, this is how Banshee does it, but I'm pretty sure I can safely remove that sleep call if it'd make you feel better.

> In Do.Platform/src/Do.Platform/AbstractPackageManagerService.cs:
> MaybePluginForPackage (string): It might be worthwhile extending the
> plugin API to allow a plugin itself to provide a list of packages that
> it supports, as well as this magic name/description matching.

I had mentioned this in irc, perhaps in the plugin manifest adding an element/attribute of keywords that could be used. This could also be handy in the search.

> What happens if multiple addins support a new package?

We take the first.

> This comment from PluginManagerService.cs seems wrong?
> /// If this class loads, we have a serious plugin because that probably
> means we have no plugin manager.

This is a comment for the Plugin Manager merge, but I'll fix it there

>  review needsfixing
> 
> Is this a merge for both PluginManagerService and PackageManagerService?
> If so, could we merge PluginManagerService separately, and then merge
> PackageManagerService after that has landed?

There is already a pending merge for PluginManagerService, this merge depends on it.

> You've added Do.Linux.JoliCloud/Makefile.in to bzr.  That's not very
> useful :).

Whoops. Removed.

> In Do.Platform.Linux.JoliCloud/src/Do.Platform/Do.Platform.Linux/Do.Platform.Linux.JoliCloud/PackageManagerService.cs:
> 
> I don't much like the magical Daemon property, particularly since you
> need to document it with the comment
> """
> // this call will instantiate the daemon, as well as make sure we also
> got a DBus object
> """
> in Initialize ().  Is there any particular reason that Initialise can't
> initialise the daemon explicitly?

I did it this way to avoid the duplication of adding the event, etc. It seems cleaner to me.

> In MaybeStartDaemon (), is it possibly to ask DBus to wait for an owner
> of BusName rather than just sleeping for 5 seconds?  I'm not sure if it
> is, but I'd find it cleaner.

I don't think it's possible, this is how Banshee does it, but I'm pretty sure I can safely remove that sleep call if it'd make you feel better.

I had mentioned this in irc, perhaps in the plugin manifest adding an element/attribute of keywords that could be used. This could also be handy in the search.
 
> What happens if multiple addins support a new package?

We take the first.

> This comment from PluginManagerService.cs seems wrong?
> /// If this class loads, we have a serious plugin because that probably
> means we have no plugin manager.
 
This is a comment for the Plugin Manager merge, but I'll fix it there

« Back to merge proposal