Closed Bug 955555 Opened 10 years ago Closed 10 years ago

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

Categories

(Chat Core :: Yahoo! Messenger, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: qheaden)

Details

Attachments

(1 file, 4 obsolete files)

*** 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...
*** 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.
Attached patch Patch 1 (obsolete) — Splinter Review
*** 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.
Attachment #8354521 - Flags: review?(clokep)
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
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?
Attachment #8354521 - Flags: review?(clokep) → review-
Attached patch Patch 2 (obsolete) — Splinter Review
*** Original post on bio 2117 as attmnt 2759 at 2013-08-23 14:21:00 UTC ***

This addresses the feedback from previous review.
Attachment #8354528 - Flags: review?(clokep)
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 ***
Attachment #8354521 - Attachment is obsolete: true
Attached patch Patch 3 (obsolete) — Splinter Review
*** 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.
Attachment #8354529 - Flags: review?(clokep)
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 ***
Attachment #8354528 - Attachment is obsolete: true
Attachment #8354528 - Flags: review?(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 ***
Attachment #8354529 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
Attached patch Patch 3 (Un-bitrotted) (obsolete) — Splinter Review
*** 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 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 ***
Attachment #8354529 - Attachment is obsolete: true
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.
Attachment #8354540 - Flags: review-
Whiteboard: [checkin-needed]
Attached patch Patch 4Splinter Review
*** 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.
Attachment #8354553 - Flags: review?(florian)
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 ***
Attachment #8354540 - Attachment is obsolete: true
*** 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 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).
Attachment #8354553 - Flags: review?(florian) → review+
*** Original post on bio 2117 at 2013-08-26 23:00:03 UTC ***

http://hg.instantbird.org/instantbird/rev/cb85b50c0af3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.