Code review comment for lp://qastaging/~3v1n0/unity/panel-p-tests

Revision history for this message
Marco Trevisan (TreviƱo) (3v1n0) wrote :

> Hi,
> Can we make this conformant to the OCP? Create a 'virtual std::string
> GetTypeName() const', and have derived classes return their type string. That
> way, when you add a new indicator type, this code won't break.

Currently both the MENU and INDICATOR IndicatorEntry types are managed by the PanelIndicatorEntry class, that's why I've just added that, since I only needed to get an human readable type on AP side.
So I can't just add a such virtual method, other than one that does basically the same of what I'm doing here: manually checks for the type and sets a string.

> ...... on the python side:
> Please remove...
>
> 581 + #FIXME, remove it as soon as the better implementation goes in trunk
> 582 + def get_bamf_application(self, name):
> 583 + apps = self.get_app_instances(name)
> 584 + self.assertThat(len(apps), Equals(1))
> 585 + return apps[0]
>
> Why not merge in the branch that has this code, and list it as a prerequisite
> branch on the MP?

I've already set another branch as prerequisite... I couldn't do that. :(

> 587 + def move_window_to_monitor(self, window, monitor):
>
> This method should go somewhere else, it doesn't belong in the test case
> class. I suggest putting it in the ScreenGeometry class instead.

Mh, ok... I just left that there for debugging now, I'll move to the proper class as you suggested soon.

Thanks a lot for your tips, Thomi ;)

« Back to merge proposal