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

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