Closed Bug 955582 Opened 8 years ago Closed 8 years ago

Stats service should maintain statistical data for conversations and use it for sorting.

Categories

(Instantbird :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

Attachments

(1 file, 11 obsolete files)

*** Original post on bio 2143 at 2013-08-31 18:16:00 UTC ***

The WIP attached works, but needs the following:
 - For contacts, the stats for the buddy with the highest score should be used.
 - Chat rooms, even those that have stored stats, are displayed only after being received from a requestChatRooms callback.

Sorry for the delay in getting this up.
Depends on: 955503
Attached patch WIP (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2812 at 2013-08-31 18:16:00 UTC ***

This didn't get attached while filing :(
Attachment #8354582 - Flags: feedback?(aleth)
Attachment #8354582 - Flags: feedback?(florian)
Attachment #8354582 - Flags: feedback?(benediktp)
Attachment #8354582 - Flags: feedback?(clokep)
Blocks: 955486
Blocks: 955585
No longer blocks: 955486
Comment on attachment 8354582 [details] [diff] [review]
WIP

*** Original change on bio 2143 attmnt 2812 at 2013-09-03 17:49:51 UTC ***

I don't know this code at all, I won't be able to give any good feedback. Canceling this. Please re-add this if there's a specific section for me to look at.
Attachment #8354582 - Flags: feedback?(clokep)
*** Original post on bio 2143 at 2013-09-04 07:37:41 UTC ***

(In reply to comment #0)

>  - For contacts, the stats for the buddy with the highest score should be used.

Shouldn't it rather be the sum of the stats for all of the contact's buddies?
Comment on attachment 8354582 [details] [diff] [review]
WIP

*** Original change on bio 2143 attmnt 2812 at 2013-09-04 08:28:30 UTC ***

I looked only quickly, but this is what I understood of the patch:
When the stats service is initialized, you:
1. read all the data from the database, and put it in memory.
2. Traverse all the logs directories, and parse all log files to create stats, and store them in memory and in the database.

Then, how is the database of any use if you:
1. Never query it (you seem to only wrote to it, or read everything at once).
2. Overwrite all its content each time Instantbird is restarted?
(Assuming I have read too quickly and missed the code checking if a conversation already had stats stored in the database before opening the log file to parse it again; if you never query the database and only use it as a cache file, you should consider a JSON file stored asynchronously in the profile instead of a database).

I don't understand why you create ConversationStats objects for contacts that the user never had any conversation with; this seems wasteful.


So the things you need to do here are:
- Stop parsing files that you already have data for (maybe store the timestamp of the last modification of the file, to check if you need to re-read it?)
- If you think having an object in memory all the time for each conversation the user ever had before is reasonable; get rid of the indexedDb and just cache stuff in a JSON file. (Hint: I'll frown at any approach that keeps some stuff in memory for each spammer who ever private messaged me ;-).)
- Or actually use the database, and only read from it the data you need. You likely want the database to be indexed on the conversation scores, so that you can easily get all the conversations that have a score above a certain threshold, and ignore the rest.


Another possible way forward would be to just drop anything related to the database from this patch; have the ranking work based only on data in memory; and handle storing/caching in another bug. This would likely make smaller patches, that would be more likely to be reviewed quickly.
Attachment #8354582 - Flags: feedback?(florian) → feedback-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2819 at 2013-09-04 19:07:00 UTC ***

This patch drops the database as suggested and simply caches all stats to a JSON file.
If the cache file doesn't exist, logs are parsed to obtain stats.
The cache is updated every 10 minutes, but only if there were messages exchanged during this time. It's also updated on quitting Instantbird.
Attachment #8354589 - Flags: review?(florian)
Attachment #8354589 - Flags: review?(benediktp)
Attachment #8354589 - Flags: review?(aleth)
Comment on attachment 8354582 [details] [diff] [review]
WIP

*** Original change on bio 2143 attmnt 2812 at 2013-09-04 19:07:41 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354582 - Attachment is obsolete: true
Attachment #8354582 - Flags: feedback?(benediktp)
Attachment #8354582 - Flags: feedback?(aleth)
Attached patch Patch 2 (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2820 at 2013-09-04 19:11:00 UTC ***

Fixed a nit, sorry.
Attachment #8354590 - Flags: review?(florian)
Attachment #8354590 - Flags: review?(benediktp)
Attachment #8354590 - Flags: review?(aleth)
Comment on attachment 8354589 [details] [diff] [review]
Patch

*** Original change on bio 2143 attmnt 2819 at 2013-09-04 19:11:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354589 - Attachment is obsolete: true
Attachment #8354589 - Flags: review?(florian)
Attachment #8354589 - Flags: review?(benediktp)
Attachment #8354589 - Flags: review?(aleth)
Attached patch Patch 3 (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2821 at 2013-09-04 19:26:00 UTC ***

Fix a bug where existing conversations get sorted into a lower position than the corresponding contact or chat.
Attachment #8354591 - Flags: review?(florian)
Attachment #8354591 - Flags: review?(benediktp)
Attachment #8354591 - Flags: review?(aleth)
Comment on attachment 8354590 [details] [diff] [review]
Patch 2

*** Original change on bio 2143 attmnt 2820 at 2013-09-04 19:26:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354590 - Attachment is obsolete: true
Attachment #8354590 - Flags: review?(florian)
Attachment #8354590 - Flags: review?(benediktp)
Attachment #8354590 - Flags: review?(aleth)
Comment on attachment 8354591 [details] [diff] [review]
Patch 3

*** Original change on bio 2143 attmnt 2821 at 2013-09-04 21:58:57 UTC ***

>   _removeChatsForAccount: function(aAccId) {
>     if (!this._chatsByAccountIdAndName.has(aAccId))
>       return;
>-    this._chats = this._chats.filter(function(c) c._accountId != aAccId);
>+    this._convs = this._convs.filter(function(c)
>+      c.source == "chat" && c._accountId != aAccId);

Did you mean:
  c.source != "chat" || c._accountId != aAccId
?


>   _sortComparator: function(aPossibleConvA, aPossibleConvB) {
>-    return (aPossibleConvB.statusType - aPossibleConvA.statusType) ||
>+    let getScoreForConv = function(aConv) {
>+      // Contacts may have multiple buddies attached to them, so we sum their
>+      // individual message counts before arriving at the final score.
>+      if (aConv.source == "contact") {
>+        let stats = new ConversationStats("", 0, 0, 0, 0);
>+        for (let id of aConv.buddyIds) {
>+          let buddyStats = this._statsByConvId.get(id);
>+          if (buddyStats) {
>+            stats.msgCount += buddyStats.msgCount;
>+            stats.incomingCount += buddyStats.incomingCount;
>+            stats.outgoingCount += buddyStats.outgoingCount;
>+            if (buddyStats.lastDate > stats.lastDate)
>+              stats.lastDate = buddyStats.lastDate;
>+          }
>+          return stats.computedScore;

Did you really mean to return here after the first iteration of the loop?


>+  _cacheAllStats: function() {
>+    let encoder = new TextEncoder();
>+    let stringToWrite = "";
>+    for (let [id, stats] of this._statsByConvId)
>+      stringToWrite += JSON.stringify(stats) + "\n";

Why are you saving this line by line rather than just JSON.stringify(this._statsByConvId)?

FYI, the reason why we do this for log files is so that:
1. The files are human readable/possible to search with grep
2. Not closing the log correctly doesn't corrupt the whole file, as each line is independent.

For a cache file I don't think you care about any of these 2 properties.


For the conversation ids, would it make sense to use the same string as the folder path? (ie. prpl/account/convname)

Nit: var gLogParser = {
...
}*;*

I wonder if the code would be more readable, if we used 'gLogParser' instead of this in logs of places, and removed the .bind(this).

Note: this isn't a full review, I only commented on stuff that jumped at me, but I figured the faster the feedback loop, the more useful the feedback.
Attachment #8354591 - Flags: review?(florian) → review-
Attached patch Patch 4 (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2823 at 2013-09-04 23:20:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354593 - Flags: review?
Comment on attachment 8354593 [details] [diff] [review]
Patch 4

*** Original change on bio 2143 attmnt 2823 at 2013-09-04 23:23:12 UTC ***

I've quickly addressed the review comments.
Attachment #8354593 - Attachment description: Patch 3 → Patch 4
Attachment #8354593 - Flags: review? → review?(florian)
Attachment #8354593 - Flags: review?(aleth)
Attachment #8354593 - Flags: review?(benediktp)
Comment on attachment 8354591 [details] [diff] [review]
Patch 3

*** Original change on bio 2143 attmnt 2821 at 2013-09-04 23:24:10 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354591 - Attachment is obsolete: true
Attachment #8354591 - Flags: review?(benediktp)
Attachment #8354591 - Flags: review?(aleth)
Comment on attachment 8354593 [details] [diff] [review]
Patch 4

*** Original change on bio 2143 attmnt 2823 at 2013-09-05 14:35:03 UTC ***

>+        catch(e) {};

Probably a good idea to display a warning about which file is being skipped, including the error e.

Please take account of legacy txt logs as we discussed.

>+  _sweepAccounts: function() {

Needs a comment explaining what it's doing.

>+  _sweepPrpls: function(aPrpls) {

Needs a comment explaining what it's doing (building this._accounts by ...).

>+      Cu.reportError("Error while sweeping log files:\n" + aError);

These would be more useful if they had a different text depending on which function the reportError was in ;)

>+    let file = Services.dirsvc.get("ProfD", Ci.nsIFile);
>+    file.append("logs");
>+    this._logsPath = file.path;

This looks like it wants to live in logger.js and be available via an API. But it might be worth discussing with flo if in fact the whole log sweeper should live there, or in a separate file (to keep it out of /chat),  if it is supposed to index the log files as well in the medium term. For now I am OK with it.

>   _init: function() {
>     let contacts = Services.contacts.getContacts();
>     for (let contact of contacts)
>       this._addContact(contact);
>     for (let notification of kNotificationsToObserve)
>       Services.obs.addObserver(this, notification, false);

Nit: add a blank line after this

>+    }.bind(this), function(aError) {
>+      // Most likely the cache didn't exist yet (or it was corrupted?).
>+      // Rebuild from logs.

Even if it's not an error, this does warrant a message to the error console (including the error just in case).

>   _removeChatsForAccount: function(aAccId) {
>     if (!this._chatsByAccountIdAndName.has(aAccId))
>       return;
>-    this._chats = this._chats.filter(function(c) c._accountId != aAccId);
>+    this._convs = this._convs.filter(function(c)
>+      c.source != "chat" || c._accountId != aAccId);

Add a comment explaining what the c.source != "chat" test is for.

>   _sortComparator: function(aPossibleConvA, aPossibleConvB) {
>-    return (aPossibleConvB.statusType - aPossibleConvA.statusType) ||
>+    let getScoreForConv = function(aConv) {
>+    let getStatusForConv = function(aConv, aScore) {

The current code calls these two functions every time a comparison is made. Is there a good way to instead call them lazily only once per sort process and conv?

> function PossibleConvFromContact(aContact) {
>   this._displayName = aContact.displayName;
>   this._statusType = aContact.statusType;
>   this._statusText = aContact.statusText;
>   this._buddyIconFilename = aContact.preferredBuddy.buddyIconFilename;
>-  this._id = aContact.id;
>+  let buddy = aContact.preferredBuddy;
>+  this._contactId = aContact.id;
>+  this._id = getConversationId(buddy.protocol.id.slice("prpl-".length),
>+                               buddy.preferredAccountBuddy.account.name,
>+                               buddy.normalizedName, false);

What happens when the preferredBuddy changes?

>+  this._id = getConversationId(account.protocol.id.slice("prpl-".length),

I don't think we need to calculate "prpl-".length every time ;) Just use 5 and explain in a comment, or a constant.

> function ExistingConversation(aUIConv) {
>+  // Not really important, since we don't store stats for existing conversations.

What's not really important? ;)
Attachment #8354593 - Flags: review?(aleth) → review-
Attached patch Patch 5 (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2862 at 2013-09-10 22:45:00 UTC ***

I've tried to address all the comments except the following...

>The current code calls these two functions every time a comparison is made. Is
>there a good way to instead call them lazily only once per sort process and
>conv?

and

>Please take account of legacy txt logs as we discussed.

...because I needed to sleep. I'll work on these tomorrow :)
Attachment #8354632 - Flags: review?(florian)
Attachment #8354632 - Flags: review?(benediktp)
Attachment #8354632 - Flags: review?(aleth)
Comment on attachment 8354593 [details] [diff] [review]
Patch 4

*** Original change on bio 2143 attmnt 2823 at 2013-09-10 22:45:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354593 - Attachment is obsolete: true
Attachment #8354593 - Flags: review?(florian)
Attachment #8354593 - Flags: review?(benediktp)
Comment on attachment 8354632 [details] [diff] [review]
Patch 5

*** Original change on bio 2143 attmnt 2862 at 2013-09-11 08:13:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354632 - Flags: review?(florian) → review-
*** Original post on bio 2143 at 2013-09-11 08:13:21 UTC ***

Comment on attachment 8354632 [details] [diff] [review] (bio-attmnt 2862)
Patch 5

>+    }).bind(this), function(aError) {
>+      Cu.reportError("Error while sweeping logs folder:\n" + aError);

Shouldn't error messages about a specific folder or log file include the path to the problematic file?


>+    OS.File.read(this._statsCacheFilePath).then(function(aArray) {
>+      try {
>+        let decoder = new TextDecoder();
>+        let statsByConvId = JSON.parse(decoder.decode(aArray));

Nit: the decoder variable doesn't seem useful (used only once).

>+        for each (let stats in statsByConvId) {

Is this a case where you could use for of?

>+          stats = new ConversationStats(stats.id, stats.lastDate, stats.msgCount,
>+                                        stats.incomingCount, stats.outgoingCount);
>+          this._statsByConvId[stats.id] = stats;

Having to recreate all the objects (that already exist at the end of JSON.parse) is a bit unfortunate.
Could it work to do:
this._statsByConvId = JSON.parse(...);
and then with the for loop, do:
  stats.__proto__ = ConversationStats.prototype
?

>+      catch (e) {
>+        // Something unexpected was encountered in the file.
>+        // (Maybe it was tampered with?) Rebuild the cache from logs.
>+        Cu.reportError("Error while reading conversation stats cache. " +
>+                       "Rebuilding from logs.\n" + e);
>+        gLogParser.sweep(this);
>+      }
>+    }.bind(this), function(aError) {
>+      Cu.reportError(
>+        "Error while reading stats cache, rebuilding from logs:\n" + aError);
>+      // Most likely the cache didn't exist yet (or it was corrupted?).
>+      // Rebuild from logs.
>+      gLogParser.sweep(this);
>+    }.bind(this));

This feels a lot like code duplication. Isn't .then automatically calling its error callback if the success callback throws?

Also, you shouldn't put an error message in the error console at startup if there's no cache file. We don't want to great users with new profiles with an error message ;). Can you detect this case? We do want an error if we are ignoring (and overwriting) an existing file.



>   _sortComparator: function(aPossibleConvA, aPossibleConvB) {
>-    return (aPossibleConvB.statusType - aPossibleConvA.statusType) ||
>+    let getScoreForConv = function(aConv) {
>+      // Contacts may have multiple buddies attached to them, so we sum their
>+      // individual message counts before arriving at the final score.
>+      if (aConv.source == "contact") {
>+        let stats = new ConversationStats("", 0, 0, 0, 0);

These default values seem strange. Why aren't they ommited, and put in the prototype?

>+        for (let id of aConv.buddyIds) {
>+          let buddyStats = this._statsByConvId[id];
>+          if (buddyStats) {
>+            stats.msgCount += buddyStats.msgCount;
>+            stats.incomingCount += buddyStats.incomingCount;
>+            stats.outgoingCount += buddyStats.outgoingCount;
>+            if (buddyStats.lastDate > stats.lastDate)
>+              stats.lastDate = buddyStats.lastDate;

What about adding a method to the ConversationStats prototype to merge with the value of another ConversationStats?

>+          }
>+        }
>+        return stats.computedScore;
>+      }
>+      else {
>+        let stats = this._statsByConvId[aConv.id];
>+        if (stats)
>+          return stats.computedScore;

Why isn't 'computedScore' an attribute of each PossibleConversation object?

>+      }
>+      return 0;
>+    }.bind(this);
>+    let scoreA = getScoreForConv(aPossibleConvA);
>+    let scoreB = getScoreForConv(aPossibleConvB);
>+    let getStatusForConv = function(aConv, aScore) {
>+      if (aConv.source != "chat" || !aScore)
>+        return aConv.statusType;
>+      // Chats that have a score get STATUS_AVAILABLE so they don't end up
>+      // at the bottom of the list (since chats are STATUS_UNKNOWN by default).
>+      return Ci.imIStatusInfo.STATUS_AVAILABLE;

This also feels like it should be part of the PossibleConversation object.



>+    else if (aTopic == "prpl-quit")
>+      this._cacheAllStats();

Writing to disk at shutdown is expensive (it's likely to delay the shutdown). Shouldn't there be a check to write only if some data has been modified since the last time we wrote the cache to disk?

>+    else if (aTopic == "new-text") {
>+      if (aSubject.system) // We don't care about system messages.
>+        return;
>+      let conv = aSubject.conversation;

>+      let id = getConversationId(conv.account.protocol.id.slice(kPrplPrefixLength),
>+                                 conv.account.name, conv.normalizedName, conv.isChat);
>+      if (!(id in this._statsByConvId))
>+        this._statsByConvId[id] = new ConversationStats(id, 0, 0, 0, 0);
>+      let stats = this._statsByConvId[id];

This seems expensive to do for each message. What about using an object to keep references to the ConversationStats for each active conversation? (The keys in this object would be conv.id).

Another possible way to handle this would be to make the ConversationStats object observe the conversation directly.

The global observer here would then only observer new conversation notifications.


>+      stats.msgCount++;

Nit: when the return value isn't used, we usually write ++i rather than i++.


>+// Length of "prpl-".
>+const kPrplPrefixLength = 5;

Why are you using protocol.id.slice(kPrplPrefixLength) instead of protocol.normalizedName?

>+function ConversationStats(aConvId, aLastDate, aMsgCount,
>+                           aIncomingCount, aOutgoingCount) {
>+  this.id = aConvId;
>+  this.lastDate = aLastDate;
>+  this.msgCount = aMsgCount;
>+  this.incomingCount = aIncomingCount;
>+  this.outgoingCount = aOutgoingCount;

I'm wondering if the constructor shouldn't take only one optional parameter that would be an existing ConversationStats object.

>+};

No ; here.

>+ConversationStats.prototype = {
>+  id: null,

Shouldn't the default value be ""?

>+  msgCount: null,

What's the point of keeping this value? Can't it be get msgCount() this.incomingCount + this.outgoingCount?

>+  incomingCount: null,
>+  outgoingCount: null,

Sounds like the default values want to be 0 here.

>+  get computedScore()
>+    this.msgCount * this.frequencyMultiplier * this.recencyMultiplier,

Trailing comma.

>+}

; here.

Stopping now, the train is at the station ;).
*** Original post on bio 2143 at 2013-09-11 08:51:29 UTC ***

(In reply to comment #13)

Thanks for the review!

> Comment on attachment 8354632 [details] [diff] [review] (bio-attmnt 2862) [details]
> Patch 5
> 
> >+    }).bind(this), function(aError) {
> >+      Cu.reportError("Error while sweeping logs folder:\n" + aError);
> 
> Shouldn't error messages about a specific folder or log file include the path
> to the problematic file?

I haven't tested, but my thinking was that the error would mention the file in question. I'll test this and make appropriate changes.

> Having to recreate all the objects (that already exist at the end of
> JSON.parse) is a bit unfortunate.
> Could it work to do:
> this._statsByConvId = JSON.parse(...);
> and then with the for loop, do:
>   stats.__proto__ = ConversationStats.prototype
> ?

Good idea, thanks.

> >+      catch (e) {
> >+        // Something unexpected was encountered in the file.
> >+        // (Maybe it was tampered with?) Rebuild the cache from logs.
> >+        Cu.reportError("Error while reading conversation stats cache. " +
> >+                       "Rebuilding from logs.\n" + e);
> >+        gLogParser.sweep(this);
> >+      }
> >+    }.bind(this), function(aError) {
> >+      Cu.reportError(
> >+        "Error while reading stats cache, rebuilding from logs:\n" + aError);
> >+      // Most likely the cache didn't exist yet (or it was corrupted?).
> >+      // Rebuild from logs.
> >+      gLogParser.sweep(this);
> >+    }.bind(this));
> 
> This feels a lot like code duplication. Isn't .then automatically calling its
> error callback if the success callback throws?

From what I understand, the error callback is called if there was a problem reading the file. The success callback is called with aArray only if the file was read properly. The try/catch in the success callback is us making sure that the read file is valid JSON. Let me know if I've misunderstood this.

> 
> >+    else if (aTopic == "prpl-quit")
> >+      this._cacheAllStats();
> 
> Writing to disk at shutdown is expensive (it's likely to delay the shutdown).
> Shouldn't there be a check to write only if some data has been modified since
> the last time we wrote the cache to disk?

Hmm, checking if we've set _statsCacheUpdateTimer would probably do to check if the cache needs to be updated.

> >+    else if (aTopic == "new-text") {
> >+      if (aSubject.system) // We don't care about system messages.
> >+        return;
> >+      let conv = aSubject.conversation;
> 
> >+      let id = getConversationId(conv.account.protocol.id.slice(kPrplPrefixLength),
> >+                                 conv.account.name, conv.normalizedName, conv.isChat);
> >+      if (!(id in this._statsByConvId))
> >+        this._statsByConvId[id] = new ConversationStats(id, 0, 0, 0, 0);
> >+      let stats = this._statsByConvId[id];
> 
> This seems expensive to do for each message. What about using an object to keep
> references to the ConversationStats for each active conversation? (The keys in
> this object would be conv.id).
> 
> Another possible way to handle this would be to make the ConversationStats
> object observe the conversation directly.
> The global observer here would then only observer new conversation
> notifications.

I like the second idea better.

> >+      stats.msgCount++;
> 
> Nit: when the return value isn't used, we usually write ++i rather than i++.

Ugh, I usually remember to do ++i :(

> >+// Length of "prpl-".
> >+const kPrplPrefixLength = 5;
> 
> Why are you using protocol.id.slice(kPrplPrefixLength) instead of
> protocol.normalizedName?

I didn't know :)
Attached patch Patch 6 (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2884 at 2013-09-13 17:59:00 UTC ***

> >+        for each (let stats in statsByConvId) {
> 
> Is this a case where you could use for of?

As mentioned on IRC, the object isn't iterable.

> 
> >+          }
> >+        }
> >+        return stats.computedScore;
> >+      }
> >+      else {
> >+        let stats = this._statsByConvId[aConv.id];
> >+        if (stats)
> >+          return stats.computedScore;
> 
> Why isn't 'computedScore' an attribute of each PossibleConversation object?

This is because PossibleConversations don't have access to _statsByConvId in the stats service, and I couldn't think of a good way to give it to them. I could pass a PossibleConversation its ConversationStats in the constructor, but it really seems unnecessary to me - why not let the StatsService do the work?

> >+      }
> >+      return 0;
> >+    }.bind(this);
> >+    let scoreA = getScoreForConv(aPossibleConvA);
> >+    let scoreB = getScoreForConv(aPossibleConvB);
> >+    let getStatusForConv = function(aConv, aScore) {
> >+      if (aConv.source != "chat" || !aScore)
> >+        return aConv.statusType;
> >+      // Chats that have a score get STATUS_AVAILABLE so they don't end up
> >+      // at the bottom of the list (since chats are STATUS_UNKNOWN by default).
> >+      return Ci.imIStatusInfo.STATUS_AVAILABLE;
> 
> This also feels like it should be part of the PossibleConversation object.

I disagree, because the status type of PossibleConversations is exposed to other consumers. The stats service needs to ensure PossibleChats with a score don't end up at the bottom of the list as mentioned in the comment - that's the only reason we're spoofing the statusType here. I'll try to think of a better way to do this if this seems vague/confusing.

> What about using an object to keep
> references to the ConversationStats for each active conversation? (The keys in
> this object would be conv.id).

I've done this.

> >+function ConversationStats(aConvId, aLastDate, aMsgCount,
> >+                           aIncomingCount, aOutgoingCount) {
> >+  this.id = aConvId;
> >+  this.lastDate = aLastDate;
> >+  this.msgCount = aMsgCount;
> >+  this.incomingCount = aIncomingCount;
> >+  this.outgoingCount = aOutgoingCount;
> 
> I'm wondering if the constructor shouldn't take only one optional parameter
> that would be an existing ConversationStats object.

Why? :S
Attachment #8354654 - Flags: review?(aleth)
Attachment #8354654 - Flags: review?(florian)
Comment on attachment 8354632 [details] [diff] [review]
Patch 5

*** Original change on bio 2143 attmnt 2862 at 2013-09-13 17:59:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354632 - Attachment is obsolete: true
Attachment #8354632 - Flags: review?(benediktp)
Attachment #8354632 - Flags: review?(aleth)
*** Original post on bio 2143 at 2013-09-13 20:43:15 UTC ***

(In reply to comment #15)

> > >+      if (aConv.source != "chat" || !aScore)
> > >+        return aConv.statusType;
> > >+      // Chats that have a score get STATUS_AVAILABLE so they don't end up
> > >+      // at the bottom of the list (since chats are STATUS_UNKNOWN by default).
> > >+      return Ci.imIStatusInfo.STATUS_AVAILABLE;
> > 
> > This also feels like it should be part of the PossibleConversation object.
> 
> I disagree, because the status type of PossibleConversations is exposed to
> other consumers.

How are the expectations of other consumers different? Would it work everywhere to give channels you never joined STATUS_UNKNOWN and channels that have been joined before STATUS_OFFLINE?
Attached patch Patch 7 (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2894 at 2013-09-15 22:20:00 UTC ***

- Errors in the log sweeper now include the path of the file/directory being accessed.
- I've tried to simplify sortComparator. Chats now get a default status of available (I think this makes sense). Convs with a score get equal priority (so the priority of an available contact with == priority of a chat if both have scores), but chats without scores are pushed to the bottom.
- Make sure scores are recomputed if stats changed.

I still need to add support for txt logs (can someone attach a sample one please?), and see if I can think of a good way to keep scores cached (so they don't have to be computed at each comparison).

As for aleth's suggestion about keeping a single list of existing convs (both for listening for new messages and inserting ExistingConversations while filtering): http://log.bezut.info/instantbird/130915/#m277
Attachment #8354664 - Flags: review?(aleth)
Attachment #8354664 - Flags: review?(florian)
Comment on attachment 8354654 [details] [diff] [review]
Patch 6

*** Original change on bio 2143 attmnt 2884 at 2013-09-15 22:20:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354654 - Attachment is obsolete: true
Attachment #8354654 - Flags: review?(florian)
Attachment #8354654 - Flags: review?(aleth)
Depends on: 955617
*** Original post on bio 2143 at 2013-09-18 22:01:59 UTC ***

(In reply to comment #17)
> As for aleth's suggestion about keeping a single list of existing convs (both
> for listening for new messages and inserting ExistingConversations while
> filtering): http://log.bezut.info/instantbird/130915/#m277

I was mostly concerned about improving getFilteredConvs. Here's a bunch of thoughts.

>let filteredConvs = this._convs.slice(0);
You could do without this if it were possible to filter first and then replace any elements corresponding to existing conversations with existingConv entries.

You have a lot of code to find the position of an existingConv (list obtained from the UI) in the possibleConv list. 

Wouldn't it be possible to ask the stats service for the position of the entry, since the stats service maintains a list of these indexed by prplConversation id, which the UIConv knows, and the stats service keeps updating this position for existing conversations? 

As as small optimization, would it make sense for the stats service to only actually do this change of position once the position is requested (saving a lot of finding the entry/replacing the entry operations while nobody is actually watching)?

Would it make sense to get rid of existingConv objects altogether and instead have a flag (or methods) in possibleConvs that temporarily turns them into existingConvs while the conv is active? Then you could stop worrying about finding existingConvs in getFilteredConvs altogether.
Comment on attachment 8354664 [details] [diff] [review]
Patch 7

*** Original change on bio 2143 attmnt 2894 at 2013-09-20 17:16:12 UTC ***

>+      if (!aError.becauseNoSuchFile) {
>+        Cu.reportError(
>+          "Error while reading stats cache, rebuilding from logs:\n" + aError);

This should be just a message or a warning, as everyone will get to see it at least once, and it's not an error ;)

>+  _getScoreForConv: function(aPossibleConv) {
>+    // Contacts may have multiple buddies attached to them, so we sum their
>+    // individual message counts before arriving at the final score.
>+    if (aPossibleConv.source == "contact") {
>+      let stats = new ConversationStats();
>+      for (let id of aPossibleConv.buddyIds) {
>+        let buddyStats = this._statsByConvId[id];
>+        if (buddyStats)
>+          stats = stats.mergeWith(buddyStats);
>+      }
>+      return stats.computedScore;
>+    }
>+    else {
>+      let stats = this._statsByConvId[aPossibleConv.id];
>+      if (stats)
>+        return stats.computedScore;
>+    }
>+    return 0;
>+  },

If this was a method of possibleConvs, you would already know the source, and you could implement it as a lazy getter. (Maybe this what flo suggested earlier, for which you would need possibleConvs to have access to this._statsByConvId?)

>+    // Contacts and chats that have a score get equally high priority.
>+    // For convs with no score, chats get lower priority than contacts.
>+    let getScorePriorityForConv = function(aIsChat, aScore) {
>+      if (aScore)
>+        return 1;
>+      if (aIsChat)
>+        return -1;
>+      return 0;
>+    }

Could you do without this altogether if getScoreForConv returned -1 for chats with no score?

>+    else if (aTopic == "new-text") {
>+      if (aSubject.system) // We don't care about system messages.
>+        return;
>+
>+      let stats = this._statsByPrplConvId.get(aSubject.conversation.id);
>+      ++stats.msgCount;
>+      aSubject.outgoing ? stats.outgoingCount++ : stats.incomingCount++;
>+      stats.lastDate = Date.now();
>+      // Ensure the score is recomputed next time it's used.
>+      delete stats._computedScore;
>+
>+      // Schedule a cache update in 10 minutes.
>+      if (!this._statsCacheUpdateTimer) {
>+        this._statsCacheUpdateTimer =
>+          setTimeout(this._cacheAllStats.bind(this), 600000);
>+      }

I was under the impression this would induce a recalculation of the position of the corresponding possibleConv (probably with an executeSoon), and if the position changed, inform observers. But I don't see how this happens in the existing code?
Attachment #8354664 - Flags: review?(aleth) → review-
*** Original post on bio 2143 at 2013-09-21 09:38:51 UTC ***

> >+  _getScoreForConv: function(aPossibleConv) {

> If this was a method of possibleConvs, you would already know the source, and
> you could implement it as a lazy getter. (Maybe this what flo suggested
> earlier, for which you would need possibleConvs to have access to
> this._statsByConvId?)

Just to clarify: I don't mean to insist on this, as the only thing which is calculated repeatedly here is the sum over buddies. So a way of making that lazy (just like the computedScore getter) would do.
Attached patch Patch 8 (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2904 at 2013-09-25 18:03:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354674 - Flags: review?(aleth)
Attachment #8354674 - Flags: review?(florian)
Comment on attachment 8354664 [details] [diff] [review]
Patch 7

*** Original change on bio 2143 attmnt 2894 at 2013-09-25 18:03:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354664 - Attachment is obsolete: true
Attachment #8354664 - Flags: review?(florian)
Comment on attachment 8354674 [details] [diff] [review]
Patch 8

*** Original change on bio 2143 attmnt 2904 at 2013-09-25 21:39:19 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354674 - Flags: review?(aleth) → review-
*** Original post on bio 2143 at 2013-09-25 21:39:19 UTC ***

Comment on attachment 8354674 [details] [diff] [review] (bio-attmnt 2904)
Patch 8

Getting there :)

>+    // Read all our conversation stats from the cache.
>+    this._statsCacheFilePath =
>+      OS.Path.join(OS.Constants.Path.profileDir, "statsservicecache.json");
>+    OS.File.read(this._statsCacheFilePath).then(function(aArray) {
>+      try {
>+        gStatsByConvId = JSON.parse((new TextDecoder()).decode(aArray));
>+        for each (let stats in gStatsByConvId)
>+          stats.__proto__ = ConversationStats.prototype;
>+        gStatsByContactId = {};
>+      }
>+      catch (e) {
>+        // Something unexpected was encountered in the file.
>+        // (Maybe it was tampered with?) Rebuild the cache from logs.
>+        Cu.reportError("Error while reading conversation stats cache. " +
>+                       "Rebuilding from logs.\n" + e);
>+        gLogParser.sweep(this);
>+      }
>+    }.bind(this), function(aError) {
>+      if (!aError.becauseNoSuchFile) {
>+        Cu.reportError(
>+          "Error while reading stats cache, rebuilding from logs:\n" + aError);
>+      }
>+      gLogParser.sweep(this);

This needs a pref that allows to turn off log sweeping (default to on). 

>+    let signScoreA = scoreA ? scoreA / Math.abs(scoreA) : 0;
>+    let signScoreB = scoreB ? scoreB / Math.abs(scoreB) : 0;

Faster and better to read to use a helper

function sign(x) x > 0 ? 1 : x < 0 ? -1 : 0;

>   getFilteredConvs: function(aFilterStr) {
>-    let filteredConvs = this._contacts.concat(this._chats);
>+    // Duplicate this._convs to avoid modifying it while adding existing convs.
>+    let filteredConvs = this._convs.slice(0);

Please file a followup to improve this. If all else fails you can filter the convs and the existingconvs separately and then merge them, but I suspect (as discussed on IRC) there is a better way. 

Please also file a followup to include possibleconvs for everything that has stats.

>   addObserver: function(aObserver) {
>     if (this._observers.indexOf(aObserver) != -1)
>       return;
>     this._observers.push(aObserver);
>+
>+    // Reposition convs whose stats have changed.
>+    for (let conv of this._convsWithUpdatedStats) {
>+      this._convs.splice(this._convs.indexOf(conv), 1);
>+      let pos = this._getPositionToInsert(conv, this._convs);
>+      this._convs.splice(pos, 0, conv);
>+    }
>+    this._convsWithUpdatedStats.clear();

This repositioning should be in a separate function.

Again, we are repositioning a subset of the existingConvs. Then we look for them again in getFilteredConvs. Should we not move this repositioning to getFilteredConvs?

So, existing observers don't ever see any changes due to stats? Is this intentional?
What happens if a newtab is open and I open a second one - doesn't the existing one break?

>+      if (!conv.isChat) {
>+        // First buddy is an imIAccountBuddy. Second one is an imIBuddy.
>+        let contact = conv.buddy.buddy.contact;
>+        if (gStatsByContactId && contact)
>+          delete gStatsByContactId[contact.id];
>+        this._convsWithUpdatedStats.add(this._contactsById.get(contact.id));

Last line will fail if !contact.

>+      }
>+      else {
>+        let chatList = this._chatsByAccountIdAndName.get(conv.account.id);
>+        if (chatList && chatList.has(conv.normalizedName)) {
>+          this._convsWithUpdatedStats.add(chatList.get(conv.name));
>+        }
>+      }

I suspect the stuff in this if...else might be a bit slow, and there will be a followup, but I think you had a reason against maintaining a list of existing conversations (map from conv to the corresponding stats and possibleconv)? (It would be useful to have some summary of your responses to previous bug comments in bug comments and not just on IRC, as it makes it easier to find.)

>+function ConversationStats(aConvId, aLastDate, aMsgCount,
>+                           aIncomingCount, aOutgoingCount) {
>+  this.id = aConvId || "";
>+  this.lastDate = aLastDate || 0;
>+  this.msgCount = aMsgCount || 0;
>+  this.incomingCount = aIncomingCount || 0;
>+  this.outgoingCount = aOutgoingCount || 0;
>+}

FYI you can put the default values in the parameter list if you prefer (function(aConvId="", aLastDate=0) etc).

>+  get computedScore() {
>+    let id = this._contactId;
>+    if (gStatsByContactId && gStatsByContactId[id])

You probably don't need to check for gStatsByContactId as if it doesn't exist, throwing an error seems appropriate ;) Or do you expect a circumstance where it doesn't exist?
Attached patch Patch 9 (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2910 at 2013-09-27 17:25:00 UTC ***

>So, existing observers don't ever see any changes due to stats? Is this
>intentional?
>What happens if a newtab is open and I open a second one - doesn't the existing
>one break?

As discussed on IRC, the repositioning is now done in getFilteredConvs, and awesometabs do a refresh() in onSelect. This should address the concerns here.

>I suspect the stuff in this if...else might be a bit slow, and there will be a
>followup, but I think you had a reason against maintaining a list of existing
>conversations (map from conv to the corresponding stats and possibleconv)? (It
>would be useful to have some summary of your responses to previous bug comments
>in bug comments and not just on IRC, as it makes it easier to find.)

Er, yeah, sorry. In this case I didn't have any reason. The new patch gets and stores the PossibleConversation corresponding to the prplConversation in the code that handles the new-conversation notification, this should be way better.

>>+  get computedScore() {
>>+    let id = this._contactId;
>>+    if (gStatsByContactId && gStatsByContactId[id])
>
>You probably don't need to check for gStatsByContactId as if it doesn't exist,
>throwing an error seems appropriate ;) Or do you expect a circumstance where it
>doesn't exist?

It doesn't exist until log sweeping is done, or the stats cache has been loaded (I think this is mentioned in a comment) - hence the check. The thought was that if we cache cumulative stats of contacts before log sweeping/cache loading is done, we'll be caching useless 0-valued stats, which we'll need to clear later anyway.
Attachment #8354680 - Flags: review?(aleth)
Attachment #8354680 - Flags: review?(florian)
Comment on attachment 8354674 [details] [diff] [review]
Patch 8

*** Original change on bio 2143 attmnt 2904 at 2013-09-27 17:25:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354674 - Attachment is obsolete: true
Attachment #8354674 - Flags: review?(florian)
Comment on attachment 8354680 [details] [diff] [review]
Patch 9

*** Original change on bio 2143 attmnt 2910 at 2013-09-27 17:40:52 UTC ***

Please file the followups asap so they don't get forgotten!

I'd like flo to take another look at this patch though.
Attachment #8354680 - Flags: review?(aleth) → review+
Blocks: 955629
Blocks: 955630
Comment on attachment 8354680 [details] [diff] [review]
Patch 9

*** Original change on bio 2143 attmnt 2910 at 2013-09-30 23:28:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354680 - Flags: review?(florian) → review-
*** Original post on bio 2143 at 2013-09-30 23:28:54 UTC ***

Comment on attachment 8354680 [details] [diff] [review] (bio-attmnt 2910)
Patch 9

>+      OS.File.read(log).then(function(aArray) {
>+        // Try to parse the log file. If anything goes wrong here, the log file
>+        // has likely been tampered with so we ignore it and move on.
>+        try {
>+          let lines = decoder.decode(aArray).split("\n");
>+          // The first line is the header which identifies the conversation.
>+          let header = JSON.parse(lines.shift());
>+          let name = header.name;
>+          let protocol = header.protocol;
>+          let account = header.account;
>+          let date = Date.parse(header.date);
>+          let id = getConversationId(protocol, account, name, header.isChat);
>+          if (!(id in gStatsByConvId)) {
>+            gStatsByConvId[id] = new ConversationStats(id);
>+          }

Nit: no {}.

>+          let stats = gStatsByConvId[id];
>+          lines.pop(); // Ignore the final line break.
>+          for (let line of lines) {
>+            line = JSON.parse(line);
>+            if (line.flags[0] == "system") // Ignore system messages.
>+              continue;
>+            ++stats.msgCount;

This line is no longer needed, right?


>+    OS.File.read(this._statsCacheFilePath).then(function(aArray) {
>+      try {
>+        gStatsByConvId = JSON.parse((new TextDecoder()).decode(aArray));
>+        for each (let stats in gStatsByConvId)
>+          stats.__proto__ = ConversationStats.prototype;
>+        gStatsByContactId = {};
>+      }
>+      catch (e) {
>+        // Something unexpected was encountered in the file.
>+        // (Maybe it was tampered with?) Rebuild the cache from logs.
>+        Cu.reportError("Error while reading conversation stats cache.\n" + e);

It annoys me that there are 2 error messages from different places that have the exact same messages.
Maybe we should replace "reading" by "parsing" in this one?


>+  _cacheAllStats: function() {
>+    let encoder = new TextEncoder();
>+    OS.File.writeAtomic(this._statsCacheFilePath,
>+                        encoder.encode(JSON.stringify(gStatsByConvId)),
>+                        {tmpPath: this._statsCacheFilePath + ".tmp"});
>+    delete this._statsCacheUpdateTimer;

Deleting the reference to the timer isn't enough to prevent it from firing.

I think you want:
  if (this._statsCacheUpdateTimer) {
    clearTimeout(this._statsCacheUpdateTimer);
    delete this._statsCacheUpdateTimer;
  }


>+    else if (aTopic == "new-text") {
>+      if (aSubject.system) // We don't care about system messages.
>+        return;
>+
>+      let conv = aSubject.conversation;
>+      let stats = this._statsByPrplConvId.get(conv.id);
>+      ++stats.msgCount;

Remove.

>+      aSubject.outgoing ? stats.outgoingCount++ : stats.incomingCount++;

Nit: ++foo rather than foo++


>+    else if (aTopic == "new-conversation") {
>+      let conv = aSubject;
>+      let id = getConversationId(conv.account.protocol.normalizedName,
>+                                 conv.account.name, conv.normalizedName, conv.isChat);
>+      if (!(id in gStatsByConvId))
>+        gStatsByConvId[id] = new ConversationStats(id);
>+      this._statsByPrplConvId.set(conv.id, gStatsByConvId[id]);
>+
>+      let possibleConv = null;
>+      if (!conv.isChat) {
>+        // First buddy is an imIAccountBuddy. Second one is an imIBuddy.

This comment took me a few seconds to understand. I think it would be easier to read with a '.' before 'buddy' and as one sentence:
// First .buddy is an imIAccountBuddy, second one is an imIBuddy.

>+        let contact = conv.buddy.buddy.contact;
>+        if (contact)
>+          possibleConv = this._contactsById.get(contact.id);
>+      }
>+      else {
>+        let chatList = this._chatsByAccountIdAndName.get(conv.account.id);
>+        if (chatList && chatList.has(conv.normalizedName))
>+          possibleConv = chatList.get(conv.name);
>+      }
>+      this._convsByPrplConvId.set(conv.id, possibleConv);
>+      conv.addObserver(this);

It looks like you are adding this observer only for the new-text notifications; it's not clear why you aren't just observing new-text from the observer service.
I suggested observing the conversations because I thought the observers you would use would be directly the ConversationStats instance that is in charge of the conversation. If you go through the whole logic of finding the possible conversation for each new message notification (I hope it's fast!), there's no point in observing each conversation separately...


>+function ConversationStats(aConvId = "", aLastDate = 0, aMsgCount = 0,
>+                           aIncomingCount = 0, aOutgoingCount = 0) {
>+  this.id = aConvId;
>+  this.lastDate = aLastDate;
>+  this.msgCount = aMsgCount;

Remove (and "aMsgCount = 0" too).

>+  mergeWith: function(aOtherStats) {
>+    let stats = new ConversationStats();
>+    stats.lastDate = Math.max(this.lastDate, aOtherStats.lastDate);
>+    stats.msgCount = this.msgCount + aOtherStats.msgCount;

Remove.


r- but all my comments are trivial to address, and in the next version I'll likely only check that these comments have been fully addressed. I won't pretend to have a full understanding of everything that's going on in this patch with Map/Set/objects, but I assume aleth has already reviewed this part thoroughly, so it's not worth me looking at it in details right now.
Attached patch Patch 10 (obsolete) — Splinter Review
*** Original post on bio 2143 as attmnt 2917 at 2013-10-01 07:11:00 UTC ***

Thanks for the review, sorry about the remnant msgCount assignments.

>It looks like you are adding this observer only for the new-text notifications;
>it's not clear why you aren't just observing new-text from the observer
>service.
>I suggested observing the conversations because I thought the observers you
>would use would be directly the ConversationStats instance that is in charge of
>the conversation. If you go through the whole logic of finding the possible
>conversation for each new message notification (I hope it's fast!), there's no
>point in observing each conversation separately...

The reason for observing new-conversation is so that we can find the PossibleConversation for the prplConversation (we map the prplConversation's id to the corresponding PossibleConversation), and not have to go through the logic at every new-text. I haven't made any changes to this in this patch.
Attachment #8354687 - Flags: review?(florian)
Comment on attachment 8354680 [details] [diff] [review]
Patch 9

*** Original change on bio 2143 attmnt 2910 at 2013-10-01 07:11:25 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354680 - Attachment is obsolete: true
*** Original post on bio 2143 at 2013-10-01 07:53:37 UTC ***

(In reply to comment #26)

> The reason for observing new-conversation

My question was about the "conv.addObserver(this);" line.
Attached patch Patch 11Splinter Review
*** Original post on bio 2143 as attmnt 2918 at 2013-10-01 08:32:00 UTC ***

This switches to using the observer service for new-text notifications.

I misunderstood since you were talking about finding the possible conv for each new-text notification, sorry.
Attachment #8354688 - Flags: review?(florian)
Comment on attachment 8354687 [details] [diff] [review]
Patch 10

*** Original change on bio 2143 attmnt 2917 at 2013-10-01 08:32:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354687 - Attachment is obsolete: true
Attachment #8354687 - Flags: review?(florian)
Blocks: 955636
Comment on attachment 8354688 [details] [diff] [review]
Patch 11

*** Original change on bio 2143 attmnt 2918 at 2013-10-01 22:58:36 UTC ***

Thanks for the updated patch. My local testing didn't reveal any major issues, so I think it's time to try this on nightly.

I did see a few minor issues, especially:
- very unresponsive UI when scrolling in the awesometab (I assume this was while the log crawling was still in progress)
- some JS errors on shutdown that seem to happen only if the awesome tab was open at the time I pressed Command+Q; not sure if that's related to the ranking patch or was an existing bug:
JavaScript strict warning: file:///.../InstantbirdDebug.app/Contents/MacOS/components/imConversations.js, line 426: reference to undefined property this._uiConv
JavaScript error: file:///.../InstantbirdDebug.app/Contents/MacOS/components/ibConvStatsService.js, line 315: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "this._uiConv is not an object" {file: "file:///.../InstantbirdDebug.app/Contents/MacOS/components/imConversations.js" line: 426}]' when calling method: [imIConversationsService::getUIConversations]

Worth investigating, but not worth delaying this patch further. Let's try it! :-)
Attachment #8354688 - Flags: review?(florian) → review+
*** Original post on bio 2143 at 2013-10-01 23:12:44 UTC ***

Can't wait to try this out!

http://hg.instantbird.org/instantbird/rev/8d0e7525f85f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
Depends on: 955646
Depends on: 955645
Depends on: 955640
Depends on: 955653
Blocks: 955013
Depends on: 955655
You need to log in before you can comment on or make changes to this bug.