Code review comment for lp:~azzar1/unity/launcher-devices-improvement

> > > void DeviceLauncherIcon::UpdateDeviceIcon()
> >
> > The extra scoping you have in this method don't really help the readability,
> > nor are they required for early destruction of the objects. Can you remove
> > them please?
> >
>
> Sure. They are not my method.

Aah. You are right. I can now see that you just replaced the gchar with the
glib::String. The old scopes were there to show the lifetime of the glib
created data, and they are no longer needed. Thanks for cleaning this up.

> > > std::list<DbusmenuMenuitem *> DeviceLauncherIcon::GetMenus()
> I can do nothing here. This is the quicklist manager if i am not wrong. Let me
> know what can i do here.

I did say that I can see this wasn't created by you :-) I will chase up the
design.

> > > void DeviceLauncherIcon::ShowMount(GMount *mount)
> >
> > Please keep the pointer with the type.
> >
> > > g_mount_unmount_with_operation(mount.RawPtr(),
> >
> > This should work without the .RawPtr() call. The object implements an
> > operator OBJECT* method, and since the g_mount_unmount_with_operation
> expects
> > a GMount, the method is used.
> >
>
> I use this because sometimes i got warnings.

There would be warnings in the places where it wasn't clear to the compiler
what you were wanting. But for most places it should work without the
.RawPtr() call.

> > std::map<GVolume *, DeviceLauncherIcon *> is screaming for a typedef. The
> > whole using a map based on pointers makes me queezy. I don't feel
> comfortable
> > that the contained values are destroyed consistently. The are newed when
> > added to the map, but removal just calls erase. I don't see a corresponding
> > delete. Instead of storing raw pointers, could we key it off a consistent
> > name perhaps? And store smart pointers?
>
> Instead of <GVolume*> we could use a uuid but according to me is too slow...
> For the key
> (GVolume *) we don't need a g_object_unref(...).
> For what regard the value (DeviceLauncherIcon *) we cannot use a smart pointer
> (i used a boost one) because when we OnVolumeRemoved is called, we call
> "self->map_[volume]->OnRemoved();" that call IconLauncher::Remove() that emit
> a signal that is handled by LancherController.cpp or LauncherModels.cpp (I
> don't remember! :) ) that unref the icon... If we use a smartpointer we
> "unref" it twice... Correct me if I'm wrong.

You see... here is my problem. None of this is obvious to me (the reader of
the code). I'll leave that logic part to Neil to verify.

= New stuff =

You have a checked_ member variable. "Checked" I think could be changed to
something a little more descriptive. "keep_in_launcher_" perhaps?

The constructor sets:
  checked_ = !(pos != favorites.end());
this is more simply written:
  checked_ = pos == favorites.end();

Which then makes me wonder, what is the purpose of "checked_".

This same double negative logic is found in
DeviceLauncherIcon::OnSettingsChanged

In summary, the only things I'd like to hear back about is the checked_
member, and the double negative logic.

Thanks for the updates.