Code review comment for lp:~abreu-alexandre/unity/dbus-stick-unstick-launcher-icon

Alexandre Abreu (abreu-alexandre) wrote :

> 22 + std::string app_uri;
> 23 + if (desktop_path.empty())
> 24 + return app_uri;
>
> You can just return "" in that case, without initializing app_uri.

Done.

> Also at the end of the function returning a temporary it's fine anyway..

mmh, it's returned by value & copy constructed ...

> 46 +Controller::Impl::OnLauncherUpdateIconStickyState(std::string const&
> icon_uri,
> 47 + bool sticky)
>
> Please, keep everything in one-line.

Done.

>
>
> 61 + AbstractLauncherIcon::Ptr existing_icon_entry =
> 62 + GetIconByUri(target_uri);
>
> One line, plus use const& (or auto const&).

Done.

>
>
> 72 + SortAndUpdate();
>
> Please avoid to do this, if nothing has been actually changed.

Good catch, done.

>
>
> 86 + else
> 87 + favorite_store.RemoveFavorite(target_uri);
>
> Add brackets here too please. However in that case, when can happen that a
> favorite has not a related application icon? Going in edge cases it's ok,
> anyway.

Done.

>
>
> 140 + ASSERT_FALSE(favorite_store.IsFavorite(app_icon->RemoteUri()));
>
> This is quite redundant as it should be guarantee by other tests we have.

Yeah, but it is somewhat consistent w/ the test's logic & strict internals here imo,

« Back to merge proposal