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

> > 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).

Really singleton makes testing easier? :) You can pass it to the icons using the ctor.

> 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? :)

Whops :) sorry

> 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.
>

ok

>
> > + 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 ;)