Merge lp:~jpakkane/unity-notifications/simpleqml into lp:unity-notifications

Proposed by Jussi Pakkanen on 2013-07-02
Status: Needs review
Proposed branch: lp:~jpakkane/unity-notifications/simpleqml
Merge into: lp:unity-notifications
Diff against target: 196 lines (+110/-28)
5 files modified
include/SimpleQmlSender.h (+50/-0)
include/notify-backend.h.in (+1/-14)
src/CMakeLists.txt (+2/-2)
src/NotificationClientPlugin.cpp (+13/-12)
src/SimpleQmlSender.cpp (+44/-0)
To merge this branch: bzr merge lp:~jpakkane/unity-notifications/simpleqml
Reviewer Review Type Date Requested Status
Michael Zanetti (community) Needs Fixing on 2013-12-04
PS Jenkins bot (community) continuous-integration Approve on 2013-07-04
Antti Kaijanmäki (community) 2013-07-02 Needs Fixing on 2013-07-03
Michał Sawicz 2013-07-08 Pending
Review via email: mp+172478@code.launchpad.net

Commit message

Added simple Qml notification sender.

Description of the change

Made a simple QML plugin for fire-and-forget notifications. Also a few other cleanups and fixes.

To post a comment you must log in.
170. By Jussi Pakkanen on 2013-07-02

Made SimpleQmlSender final.

Antti Kaijanmäki (kaijanmaki) wrote :

136 + SimpleQmlSender *cl = new SimpleQmlSender(engine);
137 + engine->rootContext()->setContextProperty("unity.notifications", cl);

I probably mentioned this before, but IMO our QML components should not mess with the QML engine directly if at all possible, but this is just my opinion. You might want to sync with the SDK team and ask if this is OK for them.

173 + QString app_name("client");
178 + QString app_icon;

So, there is no way for the app developer to set the app_name of app_icon? I guess these will not be visible on the notification bubble then.

Antti Kaijanmäki (kaijanmaki) wrote :

183 + if(!result.isValid()) {
184 + fprintf(stderr, "Sending notification failed.\n");
185 + }

Please, use qWarning() or qCritical(). This way the application which loads this component can get the messages redirected by setting the logger handlers if needed.

review: Needs Fixing
Antti Kaijanmäki (kaijanmaki) wrote :

> 183 + if(!result.isValid()) {
> 184 + fprintf(stderr, "Sending notification failed.\n");
> 185 + }
>
> Please, use qWarning() or qCritical(). This way the application which loads
> this component can get the messages redirected by setting the logger handlers
> if needed.

Also, provide the reply.error().name() and reply.error().message() as part of the error message.

If you include <QDebug> you also get operator<< for the qWarning(), qCritical(), etc. which makes your life a lot easier:

    qCritical() << "Sending notification failed:" << reply.error().name() << reply.error().message();

I havent tested, but it's possible that you can even do:

    qCritical() << "Sending notification failed:" << reply.error();

171. By Jussi Pakkanen on 2013-07-04

Use qCritical for logging.

172. By Jussi Pakkanen on 2013-07-04

Have user specify app name.

173. By Jussi Pakkanen on 2013-07-04

Made sender a singleton rather than modifying root context.

Jussi Pakkanen (jpakkane) wrote :

Fixed. App icon is still not settable and will not be.

Michael Zanetti (mzanetti) wrote :

132 - qmlRegisterUncreatableType<NotificationClient>(uri, 1, 0, "NotificationClient", "");
133 + qmlRegisterSingletonType<SimpleQmlSender>(uri, 1, 0, "unity.notifications", senderProvider);

Why this? QML types are supposed to be uppercase and the . is really confusing (if it works at all) because usually its the delimiter for named imports.

=====

I'd fancy a test :)

review: Needs Fixing

Unmerged revisions

173. By Jussi Pakkanen on 2013-07-04

Made sender a singleton rather than modifying root context.

172. By Jussi Pakkanen on 2013-07-04

Have user specify app name.

171. By Jussi Pakkanen on 2013-07-04

Use qCritical for logging.

170. By Jussi Pakkanen on 2013-07-02

Made SimpleQmlSender final.

169. By Jussi Pakkanen on 2013-07-02

Configure header cleanup.

168. By Jussi Pakkanen on 2013-07-02

Clarification and removeification.

167. By Jussi Pakkanen on 2013-07-02

Provide a simple notification sender plugin for Qml.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'include/SimpleQmlSender.h'
2--- include/SimpleQmlSender.h 1970-01-01 00:00:00 +0000
3+++ include/SimpleQmlSender.h 2013-07-04 07:27:24 +0000
4@@ -0,0 +1,50 @@
5+/*
6+ * Copyright (C) 2013 Canonical, Ltd.
7+ *
8+ * Authors:
9+ * Jussi Pakkanen <jussi.pakkanen@canonical.com>
10+ *
11+ * This program is free software: you can redistribute it and/or modify it
12+ * under the terms of the GNU General Public License version 3, as published
13+ * by the Free Software Foundation.
14+ *
15+ * This library is distributed in the hope that it will be useful, but WITHOUT
16+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
17+ * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more
18+ * details.
19+ *
20+ * You should have received a copy of the GNU General Public License
21+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
22+ */
23+
24+#ifndef SIMPLE_QML_SENDER_
25+#define SIMPLE_QML_SENDER_
26+
27+#include "SimpleQmlSender.h"
28+
29+#include <QObject>
30+#include <QDBusInterface>
31+
32+/**
33+ * A simple interface to desktop notifications. Just sends text and
34+ * ignores everything else. If and when full notification functionality
35+ * is required, drop this class and base the full implementation on
36+ * NotificationClient.
37+ */
38+
39+class SimpleQmlSender final : public QObject {
40+ Q_OBJECT
41+
42+public:
43+ SimpleQmlSender(QObject *parent = nullptr);
44+ ~SimpleQmlSender() {};
45+
46+ Q_INVOKABLE void sendNotification(QString app_name, QString summary, QString body);
47+
48+private:
49+ QDBusInterface service;
50+
51+};
52+
53+
54+#endif
55
56=== modified file 'include/notify-backend.h.in'
57--- include/notify-backend.h.in 2013-06-12 08:06:20 +0000
58+++ include/notify-backend.h.in 2013-07-04 07:27:24 +0000
59@@ -26,26 +26,13 @@
60
61 typedef unsigned int NotificationID;
62
63-/*enum Urgency {
64- URGENCY_LOW,
65- URGENCY_NORMAL,
66- URGENCY_CRITICAL
67-};
68-
69-enum NotificationType {
70- SYNCHRONOUS,
71- SNAP,
72- INTERACTIVE,
73- ASYNCHRONOUS
74-};*/
75-
76 const unsigned int MAX_NOTIFICATIONS = 50;
77
78 class Renderer;
79 class NotificationBackend;
80 class Notification;
81
82-#if 0
83+#if @PRIVATE_DBUS@
84 #define DBUS_SERVICE_NAME "com.canonical.notificationproto"
85 #define DBUS_INTERFACE "com.canonical.notificationproto"
86 #define DBUS_PATH "/com/canonical/notificationproto"
87
88=== modified file 'src/CMakeLists.txt'
89--- src/CMakeLists.txt 2013-06-18 08:36:08 +0000
90+++ src/CMakeLists.txt 2013-07-04 07:27:24 +0000
91@@ -34,8 +34,8 @@
92 add_library(notifyclientplugin MODULE
93 NotificationClientPlugin.cpp
94 ../include/NotificationClientPlugin.h
95-NotificationClient.cpp
96-../include/NotificationClient.h
97+SimpleQmlSender.cpp
98+../include/SimpleQmlSender.h
99 )
100
101 set_target_properties(notifyclientplugin PROPERTIES
102
103=== modified file 'src/NotificationClientPlugin.cpp'
104--- src/NotificationClientPlugin.cpp 2013-06-18 08:36:08 +0000
105+++ src/NotificationClientPlugin.cpp 2013-07-04 07:27:24 +0000
106@@ -17,8 +17,9 @@
107 * along with this program. If not, see <http://www.gnu.org/licenses/>.
108 */
109
110-#include "NotificationClient.h"
111+#include "SimpleQmlSender.h"
112 #include "NotificationClientPlugin.h"
113+#include "notify-backend.h"
114
115 #include <qqml.h>
116
117@@ -26,19 +27,19 @@
118 #include <QQmlEngine>
119 #include <QQmlContext>
120
121+SimpleQmlSender *s = nullptr;
122+
123+static QObject* senderProvider(QQmlEngine* engine, QJSEngine* /* scriptEngine */)
124+{
125+ if(!s)
126+ s = new SimpleQmlSender(engine);
127+ return s;
128+}
129+
130+
131 void NotificationClientPlugin::registerTypes(const char *uri) {
132- qmlRegisterUncreatableType<NotificationClient>(uri, 1, 0, "NotificationClient", "");
133+ qmlRegisterSingletonType<SimpleQmlSender>(uri, 1, 0, "unity.notifications", senderProvider);
134 }
135
136 void NotificationClientPlugin::initializeEngine(QQmlEngine *engine, const char *uri) {
137- NotificationClient *cl = new NotificationClient(engine);
138- if(!QDBusConnection::sessionBus().connect(DBUS_SERVICE_NAME, DBUS_PATH, DBUS_INTERFACE,
139- "NotificationClosed", cl, SLOT(NotificationClosed(unsigned int, unsigned int)))) {
140- printf("Could not connect to NotificationClosed signal.\n\n");
141- }
142- if(!QDBusConnection::sessionBus().connect(DBUS_SERVICE_NAME, DBUS_PATH, DBUS_INTERFACE,
143- "ActionInvoked", cl, SLOT(ActionInvoked(unsigned int, QString)))) {
144- printf("Could not connect to ActionInvoked signal.\n\n");
145- }
146- engine->rootContext()->setContextProperty("notificationclient", cl);
147 }
148
149=== added file 'src/SimpleQmlSender.cpp'
150--- src/SimpleQmlSender.cpp 1970-01-01 00:00:00 +0000
151+++ src/SimpleQmlSender.cpp 2013-07-04 07:27:24 +0000
152@@ -0,0 +1,44 @@
153+/*
154+ * Copyright (C) 2013 Canonical, Ltd.
155+ *
156+ * Authors:
157+ * Jussi Pakkanen <jussi.pakkanen@canonical.com>
158+ *
159+ * This program is free software: you can redistribute it and/or modify it
160+ * under the terms of the GNU General Public License version 3, as published
161+ * by the Free Software Foundation.
162+ *
163+ * This library is distributed in the hope that it will be useful, but WITHOUT
164+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
165+ * FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License for more
166+ * details.
167+ *
168+ * You should have received a copy of the GNU General Public License
169+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
170+ */
171+
172+#include "SimpleQmlSender.h"
173+#include "notify-backend.h"
174+#include <QStringList>
175+#include <QDBusReply>
176+#include <QtDebug>
177+
178+SimpleQmlSender::SimpleQmlSender(QObject *parent) : QObject(parent),
179+ service(DBUS_SERVICE_NAME, DBUS_PATH, DBUS_INTERFACE) {
180+}
181+
182+void SimpleQmlSender::sendNotification(QString app_name, QString summary, QString body) {
183+ unsigned int replaces_id = 0;
184+ QStringList actions;
185+ QMap<QString, QVariant> hints;
186+ hints["urgency"] = (char) 1; // Urgency normal.
187+ QString app_icon;
188+
189+ int timeout = 5000;
190+ QDBusReply<unsigned int> result = service.call("Notify",
191+ app_name, replaces_id, app_icon, summary, body, actions, hints, timeout);
192+ if(!result.isValid()) {
193+ qCritical() << "Sending notification failed: " << result.error().name() << ", "
194+ << result.error().message();
195+ }
196+}

Subscribers

People subscribed via source and target branches

to all changes: