Closed Bug 954950 Opened 6 years ago Closed 6 years ago

Don't display the topic system message again if it hasn't changed

Categories

(Chat Core :: IRC, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 1518 at 2012-06-16 15:35:00 UTC ***

It seems unnecessarily noisy (though traditional) to display the topic again, unless the rejoin has been triggered explicitly by the user. 

If we are autojoining after a disconnect, bug 954839 (bio 1404) fulfils the role this effectively plays currently (telling the user they have reconnected).
Depends on: 954839
Depends on: 953828
Summary: Don't display the topic system message again when rejoining a channel automatically → Don't display the topic system message again when rejoining and it hasn't changed
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1518 as attmnt 1619 at 2012-06-17 20:16:00 UTC ***

Only show the topic system message if the topic (or its setter) has in fact changed.

This solution isn't even in conflict with the libpurple setTopic as that already returns NS_OK (not that it matters really).
Attachment #8353376 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Summary: Don't display the topic system message again when rejoining and it hasn't changed → Don't display the topic system message again if it hasn't changed
*** Original post on bio 1518 at 2012-06-18 09:28:19 UTC ***

(In reply to comment #1)

> This solution isn't even in conflict with the libpurple setTopic as that
> already returns NS_OK (not that it matters really).

Returning NS_OK in C++ XPCOM is equivalent to not calling throw from a JS function.

And I really think you meant purplexpcom rather than libpurple here ;).
*** Original post on bio 1518 at 2012-06-18 09:35:54 UTC ***

(In reply to comment #0)

> unless the rejoin has been triggered explicitly by the user. 

Why is it different if the user has triggered the rejoin?


Why can't the system message be displayed in jsProtoHelper's setTopic method? That way the same code would be reused for XMPP MUCs.

IIRC we wanted to move all the system messages related to chats to jsProtoHelper (it we don't want to remove the ones from libpurple's conversation.c), or even imConversations.js (if we want to really guarantee a consistent behavior accross all prpls) instead of having them in irc's code.
*** Original post on bio 1518 at 2012-06-18 09:50:45 UTC ***

(In reply to comment #2)
> (In reply to comment #1)
> 
> > This solution isn't even in conflict with the libpurple setTopic as that
> > already returns NS_OK (not that it matters really).
> 
> Returning NS_OK in C++ XPCOM is equivalent to not calling throw from a JS
> function.
> 
> And I really think you meant purplexpcom rather than libpurple here ;).

You're right, of course :$

(In reply to comment #3)
> (In reply to comment #0)
> 
> > unless the rejoin has been triggered explicitly by the user. 
> 
> Why is it different if the user has triggered the rejoin?

It isn't in this patch. I changed my mind while fixing the bug, and changed the title to reflect that.

> Why can't the system message be displayed in jsProtoHelper's setTopic method?
> That way the same code would be reused for XMPP MUCs.
> 
> IIRC we wanted to move all the system messages related to chats to
> jsProtoHelper (it we don't want to remove the ones from libpurple's
> conversation.c), or even imConversations.js (if we want to really guarantee a
> consistent behavior accross all prpls) instead of having them in irc's code.

While that sounds like a good idea, this seems a large-ish reorganisation not directly related to this bug (large if you include quit/join messages etc - it would involve going through all the strings of the various protocols (JS ones at least) and unifying them).
*** Original post on bio 1518 at 2012-06-18 10:01:23 UTC ***

(In reply to comment #4)

> > IIRC we wanted to move all the system messages related to chats to
> > jsProtoHelper (it we don't want to remove the ones from libpurple's
> > conversation.c), or even imConversations.js (if we want to really guarantee a
> > consistent behavior accross all prpls) instead of having them in irc's code.
> 
> While that sounds like a good idea, this seems a large-ish reorganisation not
> directly related to this bug (large if you include quit/join messages etc - it
> would involve going through all the strings of the various protocols (JS ones
> at least) and unifying them).

I wasn't implying that you have to move all of these messages for this patch; justing reminding where we want to go eventually ;).
*** Original post on bio 1518 at 2012-06-18 10:03:46 UTC ***

(In reply to comment #5)
> I wasn't implying that you have to move all of these messages for this patch;
> justing reminding where we want to go eventually ;).

I'm not even sure it will turn out to be doable actually. For example, twitter currently never prints a system message when changing the topic, because the topic is just the last user tweet, which has already been displayed.
*** Original post on bio 1518 at 2012-06-18 10:06:14 UTC ***

(In reply to comment #6)
> I'm not even sure it will turn out to be doable actually. For example, twitter
> currently never prints a system message when changing the topic, because the
> topic is just the last user tweet, which has already been displayed.

(Though I suppose if twitter was the only exception, it could override setTopic...)
*** Original post on bio 1518 at 2012-06-18 10:25:29 UTC ***

(In reply to comment #5)
> (In reply to comment #4)
> 
> > > IIRC we wanted to move all the system messages related to chats to
> > > jsProtoHelper (it we don't want to remove the ones from libpurple's
> > > conversation.c), or even imConversations.js (if we want to really guarantee a
> > > consistent behavior accross all prpls) instead of having them in irc's code.

clokep pointed out this is bug 954662 (bio 1230).
Comment on attachment 8353376 [details] [diff] [review]
Patch

*** Original change on bio 1518 attmnt 1619 at 2012-06-19 02:02:38 UTC ***

This seems overly complicated and I think it would be simpler to add these messages to the jsProto implementation.

Of course this means we'll need to overwrite the Twitter implementation so it never prints a message.
Attachment #8353376 - Flags: review?(clokep) → review-
No longer depends on: 953828, 954839
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1518 as attmnt 1634 at 2012-06-19 12:24:00 UTC ***

Moves sending the system message to the jsProto as requested.
Attachment #8353391 - Flags: review?(clokep)
Comment on attachment 8353376 [details] [diff] [review]
Patch

*** Original change on bio 1518 attmnt 1619 at 2012-06-19 12:24:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353376 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1518 as attmnt 1635 at 2012-06-19 12:34:00 UTC ***

Fits better with the existing style of conversations.properties.
Attachment #8353392 - Flags: review?(clokep)
Comment on attachment 8353391 [details] [diff] [review]
Patch

*** Original change on bio 1518 attmnt 1634 at 2012-06-19 12:34:18 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353391 - Attachment is obsolete: true
Attachment #8353391 - Flags: review?(clokep)
Attached patch Fixed patch (obsolete) — Splinter Review
*** Original post on bio 1518 as attmnt 1639 at 2012-06-20 00:53:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353396 - Flags: review?(bugzilla)
Comment on attachment 8353392 [details] [diff] [review]
Patch

*** Original change on bio 1518 attmnt 1635 at 2012-06-20 00:53:10 UTC ***

This looks pretty good, it didn't apply perfectly though, I'll upload a fixed version in a second.

Florian, do we have a better place to put these strings or is conversations.properties?
Attachment #8353392 - Flags: review?(clokep) → review+
Comment on attachment 8353396 [details] [diff] [review]
Fixed patch

*** Original change on bio 1518 attmnt 1639 at 2012-06-20 00:54:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353396 - Flags: review?(florian)
Comment on attachment 8353396 [details] [diff] [review]
Fixed patch

*** Original change on bio 1518 attmnt 1639 at 2012-06-20 08:20:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353396 - Flags: review?(bugzilla) → review+
*** Original post on bio 1518 at 2012-06-20 09:05:33 UTC ***

>  setTopic: function(aTopic, aTopicSetter) {
...
>+    if (aTopicSetter) {
>+      if (aTopic)
>+        message = _("topicChanged", aTopicSetter, aTopic);
>+      else
>+        message = _("topicCleared", aTopicSetter);
>+    }
>+    else {
>+      aTopicSetter = null;
>+      if (aTopic)
>+        message = _("topicSet", this.name, aTopic);
>+      else
>+        message = _("topicNotSet", this.name);
>+    }

Just wanted to check that 'aTopicSetter = null' is correct JS. It doesn't throw any warnings, but I am not certain of the status of an argument that is not passed (it's undefined, but can I assign a new value to it?)
Comment on attachment 8353396 [details] [diff] [review]
Fixed patch

*** Original change on bio 1518 attmnt 1639 at 2012-06-20 11:10:47 UTC ***

Moving these strings to conversations.properties is OK with me.

>diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm
>--- a/chat/modules/jsProtoHelper.jsm
>+++ b/chat/modules/jsProtoHelper.jsm
>@@ -17,16 +17,20 @@ const EXPORTED_SYMBOLS = [
> 
> const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
> 
> Cu.import("resource:///modules/imXPCOMUtils.jsm");
> Cu.import("resource:///modules/imServices.jsm");
> 
> initLogModule("jsProtoHelper", this);
> 
>+XPCOMUtils.defineLazyGetter(this, "_", function()
>+  l10nHelper("chrome://chat/locale/conversations.properties")
>+);

One little concern is that defining _ to conversations.properties in jsProtoHelper might be too tempting for the next time we want to add a string in jsProtoHelper and don't know where to put it. I'm afraid it may become a catch all.

For now this is ok, but we may want to rename "_" to "_conv" or "_conversations" or "convStrings" or whatever later.


>@@ -476,16 +480,33 @@ const GenericConvChatPrototype = {
>     // Only change the topic if the topic and/or topic setter has changed.
>     if (this._topic == aTopic && this._topicSetter == aTopicSetter)
>       return;
> 
>     this._topic = aTopic;
>     this._topicSetter = aTopicSetter;
> 
>     this.notifyObservers(null, "chat-update-topic");

I think I would add here:
if (aQuiet)
  return;

>+
>+    // Send the topic as a message.
>+    let message;
>+    if (aTopicSetter) {
>+      if (aTopic)
>+        message = _("topicChanged", aTopicSetter, aTopic);
>+      else
>+        message = _("topicCleared", aTopicSetter);
>+    }
>+    else {
>+      aTopicSetter = null;
>+      if (aTopic)
>+        message = _("topicSet", this.name, aTopic);
>+      else
>+        message = _("topicNotSet", this.name);
>+    }
>+    this.writeMessage(aTopicSetter, message, {system: true});
>   },

>diff --git a/chat/protocols/twitter/twitter.js b/chat/protocols/twitter/twitter.js
>--- a/chat/protocols/twitter/twitter.js
>+++ b/chat/protocols/twitter/twitter.js
>@@ -316,17 +316,25 @@ Conversation.prototype = {
>     this._participants[aNick] = chatBuddy;
>     this.notifyObservers(new nsSimpleEnumerator([chatBuddy]),
>                          "chat-buddy-add");
>   },
>   setTopic: function(aTopic, aTopicSetter) {
>     const kEntities = {amp: "&", gt: ">", lt: "<"};
>     let topic =
>       aTopic.replace(/&([gl]t|amp);/g, function(str, entity) kEntities[entity]);
>-    GenericConvChatPrototype.setTopic.call(this, topic, aTopicSetter);
>+      
>+    // Only change the topic if the topic and/or topic setter has changed.
>+    if (this._topic == topic && this._topicSetter == aTopicSetter)
>+      return;
>+      
>+    this._topic = topic;
>+    this._topicSetter = aTopicSetter;
>+    
>+    this.notifyObservers(null, "chat-update-topic");

And avoid the duplication here.
Attachment #8353396 - Flags: review?(florian) → review-
Attached patch PatchSplinter Review
*** Original post on bio 1518 as attmnt 1644 at 2012-06-20 12:56:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353401 - Flags: review?(florian)
Comment on attachment 8353392 [details] [diff] [review]
Patch

*** Original change on bio 1518 attmnt 1635 at 2012-06-20 12:56:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353392 - Attachment is obsolete: true
Comment on attachment 8353396 [details] [diff] [review]
Fixed patch

*** Original change on bio 1518 attmnt 1639 at 2012-06-20 12:56:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353396 - Attachment is obsolete: true
Comment on attachment 8353401 [details] [diff] [review]
Patch

*** Original change on bio 1518 attmnt 1644 at 2012-06-21 00:14:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353401 - Flags: review?(florian) → review+
*** Original post on bio 1518 at 2012-06-21 00:29:54 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/85be28e2cc78

Thanks for fixing this!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.