Closed Bug 954899 Opened 10 years ago Closed 10 years ago

Clearing the topic doesn't update the UI and instead displays the previous topic in a system message

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: clokep)

References

Details

Attachments

(2 files, 2 obsolete files)

*** Original post on bio 1466 at 2012-05-24 17:16:00 UTC ***

When clearing the topic (either through the topic bar or the /topic command without parameter), the TOPIC command is sent to the server, but the topic UI isn't updated, and the previous topic is displayed in the conversation in a system message "The topic for <channel name> is: <previous topic>."
Attached image Error console from flo
*** Original post on bio 1466 as attmnt 1525 at 2012-05-24 17:25:00 UTC ***

[1] needs to check the case was lastParam.length == 0, in which case we have to add a colon as well.

[1] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#987
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1466 as attmnt 1526 at 2012-05-25 02:07:00 UTC ***

The changes in this patch make it so that if an empty last parameter is used when sending a message we prepend it with a colon (i.e. it's just a colon). This is necessary to clear the topic when you're supposed to send an empty last parameter.

This also handles receiving an empty last parameter.

Note that when testing this I got some kind of "strange" behavior when clearing the topic. After setting the topic and then clearing it, I would always get a 332 response (RPL_TOPIC), but with an empty topic; instead of a 331 (RPL_NOTOPIC). If I queried the server before ever setting a topic it correctly send back a 331. I tested changing the topic with Mibbit as well as saw the same results. The message we're sending exactly matches the RFC now. I'm guessing this is a quirk of the servers.
Attachment #8353279 - Flags: review?(bugzilla)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Blocks: 954803
OS: Other → All
Hardware: x86 → All
Comment on attachment 8353279 [details] [diff] [review]
Patch

*** Original change on bio 1466 attmnt 1526 at 2012-05-25 10:25:46 UTC ***

This doesn't work when we receive an empty status over TOPIC.
Attachment #8353279 - Flags: review?(bugzilla)
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1466 as attmnt 1527 at 2012-05-25 10:42:00 UTC ***

This patch also handles getting an empty message in the 332 handler. Unfortunately the code is very similar to the TOPIC handler, but the strings and parameter numbers are different. We could probably combine them if we'd like, but the method would need ~6 or 7 parameters.
Attachment #8353280 - Flags: review?(florian)
Comment on attachment 8353279 [details] [diff] [review]
Patch

*** Original change on bio 1466 attmnt 1526 at 2012-05-25 10:42:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353279 - Attachment is obsolete: true
Attached patch Patch v3Splinter Review
*** Original post on bio 1466 as attmnt 1528 at 2012-05-25 11:15:00 UTC ***

Attachment 8353280 [details] [diff] (bio-attmnt 1527) looks good, but I'm suggesting these 2 additional changes (included in this new attachment):

diff -u b/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
--- b/chat/protocols/irc/ircBase.jsm
+++ b/chat/protocols/irc/ircBase.jsm
@@ -807,7 +807,7 @@
       conversation.setTopic(""); // Clear the topic.
       let msg = _("message.topicNotSet", conversation.name);
       conversation.writeMessage(null, msg, {system: true});
-      return false;
+      return true;
     },
     "332": function(aMessage) { // RPL_TOPIC
       // <channel> :<topic>
@@ -818,7 +818,7 @@
       // Send the topic as a message.
       let message;
       if (topic)
-        message = _("message.topic", conversation.name, aMessage.params[2]);
+        message = _("message.topic", conversation.name, topic);
       else
         message = _("message.topicNotSet", conversation.name);
       conversation.writeMessage(null, message, {system: true});
Attachment #8353281 - Flags: review?(clokep)
Comment on attachment 8353280 [details] [diff] [review]
Patch v2

*** Original change on bio 1466 attmnt 1527 at 2012-05-25 11:15:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353280 - Attachment is obsolete: true
Attachment #8353280 - Flags: review?(florian)
Comment on attachment 8353281 [details] [diff] [review]
Patch v3

*** Original change on bio 1466 attmnt 1528 at 2012-05-25 11:52:38 UTC ***

Those changes look good, thanks for fixing those two details.
Attachment #8353281 - Flags: review?(clokep) → review+
*** Original post on bio 1466 at 2012-05-25 15:03:14 UTC ***

https://hg.instantbird.org/instantbird/rev/90609e9d4546
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.