← Back to team overview

unity-design team mailing list archive

Re: Thoughts on notify-osd API


Aurélien Gâteau wrote:

I have been going through notify-osd code lately and discussed it with
Mirco a bit. It seems we can provide a Qt implementation for notify-osd
while trying to keep as much code in common as possible. To do so we
need to split notify-osd in two layers: Core and UI. The actual
implementation of the Bubble class would be the GTK+ UI layer, while we
would then write a Qt implementation of this UI layer.

To get things started I decided to review the existing Bubble API and
see how we can formalize it in an interface which would be implemented
by the UI layer. Here it is:
I think you need to consider both the Bubble (rendering API) and the model part (VisualNotification?) at the same time.

There are parts of the existing "API" (rather the exposed GObject interface) that are there for wrong reasons, like "need to stabilize and release soon, so no time for fancy refactoring", though we still did some of that up to late in the development cycle.

What I think you need to have is:

[ VisualNotificationRenderer ] - the UI part, with a GTK based version and at QT version

[ DisplayManager ]             - the part that positions bubbles on screen (it's in between the manager but is tied to X and probably the desktop it's running on, for things like gcong/kconfig, etc.)
[ VisualNotification ]         - model part, essentially a struct with title, body, etc.

[ NotificationManager ]        - the controler part (stack.c), with request handling and maybe some stack management (1)(2)

(1) though the latter could have it's own module/object, but I still think that's too much overhead for a simple list. (2) ideally I think you should keep D-Bus plumbing out of this and keep it in the dbus.c file, including the automatic bindings, though that's ok to have D-Bus types in the controler API Based on that, some attributes and functions don't really belong to the Bubble API anymore.

# Enums
enum Urgency {
Keep in mind that we only manage 2 queues: the critical and the normal one. but we still accept all libnotify urgency levels. We just treat low as normal. There is currently no difference between the two level in other notification managers either.
# Properties
## Public
id: int
title: utf8
message_body: utf8
icon_only: bool
There are quite a few booleans that could be grouped in a bitfield if you want to optimize.
# RFC: We can't use GdkPixbuf here as this would link with GTK+. It
# will be up to the ui layer to interpret the icon. This means we have
# to pass the ui layer both icon_name and icon_data and can't
# factorize the distinction.
icon_name: utf8
icon_data: binary

Unless the xdg discussion impacts this, I think this is the way to go.
# RFC: was urgent, which sounds more like a bool.
urgency: Urgency

# RFC: synchronous was a string and used as a title for atk. I think
# this should be splitted.
synchronous: bool
a11y_title: utf8
Initially it was made a string because notify-send does not support bools! ;) However, there is a nice side-effect, in that we can distinguish between the different synchronous notifications we have to display: volume, brightness, etc. For example, we can detect 2 successive volume notifications while the user is adjusting his system's volume and re-use the on screen bubble if it still exists. Synchronous is a particular case: it's not for "normal" applications. It's a channel used by system modules to display notifications bubbles and restricting this channel to just a bool is too limiting. It's more than an a11y_title. Yet, the a11y part is required on top of that, ie the "synchronous" field is the technical field that says whether its a volume or brightness thing and the software relies on the string values, whereas the a11y part is the user visible string.

I think that synchronous notifications should actually have a different model part, and have a normal notification as attribute to be able to then reuse the rendering layer that is associated with that.
# RFC: Was value, which is probably too common.
a11y_value: int
it's actually used only by synchronous notifications. However, I don't see the use for the a11y_ prefix. I would have expected something like range_value or something.
timeout: msecs
sender: utf8
append_allowed: bool
Please suppress the "allowed" part of the name, it implies that there is an access control mechanism, which is totally false: we blindly trust the applications and update/concatenate as they tell us to. append_mode it is.
visible: bool (ro)
position: x,y (ro)
height: int (ro)
future_height: int (ro)
future_height was a last minute workaround for a gtk issue where the height of a widget is not updated immediately. In the layout code, we rely on the computed height of the bubble to position a second bubble. But we were not getting the right value because of this bug. So future_height stores the height that has been requested before gtk actually reflects the change.
I think we should fix the issue, or hide that workaround anyway.
## Private
size: x,y
timer_id: int
mouse_over: bool
layout: BubbleLayout
enum BubbleLayout {
    LAYOUT_NONE = 0,

# Methods
Don't forget the constructor/destructor API. The object lifecycle is not totally trivial, in that creation is triggered by a dbus call but bubbles destroy themselves after a timeout. There are some nice/nasty cases where an application may still want to reuse a notification while it is in the process of being disposed by the timeout signal.
## Public
move(x, y)

# RFC: Can't we keep those private? IMO it's up to the ui layer to
# know whether it needs to refresh its display or update its size.

# RFC: This one should not be in Bubble: it only uses Bubble as an
# information container
show_dialog(appname, utf8 actions[])
Yes, this is because bubble.c is currently both the model and the rendering part. We wanted to organize the split once the initial "naive" had provided the outline of the API to split. Except we were too late in the cycle to make the split: it was not worth the risk of instability. But now is an /excellent/ time to take some distance and cut where it makes sense.
## Private
# RFC: hide is public, so maybe this should be public as well, for
# consistency
# RFC: fade_in is public, so maybe this should be public as well, for
# consistency
fade_in/out and show/hide should be reconsidered. The Plasma API should help make that more generic.
# RFC: Is only called by recalc_size() and stack_notify_handler(), but
# stack_notify_handler() calls recalc_size() just after, so it can be
# made private (even more if recalc_size() is made private as well)
That's a thing between the "DisplayManager" and the Renderer. But I'm not sure how you can get rid of that.


Follow ups