Merge lp:~ted/indicator-network/modal-password into lp:indicator-network/13.10

Proposed by Ted Gould
Status: Merged
Approved by: Charles Kerr
Approved revision: 317
Merged at revision: 309
Proposed branch: lp:~ted/indicator-network/modal-password
Merge into: lp:indicator-network/13.10
Diff against target: 306 lines (+83/-46)
7 files modified
secret-agent/CMakeLists.txt (+1/-1)
secret-agent/PasswordMenu.cpp (+29/-12)
secret-agent/PasswordMenu.h (+1/-1)
secret-agent/SecretAgent.cpp (+24/-12)
secret-agent/SecretAgent.h (+2/-5)
secret-agent/SecretRequest.cpp (+23/-6)
secret-agent/SecretRequest.h (+3/-9)
To merge this branch: bzr merge lp:~ted/indicator-network/modal-password
Reviewer Review Type Date Requested Status
Pete Woods (community) Needs Fixing
Charles Kerr (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+191285@code.launchpad.net

Commit message

Make it so we have a single notification in flight at a time.

Description of the change

We were maintaining a list of notifications to be shown, this merge changes it so that we only have one and it handles all the close notifications, along with closing itself if disposed.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Charles Kerr (charlesk) wrote :

This change does sort of fix the original behavior, but the new behavior is also kind of strange.

Steps I used to test:

Step 1: Click on a WPA network
Expected: WPA network's checkbox toggles to checked + dialog prompt appears.
Actual: same. (Yay!)

Step 2: Click on the same WPA network
Expected: either (1) checkbox stays checked and dialog stays up, or (2) checkbox unchecks and dialog pops down
Actual: checkbox unchecks, dialog stays up

Step 3: Click a third time on the same WPA network
Expected: same as step 1
Actual: WPA network's checkbox toggles to checked. Then it toggles to unchecked. Then the overall Wi-Fi menuitem checkbox changes from checked to unchecked...?

review: Needs Fixing
315. By Ted Gould

Close the request on cancel

316. By Ted Gould

Deleted the wrong one of these earlier

317. By Ted Gould

Null check our delete

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ted Gould (ted) wrote :

On Tue, 2013-10-15 at 23:20 +0000, Charles Kerr wrote:

> This change does sort of fix the original behavior, but the new
> behavior is also kind of strange.

Went through those use-cases. I think it's better now.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Charles Kerr (charlesk) wrote :

Confirmed. The second click on the same network now pops down the dialog and unchecks that network's menuitem, which is sane. :)

review: Approve
Revision history for this message
Pete Woods (pete-woods) wrote :

Do not initialise a number with NULL, use 0. In-fact never use NULL in C++ code unless you're talking directly to a glib API.

Never write delete in C++. Use a managed pointer. When you make a new SecretRequest, you can just #reset the pointer to the new instance. This will automatically destruct the old one and put the new one in place.

I don't think you need to do the multiple attempts to export the menu thing. I think you should just export to a constant path, and guarantee the old menu is unexported before exporting a new one.

Seems a bit much to go through our own DBus API in the SecretRequest destructor.

The PasswordMenu class was in a gtk subfolder to make it clear that it was the only class to allow glib leakage.

review: Needs Fixing
Revision history for this message
Pete Woods (pete-woods) wrote :

There is also no regression test to prove you've actually fixed anything.

review: Needs Fixing
Revision history for this message
Pete Woods (pete-woods) wrote :

Scratch the comment about using our own DBus API in the destructor.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'secret-agent/CMakeLists.txt'
2--- secret-agent/CMakeLists.txt 2013-09-17 13:43:54 +0000
3+++ secret-agent/CMakeLists.txt 2013-10-16 02:50:59 +0000
4@@ -4,7 +4,7 @@
5 SecretAgent.cpp
6 SecretAgentAdaptor.cpp
7 SecretRequest.cpp
8- gtk/PasswordMenu.cpp
9+ PasswordMenu.cpp
10 )
11
12 qt5_add_dbus_adaptor(
13
14=== renamed file 'secret-agent/gtk/PasswordMenu.cpp' => 'secret-agent/PasswordMenu.cpp'
15--- secret-agent/gtk/PasswordMenu.cpp 2013-10-01 07:53:47 +0000
16+++ secret-agent/PasswordMenu.cpp 2013-10-16 02:50:59 +0000
17@@ -16,7 +16,7 @@
18 * Author: Pete Woods <pete.woods@canonical.com>
19 */
20
21-#include <gtk/PasswordMenu.h>
22+#include <PasswordMenu.h>
23 #include <gio/gio.h>
24
25 #include <QString>
26@@ -65,14 +65,13 @@
27
28 };
29
30-PasswordMenu::PasswordMenu(unsigned int requestId) :
31+PasswordMenu::PasswordMenu() :
32 p(new PasswordMenuPriv()) {
33+ int exportrev;
34+
35 p->m_busName = QString::fromUtf8(
36 g_dbus_connection_get_unique_name(p->m_connection));
37
38- p->m_actionPath = PASSWORD_ACTION_PATH.arg(requestId);
39- p->m_menuPath = PASSWORD_MENU_PATH.arg(requestId);
40-
41 // menu
42 GMenu *menu(g_menu_new());
43
44@@ -96,13 +95,31 @@
45
46 g_action_map_add_action(G_ACTION_MAP(actions), passwordAction);
47
48- p->m_exportedActionGroupId = g_dbus_connection_export_action_group(
49- p->m_connection, p->m_actionPath.toUtf8().data(), actions, NULL);
50-
51- p->m_exportedMenuModelId = g_dbus_connection_export_menu_model(
52- p->m_connection, p->m_menuPath.toUtf8().data(),
53- G_MENU_MODEL(menu), NULL);
54-
55+ /* Export the actions group. If we can't get a name, keep trying to
56+ use increasing numbers. There is possible races on fast import/exports.
57+ They're rare, but worth protecting against. */
58+ exportrev = 0;
59+ do {
60+ exportrev++;
61+ p->m_actionPath = PASSWORD_ACTION_PATH.arg(exportrev);
62+ p->m_exportedActionGroupId = g_dbus_connection_export_action_group(
63+ p->m_connection, p->m_actionPath.toUtf8().data(), actions, NULL);
64+ } while (p->m_exportedActionGroupId == 0 && exportrev < 128);
65+
66+ /* Export the menu. If we can't get a name, keep trying to
67+ use increasing numbers. There is possible races on fast import/exports.
68+ They're rare, but worth protecting against. */
69+ exportrev = 0;
70+ do {
71+ exportrev++;
72+ p->m_menuPath = PASSWORD_MENU_PATH.arg(exportrev);
73+ p->m_exportedMenuModelId = g_dbus_connection_export_menu_model(
74+ p->m_connection, p->m_menuPath.toUtf8().data(),
75+ G_MENU_MODEL(menu), NULL);
76+ } while (p->m_exportedMenuModelId == 0 && exportrev < 128);
77+
78+ /* Unref the objects as a reference is maintained by the fact that they're
79+ exported onto the bus. */
80 g_object_unref(menu);
81 g_object_unref(passwordItem);
82
83
84=== renamed file 'secret-agent/gtk/PasswordMenu.h' => 'secret-agent/PasswordMenu.h'
85--- secret-agent/gtk/PasswordMenu.h 2013-09-17 16:54:50 +0000
86+++ secret-agent/PasswordMenu.h 2013-10-16 02:50:59 +0000
87@@ -26,7 +26,7 @@
88
89 class PasswordMenu {
90 public:
91- PasswordMenu(unsigned int requestId);
92+ PasswordMenu();
93
94 virtual ~PasswordMenu();
95
96
97=== modified file 'secret-agent/SecretAgent.cpp'
98--- secret-agent/SecretAgent.cpp 2013-09-17 13:43:54 +0000
99+++ secret-agent/SecretAgent.cpp 2013-10-16 02:50:59 +0000
100@@ -48,8 +48,7 @@
101 systemConnection), m_sessionConnection(sessionConnection), m_agentManager(
102 NM_DBUS_SERVICE, NM_DBUS_PATH_AGENT_MANAGER, m_systemConnection), m_notifications(
103 "org.freedesktop.Notifications",
104- "/org/freedesktop/Notifications", m_sessionConnection), m_requestCounter(
105- 0) {
106+ "/org/freedesktop/Notifications", m_sessionConnection), m_request(NULL) {
107 if (!m_systemConnection.registerObject(NM_DBUS_PATH_SECRET_AGENT, this)) {
108 throw logic_error(
109 _("Unable to register user secret agent object on DBus"));
110@@ -112,26 +111,39 @@
111 message().createErrorReply(QDBusError::InternalError,
112 "No password found for this connection."));
113 } else {
114- SecretRequestPtr request(
115- new SecretRequest(m_requestCounter, *this, connection,
116- connectionPath, settingName, hints, flags, message()));
117- m_requests.insert(m_requestCounter, request);
118+ if (m_request != NULL) {
119+ delete m_request;
120+ m_request = NULL;
121+ }
122
123- ++m_requestCounter;
124+ m_request = new SecretRequest(*this, connection,
125+ connectionPath, settingName, hints, flags, message());
126 }
127
128 return QVariantDictMap();
129 }
130
131-void SecretAgent::FinishGetSecrets(SecretRequest &request) {
132- m_systemConnection.send(
133- request.message().createReply(
134- QVariant::fromValue(request.connection())));
135- m_requests.remove(request.requestId());
136+void SecretAgent::FinishGetSecrets(SecretRequest &request, bool error) {
137+ if (error) {
138+ m_systemConnection.send(
139+ request.message().createErrorReply(QDBusError::InternalError,
140+ "No password found for this connection."));
141+ } else {
142+ m_systemConnection.send(
143+ request.message().createReply(
144+ QVariant::fromValue(request.connection())));
145+ }
146+
147+ delete m_request;
148+ m_request = NULL;
149 }
150
151 void SecretAgent::CancelGetSecrets(const QDBusObjectPath &connectionPath,
152 const QString &settingName) {
153+ if (m_request != NULL) {
154+ delete m_request;
155+ m_request = NULL;
156+ }
157 }
158
159 void SecretAgent::DeleteSecrets(const QVariantDictMap &connection,
160
161=== modified file 'secret-agent/SecretAgent.h'
162--- secret-agent/SecretAgent.h 2013-09-17 13:43:54 +0000
163+++ secret-agent/SecretAgent.h 2013-10-16 02:50:59 +0000
164@@ -59,7 +59,7 @@
165 const QDBusObjectPath &connectionPath, const QString &settingName,
166 const QStringList &hints, uint flags);
167
168- void FinishGetSecrets(SecretRequest &request);
169+ void FinishGetSecrets(SecretRequest &request, bool error);
170
171 void CancelGetSecrets(const QDBusObjectPath &connectionPath,
172 const QString &settingName);
173@@ -83,10 +83,7 @@
174
175 org::freedesktop::Notifications m_notifications;
176
177- QMap<unsigned long long, SecretRequestPtr> m_requests;
178-
179- unsigned long long m_requestCounter;
180-
181+ SecretRequest * m_request;
182 };
183
184 #endif /* SECRETAGENT_H_ */
185
186=== modified file 'secret-agent/SecretRequest.cpp'
187--- secret-agent/SecretRequest.cpp 2013-10-15 20:26:30 +0000
188+++ secret-agent/SecretRequest.cpp 2013-10-16 02:50:59 +0000
189@@ -20,20 +20,22 @@
190 #include <SecretAgent.h>
191 #include <Localisation.h>
192
193-SecretRequest::SecretRequest(unsigned int requestId, SecretAgent &secretAgent,
194+SecretRequest::SecretRequest(SecretAgent &secretAgent,
195 const QVariantDictMap &connection,
196 const QDBusObjectPath &connectionPath, const QString &settingName,
197 const QStringList &hints, uint flags, const QDBusMessage &message,
198 QObject *parent) :
199- QObject(parent), m_requestId(requestId), m_secretAgent(secretAgent), m_connection(
200+ QObject(parent), m_secretAgent(secretAgent), m_connection(
201 connection), m_connectionPath(connectionPath), m_settingName(
202 settingName), m_hints(hints), m_flags(flags), m_message(
203- message), m_menu(requestId) {
204+ message), m_menu() {
205
206 connect(&m_secretAgent.notifications(),
207 SIGNAL(ActionInvoked(uint, const QString &)), this,
208 SLOT(actionInvoked(uint, const QString &)));
209
210+ connect(&m_secretAgent.notifications(), SIGNAL(NotificationClosed(uint, uint)), this, SLOT(notificationClosed(uint, uint)));
211+
212 // indicate to the notification-daemon, that we want to use snap-decisions
213 QVariantMap notificationHints;
214 notificationHints["x-canonical-snap-decisions"] = "true";
215@@ -73,14 +75,21 @@
216 }
217
218 SecretRequest::~SecretRequest() {
219+ /* Close the notification if it's open */
220+ if (m_notificationId != 0) {
221+ m_secretAgent.notifications().CloseNotification(m_notificationId).waitForFinished();
222+ m_notificationId = 0;
223+ }
224 }
225
226+/* Called when the user submits a password */
227 void SecretRequest::actionInvoked(uint id, const QString &actionKey) {
228 // Ignore other requests' notifications
229 if (id != m_notificationId) {
230 return;
231 }
232
233+ m_notificationId = 0;
234 QString key("");
235
236 if (actionKey == "connect_id") {
237@@ -98,11 +107,19 @@
238 wirelessSecurity->insert(SecretAgent::WIRELESS_SECURITY_WEP_KEY0, key);
239 }
240
241- m_secretAgent.FinishGetSecrets(*this);
242+ m_secretAgent.FinishGetSecrets(*this, false);
243 }
244
245-unsigned int SecretRequest::requestId() const {
246- return m_requestId;
247+/* Called when the user closes the dialog */
248+void SecretRequest::notificationClosed(uint id, uint reason) {
249+ // Ignore other requests' notifications
250+ if (id != m_notificationId) {
251+ return;
252+ }
253+
254+ m_notificationId = 0;
255+
256+ m_secretAgent.FinishGetSecrets(*this, true);
257 }
258
259 const QVariantDictMap & SecretRequest::connection() const {
260
261=== modified file 'secret-agent/SecretRequest.h'
262--- secret-agent/SecretRequest.h 2013-09-17 13:43:54 +0000
263+++ secret-agent/SecretRequest.h 2013-10-16 02:50:59 +0000
264@@ -20,21 +20,18 @@
265 #define SECRETREQUEST_H_
266
267 #include <DBusTypes.h>
268-#include <gtk/PasswordMenu.h>
269+#include <PasswordMenu.h>
270
271 #include <QDBusMessage>
272 #include <QDBusObjectPath>
273-#include <QSharedPointer>
274
275 class SecretRequest;
276 class SecretAgent;
277
278-typedef QSharedPointer<SecretRequest> SecretRequestPtr;
279-
280 class SecretRequest: public QObject {
281 Q_OBJECT
282 public:
283- explicit SecretRequest(unsigned int requestId, SecretAgent &secretAgent,
284+ explicit SecretRequest(SecretAgent &secretAgent,
285 const QVariantDictMap &connection,
286 const QDBusObjectPath &connectionPath, const QString &settingName,
287 const QStringList &hints, uint flags, const QDBusMessage &reply,
288@@ -44,17 +41,14 @@
289
290 public Q_SLOTS:
291 void actionInvoked(uint id, const QString &actionKey);
292+ void notificationClosed(uint id, uint reason);
293
294 public:
295- unsigned int requestId() const;
296-
297 const QVariantDictMap & connection() const;
298
299 const QDBusMessage & message() const;
300
301 protected:
302- const unsigned int m_requestId;
303-
304 unsigned int m_notificationId;
305
306 SecretAgent &m_secretAgent;

Subscribers

People subscribed via source and target branches