Closed Bug 955559 Opened 7 years ago Closed 7 years ago

The /join command shouldn't send a JOIN command to the server if we are already in the channel

Categories

(Chat Core :: IRC, defect)

x86
Other
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: aleth)

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 2121 at 2013-08-22 23:12:00 UTC ***

Steps to reproduce:
1. Join a channel.
2. Put the conversation on hold.
3. Forget about it.
4. Attempt to join it (doing it from the Join Chat dialog will reopen the existing conversation on hold; doing it with the /join command will just do nothing).

The debug log shows:
prpl-irc: Sending:
JOIN #channel
*** Original post on bio 2121 at 2013-08-23 10:00:36 UTC ***

While this is a bug, I suspect what really annoyed here was bug 954270 (bio 837) ;)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2121 as attmnt 2753 at 2013-08-23 10:44:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354522 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8354522 [details] [diff] [review]
Patch

*** Original change on bio 2121 attmnt 2753 at 2013-08-23 10:46:29 UTC ***

I'm pretty sure this will break if we join a channel, part it and want to rejoin.
Attachment #8354522 - Flags: review?(clokep) → review-
*** Original post on bio 2121 at 2013-08-23 10:46:54 UTC ***

The question is whether this check should be in joinChat() instead.
*** Original post on bio 2121 at 2013-08-23 10:50:12 UTC ***

(In reply to comment #1)
> While this is a bug, I suspect what really annoyed here was bug 954270 (bio 837) ;)

No, what really annoyed was a broken local debug build. This bug just added to my confusion about what was going on.
*** Original post on bio 2121 at 2013-08-23 10:53:26 UTC ***

(In reply to comment #4)
> The question is whether this check should be in joinChat() instead.

Yes.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2121 as attmnt 2754 at 2013-08-23 10:56:00 UTC ***

Thanks for catching that.
Attachment #8354523 - Flags: review?(clokep)
Comment on attachment 8354522 [details] [diff] [review]
Patch

*** Original change on bio 2121 attmnt 2753 at 2013-08-23 10:56:12 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354522 - Attachment is obsolete: true
Comment on attachment 8354523 [details] [diff] [review]
Patch

*** Original change on bio 2121 attmnt 2754 at 2013-08-23 11:44:15 UTC ***

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>@@ -1246,16 +1246,19 @@ ircAccount.prototype = {
>     // A channel prefix is required. If the user didn't include one,
>     // we prepend # automatically to match the behavior of other
>     // clients. Not doing it used to cause user confusion.
>     if (this.channelPrefixes.indexOf(channel[0]) == -1)
>       channel = "#" + channel;
Nit: Add an empty line here.

Please also include a comment saying what this check is doing ("Don't try to rejoin a room the user is already in!").
>+    if (this.hasConversation(channel) && !this.getConversation(channel).left)
>+      return;
>+
Attachment #8354523 - Flags: review?(clokep) → review-
Attached patch PatchSplinter Review
*** Original post on bio 2121 as attmnt 2755 at 2013-08-23 11:46:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354524 - Flags: review?(clokep)
Comment on attachment 8354523 [details] [diff] [review]
Patch

*** Original change on bio 2121 attmnt 2754 at 2013-08-23 11:46:47 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354523 - Attachment is obsolete: true
Comment on attachment 8354524 [details] [diff] [review]
Patch

*** Original change on bio 2121 attmnt 2755 at 2013-08-23 11:52:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354524 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2121 at 2013-08-23 21:46:39 UTC ***

Thanks.
http://hg.instantbird.org/instantbird/rev/df197dc3277d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.