[qmf-notifications-plugin] Don't emit email_exists for every received…

[dcaliste]

… email, only for the summary.

The "email_exists" kind of feedback is emiting sound and led pattern as defined in jolla-common-configurations private repo (but not in ngfd repo), while the "email" feedback is not existing. This PR should do the following:

• don't emit feedback for every new email, just publish a notification adding an entry in the event view,
• emit the "email_exists" feedback (sound and led pattern) with the bubble summary notification.

Currently, the following is happening:

• the "email_exists" feedback is emitted for every new email (if you got 12, it is emitted 12 times). These feedback hints are caught by lipstick in the notificationfeedbackplayer, that is stopping the current "email_exists" feedack and restarting it for every hint (ngfd logs in -vvvv mode is filled up with lines like [1612.18446744073709551183] INFO: dbus: >> stop received for id '1' immediately followed by [1612.18446744073709551184] INFO: dbus: >> play received for event 'email_exists' with id '2' (client :1.268 : 2 active request(s))). Ngfd is kind enough to stop the email ringing and plays only the last one.
• the "email" hint is sent for the summary, but nothinghappens because "email" feedback doesn't exist in jolla-common-configurations.

The current PR is highlighting a bug though : the feedback should be a simple "beep" when we are already in the email client (like for the message application), but it is actually ringing full email sound at the moment. With the PR, the full email sound is not emitted anymore (since it was emitted because of the notification hint for each email) but the beep is not working either, because it is using the "email" hint which does not exist in jolla-common-configurations.

@pvuorela , what do you think about these changes ? I still can't reproduce the hanging bug in ngfd, but this may be a source of it : maybe when receiving lots of emails, stopping the playback in the gstreamer thread and restarting it repeatedly may be the cause ?

[dcaliste]

@pvuorela, I don't have access to jolla-common-configurations to look at the history of events.d/email.ini file. Maybe it would be better to correct the "email" and "email_exists" definitions there to be more in lines with the one in ngfd repo ?

[pvuorela]

I'm not sure if this is the right thing here.

First off all, should mention the rationale in the commit message so it's available later on. Now only the summary line might get people looking the git history puzzled.

The "email" feedback should exist in the same /usr/share/ngfd/events.d/email.ini file. Earlier we used to have separate files for each feedback but now related ones are merged.

The summary notification is transient. Thus I'd think the feedback should be stopped when it disappears, thus doesn't feel proper for led.

It's maybe a bit surprising that now the email_exists has the sound set too. History on the email.ini from 2018 seems to be

As for example sms and sms_exists counterparts the sms event is stopped
whenever the sms notification disappears from the view. However the
sms_exists event is kept until the notification is removed or closed by
opening the notification or opening the sms message itself. This way the
possible sound associated with sms events will play to the end unless it
is stopped by opening the message. Same separation is done with chat and
email events.

[dcaliste]

First off all, should mention the rationale in the commit message so it's available later on.

Yeh, sorry, I should have put a commit message, but since it's not corresponding to what is in public ngfd repo, I was a bit undecide. In ngfd repo, "email_exists" basically do nothing and "email" is playing the email tone, and a beep in short mode. Which then is consistent with the current code here (before PR). While, email and email_exists are overwritten from the private
jolla-common-configurations repo, with a different behaviour : there is no "email" in short mode (or not working) and "email_exists" is playing sound. Thus the PR, but how to comment this in a commit message since overriding elements are not available outside the phone…

The "email" feedback should exist in the same /usr/share/ngfd/events.d/email.ini file. Earlier we used to have separate files for each feedback but now related ones are merged.

Running ngfd in debug mode returns this for "email" feedback:

[289.086] DEBUG: core: evaluating events for request 'email'
[289.086] DEBUG: core: evaluated to 'email'
[289.086] DEBUG: core: request 'email' resolved to event 'email'
[289.087] WARNING: core: no sinks that can handle the request 'email'
[289.087] DEBUG: core: stopping all sinks for request 'email'
[289.087] DEBUG: dbus: error occurred for request 'email': no fallbacks!

That's why I think that the "email" feedback is not working (and no sound is emitted either).

It's maybe a bit surprising that now the email_exists has the sound set too.

Indeed, and since this "email_exists" is emitted for every new email, it creates a lot of play, then stop, then play, then stop… requests in ngfd. Thus the PR. But as I mentioned in the first comment, a better fix maybe in the overriding ini files from the private repo.

[dcaliste]

When looking at the phone email.ini, there is this rule:

[email => context@call_state.mode != none, play.mode == *]
%include.1 = foreground
sound.stream.module-stream-restore.id = x-ringtone-volume

Should it be call_state.mode == none instead ? That would explain why there is no "email" feedback found, because all the rules are for context@call_state.mode != none cases.

For reference, there is a similar == none case for sms:

[sms => context@call_state.mode == none, play.mode == foreground]
%include.1 = foreground
%include.2 = foreground_haptic

[pvuorela]

cc @jusa too

[pvuorela]

Did some testing myself, mostly with notification test app. Didn't go checking ngfd debugs so far.

The email feedback does seem to work, as in it plays the vibra. But nothing else.
The email_exists plays the sound and turns on the led.

That's confusing. I would expect the _exists to turn on the led and plain email playing the vibra and the sound.

Second, the "// App is on screen beep only" case doesn't indeed seem to work as beep. And why should it, there's nothing requesting a beep mode there. For comparison the SMS case seems to have some play.mode = foreground setting on the event. But thinking if it would be simpler to have a totally different feedback for that case, or alternatively just play a named tone or file.

[pvuorela]

Created an internal bug. Guess we should make the definitions saner in the event config side.

[dcaliste]

I would expect the _exists to turn on the led and plain email playing the vibra and the sound.

I agree with you. But at the moment, "email_exists" is sent to ngfd for every received email. So a sync that retrieve twenty emails, will ask over DBus 20 times to play "email_exists". If we ask for a vibration there, it will actually vibrate a lot !

And why should it, there's nothing requesting a beep mode there.

Well actually if you change the != to == in [email => context@call_state.mode != none, play.mode == *], this rule is asking for a beep (see the foreground include). Except that the play.mode stays (null) when testing it...

Guess we should make the definitions saner in the event config side.

I agree. Would it make sense to actually move the ngfd rule files for email and email_exists in this qmf-notifications-plugin repo to see the consistency between the rule and the notification emission ? Or is it too linked to Android workarounds (besides logic definition, that's the only difference I spot in the file with the one coming from ngfd repo), so it must stay Jolla specific ?

[dcaliste]

Oh and I forgot, but the main motivation (from my side) is to avoid to ask to play a sound 20 times in a row as with the current scheme, because I suspect it can be the source for a race in gstreamer plugin that stucks ngfd.

[pvuorela]

I agree with you. But at the moment, "email_exists" is sent to ngfd for every received email. So a sync that retrieve twenty emails, will ask over DBus 20 times to play "email_exists". If we ask for a vibration there, it will actually vibrate a lot !

Sure. But perhaps not that much problem if that would only do the led. Sure there's some redundancy if we assume what the feedbacks do, but then again "exists" should be there as long as some of the email notifications exist, right? Thus it would need to be set for all.

Well actually if you change the != to == in [email => context@call_state.mode != none, play.mode == *], this rule is asking for a beep (see the foreground include). Except that the play.mode stays (null) when testing it...

Perhaps that's the problem, missing the mode being set on the parameters.

I agree. Would it make sense to actually move the ngfd rule files for email and email_exists in this qmf-notifications-plugin repo to see the consistency between the rule and the notification emission ? Or is it too linked to Android workarounds (besides logic definition, that's the only difference I spot in the file with the one coming from ngfd repo), so it must stay Jolla specific ?

Not sure. If it's defined here perhaps it could be without the extra feedback definition altogether.