implement notification service and API

[wkornewald]

We need a very simple API for creating notifications and a way for users to subscribe to notifications, so when an event occurs a pop-up can be shown, for example. The details still need to be discussed and maybe a short specification from the end-user point of view needs to be written.

This is probably an R1.1 task, though.

[mmadia]

Here's some ML discussions about integrating (and altering) ​InfoPopper as a notification_sever

• ​Big discussion in May 2007
• ​And some more in April 2008
• ​Year++ with plan for integration

[plfiorini]

Replying to mmadia:

Here's some ML discussions about integrating (and altering) ​InfoPopper as a notification_sever

• ​Big discussion in May 2007
• ​And some more in April 2008
• ​Year++ with plan for integration

Unfortunately my patch was lost by many virtual machine changes. Hopefully it will be rewritten soon.

[plfiorini]

infopopper integration patch

[plfiorini]

Initial InfoPopper integration patch added. It's the code from its trunk with some improvements:

• Haiku code style (well let's put it this way: I haven't checked with checkstyle yet but it's way more compatible with Haiku style than the original code)

• Fixed crash when the server closes (the window needed to be locked before invoking Quit)
• Removed overlay icon, one icon is good enough
• Use BAlert's icons if nothing else is specified (the order should be application icon, icon set on BNotification, balert icons but I haven't tested application icons)
• Hidden the notifications page from the preflet (read more below)
• Icon size is either large icon or mini icon (since we use vector icons I would encourage more standard sizes defined as icon_size)
• Icon size selector on preflet is now a menu field
• Menu fiels in preflet enable the save button
• No more hardcoded colors (or at least most of them are fixed)

What need to be fixed:

• Old _T() defines for Zeta does nothing, it would be better to use our Localization Kit here and there
• Probably there are still style violations
• Revert doesn't work in preflet and probably it's better to save settings immediately without pressing the Save button
• Fill the blank space with your suggestion, I'm sure you will find a lot of other stuff to improve :)

What I want to do in the future: This patch is just a starting point. The whole notifications filtering should be rewritten and that's why it's hidden in the preflet. I'm more inclined to do something like Growl. An application should "subscribe" the notification server in some way and tell what are its notifications. BRoster will send the application's signature in the app field so it will not be specified with BNotification::SetApplication(). The preflet will let you:

1) hide all the notification for an application 2) hide all the notifications on a per 'type' basis 3) enter a panel with all the notifications that the application can send and configure what to do (emit a sound, display the notification, etc...)

PS: Sorry for the double post and for posting a still incomplete patch but I didn't want to loose it

[plfiorini]

Also: preflet GUI should be improved (display view has large controls and BIconItem/BIconRule need improvements but they might be useful to other applications once finished), notification_server .rdef file doesn't have a good icon (there should be one in the Haiku source tree I guess), preflet need icons for its BIconRule but I just can't draw anything...

[stippi]

plfiorini, I am currently reading through the patch and preparing a number of remarks. I see you have done a great effort to make the code clean and readable and stick to the coding style guide. That's greatly appreciated!

[stippi]

Ok, here is my review. Although some remarks seem like I read everything very carefully, indeed I only skimmed some passages, so there may actually be other noteworthy things left. :-)

All in all, the patch is great, though, and since it's so large, I think it will be easier if I apply it as is, and you send changes against trunk, which will greatly reduce the size of the patches and make further reviews much easier. Here we go:

BNotification:

• Theoretically could be entirely BMessage-based protocol.
• Application signature could be initialized internally (calling team).
• Uses too much pointers and manual memory management. (For examlpe: char* -> BString)
• Methods to internal members should not return non-const pointers.
• Memory management for fFile even seriously broken, takes over ownership of potentially temporary entry_ref passed to SetOnClickFile().
• Casting to (void*) is never necessary.
• Ownership of fBitmap is also unclear. fBitmap not freed in destructor, but ownership of other stuff is transferred, so it's confusing. It should make a copy.

headers/os/Roster.h:

@@ -116,6 +117,10 @@ class BRoster {
 		void AddToRecentFolders(const entry_ref *folder,
 					const char *appSig = 0) const;
 
+		// notifications
+		status_t Notify(BNotification* notification,
+					int32 timeout = -1) const;
+
 		// private/reserved stuff starts here
 		class Private;

Shouldn't timeout be bigtime_t? Why not "const BNotification& notification"?

headers/private/notification/NotificationReceived.h is not self-contained (notification_type).

status_t BRoster::Notify(BNotification* notification, int32 timeout) const

• Documentation wrong: Message is delivered asynchronously.

src/kits/notification/AppUsage.cpp

• Code duplication in Flatten() and FlattenedSize(), should use common private method to build the message.
• AppUsage::Unflatten() does not check for errors to restore fName, fRef and fAllow.

src/kits/notification/NotificationReceived.cpp

• Code duplication in Flatten() and FlattenedSize(), should use common private method to build the message.
• Unflatten() needs to check return codes of BMessage::Find*() methods.

src/preferences/notifications/GeneralView.cpp

• Mentions "InfoPopper installation" in error alerts.
• Would replace mentions of "the server" with "the notification service".
• I think GeneralView::_CanFindServer() implements as fall-back mechanism what be_roster->FindApp() already tried.

src/servers/notification/AppGroupView.cpp

• AppGroupView::Draw(BRect updateRect): Doing a lot of unnecessary PushState() PopState() combos. Calling Sync() before returning is unnecessary, unless Draw() is invoked somewhere manually (where it should be Invalidate() instead).

src/servers/notification/BorderView.cpp

• Should pass B_FULL_UPDATE_ON_RESIZE to BView flags, instead of B_FRAME_EVENTS and overriding FrameResized().

src/servers/notification/NotificationServer.cpp

• NotificationServer::QuitRequested() is unnecessary, it's doing the very same thing that BApplication is already doing. This is only necessary if NotificationWindow needs the NotificationServer object to still be alive when it's quitting (doesn't seem to be the case).

src/servers/notification/NotificationView.cpp

• NotificationView::Draw() could use the BControlLook class for drawing the progress bar. Some PushState() PopState() combos are overhead.

• NotificationView::MouseDown() switching on buttons is not good, since buttons is a bitfield. Should be if ((buttons & B_PRIMARY_MOUSE_BUTTON) != 0).

src/servers/notification/NotificationWindow.cpp

• NotificationWindow::MessageReceived() line 187: "Error" -> "error".
• Error alerts should mention "Notification service: " before messages, since it will be unclear what application is generating the alert.

[stippi]

The patch is missing src/kits/notification/Notifications.cpp.

[stippi]

In NotificationView::SetText(), the loop to generate the lineinfos is completely broken.

[stippi]

Also in NotificationsView::SetText(): There is no "BFont& BFont::operator=(const BFont*)", the assignment operator takes "const BFont&". While I am testing the notification_server with the notify bin command, it just crashes in this method. While I am trying to fix it, I wonder why it uses this custom solution to chunk up the notification text into lines, why not simply use BTextView?

[stippi]

Applied with some fixes to make it not crash (rewritten line wrapping in NotificationsView.cpp) in hrev36949. Thanks a bunch. I am looking forward to patches for the issues I outlined above! :-)

[axeld]

I agree with stippi about BNotification - it seems pretty much superfluous at one side (vs. just using a BMessage), and unnecessarily inflexible at the other side.

For example, shouldn't it be possible to send a flattened BView to show as a notification (using the replicant mechanism)? I think limiting that to an icon and some plain text is very limiting for no good reason.

[stippi]

I probably should have mentioned this, but I am currently working on the BNotification interface. Already I found a bug because of unclear ownership with the API, in notification, it adds stack allocated entry_ref. If this code would have ever been tested, it would have crashed.

[stippi]

Did the first round of cleanup of the API in hrev36952. TODO: BNotification should inherit from BArchivable, then move the code from BRoster::Notify() into BNotification::Archive().

[plfiorini]

Replying to stippi:

Did the first round of cleanup of the API in hrev36952. TODO: BNotification should inherit from BArchivable, then move the code from BRoster::Notify() into BNotification::Archive().

Sounds better, I'm implementing it...

[plfiorini]

Replying to axeld:

I agree with stippi about BNotification - it seems pretty much superfluous at one side (vs. just using a BMessage), and unnecessarily inflexible at the other side.

For example, shouldn't it be possible to send a flattened BView to show as a notification (using the replicant mechanism)? I think limiting that to an icon and some plain text is very limiting for no good reason.

That sounds good but I don't want people to mess up with the style of a notification window. Probably, a notification should always have a title, an icon and a content as specified by the BNotification setters, and have one or more optional BViews added after the content message using BNotification::AddChild.

Then the notification_server might create NotificationViews using layouts (and probably the option to set the title above the icon or all the text right of icon should go away having a layout with an icon and all text and views right of it).

Going to look how GNOME does with notifications since I've seen an update notification using an icon a text and a button...

[axeld]

I would consider the icon optional, at least I could imagine a nice animation there instead, or even no icon at all. If you don't want to let people mess with the style, write a style guide for how notifications should look like, or how BViews should behave in this case - but I wouldn't artificially limit the interface with eventually not so future proof decisions.

If the title etc. are mandatory, they should be arguments of the constructor.

OTOH, I think at least the title can be derived automatically if none is given (ie. just the application name by default). What would be SetApplication() for? be_app should always be the sender in the context of the client.

I would understand the use of a BNotification class better if it could be updated live later on, ie. when I have already published it, and then call SetProgress(), the view is updated directly.

I'm not sure if Notify() should be part of BRoster at all. I guess I would just make it a method of BNotification instead (and name it Send() or Publish() or something like that).

[axeld]

Also, instead of that *OnClick() stuff, why not just add a message with a specific target? This way, it would be much more flexible and usable, and you could still easily open refs/apps by targetting the Tracker if wanted.

[plfiorini]

Replying to axeld:

I would consider the icon optional, at least I could imagine a nice animation there instead, or even no icon at all. If you don't want to let people mess with the style, write a style guide for how notifications should look like, or how BViews should behave in this case - but I wouldn't artificially limit the interface with eventually not so future proof decisions.

OK - I'm working on a patch that doesn't have any icon set by default. Notification windows color should be enough to know the urgency of the message (ie a red window indicates an error).

If the title etc. are mandatory, they should be arguments of the constructor.

Only the notification type is mandatory and it's already an argument of the constructor. It should be tested to see what happens if the notification doesn't have any title, though.

OTOH, I think at least the title can be derived automatically if none is given (ie. just the application name by default). What would be SetApplication() for? be_app should always be the sender in the context of the client.

In my (yet to be published) patch, SetApplication() was renamed to SetGroup() and Application() to Group() to explain its meaning.

Groups are not going to be mandatory as they were in the original InfoPopper implementation.

Grouped notifications will appear in the same group box, it's useful if an application sends a bunch of notification at the same time.

I would understand the use of a BNotification class better if it could be updated live later on, ie. when I have already published it, and then call SetProgress(), the view is updated directly.

It's possible, this is why we have SetMessageID(). By default a notification has a NULL message ID but if you set one, subsequent calls to BNotification::Send() will update live.

This is InfoPopper legacy, maybe it's better to remove SetMessageID() and MessageID() and let BNotification generate a system-wide unique identifier each time the BNotification is created.

I'm not sure if Notify() should be part of BRoster at all. I guess I would just make it a method of BNotification instead (and name it Send() or Publish() or something like that).

Done, it's now BNotification::Send(bigtime_t).

[plfiorini]

Here's a brief summary for notifications1.patch. Obviously, it's still work in progress (for example since AppGroupView now inherits from BBox has lost the collapse and close widgets that will be added soon, there are still OnClick* methods to be replace by methods to set the on click app signature and message and other things).

Changes to Application Kit:

	* BNotification now inherits from BArchivable.
	* Archiving fields in BNotification starts with an underscore now, as done by
	  other Haiku archivable classes.
	* BRoster::Notify() doesn't exist anymore, use BNotification::Send() instead
	  so it's all self-contained in BNotification.
	* BNotification::Progress() returns -1 if notification's type is
	  not B_PROGRESS_NOTIFICATION.
	* BNotification::SetProgress() validates the progress passed as argument.
	* By default icon is not automatically set.
	* Renamed Application() and SetApplication() to Group() and SetGroup().
	* Added BNotification::InitCheck() that returns B_OK if the notification object
	  has a valid type otherwise an error code.
	* Methods that returns const char* properties now returns NULL if the
	  string is empty.
	* Added documentation to BNotification except for OnClick methods that will
	  probably be removed soon.
	* Only notification's type is mandatory for BNotification.

Changes to notification_server:

	* Removed BorderView.
	* Fixed NotificationView::MouseDown() switching on buttons
	  is not valid because it is a bitfield.
	* Fixed typo in NotificationWidow::MessageReceived(), it's
	  "error" not "Error".
	* Removed infoview_layout and layout settings.
	* Renamed NotificationWindow::ViewWidth() to NotificationWindow::Width().
	* Methods returning icon size, width and timeout in NotificationWindow are now const.
	* Check if notification is allowed by looking for sender's signature.
	* Property lists indentation.
	* Removed some PushState()/PopState() and Sync() from NotificationView::Draw().
	* Removed some PushState()/PopState() and Sync() in AppGroupView::Draw().
	* Removed NotificationView::SetPosition().
	* AppGroupView inherits from BBox.

Changes to notify:

	* No manual memory allocation, use BString instead of char*.
	* Renamed all application occurences to group.
	* Title and group are not mandatory.

[axeld]

Unfortunately, you don't seem to have written that patch against what is in the repository, but against your original patch. IOW it does not apply cleanly - in src/servers/notification/NotificationWindow.cpp several chunks fail, at least.

Can you please update your patch?

[leavengood]

Taking ownership again since stippi's time is limited at the moment. I'd like to get the notification system cleaned up.

I applied the last patch and had quite a few failures as Axel did (except probably worse now.) If you are still listening plfiorini you could save me some time by making sure your patch applies cleanly to the current code. Also in general such large patches are very unwieldly because they are hard to review and quickly get out-of-date.

I think after applying the last patch this bug can be closed and further notification system enhancements should be done in new tickets, preferably as small patches :)

[leavengood]

Results of trying to apply notifications1.patch to hrev42239

[pulkomandy]

Patch applied in hrev43114, after rewriting a great part of it.