WIP: Notification infrastructure #459

kgoderis · 2015-09-18T12:27:07Z

Based on https://www.eclipse.org/forums/index.php/t/1070058/

Just a few words of explanation with respect to the consumer/producer code. I have split the initial code (#436) into two parts, in order to show the capabilities of the framework:

There is a consumer of Notifications (NotificationServiceGateway) which takes Notifications it receives and pipes them to the appropriate NotificationService (e.g. pushover, ...).
In order to feed the NotificationServiceGateway with Notifications of our interest, it registers an OSGi service (NotificationServiceBridge, which is an EventSubcriber) that is configured to transform Events into Notifications for a given NotificationService. The configuration comes from .notify definition files (no change compared to initial PR) and allows to define configurable Targets (which are different but relevant for each NotificationService) together with Filters (to filter, rate control,... whatever is send to the NotificationService)

Signed-off-by: Karel Goderis karel.goderis@me.com

Signed-off-by: Karel Goderis <karel.goderis@me.com>

kgoderis · 2015-09-19T16:59:43Z

I am just wondering if we should have the UID of Notifications set by the NotificationManager (using java.util.UUID), and have the add() method return that UID.

kaikreuzer · 2015-09-19T20:10:38Z

Returning the UID on add sounds like a good idea, using java.util.UUID not - I would stick to a String, so that producers have a chance to define their own.

kgoderis · 2015-09-20T10:04:45Z

I see merit in being able to set your own id but what does that mean for the notification consumers? They will have no means to decode potential information stored in the id itself, and so, imagine that we do have to distinguish between sources of notifications, then maybe we have to standardise how the source field is encoded? For example 'class name:hashcode' or alike. Or we have ids that are part free text and part set by the notification manager?

kaikreuzer · 2015-09-20T13:39:14Z

The id is an id, it should have no meaning that anybody should interpret (with the exception of the producer itself, maybe). To distiguish between sources, we have the source property (which again should have no further meaning than different Strings mean different producers).

Signed-off-by: Karel Goderis <karel.goderis@me.com>

kgoderis · 2016-02-13T08:33:37Z

Anything that can be done to push this a bit forward?

maggu2810 · 2016-02-13T16:39:03Z

Hi @kgoderis,
I don't know what is the last thing you discussed with @kaikreuzer?
Needs this code just a review or is the functional design still in question?

kaikreuzer · 2016-02-13T21:12:40Z

Needs this code just a review or is the functional design still in question?

A bit inbetween I would say. We discussed the architecture quite a bit in the thread mentioned above, but I didn't yet find the time to have a closer look at the code of this PR - and as usual, many questions/ideas about the functional design only come up when looking at some real code.
So @maggu2810, your feedback & input on the general functionality & design would be appreciated. I will also try to find the time to get my head around it soon. Once we are all fine with the general approach, we can do the detailed code review.

kaikreuzer · 2016-02-13T21:18:23Z

Anything that can be done to push this a bit forward?

Probably creating fewer other "urgent" PRs ;-) No, seriously, you know that all your contributions are very welcome, but I am struggling a lot to find the time to do the detailed reviews as this is quite time consuming as well - and there are so many other discussions and PRs going on as well.
So please feel free to push me anytime when you think something is urgent for you or the project - and forgive me if some things are unprocessed for a long while... If you have good ideas how to get more reviewers on board, please let me know, I hate being the bottleneck!

kgoderis · 2016-02-14T13:12:36Z

cp -p /kai /kai2 ?

/me not being a code developper by trade, I think I do not qualify to do code reviews. However, if you would ask me if I would forego code quality for speed of integration, then I would say yes, as bugs can be always fixed afterwards, even by the strong user base of OH2/ESH. "Use the force, Luke" ;-)

cdjackson · 2016-06-17T07:38:16Z

I’ve had a bit of a look over the notification system - it's not a code review - just trying to work out how it is meant to work and have a few thoughts/comments.

Firstly, from Karels comment above -:

the (ZWave) binding would implement the NotificationPublisher interface

I couldn’t find this interface in the source?

the UI would implement the NotificationSubscriber interface

I couldn’t find this interface, but I assume that the notifications are published in the standard way, so it’s just sent using SSE to the UI. Actually, looking at the diagram on the Eclipse forum, it looks like these notifications aren't sent using SSE, although I did see an event factory in the code, but this doesn't seem to be in the UML?

you could then define in a .notify which kind of Notifications you would like to get displayed on the UI

I don’t want to have to define anything in the model (I assume that’s what the .notify is?). I would just want the information to be sent to the UI – filtering out notifications might not be good.

Notification types can be shared amongst Bindings

where are these types defined? Would they go into a class/enum or something?

Persistence and alike features of the framework are complementary in this case.

How is this defined? I couldn’t see anything in the notification builder about specifying persistence. This would definitely need to be disabled for these type of notifications or things could get VERY confusing. My use case for the binding notifications should be delivered in real-time, or not at all.

For the implementation -:

I guess it’s possible to have different implementations of Notification? Eg can we extend NotificationImpl at all? If so maybe this is a good way to provide a class of notifications useful for binding events, or user admin interface type of events? I suspect that additional information may be needed to allow bindings to provide information to UIs. As I said in earlier posts, I can see a case where we want to provide a string that every UI can use to present a common, meaningful representation of the event. However, I would like to make it extendable such that a binding could provide binding specific information that could be presented by a binding specific plugin in the UI. This could be an extended event, or could use a map of properties that could be added to the event.

I’m a bit confused by the event topic ("smarthome/notifications/{notificationUID}/added"). What is the UID here? I thought the UID was generated when you create a notification - ie it uniquely identifies that speficic notification so it could be cancelled later?

I couldn’t see any way to disable persistence. I was mainly looking at the notification builder, but there’s nothing there that allows certain events to be persisted or not. Can this only be done in some static definition?

There's a LOT of files here. I'm not really sure how the models fit into this at all, and this PR seems to mix the concept and the implementation (am I right?). For example I see email and pushover in here - I wonder if it might not be easier to review and get approved with a subset, and then add the different notification implementations as separate PRs? (just a thought to try and make it easier to review, and quicker to approve).

kgoderis · 2016-06-17T11:04:30Z

@cdjackson will get back to you when I have more time to go through yr comments, but the implementation does indeed differ from the diagram, and does a bit more than the diagram, e.g. pushover and other "sample" implementations.

Notifications are in fact "encapsulated" into the already existing Events. I had some discussion with Kai regarding the ID these Notifications should get, but the idea was that it was up to the instance generating the Notification to come up with an ID

To rephrase a bit, the instance consuming the Notifications would implement NotificationService, or alternatively, be an EventSubscriber if it directly wants to listen to NotificationAdded Events for example; The instance generating them should bind via OSGi to the NotificationManager, and use the NotificationBuilder to create notifications and post them to the NotificationManager

Sorry for the confusion, has been a along time since I wrote this code.

cdjackson · 2016-07-09T07:19:33Z

Any further thoughts on this anyone?

kaikreuzer · 2016-07-11T07:51:19Z

No, not as long as we have urgent startup problems to solve...

cdjackson · 2016-07-11T08:50:18Z

Sorry @kaikreuzer - I should have stated this was really for @kgoderis as he said he would comment further on my comments

kgoderis · 2016-07-11T09:26:34Z

@cdjackson Chris, I lost this from sight as I was rather busy trying to get my ops env on the Karaf distribution. I will be travelling shortly and do not plan to take my pc with me, so I will get back to this in a few weeks time.

wborn · 2018-08-16T16:22:04Z

bundles/model/org.eclipse.smarthome.model.notification.runtime/pom.xml

+  <properties>
+    <bundle.symbolicName>org.eclipse.smarthome.model.notification.runtime</bundle.symbolicName>
+    <bundle.namespace>org.eclipse.smarthome.model.notification.runtime</bundle.namespace>
+  </properties>


Please remove these properties. See #6060.

wborn · 2018-08-16T16:22:42Z

bundles/notification/org.eclipse.smarthome.notification.action/pom.xml

+		<bundle.namespace>org.openhab.action.pushover</bundle.namespace>
+		<deb.name>openhab-addon-action-pushover</deb.name>
+		<deb.description>${project.name}</deb.description>
+	</properties>


Please remove the bundle.symbolicName and bundle.namespace properties. See #6060.

The deb.name and deb.description properties should also be removed because this is no openHAB project.

wborn · 2018-08-24T11:28:50Z

.../model/org.eclipse.smarthome.model.notification.runtime/.settings/org.eclipse.jdt.core.prefs

+org.eclipse.jdt.core.compiler.compliance=1.7
+org.eclipse.jdt.core.compiler.problem.assertIdentifier=error
+org.eclipse.jdt.core.compiler.problem.enumIdentifier=error
+org.eclipse.jdt.core.compiler.source=1.7
