Merge lp:~macslow/unity-notifications/modal-snap-decisions into lp:unity-notifications

[Mirco Müller]

Make the Roles enum available to QML.

Make the Roles enum available to QML.

This is needed for the MR https://code.launchpad.net/~macslow/unity8/modal-snap-decisions/+merge/210988 to work with.

* Are there any related MPs required for this MP to build/function as expected? Please list.
Yes. https://code.launchpad.net/~macslow/unity-api/version-bump-to-0.1.3/+merge/216122 needs to land before this one will work.

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
Yes.

* If you changed the UI, has there been a design review?
Not applicable.

[Michael Zanetti (mzanetti)]

Hmm... I think this should rather be defined in lp:unity-api and teted for its existence here in tst_NotificaitonInterface.qml, no?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:201
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/49/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-amd64-ci/24
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/24
        deb: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/24/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-i386-ci/24

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/49/rebuild

review: Approve (continuous-integration)

[Michał Sawicz (saviq)]

Yeah, please submit a similar thing to lp:unity-api, even though this is not using it yet (it should!), let's keep it as close as possible.

[Mirco Müller (macslow)]

Just working on that.

[Mirco Müller (macslow)]

Here's the Unity-API MP going alongside this one https://code.launchpad.net/~macslow/unity-api/expose-notification-data-roles-to-qml/+merge/212581

[Michael Zanetti (mzanetti)]

Even though this model doesn't inherit the one from unity-api yet, I'm wondering if it wouldn't make sense to already now use the enums in there. Without having tested it, do you think we could just drop the enum definition in here and use ModelInterface::Roles everywhere in here already? Otherwise I see it drifting apart again.

review: Needs Information

[Mirco Müller (macslow)]

Ill will make sure we can use the enum from unity-api as drop-in here and see that it will still work (or make it work) with https://code.launchpad.net/~macslow/unity8/modal-snap-decisions/+merge/210988

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:202
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/50/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-amd64-ci/25/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/25/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-i386-ci/25/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/50/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:204
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/51/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-amd64-ci/26/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/26/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-i386-ci/26/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/51/rebuild

review: Needs Fixing (continuous-integration)

[Albert Astals Cid (aacid)]

Q_ENUMS(Roles) seems like it should go away

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:204
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/52/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-amd64-ci/27
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/27
        deb: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/27/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-i386-ci/27

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/52/rebuild

review: Approve (continuous-integration)

[Albert Astals Cid (aacid)]

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes, still works

* Did CI run pass? If not, please explain why.
Yes.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:205
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/53/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-amd64-ci/28
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/28
        deb: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/28/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-i386-ci/28

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/53/rebuild

review: Approve (continuous-integration)

[Michał Sawicz (saviq)]

This needs a bump in unity-notifications-impl provided, to go with a bump in a .pc file bump in unity-api.

See https://code.launchpad.net/~macslow/unity8/modal-snap-decisions/+merge/210988/comments/512330 for details.

review: Needs Fixing

[lp:~macslow/unity-notifications/modal-snap-decisions]

206. By Mirco Müller on 2014-04-16

Make unity-notifications require at least unity-api 0.1.3 and thus also bump the Provides to unity-notifications-impl-3.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:206
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/54/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-amd64-ci/29/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/29/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-i386-ci/29/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/54/rebuild

review: Needs Fixing (continuous-integration)

[Michał Sawicz (saviq)]

You're right, in a sense, that this implements both v2 and v3 of the Notifications API, but I don't think we're at the point where we care. Just drop the -impl-2 and replace with -impl-3 please.

The whole idea is like so:
unity-api defines the API by providing headers, for each group of interfaces (application, notifications etc.), there's a pkg-config file (i.e. unity-notifications.pc), which in the Version field defines the version of the API implemented. Right now it's manual, but we'll make it automagic, that unity-notifications-impl-$version will be generated from this .pc file.

Then, any consumer of this API (unity8 in our case) depends on $default-implementation-real-package | unity-$interface-impl, unity-$interface-impl-$version. We wouldn't need the first bit, but dpkg would choke on only-virtual dependency like that.

On top of that, the unity-api tests will be ran against the actual implementation to verify the expected API.

Makes some sense now? The idea is to ensure shell-facing API stability and allow for easy drop-in replacements, as long as they implement the same interfaces.

review: Needs Fixing

[Michał Sawicz (saviq)]

This needs to build-depend on libunity-api-dev >= 7.80.7.

review: Needs Fixing

[lp:~macslow/unity-notifications/modal-snap-decisions]

207. By Mirco Müller on 2014-04-23

Make sure to depend on the new upstream-version of libunity-api-dev

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:207
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/55/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-amd64-ci/30/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/30/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-i386-ci/30/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/55/rebuild

review: Needs Fixing (continuous-integration)

[Michał Sawicz (saviq)]

18 + unity-notifications-impl-2,
19 + unity-notifications-impl-3

See my previous comment - replace, don't add.

=====

54 +pkg_check_modules(NOTIFICATIONS_API REQUIRED unity-shell-notifications=1)

This version should be the same as -impl, so 3.

=====

76 +/* check if unity-api >= 0.1.3 */
77 +#include <unity/api/Version.h>
78 +#if UNITY_API_VERSION_MAJOR > 0 || \
79 + (UNITY_API_VERSION_MAJOR == 0 && (UNITY_API_VERSION_MINOR > 1 || \
80 + (UNITY_API_VERSION_MINOR == 1 && \
81 + UNITY_API_VERSION_MICRO < 3)))
82 + #error Unity-API needs to be at least 0.1.3
83 +#endif

You're not using that part of unity-api at all, so all that is not needed.

review: Needs Fixing

[lp:~macslow/unity-notifications/modal-snap-decisions]

208. By Mirco Müller on 2014-04-28

Just bump the required unity-notification version on the packaging-side of things.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:208
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/57/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-amd64-ci/32/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/32/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-i386-ci/32/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/57/rebuild

review: Needs Fixing (continuous-integration)

[Michał Sawicz (saviq)]

See https://code.launchpad.net/~macslow/unity-api/version-bump-to-0.1.3/+merge/217882/comments/518636, bump the build-depends to (>= 7.80.8).

Also, parentheses are required in dependencies!

You might want to run `wrap-and-sort -atv` to make sure of all the formatting in debian/ is good.

review: Needs Fixing

[lp:~macslow/unity-notifications/modal-snap-decisions]

209. By Mirco Müller on 2014-05-05

Addes missing parentheses.

210. By Mirco Müller on 2014-05-05

Another version-bump in the dependency was needed, because another branch was merged to trunk before.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:209
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/58/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-amd64-ci/33/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/33/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-i386-ci/33/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/58/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:210
http://jenkins.qa.ubuntu.com/job/unity-notifications-ci/59/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-amd64-ci/34/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-armhf-ci/34/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-notifications-trusty-i386-ci/34/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-notifications-ci/59/rebuild

review: Needs Fixing (continuous-integration)

[Michał Sawicz (saviq)]

Yup.

review: Approve

=== modified file 'debian/control'
--- debian/control	2014-01-27 16:05:06 +0000
+++ debian/control	2014-05-05 08:08:03 +0000
@@ -7,7 +7,7 @@
                pkg-config,
                qtdeclarative5-dev,
                qtbase5-dev-tools,
-               libunity-api-dev,
+               libunity-api-dev (>= 7.80.8),
 # For Qml tests
                qtchooser,
                qt5-default,
@@ -18,7 +18,7 @@
 
 Package: qtdeclarative5-unity-notifications-plugin
 Provides: unity-notifications-impl,
-          unity-notifications-impl-2
+          unity-notifications-impl-3
 Architecture: any
 Multi-Arch: same
 Pre-Depends: ${misc:Pre-Depends}

=== modified file 'include/NotificationModel.h'
--- include/NotificationModel.h	2013-12-04 17:38:52 +0000
+++ include/NotificationModel.h	2014-05-05 08:08:03 +0000
@@ -30,20 +30,6 @@
 
 struct NotificationModelPrivate;
 
-enum Roles {
-    RoleType          = Qt::UserRole + 1,
-    RoleUrgency       = Qt::UserRole + 2,
-    RoleId            = Qt::UserRole + 3,
-    RoleSummary       = Qt::UserRole + 4,
-    RoleBody          = Qt::UserRole + 5,
-    RoleValue         = Qt::UserRole + 6,
-    RoleIcon          = Qt::UserRole + 7,
-    RoleSecondaryIcon = Qt::UserRole + 8,
-    RoleActions       = Qt::UserRole + 9,
-    RoleHints         = Qt::UserRole + 10,
-    RoleNotification  = Qt::UserRole + 11
-};
-
 class NotificationModel : public QAbstractListModel {
     Q_OBJECT
 

=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2013-11-29 10:55:54 +0000
+++ src/CMakeLists.txt	2014-05-05 08:08:03 +0000
@@ -1,3 +1,6 @@
+include(FindPkgConfig)
+pkg_check_modules(NOTIFICATIONS_API REQUIRED unity-shell-notifications=3)
+
 set(CORE_SRCS
 ActionModel.cpp
 ../include/ActionModel.h
@@ -9,6 +12,7 @@
 ../include/NotificationServer.h
 NotificationClient.cpp
 ../include/NotificationClient.h
+${NOTIFICATIONS_API_INCLUDEDIR}/unity/shell/notifications/ModelInterface.h
 )
 
 # Build everything twice. Since we have only a few

=== modified file 'src/NotificationModel.cpp'
--- src/NotificationModel.cpp	2014-02-18 14:51:44 +0000
+++ src/NotificationModel.cpp	2014-05-05 08:08:03 +0000
@@ -19,6 +19,9 @@
 
 #include "NotificationModel.h"
 #include "Notification.h"
+
+#include <unity/shell/notifications/ModelInterface.h>
+
 #include <QTimer>
 #include <QList>
 #include <QVector>
@@ -26,6 +29,8 @@
 #include <QStringListModel>
 #include <QQmlEngine>
 
+using namespace unity::shell::notifications;
+
 struct NotificationModelPrivate {
     QList<QSharedPointer<Notification> > displayedNotifications;
     QTimer timer;
@@ -71,37 +76,37 @@
             return QVariant();
 
     switch(role) {
-        case RoleType:
+        case ModelInterface::RoleType:
             return QVariant(p->displayedNotifications[index.row()]->getType());
 
-        case RoleUrgency:
+        case ModelInterface::RoleUrgency:
             return QVariant(p->displayedNotifications[index.row()]->getUrgency());
 
-        case RoleId:
+        case ModelInterface::RoleId:
             return QVariant(p->displayedNotifications[index.row()]->getID());
 
-        case RoleSummary:
+        case ModelInterface::RoleSummary:
             return QVariant(p->displayedNotifications[index.row()]->getSummary());
 
-        case RoleBody:
+        case ModelInterface::RoleBody:
             return QVariant(p->displayedNotifications[index.row()]->getBody());
 
-        case RoleValue:
+        case ModelInterface::RoleValue:
             return QVariant(p->displayedNotifications[index.row()]->getValue());
 
-        case RoleIcon:
+        case ModelInterface::RoleIcon:
             return QVariant(p->displayedNotifications[index.row()]->getIcon());
 
-        case RoleSecondaryIcon:
+        case ModelInterface::RoleSecondaryIcon:
             return QVariant(p->displayedNotifications[index.row()]->getSecondaryIcon());
 
-        case RoleActions:
+        case ModelInterface::RoleActions:
             return QVariant::fromValue(p->displayedNotifications[index.row()]->getActions());
 
-        case RoleHints:
+        case ModelInterface::RoleHints:
             return QVariant(p->displayedNotifications[index.row()]->getHints());
 
-        case RoleNotification:
+        case ModelInterface::RoleNotification:
             return QVariant(p->displayedNotifications[index.row()]);
 
         default:
@@ -543,17 +548,17 @@
 QHash<int, QByteArray> NotificationModel::roleNames() const {
     QHash<int, QByteArray> roles;
 
-    roles.insert(RoleType, "type");
-    roles.insert(RoleUrgency, "urgency");
-    roles.insert(RoleId, "id");
-    roles.insert(RoleSummary, "summary");
-    roles.insert(RoleBody, "body");
-    roles.insert(RoleValue, "value");
-    roles.insert(RoleIcon, "icon");
-    roles.insert(RoleSecondaryIcon, "secondaryIcon");
-    roles.insert(RoleActions, "actions");
-    roles.insert(RoleHints, "hints");
-    roles.insert(RoleNotification, "notification");
+    roles.insert(ModelInterface::RoleType, "type");
+    roles.insert(ModelInterface::RoleUrgency, "urgency");
+    roles.insert(ModelInterface::RoleId, "id");
+    roles.insert(ModelInterface::RoleSummary, "summary");
+    roles.insert(ModelInterface::RoleBody, "body");
+    roles.insert(ModelInterface::RoleValue, "value");
+    roles.insert(ModelInterface::RoleIcon, "icon");
+    roles.insert(ModelInterface::RoleSecondaryIcon, "secondaryIcon");
+    roles.insert(ModelInterface::RoleActions, "actions");
+    roles.insert(ModelInterface::RoleHints, "hints");
+    roles.insert(ModelInterface::RoleNotification, "notification");
 
     return roles;
 }

=== modified file 'src/NotificationPlugin.cpp'
--- src/NotificationPlugin.cpp	2013-10-29 03:19:47 +0000
+++ src/NotificationPlugin.cpp	2014-05-05 08:08:03 +0000
@@ -22,6 +22,8 @@
 #include "NotificationModel.h"
 #include "NotificationServer.h"
 
+#include <unity/shell/notifications/ModelInterface.h>
+
 #include <qqml.h>
 
 #include <QtDBus>
@@ -33,6 +35,7 @@
 static NotificationModel *m = NULL;
 static NotificationServer *s = NULL;
 
+using namespace unity::shell::notifications;
 
 static QObject* modelProvider(QQmlEngine* /* engine */, QJSEngine* /* scriptEngine */)
 {
@@ -41,6 +44,7 @@
 
 void NotificationPlugin::registerTypes(const char *uri) {
     // @uri Unity.Notifications
+    qmlRegisterUncreatableType<ModelInterface>(uri, 1, 0, "ModelInterface", "Abstract Interface. Cannot be instantiated.");
     qmlRegisterSingletonType<NotificationModel>(uri, 1, 0, "Model", modelProvider);
     qmlRegisterUncreatableType<Notification>(uri, 1, 0, "Notification", "Notification objects can only be created by the plugin");
 }