Merge lp://qastaging/~azzar1/unity/launcher-devices-improvement into lp://qastaging/unity

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?

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

I haven't tested what happens removing RawPtr in other computers (just on mine).

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

* I changed 'checked_' in `keep_in_launcher_` and also its behavior to make it more readable.
* I ported it to 4.0 trunk.

I'm happy with these changes. I'd like Neil to just confirm the lifecycle of the DeviceLauncherIcon you mentioned above. I've copied it here to make it easy for Neil :-)

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

@Tim, if the destructor of the DeviceLauncherIcon is called, it means that it works right?

@Tim I think that in glib::String::Str() there is a little problem:

=================================================================
std::string String::Str() const
{
  return std::string(string_);
}
=================================================================

In fact if string_ is NULL an exception is thrown (at least this happens on
my computer). I noticed this because this branch crash if we have
a gvolume without uuid (e.g. a cd-rom). This fix works good but i don't know
if this is the better way to fix:

=================================================================
std::string String::Str() const
{
  if (string_)
    return std::string(string_);
  else
    return std::string("");
}
=================================================================

You are the expert, so let me know if this is a good fix :)

On Sat, 25 Jun 2011 20:18:31 you wrote:
> @Tim I think that in glib::String::Str() there is a little problem:
>
> =================================================================
> std::string String::Str() const
> {
> return std::string(string_);
> }
> =================================================================
>
> In fact if string_ is NULL an exception is thrown (at least this happens on
> my computer). I noticed this because this branch crash if we have
> a gvolume without uuid (e.g. a cd-rom). This fix works good but i don't
> know if this is the better way to fix:
>
> =================================================================
> std::string String::Str() const
> {
> if (string_)
> return std::string(string_);
> else
> return std::string("");
> }
> =================================================================
>
> You are the expert, so let me know if this is a good fix :)

Yeah. I think that fix is fine.

Hey, sorry for the late merge. This looks good and works well. However it has a merge-conflict with trunk. If you could update it I'll merge it as soon as it's ready.

Nice work!

Could you also change the string 'springboard' to 'launcher' when fixing the merge conflict. Not many people are aware of what springboard is

Done.

Thanks Andrea, could you please ping me tomorrow as there's a few things I need to check with you before committing the code, thanks!

Of course, see you tomorrow...

Hey Andy, so I cleaned out my system and tested again on a fresh Nux and Unity install on Oneiric. Adding/removing USB/external harddrives/cd-rom's etc works perfectly. And the setting in ccsm works perfectly too, nice one!

However I never see the "Keep in Launcher" option on any device (when I have "Only Mounted" showing) :/ Is there something else that needs to change for me to see that? I've updated my system-wide unity schema with the one from your branch so it shouldn't be that...

"Keep in launcher" option should be visible only for un-removable devices (when "Only Mounted" is checked). I will work on it.

Hey Neil, maybe you have a "automounted configuration" in /etc/fstab (or something like that). I know, it's a my mistake, but if it is the problem, last commit should resolve the problem. Let me know!

Hey Andrea, fstab has no automount configuration for these drives (one is CD-ROM and other is 4G USB key). I tried your latest branch but still doesn't show the Keep In Launcher :( Are you on Oneiric? I wonder if something has changed...maybe tomorrow if you ping me we can debug this together, I can play around with the branch and get some more debug information for you.

Neil,

Why would you want a "Keep in Launcher" option for a removable device?

Neil, i am on Oneiric...
I'll ping you as soon as possible, by the way you can printf the value of:
DevicesSettings::GetDefault().GetDevicesOption() == DevicesSettings::ONLY_MOUNTED

and of:
g_drive_is_media_removable (drive)

in DeviceLauncherIcons::GetMenus

I don't, but it was part of the design I believe.

@andyrock, will try and do that tomorrow morning (am not at computer today)

Sent from my iPhone

On 7 Jul 2011, at 22:56, Tim Penhey <email address hidden> wrote:

> Neil,
>
> Why would you want a "Keep in Launcher" option for a removable device?
>
> --
> https://code.launchpad.net/~andyrock/unity/launcher-devices-improvement/+merge/65537
> You are reviewing the proposed merge of lp:~andyrock/unity/launcher-devices-improvement into lp:unity.

===================================================================
1) It should be possible to remove specific file system icons from the Launcher by un-checking the “Keep in Launcher” option (or dragging to the trash).
===================================================================

This branch use the "Keep in Launcher" option but not the dragging to the trash, since there is another bug report (#764905), and my merge proposal, with a different behavior: «Drag and drop a USB key into the trash should eject the USB key».

===================================================================
2) In the case of non-removable file systems, the system should remember these settings so that rebooting will not return these icons to the Launcher.
===================================================================
Ok, this branch does it (at least I hope :)

===================================================================
3) In the case of removable file systems (USB keys, SD cards, etc...), if a removable file system is removed and then re-attached it should re-appear in the Launcher. If the removable file system is not removed and the user has removed it's icon from the Launcher, the icon should not return to the Launcher even if the computer is re-booted.
===================================================================

According to me, this is absurd and make no sense and not so easy to do! :)

===================================================================
4) All 'specific file system' icons that the user has removed from the Launcher should be displayed in the Dash file Lens in the “Favourite Folders/Folders” category. This allows the user to re-add these icons to the Launcher at a later date
===================================================================
According to me this is in part a unity-file-place (???) bug... So first we should add the "specific file system" icons in the Dash file Lens and i cannot (i hate python!) do this.