Of course it is much better if we also have a conversation based messaging/chat application:
 * to reinforce the model of the scope of the conversation
 * provide a place to undo the mute action
 * access the conversation log/history after - once you are ready to review it

Would it be ok for now to just have "Mute"/Unmute" in the right click menu, and keep the conversation in the tray as long as there are new messages?

I'm going to assign it to some OPW applicants, so please consider this bug claimed.

Created attachment 200292 [details]
Patch implementing the feature

Hi!
Here's my patch for mute/unmute conversations.

Review of attachment 200292 [details]:

The patch works as advertised in a quick test, and the code looks generally good - thanks! Apart from the comments and suggestions below, some nitpicks for the commit message: "enable muting conversations" sounds a bit weird to me, how about "Add option to (un)mute conversations"? Also, "messagges" in the last sentence ...

::: js/ui/messageTray.js
@@ +1059,3 @@
 
+    isMuted: function() {
+        return this._isMuted;

This method is kind of pointless; it is generally not advisable to set a javascript property defined in another prototype (obvious example: setMuted() does more than setting the property), but reading is perfectly fine.

So I'd remove the underscore from _isMuted and test for source.isMuted instead. If you don't like using a method in one case and direct access in another, here is a neat alternative:

In _init:
  this.isMuted = false;  // just like you do now

Define get/set methods for isMuted:

   get isMuted() {
       return this._isMuted;
   },

   set isMuted(muted) {
       // what you have in setMuted()
   }

isMuted appears then as a public property which you can read and write, e.g.
   if (source.isMuted)
       source.isMuted = false;

@@ +1064,3 @@
+    setMuted: function(isMuted) {
+        if (this._isMuted == isMuted)
+            return;

Should there be a test for this.isChat? So far the feature is only intended for chat conversations, so maybe it is a good idea to make it explicit?

Ana, thanks for posting the patch!

Florian, thanks for reviewing it :)! The reason I suggested adding a setter and a getter is that I thought making a variable private makes it clear that no one should set it directly. But it seems that you are saying and the code corroborates that we are not setting variables from other prototypes directly.

There is actually an example in the code for Source that makes it easy to decide what we should do. this.title can be accessed directly, but has a setter that actually does other stuff. So we can be consistent with that, and have this.isMuted, access it directly when we need to get its value, and have setMuted() defined. So basically this agrees with your first suggestion.

With the second suggestion, that would make something like source.isMuted = false appear in the code, but we don't want to make it look like it is ok to set variables defined in another prototype.

The suggestion to check this.isChat in setMuted() sounds good to me.

(In reply to comment #5)
> With the second suggestion, that would make something like source.isMuted =
> false appear in the code, but we don't want to make it look like it is ok to
> set variables defined in another prototype.

We do have some precedents for this (see workspaceThumbnail.js), and I think the schema has some legitimacy considering that properties defined that way resemble GObject properties, which are perfectly fine to set.

Still, I was only mentioning it for completeness, if I wrote the code I'd go with the first suggestion :)

In addition to being able to undo the mute action through the chat application it might make sense to have it in https://live.gnome.org/Design/SystemSettings/Notifications as well.

Created attachment 200542 [details]
Patch implementing the feature v2

Hi,there!

Thank you very much for all your reviews and suggestions:)
Here's my new patch, where I applied the suggested changes - I have set this.isMuted and this.isChat is being checked in setMuted(). I hope it looks better now.

Review of attachment 200542 [details]:

(In reply to comment #8)
> I hope it looks better now.

Hey, it didn't look bad in the first place :)

The code looks good now, but you should provide a single patch - you'll need to "squash" both patches together. In order to do that, run
  git rebase -i origin/master

Change "pick" in front of the second patch to "squash", then delete the first patch's commit message.

(see man git-rebase for details)

::: js/ui/messageTray.js
@@ +1059,3 @@
 
+    setMuted: function(muted) {
+        if (this.isChat) {

I'd suggest using

if (!this.isChat)
    return;

to save one indentation level.

Review of attachment 200542 [details]:

::: js/ui/messageTray.js
@@ +1064,3 @@
+            this.isMuted = muted;
+            if (this.isMuted)
+                this.emit('muted');

I think I'd rather see 'mute-changed' so that people can listen to both a mute and an unmute. Something like:

    this.isMuted = muted;
    this.emit('mute-changed', this.isMuted);

It should actually be the following in the beginning of setMuted() :

if (!this.isChat || this.isMuted == muted)
    return;

The suggestion about emitting 'muted-changed' signal sounds good to me. I would just not pass in this.isMuted as an argument because this value is available from Source anyway. Don't forget to check source.isMuted before calling the filter() function when you connect to 'mute-changed' signal.

Created attachment 200610 [details] [review]
Patch implementing the feature

Here's the new patch, thank you all for the suggestions.

Review of attachment 200610 [details] [review]:

LGTM

(I'd slightly prefer 'muted-changed' as signal name, but that's pretty far into nitpick-territory ...)

Ana, thank you for the patch!

Changed the signal name to 'muted-changed'.

Incorporating muting/unmuting option into the system settings panel for notifications seems to be a separate issue that can be addressed when that panel is designed, so closing this bug.