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.

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";
}

  DeviceLauncherIcon::DeviceLauncherIcon(Launcher *launcher, GVolume *volume)
should be:
  DeviceLauncherIcon::DeviceLauncherIcon(Launcher* launcher, GVolume* volume)

Keep the pointer with the type not the variable name.

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

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

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

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

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

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?