955528 - Add a /conference command to JS-Yahoo

Status: RESOLVED FIXED

(Chat Core :: Yahoo! Messenger, defect)

Description

[aleth [:aleth]]

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

I just get the help message.

Comment 1

[aleth [:aleth]]

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

Shouldn't it create the chatroom and join it?

Comment 2

[Quentin Headen [:qheaden]]

Attachment: 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.

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 2091 at 2013-08-09 20:20:40 UTC ***

Does libpurple have this command? If not, is it wanted?

Comment 4

[Patrick Cloke [:clokep]]

*** 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?

Comment 5

[aleth [:aleth]]

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.

Comment 6

[Quentin Headen [:qheaden]]

*** 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.

Comment 7

[Patrick Cloke [:clokep]]

*** 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.

Comment 8

[Quentin Headen [:qheaden]]

*** 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?

Comment 9

[aleth [:aleth]]

*** 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.

Comment 10

[Quentin Headen [:qheaden]]

Attachment: 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.

Comment 11

[Quentin Headen [:qheaden]]

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 ***

Comment 12

[Patrick Cloke [:clokep]]

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.

Comment 13

[Quentin Headen [:qheaden]]

Attachment: 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.

Comment 14

[Patrick Cloke [: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.

Comment 15

[Quentin Headen [:qheaden]]

Attachment: Patch 4

Comment 16

[Quentin Headen [:qheaden]]

Attachment: Patch 5

Added localization note to the command help strings.

Comment 17

[Patrick Cloke [: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.

Comment 18

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/6ac3f24133f3

Comment 19

[Florian Quèze [:florian]]

(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?

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 21

[Patrick Cloke [:clokep]]

qheaden, can you please s/command.help.invite/command.help.invite2/ in all necessary places so people know to retranslate this string? Thanks.

Comment 22

[Patrick Cloke [:clokep]]

Attachment: Change entity name

I think qheaden is taking finals for a couple of days.

Comment 23

[Patrick Cloke [:clokep]]

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.

Comment 24

[Quentin Headen [:qheaden]]

(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.