Closed Bug 955575 Opened 10 years ago Closed 10 years ago

Yahoo! Mobile status not taken into account

Categories

(Chat Core :: Yahoo! Messenger, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: qheaden)

References

Details

Attachments

(1 file, 5 obsolete files)

*** Original post on bio 2136 at 2013-08-28 12:21:00 UTC ***

See [1] and [2] for how libpurple does this. I can test this easily.

[1] http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/yahoo/libymsg.c#73
[2] http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/yahoo/libymsg.c#148
Blocks: 955574
Attached patch Patch 1 (obsolete) — Splinter Review
*** Original post on bio 2136 as attmnt 2863 at 2013-09-11 06:10:00 UTC ***

Because of the structure of the buddy list in the initial status packet, I needed to switch to iteration. Since the mobile status isn't applied to all buddies, we must iterate over the keys to ensure that the correct buddy is given the mobile status since getKeys() does not pay attention to delimited sections within the key/value array; that requires higher-level interpretation.
Attachment #8354633 - Flags: review?(clokep)
Comment on attachment 8354633 [details] [diff] [review]
Patch 1

*** Original change on bio 2136 attmnt 2863 at 2013-09-11 18:24:04 UTC ***

>@@ -971,27 +977,52 @@ const YahooPacketHandler = {
>   // Initial user status.
>   0xf0: function (aPacket) {
>     // Return early if we find no buddy names.
>     if (!aPacket.hasKey(7))
>       return;
>-    let buddyNames = aPacket.getValues(7);
>-    let buddyStatuses = aPacket.getValues(10);
>-    let statusMessages;
>-    if (aPacket.hasKey(19))
>-      statusMessages = aPacket.getValues(19);
>+    let buddyNames = [];
>+    let buddyStatuses = [];
>+    let statusMessages = [];
>+
>+    // The key/value pairs are in order as sent by the server. So we must
>+    // iterate though them to find out information about each buddy. Each
>+    // buddy section is separated by the starting key 7.
I think the following is clearer "Each buddy section starts with key 7.", it's not really separated by it.

>+    let currentBuddy;
Can we call this currentBuddyName to match above?

>+    let currentStatus;
>+    let currentStatusMessage;
>+    for (let i = 0; i < aPacket.keyValuePairs.length; i++) {
Nit: ++i.

>+      let pair = aPacket.keyValuePairs[i];
>+
>+      // Buddy name or end of list.
>+      if (pair.key == 7 || i == aPacket.keyValuePairs.length - 1) {
>+        // Store any buddies that have been completely defined.
Maybe "Store the previous buddy and reset variables." or something like that? If we do this when we're at the last element...doesn't that mean that the last buddy will only have partial information? (As the rest of the if-statement won't run and we haven't fully set all these elements yet.)

>+        if (currentBuddy) {
>+          buddyNames[i] = currentBuddy;
>+          buddyStatuses[i] = currentStatus;
>+          statusMessages[i] = currentStatusMessage;
>+          currentBuddy = undefined;
>+          currentStatus = undefined;
>+          currentStatusMessage = undefined;
>+        }
>+        currentBuddy = pair.value;
>+      } else if (pair.key == 10 && currentBuddy) { // Buddy status.
>+        currentStatus = pair.value;
Do the kBuddyStatuses[pair.value] here.

>+      } else if (pair.key == 19 && currentBuddy) { // Buddy status message.
>+        currentStatusMessage = pair.value;
>+      } else if (pair.key == 60 && currentBuddy) { // Mobile status indicator.
>+        currentStatus = "mobile";
Directly set this to the mobile status and don't make up a "fake" constant "mobile" here.

>+      }
Nit: No { } around single line statements.

>+    }
> 
>     for (let i in buddyNames) {
>       let name = buddyNames[i];
>       let status = kBuddyStatuses[buddyStatuses[i]];
Remove this line.

>       let message = statusMessages ? statusMessages[i] : "";
> 
>       this.setBuddyStatus(name, status, message);
Attachment #8354633 - Flags: review?(clokep) → review-
Attached patch Patch 2 (obsolete) — Splinter Review
*** Original post on bio 2136 as attmnt 2874 at 2013-09-12 07:30:00 UTC ***

Addressing feedback.
Attachment #8354644 - Flags: review?(clokep)
Comment on attachment 8354633 [details] [diff] [review]
Patch 1

*** Original change on bio 2136 attmnt 2863 at 2013-09-12 07:30:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354633 - Attachment is obsolete: true
Comment on attachment 8354644 [details] [diff] [review]
Patch 2

*** Original change on bio 2136 attmnt 2874 at 2013-09-12 12:05:54 UTC ***

Looks good! I'd like aleth to look over this too.
Attachment #8354644 - Flags: review?(clokep) → review+
Attachment #8354644 - Flags: review?(aleth)
Comment on attachment 8354644 [details] [diff] [review]
Patch 2

*** Original change on bio 2136 attmnt 2874 at 2013-09-12 18:36:31 UTC ***

Patrick asked me to have a look at this patch and here's my two cents.

>    // Buddy status update.
>    0xc6: function (aPacket) {
>      let name = aPacket.getValue(7);
> -    // Try to grab a status from the well-used buddy statuses. If we can't
> -    // find it there, try the extra ones.
> -    let status = kBuddyStatuses[aPacket.getValue(10)];
> +    // If the user is mobile, use the mobile status.
> +    let status;
> +    if (aPacket.hasKey(60))
> +      status = Ci.imIStatusInfo.STATUS_MOBILE;
> +    else
> +      status = kBuddyStatuses[aPacket.getValue(10)];
> +

You can shorten that using the ?-if-operator if you like:
     let status = aPacket.hasKey(60) ? Ci.imIStatusInfo.STATUS_MOBILE :
                                       kBuddyStatuses[aPacket.getValue(10)];

> +    // The key/value pairs are in order as sent by the server. So we must
> +    // iterate though them to find out information about each buddy. Each
> +    // buddy section starts with key 7.
> +    for (let i = 0; i < aPacket.keyValuePairs.length; ++i) {
> +      let pair = aPacket.keyValuePairs[i];

Defining temporary variables for |key| and |value| could save a few characters in every line of your if-blocks:
      let {key: key, value: value} = aPacket.keyValuePairs[i];

> +      if (pair.key == 7) { // Buddy name.
> +        // Store the old buddy before working with the new one.

 Maybe call it "previous" and "next" instead of "old" and "new" buddy?

> +        if (currentBuddyName)
> +          storeBuddyInfo();
> +        currentBuddyName = pair.value;
> +      } else if (pair.key == 10 && currentBuddyName) // Buddy status.
> +        currentStatus = kBuddyStatuses[pair.value];
> +      else if (pair.key == 19 && currentBuddyName) // Buddy status message.
> +        currentStatusMessage = pair.value;
> +      else if (pair.key == 60 && currentBuddyName) // Mobile status.
> +        currentStatus = Ci.imIStatus.STATUS_MOBILE;
> +    }

Maybe it would be more readable if your moved the recurring | && currentBuddyName| conditions into a separate |if| surrounding these cases:

if (currentBuddyName) {
  if (key == 10)
    ..
  else if (key == 19) 
    ..
}

> +    // Store the final buddy.
> +    if (currentBuddyName)
> +      storeBuddyInfo();

There is always at least one buddy when reaching this point (see above: |if (!aPacket.hasKey(7)) return;|) which means that this will always be true.

>      for (let i in buddyNames) {
>        let name = buddyNames[i];
> -      let status = kBuddyStatuses[buddyStatuses[i]];
> -      let message = statusMessages ? statusMessages[i] : "";
> +      let status = buddyStatuses[i];
> +      let message = statusMessages[i] || "";
>  
>        this.setBuddyStatus(name, status, message);
>        this._session.requestBuddyIcon(name);

What about using |buddyStatuses[i]| and |statusMessages[i] || ""| as parameters directly? Line length isn't a problem here. |name| is used twice, so it's OK to have a temporary variable for it.
Attachment #8354644 - Flags: feedback-
Comment on attachment 8354644 [details] [diff] [review]
Patch 2

*** Original change on bio 2136 attmnt 2874 at 2013-09-13 11:35:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354644 - Flags: review?(aleth)
Attached patch Patch 3 (obsolete) — Splinter Review
*** Original post on bio 2136 as attmnt 2886 at 2013-09-14 03:13:00 UTC ***

Since Mic gave me the last feedback, I'll ask for a review from each of you on this patch.
Attachment #8354656 - Flags: review?(clokep)
Attachment #8354656 - Flags: review?(benediktp)
Comment on attachment 8354644 [details] [diff] [review]
Patch 2

*** Original change on bio 2136 attmnt 2874 at 2013-09-14 03:13:56 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354644 - Attachment is obsolete: true
Comment on attachment 8354656 [details] [diff] [review]
Patch 3

*** Original change on bio 2136 attmnt 2886 at 2013-09-14 08:43:30 UTC ***

I'm not familiar enough with Yahoo! code to judge if the packet keys and values here make sense but I reviewed the rest of the code and style. With the remaining comments addressed it is r+ from my point of view.

>+        if (currentBuddyName)
>+          storeBuddyInfo();
>+        currentBuddyName = pair.value;

Here's still a pair.value that wants to be replaced.

>     for (let i in buddyNames) {
>       let name = buddyNames[i];
>-      let status = kBuddyStatuses[buddyStatuses[i]];
>-      let message = statusMessages ? statusMessages[i] : "";
>+      let status = buddyStatuses[i];

This variable is unnecessary now.

>-      this.setBuddyStatus(name, status, message);
>+      this.setBuddyStatus(name, buddyStatuses[i],) statusMessages[i] || "");

Syntax error ;) There's a bracket where it shouldn't be.
Attachment #8354656 - Flags: review?(benediktp) → review-
*** Original post on bio 2136 at 2013-09-14 08:47:02 UTC ***

>+    let message = aPacket.hasKey(19) ? aPacket.getValue(19) : "";

Not related to this patch: if this pattern is occurring often enough then it might make sense to have a method like |getValueOrDefault(19, "")| on packet objects?
*** Original post on bio 2136 at 2013-09-14 13:48:42 UTC ***

(In reply to comment #8)
> >+    let message = aPacket.hasKey(19) ? aPacket.getValue(19) : "";
> 
> Not related to this patch: if this pattern is occurring often enough then it
> might make sense to have a method like |getValueOrDefault(19, "")| on packet
> objects?

Or just a second optional parameter for the default value on the getValue method?
Comment on attachment 8354656 [details] [diff] [review]
Patch 3

*** Original change on bio 2136 attmnt 2886 at 2013-09-16 17:20:08 UTC ***

Something occurred to me while re-reading this patch...there's no reason to store the information if we're not doing anything over all of it at once. Instead of storing it you should be able to just directly the set the status of each buddy in the single loop.
Attachment #8354656 - Flags: review?(clokep) → review-
Attached patch Patch 4 (obsolete) — Splinter Review
*** Original post on bio 2136 as attmnt 2895 at 2013-09-17 01:26:00 UTC ***

This patch contains revisions based on comment 10. As a result, the loop for reading the buddy status information has been simplified.

> >+    let message = aPacket.hasKey(19) ? aPacket.getValue(19) : "";

> Not related to this patch: if this pattern is occurring often enough then it
> might make sense to have a method like |getValueOrDefault(19, "")| on packet
> objects?

In my opinion, this doesn't happen within enough packets to justify modifying the getValue method.
Attachment #8354665 - Flags: review?(clokep)
Comment on attachment 8354656 [details] [diff] [review]
Patch 3

*** Original change on bio 2136 attmnt 2886 at 2013-09-17 01:26:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354656 - Attachment is obsolete: true
Comment on attachment 8354665 [details] [diff] [review]
Patch 4

*** Original change on bio 2136 attmnt 2895 at 2013-09-18 14:47:10 UTC ***

Mic and I gave some feedback on IRC around the setBuddyStatus method.
Attachment #8354665 - Flags: review?(clokep) → review-
Attached patch Patch 5 (obsolete) — Splinter Review
*** Original post on bio 2136 as attmnt 2896 at 2013-09-19 02:19:00 UTC ***

This patch fixes setBuddyStatus so that empty and non-empty status messages can be specified as the last parameter, but if set to undefined (or simply left out of the function call), it allows you to change the status without changing the message.

The main reason this functionality was added was to make it easier to write the iteration loop for the 0xf0 packet handler.
Attachment #8354666 - Flags: review?(clokep)
Comment on attachment 8354665 [details] [diff] [review]
Patch 4

*** Original change on bio 2136 attmnt 2895 at 2013-09-19 02:19:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354665 - Attachment is obsolete: true
Comment on attachment 8354666 [details] [diff] [review]
Patch 5

*** Original change on bio 2136 attmnt 2896 at 2013-09-19 12:14:25 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354666 - Flags: review?(clokep) → review+
Attachment #8354666 - Flags: review?(benediktp)
Comment on attachment 8354666 [details] [diff] [review]
Patch 5

*** Original change on bio 2136 attmnt 2896 at 2013-09-19 12:58:38 UTC ***

> diff --git a/chat/protocols/yahoo/yahoo.js b/chat/protocols/yahoo/yahoo.js
> --- a/chat/protocols/yahoo/yahoo.js
> +++ b/chat/protocols/yahoo/yahoo.js
> @@ -333,17 +333,28 @@ YahooAccount.prototype = {
>    setBuddyStatus: function(aName, aStatus, aMessage) {
>      if (!this._buddies.has(aName))
>        return;
> -    this._buddies.get(aName).setStatus(aStatus, aMessage);
> +    let buddy = this._buddies.get(aName);
> +    // If the message is set as undefined, use the existing message.
> +    if (aMessage === undefined)
> +      aMessage = buddy.statusText;
> +    buddy.setStatus(aStatus, aMessage);
> +  },

setBuddyStatus could do all the work if it had a similar check for an undefined value of aStatus here.

> diff --git a/chat/protocols/yahoo/yahoo-session.jsm b/chat/protocols/yahoo/yahoo-session.jsm
> --- a/chat/protocols/yahoo/yahoo-session.jsm
> +++ b/chat/protocols/yahoo/yahoo-session.jsm
> 
> @@ -985,35 +983,36 @@ const YahooPacketHandler = {
> +      if (key == 7) { // Buddy name.
> +        currentBuddyName = value;
> +        this._session.requestBuddyIcon(currentBuddyName);
> +      } else if (key == 10) // Buddy status.
> +        this.setBuddyStatus(currentBuddyName, kBuddyStatuses[value]);
> +      else if (key == 19) // Buddy status message.
> +        this.setBuddyStatusMessage(currentBuddyName, value);

This would call |this.setBuddyStatus(currentBuddyName, undefined, value)| instead.

The call itself is longer than before but we'd simplify the account object by having one method less (which was called from only one place and contained some duplicated code). I think that's worth it.

If you don't agree then let's talk about it again.
Attachment #8354666 - Flags: review?(benediktp) → review-
Attached patch Patch 6Splinter Review
*** Original post on bio 2136 as attmnt 2897 at 2013-09-20 03:52:00 UTC ***

I agree with your changes Mic. I removed setBuddyStatusMessage and made setBuddyStatus more versatile.
Attachment #8354667 - Flags: review?(clokep)
Comment on attachment 8354666 [details] [diff] [review]
Patch 5

*** Original change on bio 2136 attmnt 2896 at 2013-09-20 03:52:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354666 - Attachment is obsolete: true
Comment on attachment 8354667 [details] [diff] [review]
Patch 6

*** Original change on bio 2136 attmnt 2897 at 2013-09-20 12:11:28 UTC ***

I'm going to r+ this, but there are a couple things I (and aleth) find weird:
* We overwrite aFoo, but we do seem to do this in other places in the code if it makes it easier to read, so this is fine.
* aleth would prefer to use "null" instead of undefined, but I don't have an opinion on this.
Attachment #8354667 - Flags: review?(clokep) → review+
*** Original post on bio 2136 at 2013-10-03 21:18:47 UTC ***

Let's get this checked in.
Whiteboard: [checkin-needed]
*** Original post on bio 2136 at 2013-10-03 22:47:55 UTC ***

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