> 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.
> 340 +std::string DesktopLauncher Icon::GetRemote Uri() //desktop- icon"; Icon::ShowInSwi tcher(bool current)
> 341 +{
> 342 + return "unity:
> 343 +}
>
> +bool DesktopLauncher
> 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; :Controller.
>
> FavoriteStore should not be a singleton. You can use DI in
> launcher:
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). :Controller. I see how it impacts on tests.
However, I could probably avoid to use it in this manner by adding a new icon signal and then using launcher:
> 621 +bool FavoriteStore: :IsValidFavorit eUri(std: :string const& uri) URI_PREFIX_ APP) == 0 || uri.find( URI_PREFIX_ FILE) == :impl:: IsDesktopFilePa th(uri) ;
> 622 +{
> 623 + if (uri.empty())
> 624 + return false;
> 625 +
> 626 + if (uri.find(
> 0)
> 627 + {
> 628 + return internal:
> 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 FavoriteStoreGS ettings: :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 ;)