Closed Bug 954903 Opened 10 years ago Closed 10 years ago

Reply to tweet only auto-fills the first (of possibly multiple) nicks

Categories

(Chat Core :: Twitter, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 1470 at 2012-05-29 19:34:00 UTC ***

STR
- Receive a tweet addressed to a number of people: "@a @b @c message"
- 'Reply to tweet' (context menu)
- Only "@a:" is added to the textbox.
*** Original post on bio 1470 at 2012-05-29 19:36:11 UTC ***

So, "reply to tweet" is used to reply to the author. It sounds like you want to add a "reply to all".
*** Original post on bio 1470 at 2012-05-29 19:43:29 UTC ***

Yes, that would be a good solution.
*** Original post on bio 1470 at 2012-05-29 20:59:06 UTC ***

I remember someone (either ecaron, or someone commenting on twitter) requesting at the time we introduced the reply feature that it adds all the @username at the beginning and all the #hashtags at the end of the input box.
Given the way this request was phrased, it seemed like it was the usual default behavior (either of the client the requester used before, or of twitter.com itself). If it's the default behavior of twitter.com, then it should also be our default behavior; if not, maybe it's something for an add-on? (the longer our context menu becomes, the harder it is to find anything in it)
*** Original post on bio 1470 at 2012-05-29 22:12:46 UTC ***

(In reply to comment #3)
> I remember someone (either ecaron, or someone commenting on twitter) requesting
> at the time we introduced the reply feature that it adds all the @username at
> the beginning and all the #hashtags at the end of the input box.
> Given the way this request was phrased, it seemed like it was the usual default
> behavior (either of the client the requester used before, or of twitter.com
> itself). If it's the default behavior of twitter.com, then it should also be
> our default behavior; if not, maybe it's something for an add-on? (the longer
> our context menu becomes, the harder it is to find anything in it)

Listing all the leading nicks is indeed the default twitter.com reply behaviour, which is why I phrased the bug report as I did. Hashtags are not included by default though.

Before implementing this, one should check however what twitter.com does when there is a RT involved (a possible subtlety).
*** Original post on bio 1470 at 2012-10-18 18:49:53 UTC ***

- It's not just the leading nicks, it's all the @mentioned nicks.
- Replying to a RT includes the leading nicks in the retweeted tweet.
- One must be careful to exclude duplicates and the user's nick.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1470 as attmnt 1985 at 2012-10-18 20:03:00 UTC ***

Follows the behaviour of twitter.com, with the small improvement that duplicates and the user's own nick are removed automatically from the list of nicks replied to.
Attachment #8353744 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8353744 [details] [diff] [review]
Patch

*** Original change on bio 1470 attmnt 1985 at 2012-10-18 20:15:09 UTC ***

>diff --git a/components/twitter.js b/components/twitter.js
>@@ -119,18 +119,30 @@ Conversation.prototype = {
>   inReplyToStatusId: null,
>   startReply: function(aTweet) {
>     this.inReplyToStatusId = aTweet.id_str;
>-    this.notifyObservers(null, "replying-to-prompt",
>-                         "@" + aTweet.user.screen_name + " ");
>+    let entities = aTweet.entities;
>+    // Twitter replies go to all the users mentioned in the tweet.
>+    let nicks = ["@" + aTweet.user.screen_name];
>+    if (Object.keys(entities).length && "user_mentions" in entities &&
Why the Object.keys(entities).length check? The next check kind of implies this...if you took this from the other Twitter code I think that was there as a shortcut if there are no entities. We don't need that here when checking for only one condition.

>+      nicks = nicks.concat(entities.user_mentions
>+                                   .map(function(um) "@" + um.screen_name));
I'd prefer we don't have this map here and we do it all at the end, this way we don't have to add a "@" in front a lot of times.

>+    }
>+    // Remove duplicates and the user's nick.
>+    nicks = nicks.filter(function(nick, pos) {
>+      return nicks.indexOf(nick) == pos && nick != "@" + this._account.name;
aNick and aPos, if it fits. Does this still work well after the array is mutated by removing a duplicated nick?

>+    }, this);
Put the map call here!
Attachment #8353744 - Flags: review?(clokep) → review-
*** Original post on bio 1470 at 2012-10-18 20:37:04 UTC ***

Bonus points for hashtags too.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1470 as attmnt 1986 at 2012-10-18 20:39:00 UTC ***

The (first) map is needed because user_mentions is an array of objects, not of names.

>Does this still work well after the array is mutated by removing a
>duplicated nick?
I don't understand this question?
Attachment #8353745 - Flags: review?(clokep)
Comment on attachment 8353744 [details] [diff] [review]
Patch

*** Original change on bio 1470 attmnt 1985 at 2012-10-18 20:39:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353744 - Attachment is obsolete: true
Comment on attachment 8353745 [details] [diff] [review]
Patch

*** Original change on bio 1470 attmnt 1986 at 2012-10-18 20:58:08 UTC ***

Thanks for working on this. :)
Attachment #8353745 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1470 as attmnt 1987 at 2012-10-18 21:50:00 UTC ***

Avoid "nicks = nicks.filter(..." to reduce confusion.
Attachment #8353746 - Flags: review?(florian)
Comment on attachment 8353745 [details] [diff] [review]
Patch

*** Original change on bio 1470 attmnt 1986 at 2012-10-18 21:50:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353745 - Attachment is obsolete: true
Comment on attachment 8353746 [details] [diff] [review]
Patch

*** Original change on bio 1470 attmnt 1987 at 2012-10-18 22:50:04 UTC ***

>diff --git a/components/twitter.js b/components/twitter.js

>+    // Ignore duplicates and the user's nick.
>+    let prompt =
>+      nicks.filter(function(aNick, aPos) {
>+             return nicks.indexOf(aNick) == aPos && aNick != this._account.name;
>+           }, this)
>+           .map(function(aNick) "@" + aNick)
>+           .join(" ") + " ";
>+    
Nit: trailing whitespace!

You said on IRC "r? purely for the indentation" so I looked only at the indentation and I think it's what I would have done (even though it's ugly).

If you are really worried by the ugliness, the alternative is to do:
let filter = function(aNick, aPos) {
  return nicks.indexOf(aNick) == aPos && aNick != this._account.name;
};
And then:
let prompt = nicks.filter(filter, this)
                  .map(function(aNick) "@" + aNick)
                  .join(" ") + " ";
Attachment #8353746 - Flags: review?(florian) → review+
Attached patch PatchSplinter Review
*** Original post on bio 1470 as attmnt 1991 at 2012-10-19 12:01:00 UTC ***

Nit fix

Imho pulling out the function is prettier but at the expense of legibility, so I left it as is...
Attachment #8353750 - Flags: review+
Comment on attachment 8353746 [details] [diff] [review]
Patch

*** Original change on bio 1470 attmnt 1987 at 2012-10-19 12:01:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353746 - Attachment is obsolete: true
*** Original post on bio 1470 at 2012-10-26 10:20:35 UTC ***

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

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