Closed Bug 954284 Opened 6 years ago Closed 6 years ago

Twitter commands

Categories

(Chat Core :: Twitter, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 851 at 2011-06-22 01:21:00 UTC ***

This might overlap a bit with other bugs (bug 954117 (bio 682) and bug 954119 (bio 684)). But twitter should have some commands built in, most of them can probably be gleaned from the SMS commands [1].

But we should really have a /follow, /unfollow, /rt, @ (at message), /d (direct message) commands at least.

[1] http://support.twitter.com/entries/14020-official-twitter-text-commands
Component: General → Twitter
Duplicate of this bug: 954559
*** Original post on bio 851 at 2012-02-11 00:02:46 UTC ***

/follow and /unfollow already exist, but are not in /help? (This came up in IRC)

Also, /follow strangely enough responds with a system message (good) and displaying a repeat of the last tweet the user sent (bad).
*** Original post on bio 851 at 2012-02-11 00:04:59 UTC ***

Whereas /unfollow does not respond with any kind of message :( So there is no feedback whether the command succeeded.
*** Original post on bio 851 at 2012-02-11 05:25:07 UTC ***

(In reply to comment #2)
> /follow and /unfollow already exist, but are not in /help? (This came up in
> IRC)
No, FOLLOW and UNFOLLOW are handled by the server when the server receives them (as they are for SMS), we should make actually /follow and /unfollow commands, however.
Blocks: 954560
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 851 as attmnt 2257 at 2013-03-09 14:22:00 UTC ***

This adds a "follow" and "unfollow" command.
Attachment #8354022 - Flags: review?(aleth)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8354022 [details] [diff] [review]
Patch

*** Original change on bio 851 attmnt 2257 at 2013-03-09 14:29:56 UTC ***

Thanks for fixing this!

>+command.follow=%S <screen name>: Start following tweets from a user
>+command.unfollow=%S <screen name>: Stop following tweets from a user
Nit: Doesn't twitter use "username" when referring to this to users? (E.g. twitter.com)

>+      run: function(aMsg, aConv) {
>+        if (aMsg.contains(" "))
>+          return false;
For both of these, I think adding something of the sort 
   aMsg = aMsg.trim(); if (!aMsg) return false
first would be a good idea.
Attachment #8354022 - Flags: review?(aleth) → review-
*** Original post on bio 851 at 2013-03-09 14:31:19 UTC ***

I'd say we should support lists of user names for both commands.
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 851 as attmnt 2258 at 2013-03-09 15:04:00 UTC ***

This adds a aMsg.trim() and then splits on spaces and follows/unfollows each one.

I tested it with aleth and Mic's twitter handles (and bwinton's by mistake...)
Attachment #8354023 - Flags: review?(aleth)
Comment on attachment 8354022 [details] [diff] [review]
Patch

*** Original change on bio 851 attmnt 2257 at 2013-03-09 15:04:08 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354022 - Attachment is obsolete: true
Comment on attachment 8354023 [details] [diff] [review]
Patch v2

*** Original change on bio 851 attmnt 2258 at 2013-03-09 15:15:15 UTC ***

Thanks!
Attachment #8354023 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
Comment on attachment 8354023 [details] [diff] [review]
Patch v2

*** Original change on bio 851 attmnt 2258 at 2013-03-11 12:34:44 UTC ***

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

>+command.follow=%S <username>[ <username>]: Start following tweets from a user
>+command.unfollow=%S <username>[ <username>]: Stop following tweets from a user

Shouldn't there be a period at the end of these sentences? We do it for IRC at least: http://lxr.instantbird.org/instantbird/source/chat/locales/en-US/irc.properties#49

Is "Start following tweets from a user" really better than "Start following a user"?

If the number of usernames can be arbitrary, shouldn't we include [ ...] inside the help string? How do we do it in other similar cases?

Should we write "a user / users" in the help string?

>+  commands: [
>+    {
>+      name: "follow",
>+      get helpString() _("command.follow", "follow"),

Is there a reason why we need a formatted string here? Seems to be just for consistency with what we do elsewhere, but do you remember why we do it elsewhere?

>+      run: function(aMsg, aConv) {
>+        aMsg = aMsg.trim();
>+        if (!aMsg)
>+          return false;
>+        let account = getAccount(aConv);
>+        aMsg.split(" ").forEach(function(aName) account.follow(aName));

The function created here seems a waste, would .forEach(account.follow); work?
If not, would .forEach(account.follow, account); work?

>+      name: "unfollow",
>+      get helpString() _("command.unfollow", "unfollow"),
>+      run: function(aMsg, aConv) {
>+        aMsg = aMsg.trim();
>+        if (!aMsg)
>+          return false;
>+        let account = getAccount(aConv);
>+        aMsg.split(" ").forEach(function(aName) account.stopFollowing(aName));

Same here.
Attachment #8354023 - Flags: review-
*** Original post on bio 851 at 2013-03-11 12:42:25 UTC ***

(In reply to comment #10)
> Comment on attachment 8354023 [details] [diff] [review] (bio-attmnt 2258) [details]
> Is "Start following tweets from a user" really better than "Start following a
> user"?
> 
> If the number of usernames can be arbitrary, shouldn't we include [ ...] inside
> the help string? How do we do it in other similar cases?
It looks like we typically put a * after the optional part http://lxr.instantbird.org/instantbird/source/chat/locales/en-US/irc.properties#59


> Should we write "a user / users" in the help string?
Probably.

> >+  commands: [
> >+    {
> >+      name: "follow",
> >+      get helpString() _("command.follow", "follow"),
> 
> Is there a reason why we need a formatted string here? Seems to be just for
> consistency with what we do elsewhere, but do you remember why we do it
> elsewhere?
So translators do not translate the command name.

The other comments I agree with and will provide a new patch for.
Whiteboard: [checkin-needed]
*** Original post on bio 851 at 2013-03-11 12:47:44 UTC ***

(In reply to comment #11)

> > Is there a reason why we need a formatted string here? Seems to be just for
> > consistency with what we do elsewhere, but do you remember why we do it
> > elsewhere?
> So translators do not translate the command name.

Makes sense, thanks for the reminder! I wonder if we should explain that in a code comment somewhere.
Attached patch Patch v3 (obsolete) — Splinter Review
*** Original post on bio 851 as attmnt 2269 at 2013-03-13 01:50:00 UTC ***

(In reply to comment #12)
> (In reply to comment #11)
> 
> > > Is there a reason why we need a formatted string here? Seems to be just for
> > > consistency with what we do elsewhere, but do you remember why we do it
> > > elsewhere?
> > So translators do not translate the command name.
> 
> Makes sense, thanks for the reminder! I wonder if we should explain that in a
> code comment somewhere.

Sure, where would we put that comment?
Attachment #8354034 - Flags: review?(aleth)
Comment on attachment 8354023 [details] [diff] [review]
Patch v2

*** Original change on bio 851 attmnt 2258 at 2013-03-13 01:50:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354023 - Attachment is obsolete: true
*** Original post on bio 851 at 2013-03-13 08:41:46 UTC ***

(In reply to comment #13)

> > Makes sense, thanks for the reminder! I wonder if we should explain that in a
> > code comment somewhere.
> 
> Sure, where would we put that comment?

Maybe just before the |var commands = [| line of ircCommands.jsm, and before |commands: [| for Twitter?
Attached patch Patch v4Splinter Review
*** Original post on bio 851 as attmnt 2274 at 2013-03-13 22:49:00 UTC ***

With updated comments for Twitter and IRC.
Attachment #8354039 - Flags: review?(florian)
Comment on attachment 8354034 [details] [diff] [review]
Patch v3

*** Original change on bio 851 attmnt 2269 at 2013-03-13 22:49:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354034 - Attachment is obsolete: true
Attachment #8354034 - Flags: review?(aleth)
Comment on attachment 8354039 [details] [diff] [review]
Patch v4

*** Original change on bio 851 attmnt 2274 at 2013-03-14 12:08:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354039 - Flags: review?(florian) → review+
Whiteboard: [checkin-needed]
Comment on attachment 8354039 [details] [diff] [review]
Patch v4

*** Original change on bio 851 attmnt 2274 at 2013-03-15 21:44:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354039 - Flags: review+
*** Original post on bio 851 at 2013-03-16 12:47:49 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/f28f9f841f5d

Thanks.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.