GNOME Bugzilla – Bug 662900
Show feedback notifications when the user is busy
Last modified: 2012-11-02 18:28:42 UTC
Notifications that are created in response to direct user actions like "is ready" or "'foo' has been removed from favorites" should always be displayed even though the user has marked him/herself busy. Because no feedback at all is surly not what the user expects in this case. As for the "is ready" in lots of situations we should probably just show the window but that is kind of a different bug.
I'm going to assign this bug to some OPW applicant, so please consider this bug claimed.
(In reply to comment #1) > I'm going to assign this bug to some OPW applicant, so please consider this bug > claimed. Hi Marina, is this bug not claimed any more?
(In reply to comment #2) > (In reply to comment #1) > > I'm going to assign this bug to some OPW applicant, so please consider this bug > > claimed. > > Hi Marina, is this bug not claimed any more? I'll assume that it's free, then. :)
Created attachment 227352 [details] [review] Tentative patch Trivial patch with urgency set to URGENT.
A couple of comments: - the enhancement makes sense, but maybe the message you get when you activate the "Notifications" switch should be changed? "Notifications are now disabled, including chat messages. Your online status has been adjusted to let others know that you might not see their messages." Or from a design pov you think it's ok? - I addressed only the two sources of notifications you mentioned, I did not find any other where this can be applied - from a code perspective, I've got mixed feelings about the way to go, as there are at least two ways to do this: * just change the urgency (but this is not "urgent") * add a flag initiatedByUser on Notification (but _updateState is quite complex already) And I kinda understand the point of Overview.setMessage but it is only used by the favorites. What is the best way to go with it? (add custom urgency param, lambda to customize notification, add Overview.setNotification or similar just to use the same source, expose the source?) Please help with the technical way to go.
Review of attachment 227352 [details] [review]: This is wrong: if I'm talking with a friend in the overview, this new silly message will take over, since it's CRITICAL. It will also have all the CRITICAL side effects: not timing out, expanding by default, etc. I'd rather go with another boolean about this. It shouldn't be too hard.
Ok, I should not have even posted the patch in the first place. It's easy to have a flag on the notification, what about Overview.setMessage in my previous comment? How to set this new flag on the notification? In the favorites case, I'd rather have it take over my friend message though, as I'm the one who initiated it, so I'm chatting "passively" (doing something else at the same time).
Created attachment 227373 [details] [review] Use a flag instead I've added a flag, and converted the options of Overview#setMessage to an Object so it is easier to understand from the caller side since all of them are optional. I do not know if this could break extensions: most of them use Main.notify*, right? This is because I do not really understand the point of #setMessage (a single user, keep the source). It would as easy to directly pass a notification object (or add a setNotification API) and factor its construction in appFavorites.
Review of attachment 227373 [details] [review]: ::: js/ui/appFavorites.js @@ +88,3 @@ + this._removeFavorite(appId); + }) + }; This is a bit awkward, I'd prefer this inline with the call. ::: js/ui/messageTray.js @@ +717,3 @@ + setForFeedback: function(forFeedback) { + this.forFeedback = forFeedback; + }, .oO( One of these days we got to get rid of these useless setters ) ::: js/ui/overview.js @@ +61,3 @@ + let undoCallback = options.undoCallback || null; + let undoLabel = options.undoLabel || _("Undo"); + let forFeedback = options.forFeedback || false; Please use Params.parse for this. @@ +240,3 @@ + // options: + // - undoCallback (function): the callback to be called if undo support is needed + // - undoLabel (string): the possible custom label for the undo action Hm, we don't ever seem to use undoLabel. As a first part, I think we can just safely remove it. @@ +247,3 @@ return; + this._shellInfo.setMessage(text, options || {}); Params.parse will handle this case too.
Created attachment 227888 [details] [review] Better patch For the setters, I can provide a patch, but I suppose it could break many extensions... or not? Main.notify and the notification spec seems to be the most used way to notify the user. Is it easy to grep for setTransient setUrgency in all published extensions? are they uncompressed server-side?
(In reply to comment #10) > Created an attachment (id=227888) [details] [review] > Better patch > > For the setters, I can provide a patch, but I suppose it could break many > extensions... or not? Main.notify and the notification spec seems to be the > most used way to notify the user. Extension authors have to explicitly mark extensions as compatible with a version. > Is it easy to grep for setTransient setUrgency in all published extensions? are > they uncompressed server-side? Unfortunately, no.
Review of attachment 227888 [details] [review]: Looks good to me, I like it.
Created attachment 227916 [details] [review] Ready to go! I do not have commit access, so please commit for me. In this final patch, I fixed the bug ref.
Sure, thanks for the patch! BTW, it looks like you are not using git-bz to attach your patches - there's no requirement to do so of course, but it smoothes the workflow quite a bit, so you might want to look into that ... (tools/build/gnome-shell-build-setup.sh installs git-bz for you)