Closed
Bug 955528
Opened 11 years ago
Closed 10 years ago
Add a /conference command to JS-Yahoo
Categories
(Chat Core :: Yahoo! Messenger, defect)
Chat Core
Yahoo! Messenger
Tracking
(Not tracked)
RESOLVED
FIXED
1.6
People
(Reporter: aleth, Assigned: qheaden)
References
Details
Attachments
(2 files, 4 obsolete files)
2.46 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
2.25 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 2091 at 2013-08-02 11:50:00 UTC *** I just get the help message.
Reporter | ||
Comment 1•11 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•11 years ago
|
||
*** 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•11 years ago
|
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
Comment 3•11 years ago
|
||
*** 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•11 years ago
|
||
*** 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•11 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•11 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.
Comment 7•11 years ago
|
||
*** 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•11 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 | ||
Comment 9•11 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•11 years ago
|
||
*** 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•11 years ago
|
Attachment #8354913 -
Flags: review?(clokep)
Assignee | ||
Comment 11•11 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 12•11 years ago
|
||
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•10 years ago
|
||
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 14•10 years ago
|
||
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•10 years ago
|
||
Attachment #8418163 -
Attachment is obsolete: true
Attachment #8418199 -
Flags: review?(clokep)
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
Comment 18•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/6ac3f24133f3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Comment 19•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
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 → ---
Comment 22•10 years ago
|
||
I think qheaden is taking finals for a couple of days.
Attachment #8418733 -
Flags: review?(florian)
Updated•10 years ago
|
Attachment #8418733 -
Flags: review?(florian) → review+
Comment 23•10 years ago
|
||
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
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Summary: /join chatroom does not do anything. → Add a /conference command to JS-Yahoo
Assignee | ||
Comment 24•10 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.
Description
•