Closed Bug 955064 Opened 10 years ago Closed 10 years ago

The /join IRC command should work without argument when typed in a parted room.

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: aleth)

Details

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 1634 at 2012-08-09 12:41:00 UTC ***

As discussed on IRC, this seems to make sense and not be too difficult to implement.
Severity: normal → enhancement
OS: Other → All
Hardware: x86 → All
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1634 as attmnt 1816 at 2012-08-21 19:58:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353575 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1634 as attmnt 1817 at 2012-08-21 20:01:00 UTC ***

Forgot to generate the diff ;)
Attachment #8353576 - Flags: review?(clokep)
Comment on attachment 8353575 [details] [diff] [review]
Patch

*** Original change on bio 1634 attmnt 1816 at 2012-08-21 20:01:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353575 - Attachment is obsolete: true
Attachment #8353575 - Flags: review?(clokep)
Comment on attachment 8353576 [details] [diff] [review]
Patch

*** Original change on bio 1634 attmnt 1817 at 2012-08-21 20:08:57 UTC ***

>diff --git a/modules/ircCommands.jsm b/modules/ircCommands.jsm
>@@ -156,18 +156,23 @@ var commands = [
>     name: "join",
>     run: function(aMsg, aConv) {
>       let params = aMsg.trim().split(/,\s*/);
>-      if (!params[0])
>-        return false;
>+      let conv = getConv(aConv);
>+      if (!params[0]) {
>+        if (conv.isChat && conv.left)
>+          params = [conv.name];

I think you want to use the_chatRoomFields here so it will have a password, etc.:
params = [conv._chatRoomFields];

(From http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircBase.jsm#336)

>+        else
>+          return false;
Attachment #8353576 - Flags: review?(clokep) → review-
*** Original post on bio 1634 at 2012-08-21 20:10:37 UTC ***

> I think you want to use the_chatRoomFields here so it will have a password,
> etc.:
> params = [conv._chatRoomFields];

No, when the user has parted the channel, we purposely delete the chatRoomFields entry.
http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#224
*** Original post on bio 1634 at 2012-08-21 20:13:16 UTC ***

(In reply to comment #4)
> > I think you want to use the_chatRoomFields here so it will have a password,
> > etc.:
> > params = [conv._chatRoomFields];
> 
> No, when the user has parted the channel, we purposely delete the
> chatRoomFields entry.
> http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#224
We seem to be doing it to avoid automatic reconnection...but it seems like it'd be wanted for us to keep this information now. Bah...
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1634 as attmnt 1818 at 2012-08-21 20:28:00 UTC ***

I'm not so sure. It's not like the user told IB to remember the password, and the user explicitly parted. I changed the patch to make use of the stored chatRoomFields info when it is available (that is, when the user did not /part), e.g. if the user was kicked.
Attachment #8353577 - Flags: review?(clokep)
Comment on attachment 8353576 [details] [diff] [review]
Patch

*** Original change on bio 1634 attmnt 1817 at 2012-08-21 20:28:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353576 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1634 as attmnt 1819 at 2012-08-21 20:32:00 UTC ***

Get rid of some unneccessary nesting.
Attachment #8353578 - Flags: review?(clokep)
Comment on attachment 8353577 [details] [diff] [review]
Patch

*** Original change on bio 1634 attmnt 1818 at 2012-08-21 20:32:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353577 - Attachment is obsolete: true
Attachment #8353577 - Flags: review?(clokep)
Comment on attachment 8353578 [details] [diff] [review]
Patch

*** Original change on bio 1634 attmnt 1819 at 2012-08-21 21:42:44 UTC ***

This looks good, I think it would be good to have a comment or two in here (about when we expect chatRoomFields to exist and not and what the parameterless version is doing). Carry my r+ forward with the next patch. :)
Attachment #8353578 - Flags: review?(clokep) → review+
Attached patch PatchSplinter Review
*** Original post on bio 1634 as attmnt 1821 at 2012-08-21 21:56:00 UTC ***

Add comment.
Attachment #8353580 - Flags: review+
Comment on attachment 8353578 [details] [diff] [review]
Patch

*** Original change on bio 1634 attmnt 1819 at 2012-08-21 21:56:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353578 - Attachment is obsolete: true
Whiteboard: [checkin-needed]
*** Original post on bio 1634 at 2012-08-22 00:11:08 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/4286f8fd2621 with a slight modification by flo before check-in. (Moving let conv... to inside the if statement.)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.