955555 - /invite should print a system message to show it's done something

Status: RESOLVED FIXED

(Chat Core :: Yahoo! Messenger, defect)

Description

[aleth [:aleth]]

*** Original post on bio 2117 at 2013-08-21 15:24:00 UTC ***

Otherwise there is no user feedback whatsoever.

This might also apply to the IRC command...

Comment 1

[Quentin Headen [:qheaden]]

*** Original post on bio 2117 at 2013-08-21 18:08:43 UTC ***

I also think an error message should be shown if a user invites someone that is in their buddy list, but is offline. Of course, those who aren't in the buddy list can also be invited.

Comment 2

[Quentin Headen [:qheaden]]

Attachment: Patch 1

*** Original post on bio 2117 as attmnt 2752 at 2013-08-23 04:04:00 UTC ***

This patch adds code to write a simple system message whenever a conference invite is sent out.

Comment 3

[Patrick Cloke [:clokep]]

Comment on attachment 8354521 [details] [diff] [review]
Patch 1

*** Original change on bio 2117 attmnt 2752 at 2013-08-23 12:57:55 UTC ***

>diff --git a/chat/locales/en-US/yahoo.properties b/chat/locales/en-US/yahoo.properties
>+# LOCALIZATION NOTE (command.feedback.invite-single):
>+#   %S is the user, or list of users, invited to the conference.
>+command.feedback.invite=Conference invite sent to %S
The comment doesn't match the name given.

>diff --git a/chat/protocols/yahoo/yahoo.js b/chat/protocols/yahoo/yahoo.js
>@@ -509,16 +509,19 @@ YahooProtocol.prototype = {
>         conf._account._session.inviteToConference(invitees, conf._roomName,
>                                                   conf.getParticipantNames(),
>                                                   message);
>+        conf.writeMessage(conf._roomName,
>+                          _("command.feedback.invite", invitees.join(',')),
>+                          {system: true});
Should we be logging this or no?

Comment 4

[Quentin Headen [:qheaden]]

Attachment: Patch 2

*** Original post on bio 2117 as attmnt 2759 at 2013-08-23 14:21:00 UTC ***

This addresses the feedback from previous review.

Comment 5

[Quentin Headen [:qheaden]]

Comment on attachment 8354521 [details] [diff] [review]
Patch 1

*** Original change on bio 2117 attmnt 2752 at 2013-08-23 14:21:20 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Quentin Headen [:qheaden]]

Attachment: Patch 3

*** Original post on bio 2117 as attmnt 2760 at 2013-08-23 14:43:00 UTC ***

This third patch only adds noLog to the conference invite system message. In this way, invite feedback isn't logged in the conversation history.

Comment 7

[Quentin Headen [:qheaden]]

Comment on attachment 8354528 [details] [diff] [review]
Patch 2

*** Original change on bio 2117 attmnt 2759 at 2013-08-23 14:43:26 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8354529 [details] [diff] [review]
Patch 3

*** Original change on bio 2117 attmnt 2760 at 2013-08-23 14:54:06 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[Quentin Headen [:qheaden]]

Attachment: Patch 3 (Un-bitrotted)

*** Original post on bio 2117 as attmnt 2771 at 2013-08-23 22:32:00 UTC ***

The previous patch couldn't be applied cleanly to the current tree. This patch can be applied successfully.

Comment 10

[Quentin Headen [:qheaden]]

Comment on attachment 8354529 [details] [diff] [review]
Patch 3

*** Original change on bio 2117 attmnt 2760 at 2013-08-23 22:32:57 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8354540 [details] [diff] [review]
Patch 3 (Un-bitrotted)

*** Original change on bio 2117 attmnt 2771 at 2013-08-23 23:34:34 UTC ***

>diff --git a/chat/locales/en-US/yahoo.properties b/chat/locales/en-US/yahoo.properties

> options.ignoreInvites=Ignore conference and chatroom invitations

What's the difference between a conference and a chatroom? I thought one of the two was now dead and irrelevant.

> #options.proxySSL=Use account proxy for HTTP and HTTPS connections

Remove?

> command.help.invite=/invite <user1>[,<user2>,...] [<invite message>]: invite a user into a conference chat.

This message needs to be fixed. It's not 'a' user (there can be more than one) and it's not 'a conference chat' (it's the current one).

>+# LOCALIZATION NOTE (command.feedback.invite):
>+#   %S is the user, or list of users, invited to the conference.
>+command.feedback.invite=Conference invite sent to %S

We disliked this string on IRC. I dislike it. I would probably prefer 'invitation' instead of 'invite' (it would be consistent with the options.ignoreInvites above). We also suggested a few other wordings. Some starting with 'You have invited' seemed slightly better.

Comment 12

[Quentin Headen [:qheaden]]

Attachment: Patch 4

*** Original post on bio 2117 as attmnt 2784 at 2013-08-26 05:49:00 UTC ***

This patch addresses the feedback given about certain strings needing to change.

> > #options.proxySSL=Use account proxy for HTTP and HTTPS connections
> Remove?

I decided not to remove this string. The option it describes is also commented out in the source of yahoo.js, and I'm not sure if this is some sort of feature would could use in the future. So I'm just going to leave it for now.

Comment 13

[Quentin Headen [:qheaden]]

Comment on attachment 8354540 [details] [diff] [review]
Patch 3 (Un-bitrotted)

*** Original change on bio 2117 attmnt 2771 at 2013-08-26 05:49:42 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 14

[Florian Quèze [:florian]]

*** Original post on bio 2117 at 2013-08-26 10:11:32 UTC ***

(In reply to comment #8)

> > > #options.proxySSL=Use account proxy for HTTP and HTTPS connections
> > Remove?
> 
> I decided not to remove this string. The option it describes is also commented
> out in the source of yahoo.js, and I'm not sure if this is some sort of feature
> would could use in the future. So I'm just going to leave it for now.

There's no 'account proxy' for JS prpls. Having a different proxy for each account is a misfeature of libpurple. For our JS protocols, we use instead the Mozilla proxy dialog, that lets the user define proxies per protocol (ie HTTP vs HTTPS vs Socks).

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8354553 [details] [diff] [review]
Patch 4

*** Original change on bio 2117 attmnt 2784 at 2013-08-26 18:21:44 UTC ***

>diff --git a/chat/locales/en-US/yahoo.properties b/chat/locales/en-US/yahoo.properties

>-options.ignoreInvites=Ignore conference and chatroom invitations
>+options.ignoreInvites=Ignore conference invitations

Usually when we change an existing string, we also change its id to force translators to retranslate it. Given that JS-Yahoo has never been released and is still preffed off, I'm inclined to not do it here, but we should ensure that the (2) translators who have already translated this file will notice the change (I suspect they watch the hg repository and will notice quickly; but if they don't we should email them).

> #options.proxySSL=Use account proxy for HTTP and HTTPS connections

As you said in comment 8, we should also remove the option in the JS code, so maybe this is better for another bug.

>-command.help.invite=/invite <user1>[,<user2>,...] [<invite message>]: invite a user into a conference chat.
>+command.help.invite=/invite <user1>[,<user2>,...] [<invite message>]: invite one or more users into this conference chat.
>+# LOCALIZATION NOTE (command.feedback.invite):
>+#   %S is the user, or list of users, invited to the conference.

I think this should be even more specific and say "comma separated list" instead of "list".

r=me with this changed (but I can do it myself when doing the checkin if you prefer).

Comment 16

[Florian Quèze [:florian]]

*** Original post on bio 2117 at 2013-08-26 23:00:03 UTC ***

http://hg.instantbird.org/instantbird/rev/cb85b50c0af3