Unity

Merge lp://staging/~azzar1/unity/fix-doubled-device-launcher-icons into lp://staging/unity

fix-doubled-device-launcher-icons
Merge into trunk

Proposed by Andrea Azzarone on 2012-07-18

Status:

Merged

Approved by:

Andrea Azzarone on 2012-07-18

Approved revision:

no longer in the source branch.

Merged at revision:

2511

Proposed branch:

lp://staging/~azzar1/unity/fix-doubled-device-launcher-icons

Merge into:

lp://staging/unity

Diff against target:

791 lines (+562/-45)

12 files modified

launcher/AbstractVolumeMonitorWrapper.h (+61/-0)
launcher/CMakeLists.txt (+1/-0)
launcher/DeviceLauncherIcon.h (+2/-1)
launcher/DeviceLauncherSection.cpp (+23/-27)
launcher/DeviceLauncherSection.h (+8/-17)
launcher/LauncherController.cpp (+4/-0)
launcher/VolumeMonitorWrapper.cpp (+67/-0)
launcher/VolumeMonitorWrapper.h (+51/-0)
tests/CMakeLists.txt (+5/-0)
tests/gmockvolume.c (+176/-0)
tests/gmockvolume.h (+53/-0)
tests/test_device_launcher_section.cpp (+111/-0)

To merge this branch:

bzr merge lp://staging/~azzar1/unity/fix-doubled-device-launcher-icons

Related bugs:

Bug #1010714: logging out/in with mounted drives causes unity launcher icons to be duplicated	Medium	Fix Released
Bug #1010858: Mounted volume icons doubled up in launcher	Medium	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Marco Trevisan (Treviño)		2012-07-18	Approve on 2012-07-18
jenkins (community)	continuous-integration	2012-07-18	Needs Fixing on 2012-07-18
Review via email: mp+115601@code.staging.launchpad.net

This proposal supersedes a proposal from 2012-07-17.

Commit message

Fix duplicated device launcher icons.

Description of the change

== Problem ==
Mounted volume icons doubled up in launcher.

== Fix ==
Add a check to avid duplicated.

== Test ==
Unit test added.

Revision history for this message

jenkins (martin-mrazik+qa) wrote on 2012-07-17: Posted in a previous version of this proposal

PASSED: Continuous integration, rev:2511
http://jenkins.qa.ubuntu.com/job/unity-ci/90/
Executed test runs:
SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=amd64/90/console
SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=i386/90/console

review: Approve (continuous-integration)

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2012-07-18: Posted in a previous version of this proposal

See if it's convenient to change the map_ to use AbstractLauncherIcon::Ptr so that you can do

map_[volume] = std::make_shared<DeviceLauncherIcon>(volume); :P

299 +class VolumeMonitorWrapper : public sigc::trackable

I would make it uncopyable as well.

225 + GList* volumes = g_volume_monitor_get_volumes(monitor_);
238 + g_list_free(volumes);

You can be smarter doing:

auto volumes = std::shared_ptr<GList>(g_volume_monitor_get_volumes(monitor_), g_list_free))
then for (GList* v = volumes.get() ....)

306 + std::list<glib::Object<GVolume>> GetVolumes();

Since it's public a typedef can help here (even if I'm not a fan of them generally).

See if you can test the wrapper using a fake GVolume or an object implementing GVolumeIface

Revision history for this message

Andrea Azzarone (azzar1) wrote on 2012-07-18: Posted in a previous version of this proposal

> See if it's convenient to change the map_ to use AbstractLauncherIcon::Ptr so
> that you can do
>
> map_[volume] = std::make_shared<DeviceLauncherIcon>(volume); :P

Done :)

>
> 299 +class VolumeMonitorWrapper : public sigc::trackable
>
> I would make it uncopyable as well.

Not essential, but done.

>
> 225 + GList* volumes = g_volume_monitor_get_volumes(monitor_);
> 238 + g_list_free(volumes);
>
> You can be smarter doing:
>
> auto volumes = std::shared_ptr<GList>(g_volume_monitor_get_volumes(monitor_),
> g_list_free))
> then for (GList* v = volumes.get() ....)
>