Code review comment for lp:~gordallott/unity/hud

Revision history for this message
Gord Allott (gordallott) wrote :

> Hi,
>
> There are a few introspection / autopilot issues with this branch:
>
> Your HUDController derives from Introspectable, but you never use it. You need
> to:
> 1) Add it to the introspection tree.At Diff line 2729 (where you create the
> HUDController instance) you need to call "AddChild(hud_controller_.get());".
> If the HUDController is ever deleted, you need to call
> "RemoveChild(hud_controller_.get());" BEFFORE the deletion takes place.
>
> 2) Add properties to the HUD controller that will be useful in autopilot
> tests.
>
>
> You need to repeat this process with all child objects of the HUDController
> that derive from Introspectable.
>
> Finally, the string returned from "GetName" should be "HUDController" - you
> don't need to qualify it with "unity.hud".
>
> In HUDIcon.h you #include "Introspectable.h", but never use it.
>
> In HUDView.h, you've declared a "const gchar* GetName()" method, which is from
> Introspectable, but there's two problems:
> 1) You don't derive from Introsopectable (as noted above, you should).
> 2) That's not the correct method signiture - the return type is
> "std::string", and the method is const.
>
> In the same class, make sure you add the search bar to the introspection tree
> - that way we can get the current search string.
>

Very much so, Introspection was on my todo list for earlier in the week before last minute changes bumped it off, will fix that up

> Finally, it'd be really nice to have some autopilot tests written when this is
> merged. I don't mind helping you with this.
>

Already have a guy from Systems helping out there, for now manual but he's going through and converting them :)

> Cheers,

« Back to merge proposal