Code review comment for lp:~3v1n0/unity/launcher-controller-ensure-new

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

> Looks good.
>
> There are some merge markers in added file
> 'tests/test_launcher_controller.cpp'.

Yeah, fixed...

> General comments:
>
> * If you want to avoid the friend declaration, you can do something like
> [...]
> This method is pretty ugly though, and the friend might be better.

Yes, that's what I generally do, but I didn't want all this even for future testing...

> Generally speaking, testing implementation details isn't really recommended,
> but in this case we want to do that.

Yep, that's the fact.

> * Its a bit unclear as to how UScreen.SetupFakeMultimonitor () causes more
> launchers to be added to an lc it doesn't know about, unless some singleton
> knows about lc, and that's pretty confusing.

As you can see, int SetupFakeMultimonitor implementation, basically I'm adding to it some fake monitors, and then emitting that UScreen has changed, exactly like if I'd have plugged some monitors.

« Back to merge proposal