Code review comment for lp:~bilalakhtar/unity/sc-integration-phase2

Revision history for this message
Bilal Akhtar (bilalakhtar) wrote :

> 189 + _finished = true;
> 190 + _finished_just_now = true;
>
> You don't need them anymore.

Those lines are part of the Finished function, so they're very much needed for the Activate function to work.

>
> 329 + AbstractLauncherIcon::Ptr self_abstract;
>
> Are you sure you need to save that on the class? I guess you can just
> Reference your icon before that it gets unreferenced...
>

That's the problem. I did try that, but it didn't work. Got a segfault (totally strange). Storing it's own reference in a variable worked, though.

> + if (((int)current_bamf_icon->GetCenter(launcher->monitor).x) != 0 &&
> 248 + ((int)current_bamf_icon->GetCenter(launcher->monitor).y) !=
> 0)
>
> Those are still C-style casts :)

Fixed in latest commit.

>
> 139 + static_cast<SoftwareCenterLauncherIcon*>(result.GetPointer())->Anim
> ate(launcher_, icon_x, icon_y, icon_size);
>
> Do this only if you're sure that this won't happen for another kind of icon...
> Otherwise use, at least, the dynamic_cast.

Yes, it won't happen, because I'm using a special function to create the result variable. That function is hard-coded to create only a SoftwareCenterLauncherIcon and nothing else. So a static cast is safe here.

Thanks for the review anyway, it would be great if you reviewed again!

« Back to merge proposal