Merge lp://staging/~3v1n0/unity/launcher-draggable-icons into lp://staging/unity

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 *

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

What about the switcher icon? Is it also handled now by the favorites?

> What about the switcher icon? Is it also handled now by the favorites?

Yep, the special uri unity://expo-icon controls it, however it wont' be visible if the WS have been disabled.

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

1959 - uscreen->resuming.connect([&]() -> void {
1960 - for (auto launcher : launchers)
1961 - launcher->QueueDraw();
1962 - });

Why have you removed this code?

In volume VolumeLauncherIcon you are calling parent_->UnStick(); before TryToUnblacklist. I think you should call it when you receive the changed signal from device_settings.

+ void Stick(bool save = true);

You should not use default values for virtual function (consider NVI to fix this "issue").

200 +#ifndef TEST_MOCK_DEVICES_H
4201 +#define TEST_MOCK_DEVICES_H

Why do you need this guards?

*these

> 1959 - uscreen->resuming.connect([&]() -> void {
> 1960 - for (auto launcher : launchers)
> 1961 - launcher->QueueDraw();
> 1962 - });
>
> Why have you removed this code?

This code was added in order to workaround a nvidia redraw issue that we had on resuming, but since now we properly fixed the issue, we can safely remove this hack (tested here on one machine that was affected).

> In volume VolumeLauncherIcon you are calling parent_->UnStick(); before
> TryToUnblacklist. I think you should call it when you receive the changed
> signal from device_settings.

Why on changed signal?

> + void Stick(bool save = true);
>
> You should not use default values for virtual function (consider NVI to fix
> this "issue").

Yeah, this is not the best thing ever... It was introduced a lot time ago, probably it's better to split it into two methods (Stick and StickAndSave)

> 200 +#ifndef TEST_MOCK_DEVICES_H
> 4201 +#define TEST_MOCK_DEVICES_H
>
> Why do you need this guards?

Well, to avoid the redefinition of these objects. These won't probably be ever broken, but they won't surely create any issue.

> > 1959 - uscreen->resuming.connect([&]() -> void {
> > 1960 - for (auto launcher : launchers)
> > 1961 - launcher->QueueDraw();
> > 1962 - });
> >
> > Why have you removed this code?
>
> This code was added in order to workaround a nvidia redraw issue that we had
> on resuming, but since now we properly fixed the issue, we can safely remove
> this hack (tested here on one machine that was affected).
>

Cool.

>
> > In volume VolumeLauncherIcon you are calling parent_->UnStick(); before
> > TryToUnblacklist. I think you should call it when you receive the changed
> > signal from device_settings.
>
> Why on changed signal?
>

This is how it works now. When you call TryToUnblacklist devices_settings emit a changed signal. In the changed signal handler of VoluleLauncherIcon we update the visibility. Before we had ignore_signals inside device_settings and was error prone.

>
> > + void Stick(bool save = true);
> >
> > You should not use default values for virtual function (consider NVI to fix
> > this "issue").
>
> Yeah, this is not the best thing ever... It was introduced a lot time ago,
> probably it's better to split it into two methods (Stick and StickAndSave)
>
>
> > 200 +#ifndef TEST_MOCK_DEVICES_H
> > 4201 +#define TEST_MOCK_DEVICES_H
> >
> > Why do you need this guards?
>
> Well, to avoid the redefinition of these objects. These won't probably be ever
> broken, but they won't surely create any issue.

Yeah but i don't like it in a cpp file :) Use an unnamed namespace.

One question...

Is this intended?

Setup:
Open gedit
Move it to the top of the launcher
Make sure it is not locked to the launcher

Then:
Open the gnome-cacluator
Make sure it is on the bottom of the launcher
Right click and lock it to the launcher

Result:
The gnome-calc moves below the gedit icon that is not locked to the launcher...

Just wanted to double check that is expected :).

Other then that question, all the code looks good, the tests pass, and testing it manually also works :).

+1

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/1329/console reported an error when processing this lp:~3v1n0/unity/launcher-draggable-icons branch.
Not merging it.

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/1336/console reported an error when processing this lp:~3v1n0/unity/launcher-draggable-icons branch.
Not merging it.