Merge lp:~fhd/unity-2d/fix-for-bug-6600100 into lp:unity-2d

Proposed by Felix H. Dahlke
Status: Merged
Approved by: Florian Boucault
Approved revision: 634
Merged at revision: 638
Proposed branch: lp:~fhd/unity-2d/fix-for-bug-6600100
Merge into: lp:unity-2d
Diff against target: 157 lines (+77/-21)
2 files modified
libunity-2d-private/src/launcherdevice.cpp (+73/-21)
libunity-2d-private/src/launcherdevice.h (+4/-0)
To merge this branch: bzr merge lp:~fhd/unity-2d/fix-for-bug-6600100
Reviewer Review Type Date Requested Status
Florian Boucault (community) Approve
Marco Biscaro (community) Approve
Review via email: mp+70291@code.launchpad.net

This proposal supersedes a proposal from 2011-08-03.

Description of the change

[launcher] Added a "safely remove" option for USB sticks.

To post a comment you must log in.
Revision history for this message
Marco Biscaro (marcobiscaro2112) wrote : Posted in a previous version of this proposal

16 + if (g_mount_can_unmount(mount)) {
17 + g_mount_unmount_with_operation(mount, G_MOUNT_UNMOUNT_NONE, mountOperation, NULL,
18 + (GAsyncReadyCallback) LauncherDevice::onMountUnmounted,
19 + NULL);
20 + } else {
21 + g_object_unref(mount);
22 + }

The GMount object always need to be unref-ed, not only when if fails to unmount. The code should be something like:

if (mount) {
  if (g_mount_can_unmount(mount)) {
    g_mount_unmount_with_operation(mount, G_MOUNT_UNMOUNT_NONE, mountOperation, NULL,
                                   (GAsyncReadyCallback) LauncherDevice::onMountUnmounted,
                                   NULL);
  }
  g_object_unref(mount);
}

82 + g_object_unref(drive);

If, for some weird reason, drive is null, we can get in trouble. I think you need to check if this variable is set before trying to unref it.

review: Needs Fixing
Revision history for this message
Felix H. Dahlke (fhd) wrote : Posted in a previous version of this proposal

I've addressed the issues with the previous proposal.

Revision history for this message
Florian Boucault (fboucault) wrote : Posted in a previous version of this proposal

Superb work Felix. It works reliably here.
If you want to make the code more concise and ultimately less prone to memory leaks you may want to use GObjectScopedPointer from libunity-2d-private/src/gscopedpointer.h
You would use it like the following:

GObjectScopedPointer<GMount> g_volume_get_mount(m_volume);
[...]
if (g_mount_can_unmount(mount.data())) {
[...]

Revision history for this message
Florian Boucault (fboucault) wrote : Posted in a previous version of this proposal

Nitpick: in LauncherDevice::unmount(GMountOperation* mountOperation) there is no check for m_volume != NULL.

Revision history for this message
Florian Boucault (fboucault) wrote : Posted in a previous version of this proposal

One UX issue: the option should be present in the menu only if it can succeed, that is concretely if g_drive_can_stop returns true.

review: Needs Fixing
Revision history for this message
Felix H. Dahlke (fhd) wrote : Posted in a previous version of this proposal

> One UX issue: the option should be present in the menu only if it can succeed,
> that is concretely if g_drive_can_stop returns true.

How about the eject operation? It is always visible, not only when g_volume_can_eject returns true.

Revision history for this message
Florian Boucault (fboucault) wrote : Posted in a previous version of this proposal

> > One UX issue: the option should be present in the menu only if it can
> succeed,
> > that is concretely if g_drive_can_stop returns true.
>
> How about the eject operation? It is always visible, not only when
> g_volume_can_eject returns true.

I suppose the user wants to have a fallback in case neither g_volume_can_eject nor g_volume_can_stop return true that will call unmount.

Revision history for this message
Felix H. Dahlke (fhd) wrote : Posted in a previous version of this proposal

> Superb work Felix. It works reliably here.
> If you want to make the code more concise and ultimately less prone to memory
> leaks you may want to use GObjectScopedPointer from libunity-2d-
> private/src/gscopedpointer.h
> You would use it like the following:
>
> GObjectScopedPointer<GMount> g_volume_get_mount(m_volume);
> [...]
> if (g_mount_can_unmount(mount.data())) {
> [...]

I didn't know you had smart pointers for GObjects, glad to hear :)

Revision history for this message
Felix H. Dahlke (fhd) wrote : Posted in a previous version of this proposal

> > > One UX issue: the option should be present in the menu only if it can
> > succeed,
> > > that is concretely if g_drive_can_stop returns true.
> >
> > How about the eject operation? It is always visible, not only when
> > g_volume_can_eject returns true.
>
> I suppose the user wants to have a fallback in case neither g_volume_can_eject
> nor g_volume_can_stop return true that will call unmount.

Okay, I'll just add the check for safely remove then.

Revision history for this message
Felix H. Dahlke (fhd) wrote : Posted in a previous version of this proposal

> > > One UX issue: the option should be present in the menu only if it can
> > succeed,
> > > that is concretely if g_drive_can_stop returns true.
> >
> > How about the eject operation? It is always visible, not only when
> > g_volume_can_eject returns true.
>
> I suppose the user wants to have a fallback in case neither g_volume_can_eject
> nor g_volume_can_stop return true that will call unmount.

Another question though. I've changed the code so that the menu item is only added if g_drive_can_stop returns true. But isn't it possible that the drive becomes stoppable/unstoppable between the creation of the launcher icon and the user triggering "safely remove"?

Revision history for this message
Felix H. Dahlke (fhd) wrote :

I've addressed the new issues, but please note the question in my previous comment.

Revision history for this message
Marco Biscaro (marcobiscaro2112) wrote :

The code looks good. +1 for me.

> But isn't it possible that the drive becomes stoppable/unstoppable between the creation of the launcher icon and the user triggering "safely remove"?

I think this doesn't happens (afaik, the method g_drive_can_stop returns the correct information as soon as the drive is mounted and this info doesn't change after that).

review: Approve
Revision history for this message
Florian Boucault (fboucault) wrote :

Indeed it's great work!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libunity-2d-private/src/launcherdevice.cpp'
2--- libunity-2d-private/src/launcherdevice.cpp 2011-07-29 13:49:34 +0000
3+++ libunity-2d-private/src/launcherdevice.cpp 2011-08-03 16:12:37 +0000
4@@ -186,35 +186,65 @@
5 }
6
7 void
8+LauncherDevice::unmount(GMountOperation* mountOperation)
9+{
10+ if (m_volume == NULL) {
11+ return;
12+ }
13+
14+ GObjectScopedPointer<GMount> mount(g_volume_get_mount(m_volume));
15+
16+ if (mount.isNull()) {
17+ return;
18+ }
19+
20+ if (g_mount_can_unmount(mount.data())) {
21+ g_mount_unmount_with_operation(mount.data(), G_MOUNT_UNMOUNT_NONE, mountOperation, NULL,
22+ (GAsyncReadyCallback) LauncherDevice::onMountUnmounted,
23+ NULL);
24+ }
25+}
26+
27+void
28 LauncherDevice::eject()
29 {
30 if (m_volume == NULL) {
31 return;
32 }
33
34- GMountOperation *mountOperation;
35- mountOperation = gtk_mount_operation_new(NULL);
36+ GObjectScopedPointer<GMountOperation> mountOperation(gtk_mount_operation_new(NULL));
37
38 if (g_volume_can_eject(m_volume)) {
39- g_volume_eject_with_operation(m_volume, G_MOUNT_UNMOUNT_NONE, mountOperation,
40- NULL, (GAsyncReadyCallback) LauncherDevice::onVolumeEjected, NULL);
41- } else {
42- GMount* mount = g_volume_get_mount(m_volume);
43-
44- if (mount == NULL) {
45- return;
46- }
47-
48- if (g_mount_can_unmount(mount)) {
49- g_mount_unmount_with_operation(mount, G_MOUNT_UNMOUNT_NONE, mountOperation,
50- NULL, (GAsyncReadyCallback) LauncherDevice::onMountUnmounted,
51- NULL);
52- } else {
53- g_object_unref(mount);
54- }
55- }
56-
57- g_object_unref(mountOperation);
58+ g_volume_eject_with_operation(m_volume, G_MOUNT_UNMOUNT_NONE, mountOperation.data(),
59+ NULL,
60+ (GAsyncReadyCallback) LauncherDevice::onVolumeEjected,
61+ NULL);
62+ } else {
63+ unmount(mountOperation.data());
64+ }
65+}
66+
67+void
68+LauncherDevice::stop()
69+{
70+ if (m_volume == NULL) {
71+ return;
72+ }
73+
74+ GObjectScopedPointer<GDrive> drive(g_volume_get_drive(m_volume));
75+
76+ if (drive.isNull()) {
77+ return;
78+ }
79+
80+ GObjectScopedPointer<GMountOperation> mountOperation(gtk_mount_operation_new(NULL));
81+
82+ if (g_drive_can_stop(drive.data())) {
83+ g_drive_stop(drive.data(), G_MOUNT_UNMOUNT_NONE, mountOperation.data(), NULL,
84+ (GAsyncReadyCallback) LauncherDevice::onDriveStopped, NULL);
85+ } else {
86+ unmount(mountOperation.data());
87+ }
88 }
89
90 void
91@@ -224,6 +254,12 @@
92 }
93
94 void
95+LauncherDevice::onDriveStopped(GDrive* drive, GAsyncResult* res)
96+{
97+ g_drive_stop_finish(drive, res, NULL);
98+}
99+
100+void
101 LauncherDevice::onMountUnmounted(GMount* mount, GAsyncResult* res)
102 {
103 g_mount_unmount_with_operation_finish(mount, res, NULL);
104@@ -237,6 +273,15 @@
105 eject->setText(u2dTr("Eject"));
106 m_menu->addAction(eject);
107 QObject::connect(eject, SIGNAL(triggered()), this, SLOT(onEjectTriggered()));
108+
109+ GObjectScopedPointer<GDrive> drive(g_volume_get_drive(m_volume));
110+
111+ if (!drive.isNull() && g_drive_can_stop(drive.data())) {
112+ QAction* stop = new QAction(m_menu);
113+ stop->setText(u2dTr("Safely remove"));
114+ m_menu->addAction(stop);
115+ QObject::connect(stop, SIGNAL(triggered()), this, SLOT(onStopTriggered()));
116+ }
117 }
118
119 void
120@@ -246,4 +291,11 @@
121 eject();
122 }
123
124+void
125+LauncherDevice::onStopTriggered()
126+{
127+ m_menu->hide();
128+ stop();
129+}
130+
131 #include "launcherdevice.moc"
132
133=== modified file 'libunity-2d-private/src/launcherdevice.h'
134--- libunity-2d-private/src/launcherdevice.h 2011-07-29 13:49:34 +0000
135+++ libunity-2d-private/src/launcherdevice.h 2011-08-03 16:12:37 +0000
136@@ -51,18 +51,22 @@
137 Q_INVOKABLE GVolume* getVolume();
138 Q_INVOKABLE void setVolume(GVolume* volume);
139 Q_INVOKABLE void open();
140+ Q_INVOKABLE void unmount(GMountOperation* mountOperation);
141 Q_INVOKABLE void eject();
142+ Q_INVOKABLE void stop();
143
144 Q_INVOKABLE virtual void createMenuActions();
145
146 private Q_SLOTS:
147 void onEjectTriggered();
148+ void onStopTriggered();
149
150 private:
151 GVolume* m_volume;
152
153 static void onVolumeMounted(GVolume* volume, GAsyncResult* res);
154 static void onVolumeEjected(GVolume* volume, GAsyncResult* res);
155+ static void onDriveStopped(GDrive* drive, GAsyncResult* res);
156 static void onMountUnmounted(GMount* mount, GAsyncResult* res);
157 };
158

Subscribers

People subscribed via source and target branches