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

Revision history for this message
Andrea Azzarone (azzar1) 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?

---

+#ifndef EXPO_LAUNCHER_ICON_H

I'd prefer UNITYSHELL_EXPO... But it's not blocking :)

...

+FavoriteStore* favoritestore_instance = nullptr;

FavoriteStore should not be a singleton. You can use DI in launcher::Controller.

---

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

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

//auto const& system_dirs = DesktopUtilities::GetDataDirectories();

Remove it if you don't need it.

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

const :)

+ const std::string desktop_ext = ".desktop";

Make it static.

1199 - if (max > 0.0f)
1200 - {
1201 - color = color * (1.0f / max);
1202 - }
1203 + color = color * (1.0f / max);

Why have you done this?

* I'll continue the review later *

« Back to merge proposal