Re: [Evolution-hackers] Problems with calendar notification under unity (eds >= 3.30)

[Milan Crha <mcrha redhat com>]

On Fri, 2019-01-04 at 14:07 +0500, Khurshid Alam wrote:
> We can always query org.freedesktop.Notifications for that. Here is
> basic patch that is working.

        Hi,
it can be done, but it also means some kind of expectation in the code.
What if the system the glib is running on uses different method for its
GNotification-s? That's why it's more correct to (add and) use glib
API. Imagine the same on MacOS or Windows, I doubt Windows
implementation uses org.freedesktop.Notifications (if GNotification
works there at all, because the developer documentation gives no
guarantees). I'm not against the change, it can be added at least
temporarily, until glib adds the needed API.

> https://paste.ubuntu.com/p/Td4nGvWq2v/

Could you open a bug report against eds and add the patch there,
please? I briefly read it, I didn't compile it, and it has some issues:
a) an error message says "system bus", but you open session bus
b) "to look up" in the error message sounds weird, neither I'm sure
   why you add g_get_user_name() into that error message.
c) I'd prefer to use g_strcmp0() and g_clear_error() with
   changed 'error->message' to 'error ? error->message :
   "Unknown error"', just in case
d) the code introduces a compiler warning (variables defined in
   the middle of the code block)
e) the 'str' can leak
f) the 'bus' can leak
g) ideally call this only once and remember the result; it's highly
   unlikely that the notification server will change its capabilities
   in runtime, at least from my point of view, and D-Bus calls can
   cause delays.
Having opened a related glib bug is also required, thus they can be
linked together. Well, liked in a sense of gitlab, not the real linking
with all the notifications and such as bugzilla used to do.

> By default it doesn't add action if server doesn't support it, but it
> can be made to do it.

The default is fine.

> This probably can't go in upstream since indicator-messages is not in
> debian, but ayatana-indicator-messages is.

Right, I'd like to avoid desktop environment specific code as much as
possible. Also because that can require desktop specific libraries,
which can limit the places where the code can be developed (as you
enabled the MM in the DISTCONFIGURE parameters). Not talking that such
code makes it harder to test properly (more code paths to verify by the
testers and/or builtin tests).

I'm thinking whether it would be helpful to make EAlarmNotify an
extensible. In that case you could write an extension for it (instead
of patching upstream code), that could live in your repositories. I'm
not sure whether it might make things easier or worse for you, though.
        Bye,
        Milan