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

> I notice that your editor is using tab stops. Can you configure it to use
> spaces instead of tabs? See the diff for
> plugins/unityshell/src/DeviceLauncherSection.cpp to see what I see.

Of course.

> If you are going to the trouble of adding an anonymous namespace, then you
> should replace the #define:
>
> plugins/unityshell/src/DeviceLauncherIcon.cpp
>
> namespace {
> const char* DEFAULT_ICON = "drive-removable-media";
> }

Of course.

> DeviceLauncherIcon::DeviceLauncherIcon(Launcher *launcher, GVolume *volume)
> should be:
> DeviceLauncherIcon::DeviceLauncherIcon(Launcher* launcher, GVolume* volume)
>
> Keep the pointer with the type not the variable name.

I don't like it, but it will be done ;)

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

> There is no point at all in adding the "inline" keyword to a function in the
> source file. Inlining only works if the compiler knows at the time the other
> source file is being compiled (at least I'm fairly sure about that).
>

Ops...

> > std::list<DbusmenuMenuitem *> DeviceLauncherIcon::GetMenus()
>
> I'm not a fan of returning containers from methods. Normally there is a
> better way to structure the program. You don't need to do anything about this
> at this stage, just somethingn to consider for the future.
>
> Although I just noticed that the list is containing things that you explicitly
> create. This means that every time you call this method you need to remember
> to explicitly free all of the objects. Hmm... I also see that you didn't
> create this method. I'll chase this up elsewhere.
>

I can do nothing here. This is the quicklist manager if i am not wrong. Let me know
what can i do here.

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

> Same for g_drive_stop.
> Also try for the g_signal_connect. It should work. Let me know if it
> doesn't.
>

I will try...

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