Closed
Bug 954950
Opened 10 years ago
Closed 10 years ago
Don't display the topic system message again if it hasn't changed
Categories
(Chat Core :: IRC, defect)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: aleth, Assigned: aleth)
Details
Attachments
(1 file, 4 obsolete files)
8.59 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** 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).
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 1•10 years ago
|
||
*** 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 | ||
Updated•10 years ago
|
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
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
Comment 2•10 years ago
|
||
*** 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 ;).
Comment 3•10 years ago
|
||
*** 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.
Assignee | ||
Comment 4•10 years ago
|
||
*** 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).
Comment 5•10 years ago
|
||
*** 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 ;).
Assignee | ||
Comment 6•10 years ago
|
||
*** 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.
Assignee | ||
Comment 7•10 years ago
|
||
*** 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...)
Assignee | ||
Comment 8•10 years ago
|
||
*** 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 9•10 years ago
|
||
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-
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 10•10 years ago
|
||
*** 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)
Assignee | ||
Comment 11•10 years ago
|
||
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
Assignee | ||
Comment 12•10 years ago
|
||
*** 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)
Assignee | ||
Comment 13•10 years ago
|
||
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)
Comment 14•10 years ago
|
||
*** 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 15•10 years ago
|
||
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 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
*** 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 19•10 years ago
|
||
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-
Assignee | ||
Comment 20•10 years ago
|
||
*** 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)
Assignee | ||
Comment 21•10 years ago
|
||
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
Assignee | ||
Comment 22•10 years ago
|
||
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 23•10 years ago
|
||
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+
Comment 24•10 years ago
|
||
*** 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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in
before you can comment on or make changes to this bug.
Description
•