Code review comment for lp://staging/~3v1n0/unity/launcher-draggable-icons

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

> 340 +std::string DesktopLauncherIcon::GetRemoteUri()
> 341 +{
> 342 + return "unity://desktop-icon";
> 343 +}
>
> +bool DesktopLauncherIcon::ShowInSwitcher(bool current)
> 351 +{
> 352 + return show_in_switcher_;
> 353 +}
>
> Can we make these methods const?

The first one no (due to BamfLauncherIcon). Probably it should be fixed there as well... I'll see.

> +FavoriteStore* favoritestore_instance = nullptr;
>
> FavoriteStore should not be a singleton. You can use DI in
> launcher::Controller.

I kept it in this way since it used to be like that, also it made testing easier (not to mention that it was a singleton because it was used also in the icons).
However, I could probably avoid to use it in this manner by adding a new icon signal and then using launcher::Controller. I see how it impacts on tests.

> 621 +bool FavoriteStore::IsValidFavoriteUri(std::string const& uri)
> 622 +{
> 623 + if (uri.empty())
> 624 + return false;
> 625 +
> 626 + if (uri.find(URI_PREFIX_APP) == 0 || uri.find(URI_PREFIX_FILE) ==
> 0)
> 627 + {
> 628 + return internal::impl::IsDesktopFilePath(uri);
> 629 + }
>
> Can this method be const? Also you know we prefer to use operator ! :)

A static method? :)
We use the operator! when a method returns false, but find returns the position of the found item, and we want to make sure that our strings starts with it.

> + const std::string prefix_separator = "://";
> + const std::string proto_separator = "://";
> 813 + const std::string desktop_ext = ".desktop";
>
> I'd make it static or I'd move it out of the function.

Yeah, probably... I thought the same.

> +bool FavoriteStoreGSettings::IsFavorite(std::string const& icon_uri)
>
> const :)

Yeah, I fogot to do it also for FavoritePosition.

> 1199 - if (max > 0.0f)
> 1200 - {
> 1201 - color = color * (1.0f / max);
> 1202 - }
> 1203 + color = color * (1.0f / max);
>
> Why have you done this?

Mhm, probably I made a wrong merge. Thanks for pointing out.

> * I'll continue the review later *

Nice, thanks ;)

« Back to merge proposal