Add a /conference command to JS-Yahoo

RESOLVED FIXED in 1.6

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aleth, Assigned: qheaden)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
*** Original post on bio 2091 at 2013-08-02 11:50:00 UTC ***

I just get the help message.
(Reporter)

Comment 1

5 years ago
*** Original post on bio 2091 at 2013-08-02 11:50:44 UTC ***

Shouldn't it create the chatroom and join it?
(Assignee)

Comment 2

5 years ago
Created attachment 8354459 [details] [diff] [review]
Patch 1

*** Original post on bio 2091 as attmnt 2690 at 2013-08-09 13:57:00 UTC ***

This patch adds the /join command to conversations and conferences, but it is used only as a shortcut for creating a conference and joining it. On the Yahoo! protocol, you cannot join other people's conferences, but must always be invited to one.
Attachment #8354459 - Flags: review?(clokep)
(Assignee)

Updated

5 years ago
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
*** Original post on bio 2091 at 2013-08-09 20:20:40 UTC ***

Does libpurple have this command? If not, is it wanted?
*** Original post on bio 2091 at 2013-08-12 12:37:47 UTC ***

libpurple seems to have a join command for joining their public chat rooms: http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/yahoo/libyahoo.c#38

Do we really want this or is aleth running into some weird issue between the libpurple vs. js protocols?
(Reporter)

Comment 5

5 years ago
Comment on attachment 8354459 [details] [diff] [review]
Patch 1

*** Original change on bio 2091 attmnt 2690 at 2013-08-16 11:15:25 UTC ***

I would suggest improving the help string to explain why the yahoo join command does not take a chatroom name as a parameter, and to check that the user has not added any parameters (show the help string if she does).

Basically I ran into the issue of trying to use yahoo MUCs without googling what a "conference" was (and how it gets named/created) first. As a multiprotocol client, we should try to smooth the way for users unfamiliar with yahoo-specific terminology and quirks.
Attachment #8354459 - Flags: review?(clokep) → review-
(Assignee)

Comment 6

5 years ago
*** Original post on bio 2091 at 2013-08-28 19:44:02 UTC ***

I agree with aleth's suggestion. We should include the join command and explain that it is just for opening a conference chat room that people can be invited to. If libpurple has the /join command, it is best that we add it to JS-Yahoo as well.
*** Original post on bio 2091 at 2013-08-29 10:42:31 UTC ***

I think this is named wrong. You're not joining anything, you're creating a chat. I also find it to be unnecessary.
(Assignee)

Comment 8

5 years ago
*** Original post on bio 2091 at 2013-08-29 14:27:19 UTC ***

(In reply to comment #7)
> I think this is named wrong. You're not joining anything, you're creating a
> chat. I also find it to be unnecessary.

Okay. Would you recommend maybe adding a /conference command, or just drop the whole idea together?
(Reporter)

Updated

5 years ago
Blocks: 955574
(Reporter)

Comment 9

5 years ago
*** Original post on bio 2091 at 2013-09-23 19:21:41 UTC ***

The real bug here is that the join command is listed despite only existing for libpurple yahoo.

If you'd like to add a conference command, why not? I don't think many people will use it, though that's not a reason not to have it if you think it's a good idea.
(Assignee)

Comment 10

5 years ago
Created attachment 8354913 [details] [diff] [review]
Patch 2

*** Original post on bio 2091 as attmnt 3127 at 2013-12-18 17:41:00 UTC ***

This patch adds the /conference command instead of the /join command. I agree that /join is very misleading since Yahoo's protocol does not allow one to join a conference at will, but rather, must be invited.

The /conference command simply creates a new conference that you can invite others to join.
Attachment #8354913 - Flags: review?(aleth)
(Assignee)

Updated

5 years ago
Attachment #8354913 - Flags: review?(clokep)
(Assignee)

Comment 11

5 years ago
Comment on attachment 8354459 [details] [diff] [review]
Patch 1

*** Original change on bio 2091 attmnt 2690 at 2013-12-18 17:41:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354459 - Attachment is obsolete: true
Comment on attachment 8354913 [details] [diff] [review]
Patch 2

*** Original change on bio 2091 attmnt 3127 at 2013-12-18 18:24:36 UTC ***

>JS-Yahoo: /join chatroom does not do anything
Please use a better commit message.

>diff -r e119e96ba074 -r 54ab9bde5677 chat/protocols/yahoo/yahoo.js
>+    {
>+      name: "conference",
>+      get helpString() _("command.help.conference"),
>+      usageContext: Ci.imICommand.CMD_CONTEXT_CHAT,
Why context chat? Can't this be used everywhere?

>+      run: function(aMsg, aConv) {
>+        let account = aConv.wrappedJSObject._account;
>+        account.joinChat(null);
You only use account once, don't create a variable for it. :) (Is joinChat part of the interface? You might not need the wrappedJSObject.)


This still won't fix the issue of libpurple commands appearing when not using the libpurple stuff.
Attachment #8354913 - Flags: review?(clokep)
Attachment #8354913 - Flags: review?(aleth)
Attachment #8354913 - Flags: review-
(Assignee)

Comment 13

5 years ago
Created attachment 8418163 [details] [diff] [review]
Patch 3

The /join command no longer shows up in the list of available commands for JS-Yahoo. Apparently this has been fixed somewhere in another patch. But this patch adds the /conference command for creating conferences.
Attachment #8354913 - Attachment is obsolete: true
Attachment #8418163 - Flags: review?(clokep)
Comment on attachment 8418163 [details] [diff] [review]
Patch 3

Review of attachment 8418163 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with two minor changes to the way help strings are done.

::: chat/locales/en-US/yahoo.properties
@@ +26,4 @@
>  system.message.conferenceLogon=%S has joined the conference.
>  
>  command.help.invite=/invite <user1>[,<user2>,...] [<invite message>]: invite one or more users into this conference chat.
> +command.help.conference=/conference: Create a new conference room and join that room. To invite people to the conference, use the /invite command.

Can you set both of these up so that invite and conference is not translatedable (also they shouldn't have a / in the front of them), see https://mxr.mozilla.org/comm-central/source/chat/protocols/irc/ircCommands.jsm#436

::: chat/protocols/yahoo/yahoo.js
@@ +544,3 @@
>      {
>        name: "invite",
>        get helpString() _("command.help.invite"),

You'll need to add a |, "invite"| here.

@@ +575,5 @@
> +    },
> +
> +    {
> +      name: "conference",
> +      get helpString() _("command.help.conference"),

And a |, "conference"| here.
Attachment #8418163 - Flags: review?(clokep) → review+
(Assignee)

Comment 15

5 years ago
Created attachment 8418199 [details] [diff] [review]
Patch 4
Attachment #8418163 - Attachment is obsolete: true
Attachment #8418199 - Flags: review?(clokep)
(Assignee)

Comment 16

5 years ago
Created attachment 8418203 [details] [diff] [review]
Patch 5

Added localization note to the command help strings.
Attachment #8418199 - Attachment is obsolete: true
Attachment #8418199 - Flags: review?(clokep)
Attachment #8418203 - Flags: review?(clokep)
Comment on attachment 8418203 [details] [diff] [review]
Patch 5

Review of attachment 8418203 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Sorry for the back and forth here.
Attachment #8418203 - Flags: review?(clokep) → review+
https://hg.mozilla.org/comm-central/rev/6ac3f24133f3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
(In reply to Quentin Headen from comment #13)

> The /join command no longer shows up in the list of available commands for
> JS-Yahoo. Apparently this has been fixed somewhere in another patch.

Could it just be that you haven't added the line to build purplexpcom in your local mozconfig?
The string id didn't change
https://hg.mozilla.org/comm-central/rev/6ac3f24133f3#l1.12

From IRC:
Archaeopteryx: https://hg.mozilla.org/comm-central/rev/6ac3f24133f3 changed the string value without changing the label. i just want to confirm this won't cause any issues with the old style of the string in localizations (which lack the %S)
clokep_work: Archaeopteryx: It'll cause huge issues. I doubt those strings are translated anywhere.
clokep_work: Bleh I found at least one locale that is affected.
clokep_work: Can you comment on the bug please and we'll have the entity name bumped.
qheaden, can you please s/command.help.invite/command.help.invite2/ in all necessary places so people know to retranslate this string? Thanks.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8418733 [details] [diff] [review]
Change entity name

I think qheaden is taking finals for a couple of days.
Attachment #8418733 - Flags: review?(florian)
Attachment #8418733 - Flags: review?(florian) → review+
https://hg.mozilla.org/comm-central/rev/4e25b8ca7e53

qheaden: Can you please answer comment 19 and if the libpurple commands interfere with the JS ones, please file a new bug! Thanks.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Summary: /join chatroom does not do anything. → Add a /conference command to JS-Yahoo
(Assignee)

Comment 24

5 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #19)
> (In reply to Quentin Headen from comment #13)
> 
> > The /join command no longer shows up in the list of available commands for
> > JS-Yahoo. Apparently this has been fixed somewhere in another patch.
> 
> Could it just be that you haven't added the line to build purplexpcom in
> your local mozconfig?

Okay, I rebuilt with purplexpcom enabled, and I now see the /join command again. I have created a new bug report for this in bug 1007239.
You need to log in before you can comment on or make changes to this bug.