After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 652718 - Should tell user he won't get more notifications when switching status to Busy
Should tell user he won't get more notifications when switching status to Busy
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2011-06-16 11:30 UTC by Guillaume Desmottes
Modified: 2011-09-17 15:26 UTC
See Also:
GNOME target: 3.2
GNOME version: ---


Attachments
status-menu: Tell user notifications are blocked while busy (1.13 KB, patch)
2011-06-17 10:52 UTC, Florian Müllner
reviewed Details | Review
main: Add Main.notify() for simple system messages (1.44 KB, patch)
2011-07-28 18:41 UTC, Florian Müllner
committed Details | Review
status-menu: Tell user notifications are blocked while busy (1.22 KB, patch)
2011-08-26 08:59 UTC, Florian Müllner
accepted-commit_now Details | Review
user-menu: Rename "Do Not Disturb" to "Notifications" (2.25 KB, patch)
2011-09-04 17:02 UTC, Florian Müllner
committed Details | Review
user-menu: Explain why disabling notifications changes IM status (4.69 KB, patch)
2011-09-09 17:08 UTC, Florian Müllner
none Details | Review
user-menu: Explain why disabling notifications changes IM status (4.68 KB, patch)
2011-09-12 22:11 UTC, Florian Müllner
committed Details | Review

Description Guillaume Desmottes 2011-06-16 11:30:38 UTC
No one understands than changing the desktop status to Busy using the user menu is secretly blocking notifications to pop up.

It has been suggested at the IM hackfest [1] that the Shell should display a "You won't get any more notifications” as his last notification so user would know about this behaviour.
This notification should probably be removed automatically without user having to ack it.

[1] https://live.gnome.org/Hackfests/IMContacts%20Social2011/Tasks/ShellDesignPresenc
Comment 1 Bastien Nocera 2011-06-16 11:38:40 UTC
Make that:
https://live.gnome.org/Hackfests/IMContacts Social2011/Tasks/ShellDesignPresence
Comment 2 William Jon McCann 2011-06-16 17:25:26 UTC
A transient notification for this seems like a good idea.

Need to decide on the text.
Comment 3 Florian Müllner 2011-06-17 10:52:12 UTC
Created attachment 190103 [details] [review]
status-menu: Tell user notifications are blocked while busy

For most users it is obvious that the user status controls the
IM/online status, but the additional effect of suppressing
(non-critical) notifications is less so. Display a transient
notifications educating the user about the feature.

(In reply to comment #2)
> Need to decide on the text.

Right. I attached a proposal, but it'll need to be improved ...
Comment 4 Dan Winship 2011-06-17 13:24:37 UTC
Comment on attachment 190103 [details] [review]
status-menu: Tell user notifications are blocked while busy

>+            Main.overview.shellInfo.setMessage(_("You won't get any more notifications while busy"));

If we're going to be using that from outside the overview, then it should really be outside the overview. Maybe add Main.notify() to go along with Main.notifyError().
Comment 5 Florian Müllner 2011-07-28 18:41:54 UTC
Created attachment 192830 [details] [review]
main: Add Main.notify() for simple system messages

... similar to Main.notifyError(), but don't duplicate the message
on stderr/in the log.

(In reply to comment #4)
> If we're going to be using that from outside the overview, then it should
> really be outside the overview. Maybe add Main.notify() to go along with
> Main.notifyError().

Sure. Not updating the other patch for now, as it conflicts with the status menu work in bug 652837.
Comment 6 Florian Müllner 2011-08-03 15:20:23 UTC
Comment on attachment 192830 [details] [review]
main: Add Main.notify() for simple system messages

Attachment 192830 [details] pushed as 8bc85d4 - main: Add Main.notify() for simple system messages
Comment 7 Florian Müllner 2011-08-26 08:59:53 UTC
Created attachment 194797 [details] [review]
status-menu: Tell user notifications are blocked while busy

Woops, forgot to attach an updated version rebased on the patches in bug 652837 ...
Comment 8 Jasper St. Pierre (not reading bugmail) 2011-08-26 21:06:52 UTC
Review of attachment 194797 [details] [review]:

Sure.
Comment 9 Matthias Clasen 2011-08-29 00:23:30 UTC
Not sure if Florian is around to commit this
Comment 10 Florian Müllner 2011-08-29 15:09:39 UTC
(In reply to comment #9)
> Not sure if Florian is around to commit this

I am, but I'd like to land bug 652837 first ...
Comment 11 Florian Müllner 2011-09-03 22:13:54 UTC
Allan, Jon: you had some concerns here - can we try figuring it out (e.g. rename "Do Not Disturb" to "Notifications" and not show the popup, land this patch as-is, or with a different message, ...) before string freeze?
Comment 12 Allan Day 2011-09-04 15:11:49 UTC
(In reply to comment #11)
> Allan, Jon: you had some concerns here - can we try figuring it out (e.g.
> rename "Do Not Disturb" to "Notifications" and not show the popup, land this
> patch as-is, or with a different message, ...) before string freeze?

From my conversation with Jon:

<mccann>	is do not disturb = notifications off?
<mccann>	if so then maybe it should just be called Notifications [Off]
<aday>	yes, dnd == no notifications
<mccann>	do not disturb is kinda wrong there
<mccann>	because that is a signal
<mccann>	it is a request to others
<mccann>	"do not disturb" has the implied "please"
<mccann>	Notifications [On/Off] is much clearer I think

That makes sense to me - let's change it to 'Notifications [ on / off ]'.

I'm not sure where that leaves this bug. Maybe this change will be enough to make the functionality clear?
Comment 13 Florian Müllner 2011-09-04 17:02:30 UTC
Created attachment 195631 [details] [review]
user-menu: Rename "Do Not Disturb" to "Notifications"

At least for the foreseeable future, the gnome-session desktop
presence won't be used for anything but suppressing (non-urgent)
notifications. To clarify this behavior, rename the "Do Not Disturb"
switch to "Notifications" (and adjust the switch logic accordingly).

(In reply to comment #12)
> That makes sense to me - let's change it to 'Notifications [ on / off ]'.

Sure.


> I'm not sure where that leaves this bug. Maybe this change will be enough to
> make the functionality clear?

It does in my opinion. However, the interaction with the IM status is a lot more mysterious with that patch - why would disabling "Notifications" switch my chat status to "Busy"? On the other hand, the behavior itself still makes sense to me - when sidestepping empathy by only using gnome-shell for IM, turning off notifications means missing every single chat message; notifying chat partners about this (by setting the IM status accordingly) looks like the right (polite) thing to do ...

Comments?
Comment 14 Milan Bouchet-Valat 2011-09-04 18:28:50 UTC
(In reply to comment #13)
> Comments?
That's probably OK, but I'd change the text of the "You will not receive further notifications while busy" notification to explain this too. If you say something, say it all.
Comment 15 Allan Day 2011-09-04 18:30:36 UTC
(In reply to comment #13)
> > I'm not sure where that leaves this bug. Maybe this change will be enough to
> > make the functionality clear?
> 
> It does in my opinion. However, the interaction with the IM status is a lot
> more mysterious with that patch - why would disabling "Notifications" switch my
> chat status to "Busy"? On the other hand, the behavior itself still makes sense
> to me - when sidestepping empathy by only using gnome-shell for IM, turning off
> notifications means missing every single chat message; notifying chat partners
> about this (by setting the IM status accordingly) looks like the right (polite)
> thing to do ...
> 
> Comments?

I'd agree with all of that.
Comment 16 Florian Müllner 2011-09-04 20:30:30 UTC
(In reply to comment #14)
> (In reply to comment #13)
> > Comments?
> That's probably OK, but I'd change the text of the "You will not receive
> further notifications while busy" notification to explain this too. If you say
> something, say it all.

I think I'd rather dump this notification (turning "Notifications" off disables notifications - sounds rather obvious to me) and display another one when we change the IM status automatically to explain the reasoning behind this.
Comment 17 Florian Müllner 2011-09-05 19:53:42 UTC
(In reply to comment #13)
> Created an attachment (id=195631) [details] [review]
> user-menu: Rename "Do Not Disturb" to "Notifications"

Ping? String freeze is about to start ...
Comment 18 Dan Winship 2011-09-06 13:53:12 UTC
Comment on attachment 195631 [details] [review]
user-menu: Rename "Do Not Disturb" to "Notifications"

ok, looks right

(string freeze doesn't start until the tarball is released, which wasn't going to happen yesterday since it was a US holiday and Owen wasn't around.)
Comment 19 Florian Müllner 2011-09-06 15:22:05 UTC
Comment on attachment 195631 [details] [review]
user-menu: Rename "Do Not Disturb" to "Notifications"

Attachment 195631 [details] pushed as a5d0ac7 - user-menu: Rename "Do Not Disturb" to "Notifications"

(In reply to comment #18)
> (string freeze doesn't start until the tarball is released [...])

Oh, I wasn't aware of that - thanks!
Comment 20 Matthias Clasen 2011-09-09 13:43:51 UTC
so, this is still going to land ?
Comment 21 Florian Müllner 2011-09-09 17:08:39 UTC
Created attachment 196128 [details] [review]
user-menu: Explain why disabling notifications changes IM status

While the current behavior of setting the IM status to "busy" while
notifications are disabled makes sense, as incoming messages are
very likely to be missed, it is not immediately obvious.
Display a transient notification to explain the behavior to the user.


(In reply to comment #20)
> so, this is still going to land ?

Maybe, but not the previous patch, which does no longer make much sense after renaming "Do Not Disturb" to "Notifications".

I attached a new patch which tries to explain why we adjust the IM status when disabling notifications - I think the message is way too wordy, but right now I am unable to come up with anything better (hey, we've got a public holiday over here today, so I shouldn't be in front of a computer anyway ;-)

Seriously, any suggestion to improve the notification will be much appreciated.
Comment 22 Matthias Clasen 2011-09-12 21:39:30 UTC
Here is some rewording that tries to avoid the passive language, and some of the repetitiveness:

Notifications are now disabled, including chat messages. Your online status has been adjusted to let others know that you might not see their messages.
Comment 23 Florian Müllner 2011-09-12 22:11:41 UTC
Created attachment 196316 [details] [review]
user-menu: Explain why disabling notifications changes IM status

(In reply to comment #22)
> Here is some rewording that tries to avoid the passive language, and some of
> the repetitiveness

Sure, thanks!
Comment 24 Dan Winship 2011-09-13 12:57:37 UTC
Comment on attachment 196316 [details] [review]
user-menu: Explain why disabling notifications changes IM status


>-            if (presence == Tp.ConnectionPresenceType.AVAILABLE ||
>-                presence == Tp.ConnectionPresenceType.EXTENDED_AWAY) {

>+            if (this._currentPresence == Tp.ConnectionPresenceType.AVAILABLE ||
>+                this._currentPresence == Tp.ConnectionPresenceType.EXTENDED_AWAY)

it's not obvious to me that these are the same... and if they are, is there any reason to call get_most_available_presence() in _sessionStatusChanged rather than just using this._currentPresence there as well?
Comment 25 Florian Müllner 2011-09-13 18:12:34 UTC
(In reply to comment #24)
> (From update of attachment 196316 [details] [review])
> 
> >-            if (presence == Tp.ConnectionPresenceType.AVAILABLE ||
> >-                presence == Tp.ConnectionPresenceType.EXTENDED_AWAY) {
> 
> >+            if (this._currentPresence == Tp.ConnectionPresenceType.AVAILABLE ||
> >+                this._currentPresence == Tp.ConnectionPresenceType.EXTENDED_AWAY)
> 
> it's not obvious to me that these are the same...

this._currentPresence is set in the handler of Tp.AccountManager::most-available-presence-changed, so I very much expect it to always represent the correct presence.
(If "expecting" it from telepathy is too weak, I wouldn't mind a call to get_most_available_presence() here as well ...)


> and if they are, is there any reason to call get_most_available_presence() in
> _sessionStatusChanged rather than just using this._currentPresence there as well?

Yes, if the user has set a custom status message, we want to preserve that (3rd return value of get_most_available_presence(), 3rd parameter to set_all_requested_presences()) ...
Comment 26 Florian Müllner 2011-09-17 15:26:41 UTC
Attachment 196316 [details] pushed as cd7b9e1 - user-menu: Explain why disabling notifications changes IM status

Pushed with i18n team approval