Code review comment for lp:~robertcarr/bamf/webapps

Thanks for your fixes, they all looks fine to me.

>> 1308 +void
>> 1309 +bamf_application_set_application_type (BamfApplication *application, const gchar *type)

>> An enum for types would be better than using a string value.

> What is the preferred way to keep the enum in sync over DBus?

Well, probably not on DBUs, but on the daemon side it would be better a more type-safe and defined value than just a string.

>> get_focused_child would be a better name imho

> This method is perhaps a little confusing. It is not to return the focused child though,

Mh, so maybe getting a proper name could be better :)

I know it's annoying... But I still see some tabs around in the code. It would be nice to get these removed before merging.

>> This looks over complicated... also instead of checking for == TRUE/FALSE I'd prefer to just check running/!running
>> Also, isn't enough not to set the running state?

> ?

I meant about this, sorry:

1454 + if (!((close_when_empty == FALSE) && running == FALSE))
1455 + bamf_view_set_user_visible (BAMF_VIEW (self), visible);
1456 + if ((running == TRUE) || close_when_empty)
1457 + bamf_view_set_running (BAMF_VIEW (self), running);

The first check for example could be just (!(!close_when_empty && !running)), even if I'd prefer the reversed form.
Also the bamf_view_set_running probably is enough, isn't it?

>> Also, the library is not currently testable in an easy way, but the daemon is (and all the types it handles) so I think we need tests for them.
> -.- We have some coverage by the webapps autopilot tests? Is this enough for now?

Well, AP could look too "far" from the actual implementation and it would be nice to get some new features tested here as well, however I can go with this, but the last word is up to the distro, btw.