Re: [Ayatana] Thoughts on notify-osd API

[Aurélien Gâteau <aurelien.gateau@xxxxxxxxxxxxx>]

David Barth wrote:

> 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)

This looks like a full rewrite, am I wrong? I was under the impression
we were to go for a limited refactoring to make it possible to provide
different renderings.

Splitting the notification between rendering and model is a good idea,
as it will fix problems like the Bubble class being used as a model to
show the action dialog.

>> -------------------------
>> # 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.

I suggest keeping the enum anyway, as it matches the galago spec.

>> # 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.

Isn't this what the bubble id is for?

> 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.

Can we use the title property for this?

> 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.

I did not understand what it was used for at the time I wrote it. I just
realized it is used to show volume levels. Indeed, it does not need any
a11y_ prefix, range_value is fine.

> 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.

Yes, I forgot to mention signals. Without having experienced the bugs,
the situations looks not too complicated though:

NotificationManager maintains a list of renderers.

VisualNotificationRenderer emits a destroyed(renderer) signal when it
gets destroyed by the timeout.

NotificationManager receives the signal and removes the renderer from
its list.

If a renderer must be reused while fading out, it should cancel its
internal timer to avoid destroying itself.

I probably miss tricky situations, though...


>> ## 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.

Sounds reasonable. The split of Bubble class into
VisualNotificationRenderer and Notification addresses this issue I think.

>> # 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.

Since it's between the DisplayManager and the renderer, which are both
part of the ui layer, then I think it is implementation specific and do
not need to be specified in the API.

Aurélien