unity-design team mailing list archive

[David Barth <david.barth@xxxxxxxxxxxxx>]

Aurélien Gâteau wrote:
> Hi,
>
> 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 {
>     LOW,
>     NORMAL,
>     CRITICAL
> }
>   
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,
>     LAYOUT_ICON_ONLY,
>     LAYOUT_ICON_INDICATOR,
>     LAYOUT_ICON_TITLE,
>     LAYOUT_ICON_TITLE_BODY,
>     LAYOUT_TITLE_BODY,
>     LAYOUT_TITLE_ONLY
> }
>
> # 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)
> hide()
> start_timer()
> fade_in(msecs)
> append_message_body(utf8)
> sync_with(Bubble)
>
> # 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.
> refresh()
> recalc_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
> show()
>   
> # RFC: fade_in is public, so maybe this should be public as well, for
> # consistency
> fade_out(msecs)
>   
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)
> determine_layout()
>   
That's a thing between the "DisplayManager" and the Renderer. But I'm 
not sure how you can get rid of that.

David