788145 - Provide a way to delete chat logs

Status: NEW

(Thunderbird :: Instant Messaging, defect)

Description

[Vincent (caméléon)]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20100101 Firefox/15.0 Firefox/15.0
Build ID: 20120824154833

Steps to reproduce:

Ability to delete some chat history is very often ask on GSFN, see http://gsfn.us/t/33pbj


Actual results:

this is not possible at this moment or only with a workaround which is quite complicated:
"With Thunderbird closed, go into Thunderbird's Profile folder, go to the folder LOGS, then the folder of the chat platform you want to delete the history from, then go to the username of the person who's chat history you want to delete and delete the JavaScript Object Notation (.JSON) file in that directory.

For example: profile\logs\gtalk\[e-mail]

Then delete, the file that is in this dated type format: 2012-08-31.032541-0600.json "


Expected results:

it should be possible to delete all or only some parts of the chat history directly in Thunderbird.

Comment 1

[Florian Quèze [:florian]]

(In reply to Vincent (caméléon) from comment #0)

> Actual results:
> 
> this is not possible at this moment or only with a workaround which is quite
> complicated:
> "With Thunderbird closed, go into Thunderbird's Profile folder, go to the
> folder LOGS, then the folder of the chat platform you want to delete the
> history from, then go to the username of the person who's chat history you
> want to delete and delete the JavaScript Object Notation (.JSON) file in
> that directory.
> 
> For example: profile\logs\gtalk\[e-mail]
> 
> Then delete, the file that is in this dated type format:
> 2012-08-31.032541-0600.json "

This works for Instantbird but isn't enough for Thunderbird, as IM conversations are also indexed in Gloda.

Comment 2

[Cœur Noir]

This un-ability to delete chat history - and its workaround - should also be mentionned in
https://support.mozillamessaging.com/fr/kb/messagerie-instantanee-et-tchat#w_options-avancaees

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

Possible fix.

Comment 4

[Instantbot]

Comment on attachment 8355307 [details] [diff] [review]
Patch v1

Review of attachment 8355307 [details] [diff] [review]:
-----------------------------------------------------------------

I'm changing from r? to f?. Overall this looks pretty good, I have a couple of comments / nits. I'll let Florian do a full review, however.

::: chat/components/src/logger.js
@@ +661,5 @@
>      let enumerator = aGroupByDay ? DailyLogEnumerator : LogEnumerator;
>      return new enumerator([new LocalFile(aLog.path).parent.directoryEntries]);
>    },
> +  removeLog: function(aLog) {
> +    new LocalFile(aLog.path).remove(true);

The interface says this returns a boolean.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +465,5 @@
> +    // See if the selected row is a group and if the group has children.
> +    if (logGroup.level == 0 && logGroup.children.length != 0) {
> +      for each (let logs in logGroup.children) {
> +        imServices.logs.removeLog(logs.log);
> +      }

Nit: No { } for a single line statement.

@@ +469,5 @@
> +      }
> +    }
> +    else {
> +      imServices.logs.removeLog(logGroup.log);
> +    }

Nit: Here also.

Comment 5

[Patrick Cloke [:clokep]]

Comment on attachment 8355307 [details] [diff] [review]
Patch v1

Oops, the above changes were by me. I failed to switch accounts. Sorry.

Comment 6

[Josiah Bruner [:jsbruner]]

So we have a few options here as the UI goes to avoid falling into the trap of data loss.

----------------
1. | The most difficult, but arguably the ideal solution would be to implement a "trash" specifically for chat messages, since there currently is no such thing. Then deleted messages would go there and the user would be able to retrieve them later. We would have to figure out when we would want to permanently delete messages, but we can hold off on that. If/when we go this route, you'll probably want to deal with this in a separate bug.
----------------
2. | The easier solution (one that I suggest you add to this bug), would be to implement a "notification bar that shows up when you delete a message. This bar would ask if you want to undo the action, or alternatively, you can dismiss it with an |x|.

This is similar to the add-on notification bar: http://ge.tt/6MmqJAC1/v/0

If you delete one message and then delete another, tell them that there are two deleted messages to undo.

If say, a user deletes a message, deletes another, and then deletes a third and realizes he didn't mean to delete the second, he can at least hit the button to recover all three.

Also, if we allow the user to delete with the "delete" key, recovering all deleted messages gives extra cushion to protect against accidental deletions. Of course not use the delete key would be even safer, but perhaps not super practical.

We can dismiss this bar automatically after, say, 2 minutes, which should give the user plenty of time to panic, find the bar, and undo the action.
----------------

So again, I suggest you implement option 2 right now, and we can add 1 later if needed.

- Josiah

Comment 7

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Fixed the nits.

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8355780 [details] [diff] [review]
Patch v2

Review of attachment 8355780 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't done a full review yet, but I figured getting some comments quickly would be appreciated. You've made a lot of progress. The chat/ part looks good. I'll need to review more carefully the mail/ part.

Things you still need to deal with:
- make gloda forget chat logs that you delete. Gloda's indexing of IM happens in http://mxr.mozilla.org/comm-central/source/mail/components/im/modules/index_im.js so I guess you'll need to add some code to this file to support removing logs.
- make the gloda search results hide the logs that have been deleted from the UI, but that aren't permanently deleted yet. You'll likely need to touch http://mxr.mozilla.org/comm-central/source/mail/base/content/glodaFacetView.js or another file in the same folder starting with glodaFacet
- delete logs permanently at shutdown if the user quits Thunderbird before the timer finishes.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +10,5 @@
> +Components.utils.import("resource://gre/modules/PluralForm.jsm");
> +
> +// A global array holding the paths to the log files marked
> +// for deletion.
> +var logsMarkedForDeletion = [];

I think you want to use a Set here rather than an array (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set).

@@ +13,5 @@
> +// for deletion.
> +var logsMarkedForDeletion = [];
> +
> +var timer = Components.classes["@mozilla.org/timer;1"]
> +                      .createInstance(Components.interfaces.nsITimer);

Any reason for using a timer here rather than just a setTimeout?

@@ +41,5 @@
> +  if (notificationBox) {
> +    notification =
> +      notificationBox.appendNotification(deletedMessage,
> +                                         "logDeletionNotification",
> +                                         "null",

I think you meant null here instead of "null".

@@ +57,5 @@
> +  if (notificationBox && notification)
> +    notificationBox.removeNotification(notification);
> +  for each (let log in logsMarkedForDeletion) {
> +    imServices.logs.removeLog(log);
> +  }

Nit: you don't need the {} here.

@@ +450,5 @@
> +   *
> +   * @param aLogs:    An enumerator of logs.
> +   * @return:         an array of logs not marked for deletion.
> +   */
> +  _getUndeletedLogs: function(aLogs) {

'Undeleted' makes me think you are looking for logs that have been deleted, and where the user has undone the action.

::: mail/locales/en-US/chrome/messenger/chat.dtd
@@ +19,5 @@
>  <!ENTITY chat.participants             "Participants:">
>  <!ENTITY chat.previousConversations    "Previous Conversations:">
>  <!ENTITY chat.ongoingConversation      "Ongoing conversation">
>  
> +<!ENTITY deleteConversationCmd.label   "Delete this conversation">

I wonder if "Delete" is the right word here, or if "Forget" would be better. That's a UI question I guess.

Comment 9

[Suyash Agarwal (:sshagarwal)]

(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> Comment on attachment 8355780 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8355780 [details] [diff] [review]:
> -----------------------------------------------------------------
> Things you still need to deal with:
> - delete logs permanently at shutdown if the user quits Thunderbird before
> the timer finishes.
I am doing it in the closeTab(). Will that not work?
> ::: mail/components/im/content/chat-messenger-overlay.js
> @@ +13,5 @@
> > +// for deletion.
> > +var logsMarkedForDeletion = [];
> > +
> > +var timer = Components.classes["@mozilla.org/timer;1"]
> > +                      .createInstance(Components.interfaces.nsITimer);
> 
> Any reason for using a timer here rather than just a setTimeout?

Ya, at first I used window.setTimeout() but then switched to it.
Because of the use case, when the user deletes some conversations (say n)
and when the timer is running, again deletes a few more conversations (say k).
But before k's 30 seconds finish, if I allowed the n's timer to run, it will timeout
and execute the delete function and all (n+k) conversations will be deleted.
So, I am re-starting this timer, cancelling the previous one.
So, before starting any new timers I am stopping the previous ones so that the freshly deleted
conversations are not deleted before their notification goes away.
I could do it with keeping track of the timeout IDs but that would require maintaining another
set. So, if this isn't acceptable, please let me know I'll switch to it.

> > +<!ENTITY deleteConversationCmd.label   "Delete this conversation">
> 
> I wonder if "Delete" is the right word here, or if "Forget" would be better.
> That's a UI question I guess.

Ya, "Forget" sounds nice, thanks :)

Comment 10

[Florian Quèze [:florian]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #9)
> (In reply to Florian Quèze [:florian] [:flo] from comment #8)

> > - delete logs permanently at shutdown if the user quits Thunderbird before
> > the timer finishes.
> I am doing it in the closeTab(). Will that not work?

Have you verified that closeTab() is called if the user quits Thunderbird with the Chat tab still open?


> > > +var timer = Components.classes["@mozilla.org/timer;1"]
> > > +                      .createInstance(Components.interfaces.nsITimer);
> > 
> > Any reason for using a timer here rather than just a setTimeout?
> 

> I could do it with keeping track of the timeout IDs but that would require
> maintaining another set.

How is it different to keep a timeout ID to call clearTimeout on it, or to keep a reference to the timer object so that you can call the cancel method on it? I don't see why the former would require another set; it looks to me like both cases are just a single variable.

By the way, I hadn't paid attention to this yet, but the variable should have a name more specific than "timer", or not be at the top level. chat-messenger-overlay.js isn't an isolated scope like a module or a JS component would be; it's running directly inside Thunderbird's main XUL window.

Comment 11

[Josiah Bruner [:jsbruner]]

Comment on attachment 8355780 [details] [diff] [review]
Patch v2

Review of attachment 8355780 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately this patch does not apply cleanly on trunk anymore, almost certainly due to the recent Instantbird -> cc merge.

Comment 12

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.1

Fixed the patch.
Made the changes suggested by :Florian.
Indexing related changes pending.
If there are other nits/changes to be made apart from the indexing thing, please let me know.

Thanks.

Comment 13

[Suyash Agarwal (:sshagarwal)]

(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> Have you verified that closeTab() is called if the user quits Thunderbird
> with the Chat tab still open?
Ya.

Comment 14

[Patrick Cloke [:clokep]]

Comment on attachment 8357746 [details] [diff] [review]
Patch v2.1

Review of attachment 8357746 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good. I had a couple of nits, but I don't think they'd stop you from moving on to looking at gloda.

::: chat/components/src/logger.js
@@ +718,5 @@
>                                 aGroupByDay);
>    },
> +  removeLog: function(aLogPath) {
> +    OS.File.remove(aLogPath);
> +  },

Do we care if there are any errors? This would currently just eat them all. I think we'd at least want to throw a warning/error into the error console? Florian, what're your thoughts?

::: mail/components/im/content/chat-messenger-overlay.js
@@ +14,5 @@
> +var logsMarkedForDeletion = new Set();
> +
> +// Notifying the user of deletion of logs, giving a chance to undo
> +// his/her action.
> +var timeoutID;

I think Florian already mentioned this once, but be careful about scope, this name is pretty vague. Maybe "logDeletionTimeoutId" or something.

@@ +35,5 @@
> +      logsMarkedForDeletion.clear();
> +      chatHandler.refreshLogList();
> +    }
> +  }];
> +  if (notificationBox) {

What if there isn't a notification box? What does that mean?

@@ +42,5 @@
> +                                         "logDeletionNotification",
> +                                         "null",
> +                                         notificationBox.PRIORITY_WARNING_MEDIUM,
> +                                         buttons);
> +    if (timeoutID != -1) {

When would this be set to -1? It doesn't look like it ever is above. Please include a comment saying what this box does.

@@ +44,5 @@
> +                                         notificationBox.PRIORITY_WARNING_MEDIUM,
> +                                         buttons);
> +    if (timeoutID != -1) {
> +      window.clearTimeout(timeoutID);
> +      timeoutID = window.setTimeout(deleteLogs, 30000);

Please include a comment saying how long this is (30 seconds? I think we decided on 2 minutes? Or am I doing the math wrong? Hence why I want a comment. :))

@@ +57,5 @@
> +  if (notificationBox && notification)
> +    notificationBox.removeNotification(notification);
> +  for (let log of logsMarkedForDeletion)
> +    imServices.logs.removeLog(log);
> +  logsMarkedForDeletion.clear();

What happens here if I request logs be deleted and then while they're deleting mark another log to be deleted?

@@ +524,5 @@
> +    let selection = this._treeView.selection;
> +    let currentIndex = selection.currentIndex;
> +    // The current (focused) row may not be actually selected...
> +    if (!selection.isSelected(currentIndex))
> +      return;

Is this disallowing someone from right clicking on something that isn't the actual selection? That seems odd.

@@ +526,5 @@
> +    // The current (focused) row may not be actually selected...
> +    if (!selection.isSelected(currentIndex))
> +      return;
> +
> +    let logTree = document.getElementById("logTree");

I think this is unused?

@@ +528,5 @@
> +      return;
> +
> +    let logTree = document.getElementById("logTree");
> +    let treeView = this._treeView;
> +    let rowMap = treeView._rowMap;

Nit: You only use treeView and rowMap once, I don't think we need a separate variables.

@@ +546,5 @@
> +    // Show the notification for deleted conversation(s).
> +    showDeletionNotification();
> +  },
> +
> +  // Function to refresh the shown previous conversations list.

This comment does weird things with tenses. Maybe just "Refresh the list of previous conversations."

::: mail/locales/en-US/chrome/messenger/chat.dtd
@@ +20,5 @@
>  <!ENTITY chat.previousConversations    "Previous Conversations:">
>  <!ENTITY chat.ongoingConversation      "Ongoing conversation">
>  
> +<!ENTITY deleteConversationCmd.label   "Forget this conversation">
> +<!ENTITY deleteConversationCmd.accesskey "F">

If we're now calling this "forget", let's update the keys to match this nomenclature.

::: mail/locales/en-US/chrome/messenger/chat.properties
@@ +97,5 @@
> +# deletion of previous log(s).
> +# #1 is the number of conversations deleted.
> +logsDeleted=#1 conversation deleted;#1 conversations deleted
> +undoDeletion=Undo
> +undoDelete.accesskey=U

"Delete" doesn't match the nomenclature used in chat.dtd. We should be consistent.

Comment 15

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #14)

> > +  removeLog: function(aLogPath) {
> > +    OS.File.remove(aLogPath);
> > +  },
> 
> Do we care if there are any errors? This would currently just eat them all.
> I think we'd at least want to throw a warning/error into the error console?
> Florian, what're your thoughts?

I think reporting to the error console unhandled promise errors is the default behavior since bug 720628 got fixed.

Comment 16

[Josiah Bruner [:jsbruner]]

Comment on attachment 8357746 [details] [diff] [review]
Patch v2.1

Review of attachment 8357746 [details] [diff] [review]:
-----------------------------------------------------------------

So this works well, but there are a couple of issues:

1. There is a significant gap on the left side of the notification bar before the text. We should probably put a trash icon there.

2. The bar disappears after 30 seconds, but I think we want about two minutes.

ui-r- until those things are fixed.

Sorry about the delay.

Comment 17

[Suyash Agarwal (:sshagarwal)]

(In reply to Josiah Bruner [:JosiahOne] from comment #16)
> So this works well, but there are a couple of issues:
> 
> 1. There is a significant gap on the left side of the notification bar
> before the text. We should probably put a trash icon there.
We think alike :)
> 2. The bar disappears after 30 seconds, but I think we want about two
> minutes.
I think, 2 minutes is too much time, and 30 seconds should be fine/enough, isn't it?

Comment 18

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Addressed a few nits.
Changing the option name from "Delete" to "Forget" will require renaming everything from delet* to Forget* (for consistency), so please let me know if that's desired.
Trash icon isn't implemented (hard to say but I couldn't find a trach icon image).

Comment 19

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (indexing) v1

This patch tries to cover the indexing part. To be applied over core patch.
There are a few TODO comments within the patch. I would like a few ideas on them before I implement them.
And please let me know if this works well.

Comment 20

[Josiah Bruner [:jsbruner]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #18)
> Created attachment 8365566 [details] [diff] [review]
> Patch v3
> 
> Addressed a few nits.
> Changing the option name from "Delete" to "Forget" will require renaming
> everything from delet* to Forget* (for consistency), so please let me know
> if that's desired.
> Trash icon isn't implemented (hard to say but I couldn't find a trach icon
> image).

The trash icon is in the folder pane graphic. That said, using junk.png would work fine.

-- OS X --
http://mxr.mozilla.org/comm-central/source/mail/themes/osx/mail/icons/junk.png

-- Windows --
Aero: http://mxr.mozilla.org/comm-central/source/mail/themes/windows/mail/icons/junk-aero.png
Other: http://mxr.mozilla.org/comm-central/source/mail/themes/windows/mail/icons/junk.png

-- Linux --
http://mxr.mozilla.org/comm-central/source/mail/themes/linux/mail/icons/junk.png

As "Delete" vs "Forget" goes; I think "Delete" should be fine since "Forget" indicates something that can usually be re-learned, which can not happen here. Example: One can "forget" a shortcut, since you can re-learn that shortcut, but one "deletes" mail, since it can not be recovered (in theory).

Comment 21

[Richard Marti (:Paenglab)]

Would we really use the junk icon for a deletion operation? This would mix the meaning of the icons. I think it's better to set a id for the icon and then select the correct trash icons through CSS. This would also be friendlier to 3rd party themes.

Comment 22

[Patrick Cloke [:clokep]]

Comment on attachment 8365568 [details] [diff] [review]
Patch (indexing) v1

Review of attachment 8365568 [details] [diff] [review]:
-----------------------------------------------------------------

I really don't know anything about gloda, so I probably shouldn't review this.

Comment 23

[Patrick Cloke [:clokep]]

Comment on attachment 8365566 [details] [diff] [review]
Patch v3

Review of attachment 8365566 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like it's getting close! Please ask me if you have any questions about my comments. Remember that me asking a question in this bug doesn't necessarily mean any code change is required. I might just be trying to understand!

::: mail/components/im/content/chat-messenger-overlay.js
@@ +16,5 @@
> +// Notifying the user of deletion of logs, giving a chance to undo
> +// his/her action.
> +// Initializing timeoutID to -1 to know if we are setting the timeout
> +// for the first time.
> +var logDeletionTimeoutID = -1;

I'd prefer null or just leaving it undefined.

@@ +38,5 @@
> +      chatHandler.refreshLogList();
> +    }
> +  }];
> +  // Proceed if we have a notification box in the window where we can
> +  // append our notification, else continue.

I understand that you put this in response to a question I had in the bug, but I don't find this comment useful. It's obvious from the code, you didn't answer my actual question though: when do we expect there not to be a notification box or is this just defensive programming? If this is an unexpected condition, I feel like some sort of error would be wanted...

@@ +47,5 @@
> +                                         "null",
> +                                         notificationBox.PRIORITY_WARNING_MEDIUM,
> +                                         buttons);
> +    // clear last timeout if there is one.
> +    if (logDeletionTimeoutID != -1)

If you use null or undefined above this just becomes |if (logDeletionTimeoutID)|.

@@ +50,5 @@
> +    // clear last timeout if there is one.
> +    if (logDeletionTimeoutID != -1)
> +      window.clearTimeout(logDeletionTimeoutID);
> +
> +    // setting timeout of 60 seconds.

Nit: Capital s and proper grammar, please: "Set a timeout of 60 seconds."

@@ +65,5 @@
> +  if (notificationBox && notification)
> +    notificationBox.removeNotification(notification);
> +  // Copying logsMarkedForDeletion in another set to
> +  // avoid deletion of the logs the user is marking
> +  // for deletion while we are here.

Nit: The width of this comment is very odd.

Also, can we reword this a bit? "Copy logsMarkedForDeletion to avoid immediate deletion of any new logs the user attempts to delete."

@@ +66,5 @@
> +    notificationBox.removeNotification(notification);
> +  // Copying logsMarkedForDeletion in another set to
> +  // avoid deletion of the logs the user is marking
> +  // for deletion while we are here.
> +  let _toBeDeletedLogs = new Set(logsMarkedForDeletion);

Nit: I'd prefer "logsToDelete", no need to prefix with a _.

@@ +70,5 @@
> +  let _toBeDeletedLogs = new Set(logsMarkedForDeletion);
> +  logsMarkedForDeletion.clear();
> +  for (let log of _toBeDeletedLogs)
> +    imServices.logs.removeLog(log);
> +  _toBeDeletedLogs.clear();

No need to do this, the Set is in the local scope.

@@ +202,5 @@
>      chatHandler._updateSelectedConversation();
>      chatHandler._updateFocus();
>    },
>    closeTab: function(aTab) {
> +    deleteLogs();

Maybe we should include a comment above this (this sounds dangerous to someone who doesn't know the code!): "If there are logs pending deletion, delete them before closing the tab."

@@ +543,5 @@
> +    let logGroup = this._treeView._rowMap[currentIndex];
> +    if (!logGroup)
> +      return;
> +
> +    // See whether the selected row is a group and if yes, it has children.

Can we tweak this a bit? "Check whether the selected row is a group and if it has children."

::: mail/locales/en-US/chrome/messenger/chat.properties
@@ +92,5 @@
>  log.yesterday=Yesterday
>  log.lastWeek=Last Week
>  log.twoWeeksAgo=Two Weeks Ago
> +# LOCALIZATION NOTE(logsDeleted):
> +# String message to be shown on the notification after deletion of previous log(s).

Florian would have a better idea than me, but I don't know how helpful this comment would be. (My thought on reading it would be "What notification?") I'm usually pretty bad at understanding what needs to go into these comments, however.

Comment 24

[Suyash Agarwal (:sshagarwal)]

(In reply to Richard Marti (:Paenglab) from comment #21)
> Would we really use the junk icon for a deletion operation? This would mix
> the meaning of the icons. I think it's better to set a id for the icon and
> then select the correct trash icons through CSS. This would also be
> friendlier to 3rd party themes.

Can you please tell me which trash icon(s) I can use?

Comment 25

[Richard Marti (:Paenglab)]

If you want to use the "Trash" icon you can use the rules treechildren::-moz-tree-image(folderNameCol, specialFolder-Trash) in folderPane.css is using. If you want to use the "delete" icon you can use the rules from .delete-button in primaryToolbar.css. For the 16px icons on Linux and XP you need the rules for toolbar[iconsize="small"].

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8365568 [details] [diff] [review]
Patch (indexing) v1

You're headed in the right direction here.  Main comments:

- In databind.js, argument names should be more generic and not explicitly based on the IM conversation terminology.  Super specifically, only the IM schema has a 'path' column, so you definitely should not be hard-coding deletion to depend on that.  If the IM code has access to the gloda 'id' for the conversation, you should just delete using that.  In the event that you only have access to the path, adding support to delete based on another key seems quite reasonable, but that should be done by having it be based on the schema already fed in (possibly by creating a helper method) or explicit caller arguments.  You'll want to make sure there's an index on the value too; path currently does not have an index.

Note: While it's pretty easy to add an index to the schema def (see http://dxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/datastore.js#800 for example), this will not update existing databases.  Schema migration is required.  Or just being okay with users with existing databases having a slower experience.  If the number of chat logs is going to be low (less than a hundred), inefficient deletion is not a huge deal.  Otherwise you'll want an ALTER TABLE in there and such.  This is a big argument for just using the id if you have it in the first place.

- As discussed on IRC before, we want to make sure that any attributes get deleted too.  This can be done with deletion triggers, but I don't think we insert them currently.  (My memory of the discussion and my investigations is somewhat faded now, although I should have logs.)  We need the triggers; if we do have them, a comment indicating we're using them is definitely required.  (Reiterating, there are some 'triggers' listed in the existing schemas.  But these are lies and not used!  See http://dxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/datastore.js#800 for actual schema creation.)

Comment 27

[Suyash Agarwal (:sshagarwal)]

(In reply to Andrew Sutherland (:asuth) from comment #26)
Thanks for the quick review.

> Super specifically, only the IM
> schema has a 'path' column, so you definitely should not be hard-coding
> deletion to depend on that.  If the IM code has access to the gloda 'id' for
> the conversation, you should just delete using that.
Ya, I found that there is 'id' column for imConversations table [1], but I am unable
to access it from the code where the deletion takes place. In fact, I was unable
to find the 'id' of the deleted chat log anywhere.

> In the event that you
> only have access to the path, adding support to delete based on another key
> seems quite reasonable, but that should be done by having it be based on the
> schema already fed in (possibly by creating a helper method) or explicit
> caller arguments.  You'll want to make sure there's an index on the value
> too; path currently does not have an index.
Sorry, I didn't understand this part; completely, I might add.

> Note: While it's pretty easy to add an index to the schema def (see
> http://dxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/
> datastore.js#800 for example), this will not update existing databases. 
> Schema migration is required.  Or just being okay with users with existing
> databases having a slower experience. This is a big
> argument for just using the id if you have it in the first place.
> 
I was wondering if there is any possible way by which I can get the 'id' for
the conversation from the 'path' but I couldn't find 'id' of a conversation
from anywhere, so I couldn't write this as well.

Moreover, as the chat log paths will be unique, can I use paths itself in a manner
similar to [2]?

Thanks.

[1] http://dxr.mozilla.org/comm-central/source/mail/components/im/modules/index_im.js#123
[2] http://dxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/datastore.js#846

Comment 28

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.2

Tried to add Trash icon to the notification. No success so far. Can you please let me know what needs to be done?

Thanks.

Comment 29

[Jonathan Protzenko [:protz]]

Comment on attachment 8365568 [details] [diff] [review]
Patch (indexing) v1

Review of attachment 8365568 [details] [diff] [review]:
-----------------------------------------------------------------

Hi,

Regarding your comments, yes, observers seem like the way to go. Although I'm not sure it's worth the extra trouble updating search results as conversations get deleted; maybe a message such as "we can't display this conversation since it may have been deleted in the meanwhile" would be acceptable. You'd have to figure that out with owners of the IM module.

Generally speaking, I concur with Andrew's comments.
- The "attributes" table contains triples that (probably) point to the conversation; these need to be deleted as well.
- Also, we may want to be more generic and not hardcode the deletion on the specific path attribute, so that it can be used for other purposes. Splitting on commas seems very specific to that particular use case that you're addressing; I'm sure being able to delete an array of conversation ID's is better. The code for email deletion is in datastore.js (the statements) and index_msg.js:3219 (the actual logic). Are you not able to leverage already-existing code for IM?
- I'm sure someone mentioned that already but such a patch is likely to require tests to make sure deletions happen properly _and_ don't mess up anything.

Keep up the good work!

Comment 30

[Suyash Agarwal (:sshagarwal)]

(In reply to Jonathan Protzenko [:protz] from comment #29)
> Hi,

Hi, thanks for the review.
> - Also, we may want to be more generic and not hardcode the deletion on the
> specific path attribute, so that it can be used for other purposes.
> Splitting on commas seems very specific to that particular use case that
> you're addressing; I'm sure being able to delete an array of conversation
> ID's is better. The code for email deletion is in datastore.js (the
> statements) and index_msg.js:3219 (the actual logic). Are you not able to
> leverage already-existing code for IM?

Ya, I am unable to find the id for the log I am deleting. The nsILog only gives me the path.

Comment 31

[Richard Marti (:Paenglab)]

Comment on attachment 8385512 [details] [diff] [review]
Patch v3.2

I clear the f? as I can't test with the actual patches getting a error before showing the notification and you figured it out to show the icon.

Comment 32

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.3

Please let me know if this works for you now.
The image on the notification appears on Linux but not on windows.

Comment 33

[Richard Marti (:Paenglab)]

Comment on attachment 8386679 [details] [diff] [review]
Patch v3.3

Review of attachment 8386679 [details] [diff] [review]:
-----------------------------------------------------------------

With my changes in comments the icon is shown.

::: mail/components/im/content/chat.css
@@ +119,5 @@
>  }
> +
> +.messageImage[value="logDeletionNotification"] {
> +  list-style-image: url("chrome://messenger/skin/icons/folder-pane.png");
> +  -moz-image-region: rect(176px 16px 192px 0px) !important;

Is the !important needed? I don't think so. If yes, you need this to add to the Windows rule.

::: mail/themes/windows/mail/chat-aero.css
@@ +226,5 @@
>    list-style-image: url("chrome://messenger/skin/icons/mail-toolbar.png");
>    -moz-image-region: rect(1px, 395px, 17px, 379px);
>  }
> +
> +.messageImage[value="logDeletionNotification"] {

Please remove the whole rule. The one in /windows/mail/chat.css is included in chat-aero.css. And folder-aero.png is renamed to folder.png in omni.ja.

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Suyash Agarwal (:sshagarwal) away till 7th of March from comment #27)
> (In reply to Andrew Sutherland (:asuth) from comment #26)
> > In the event that you
> > only have access to the path, adding support to delete based on another key
> > seems quite reasonable, but that should be done by having it be based on the
> > schema already fed in (possibly by creating a helper method) or explicit
> > caller arguments.  You'll want to make sure there's an index on the value
> > too; path currently does not have an index.
> Sorry, I didn't understand this part; completely, I might add.

I propose doing something like the following:

  objDeleteByColumn: function(columnName, columnValue) {
    let sql = "DELETE FROM " + this._tableName + " WHERE ?1 = ?2";
    ...
  }

The key things being that you really want to have an index on whatever columnName you are using, so "path" in this case.  And you need to have a trigger so that the deletion cascades and deletes the attributes.  And you need to make sure to use GlodaCollection.itemsDeletedByAttribute with a filtering function that checks if the path is the path(s) you are deleting.

Comment 35

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.5

Made some corrections to the chat logs deletion process.
The icon appears on the notification bar.
Made a UX change:
Whenever the user switches to another contact (after marking a few logs for deletion, with the notification bar to undo visible), the conversations are deleted and the notification bar goes away.
Discussed with Paenglab on the channel.

Thanks.

Comment 36

[aleth [:aleth]]

Comment on attachment 8388784 [details] [diff] [review]
Patch v3.5

Review of attachment 8388784 [details] [diff] [review]:
-----------------------------------------------------------------

I think you can request review on the next iteration.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +11,5 @@
> +
> +// A global set holding the log files marked
> +// for deletion.
> +var logsMarkedForDeletion = new Set();
> +var tempLogStore = [];

tempLogStore needs a comment and a more descriptive name.
Could it not be a property of chatLogTreeView?

@@ +55,5 @@
> +    logDeletionTimeoutID = window.setTimeout(deleteLogs, 60000);
> +  }
> +}
> +
> +function deleteLogs() {

Can we call this something more specific as it is called in various places where a reader may think "what deleted logs?". e.g. removeLogsMarkedForDeletion

@@ +59,5 @@
> +function deleteLogs() {
> +  tempLogStore = [];
> +  if (logsMarkedForDeletion.size == 0)
> +    return;
> +  window.clearTimeout(logDeletionTimeoutID);

You also want to reset logDeletionTimeoutID.

@@ +289,5 @@
>  
>    saveTabState: function(aTab) { }
>  };
>  
> +function shouldShowDeletionContext() {

I believe onpopupshowing handlers control whether the menu is to open or not by returning true/false? Use that and inline the code in the xul file.

@@ +461,5 @@
>      let date = dts.FormatDate("", dts.dateFormatShort, aDate.getFullYear(),
>                                aDate.getMonth() + 1, aDate.getDate());
>      return bundle.getFormattedString("dateTime", [date, time]);
>    },
> +  

Check your editor settings to remove trailing whitespace ;)

@@ +569,5 @@
> +
> +  // Refresh the list of previous conversations.
> +  // @param aRow        -1 to revert deletion changes
> +  //                    rowNumber otherwise
> +  refreshLogList: function(rowMarkedForDeletion) {

If this is really only called from the function above, you might as well inline it for easier legibility.

@@ +1268,5 @@
> +    }
> +    tempLogStore = [];
> +  },
> +
> +  recursivelyAddToMap: function chatHandler_recursivelyAddToMap(aChild, aNewIndex) {

Instead of overriding this, would it make sense to add the "hidden" functionality to jsTreeView? (A question to ask the appropriate reviewer for jsTreeView - have a look who last reviewed that code.)

Comment 37

[Suyash Agarwal (:sshagarwal)]

(Quoting aleth from comment #36)
> @@ +1268,5 @@
> > +    }
> > +    tempLogStore = [];
> > +  },
> > +
> > +  recursivelyAddToMap: function chatHandler_recursivelyAddToMap(aChild, aNewIndex) {
> 
> Instead of overriding this, would it make sense to add the "hidden"
> functionality to jsTreeView? (A question to ask the appropriate reviewer for
> jsTreeView - have a look who last reviewed that code.)

Comment 38

[:aceman]

I think a generic way to handle hidden rows in the mailnews jsTreeView could be nice. However if chat will be the single user it depends on a mailnews reviewer whether this will pass. You should try Neil. Thundebird also uses a copy of this functionality in folderPane.js and I am not sure it uses the mailnews/jsTreeView for anything. Maybe for some other small trees (like addressbook?).

Comment 39

[Suyash Agarwal (:sshagarwal)]

(Quoting to aleth from comment #36)
> ::: mail/components/im/content/chat-messenger-overlay.js
> > +  recursivelyAddToMap: function chatHandler_recursivelyAddToMap(aChild, aNewIndex) {
> 
> Instead of overriding this, would it make sense to add the "hidden"
> functionality to jsTreeView? (A question to ask the appropriate reviewer for
> jsTreeView - have a look who last reviewed that code.)

I have used this._hidden boolean attribute in chatlogTreeItem and chatLogTreeGroupItem to note if the conversation is marked for deletion by the user. So, instead of storing them in a separate array and then iterating over it to remove the entries marked for deletion and rebuilding the tree (O(n^2) ? ), I am simply removing the entries with their hidden attribute set to true from the treeView and rebuilding it. Should I add this "hidden" attribute filtering functionality in recursivelyAddToMap() to jsTreeView.js in mailnews/ ? or it can stay here specifically?

Thanks.

Comment 40

[aleth [:aleth]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #39)

My suggestion was to consider adding the hidden boolean in jsTreeView.js and then to have code in chatlogTreeView which uses it to hide the deleted entries. This makes it easy later to reuse this functionality should we want to temporarily hide certain entries for other reasons (e.g. to filter the list based on a search string).

Comment 41

[[e-mail]]

(From update of attachment 8388784 [details] [diff] [review])
>+  reflectDeletionChanges: function reflectDeletionChanges(aIndex) {
>+    let item = document.getElementById("contactlistbox").selectedItem;
>+    if (item && item.contact)
>+      item = item.contact;
>+    this._rowMap[aIndex]._hidden = true;
>+    tempLogStore.push([aIndex, this._rowMap[aIndex], item ? item : ""]);
>+    let count = 1;
>+    // See if we have to blow an entire conversation group.
>+    if (this._rowMap[aIndex].children.length) {
>+      let level = this._rowMap[aIndex].level;
>+      let row = aIndex + 1;
>+      while (row < this._rowMap.length && this._rowMap[row].level > level) {
>+        row++;
>+      }
>+      count = row - aIndex;
>+      this._rowMap.splice(aIndex, count);
>+    } else {
>+      this._rowMap.splice(aIndex, 1);
>+    }
>+    if (this._tree) {
>+      this._tree.rowCountChanged(aIndex + 1, -count);
>+      this._tree.invalidateRow(aIndex);
>+    }
>+  },
I don't understand; you're setting the item as hidden, but then immediately removing it from the row map anyway.

>+      this._rowMap.splice(aIndex, 1);
Nit: count is 1 at this point, so you can move the splice outside the if.
(You could go further and replace row with aIndex + count or count with row - aIndex.)

Comment 42

[Suyash Agarwal (:sshagarwal)]

(In reply to [e-mail] from comment #41)
> (From update of attachment 8388784 [details] [diff] [review])
> >+  reflectDeletionChanges: function reflectDeletionChanges(aIndex) {
> >+    let item = document.getElementById("contactlistbox").selectedItem;
> >+    if (item && item.contact)
> >+      item = item.contact;
> >+    this._rowMap[aIndex]._hidden = true;
> >+    tempLogStore.push([aIndex, this._rowMap[aIndex], item ? item : ""]);
> >+    let count = 1;
> >+    // See if we have to blow an entire conversation group.
> >+    if (this._rowMap[aIndex].children.length) {
> >+      }
> >+      count = row - aIndex;
> >+      this._rowMap.splice(aIndex, count);
> >+    } else {
> >+      this._rowMap.splice(aIndex, 1);
> >+    }
> >+    if (this._tree) {
> >+      this._tree.rowCountChanged(aIndex + 1, -count);
> I don't understand; you're setting the item as hidden, but then immediately
> removing it from the row map anyway.
Ya. Actually I don't understand how to hide the values from the view once marked hidden. I think, the tree needs to be rebuilt anyway. Also I don't know if rowCountChanged() triggers a complete tree rebuild. If yes, I could go with implementing a rebuild here (by not including the hidden rows) instead of removing the values here; else, what should I do?

Comment 43

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (indexing) v2

I have tried to make the suggested changes. Please let me know if that's not correct.

(In reply to Jonathan Protzenko [:protz] from comment #29)
> Regarding your comments, yes, observers seem like the way to go. Although
> I'm not sure it's worth the extra trouble updating search results as
> conversations get deleted; maybe a message such as "we can't display this
> conversation since it may have been deleted in the meanwhile" would be
> acceptable. You'd have to figure that out with owners of the IM module.

But will that not be inconsistent? Because when the user marks the log(s) for deletion, we aren't deleting them right away, so, we have the logs and we are showing them in the search results just we aren't allowing them to be viewed. Whereas, the user thinks that the logs are gone. So, shouldn't we hide them from the search results too?

Because showing the error message for the logs marked for deletion would require almost the same amount of work (looking for the selected log in the deleted logs list, if the log exists, we show the error else we show the log.. or ..we look for the logs in the deleted logs list and remove them from the logs list returned by getCollection()).
Please correct me if I am making some wrong interpretations/assumptions.

Thanks.

Comment 44

[Florian Quèze [:florian]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #43)
> So, shouldn't we hide them from the search results too?

Are you talking about hiding them from already displayed search results, or from new searches made between the time when a log was marked as deleted and the time when it was actually deleted?

Comment 45

[Suyash Agarwal (:sshagarwal)]

(In reply to Florian Quèze [:florian] [:flo] from comment #44)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #43)
> > So, shouldn't we hide them from the search results too?
> 
> Are you talking about hiding them from already displayed search results, or
> from new searches made between the time when a log was marked as deleted and
> the time when it was actually deleted?

Can we hide them from the already displayed search results? I think, in this case we will have to show the error message.

But I am talking about the new searches made between the time when a log was marked as deleted and the time it will actually be deleted.

Comment 46

[Andrew Sutherland [:asuth] (he/him)]

As it relates to gloda and searching; gloda already explicitly provides life-cycle for removal of items with events generated appropriately.  If you have a deferred deletion model, this is similar to how gloda's handling of messages already work.  In general I think it would be inadvisable to modify the faceted search UI code to duplicate already-available mechanism.  If you want to be able to 'undo' the deletion, then the right thing gloda-wise is to:
- Add a column that indicates the chatlog is deleted
- When you query for chatlogs, filter on making sure the chatlog is not deleted.
- Generate the collection removal notification immediately upon initial deletion
- Later on, when you actually delete, trigger the actual removal of the rows from the database.

If you don't want to undo the deletion, then you should just trigger the gloda deletion immediately and be done with it.  The operations all happen asynchronously.

Comment 47

[[e-mail]]

(In reply to Suyash Agarwal from comment #42)
> Ya. Actually I don't understand how to hide the values from the view once
> marked hidden. I think, the tree needs to be rebuilt anyway. Also I don't
> know if rowCountChanged() triggers a complete tree rebuild. If yes, I could
> go with implementing a rebuild here (by not including the hidden rows)
> instead of removing the values here; else, what should I do?

Sorry, I'm still unclear as what you're trying to achieve here. What's a hidden row, and why does a row need to be hidden rather than removed?

Comment 48

[Suyash Agarwal (:sshagarwal)]

(In reply to [e-mail] from comment #47)
> Sorry, I'm still unclear as what you're trying to achieve here. What's a
> hidden row, and why does a row need to be hidden rather than removed?

I was wondering of the use-case where a user deletes some logs, and the undoes the operation. In this case, the logs are to be gone from the list (when deleted) and then re-appear when undone. So, will removing and re-adding be efficient? If not, can we somehow set an attribute of a row so that it isn't visible in the list; and when the user undoes his/her deletion operation, we unset that attribute so that the rows are visible again.

Comment 49

[[e-mail]]

(In reply to Suyash Agarwal from comment #48)
> (In reply to [e-mail] from comment #47)
> > Sorry, I'm still unclear as what you're trying to achieve here. What's a
> > hidden row, and why does a row need to be hidden rather than removed?
> 
> I was wondering of the use-case where a user deletes some logs, and the
> undoes the operation. In this case, the logs are to be gone from the list
> (when deleted) and then re-appear when undone. So, will removing and
> re-adding be efficient?

While you can in theory avoid rebuilding the whole tree if you know the index at which you need to reinsert the row(s), you can't assume it was the index at which they were deleted, as it's really easy to change the target index just by collapsing a previous parent. Your current code looks as if it will only work in the limited case of immediately undoing a deletion of a single row.

The trick the mailnews trees use is to rebuild the tree anyway, but having done that they can then find the inserted row so they now know the correct index.

I notice that you had overriden recursivelyAddToMap in case you needed to avoid re-adding the rows that you had previously deleted which would help you here.

Don't forget to set the selection to something useful after the undo (typically either the undeleted item or the previously selected item).

As for your code, for some reason your deletion code changes the count rows after the row to be deleted, perhaps you copied that bit from the twisty collapse code, which does need to do this, but you should change the row being deleted. It's possible that deleting rows will cause the treelines to be mispainted but there's no easy solution other than invalidating the whole tree. (SeaMonkey's addressbook only bothers to repaint the parent, in case you deleted the last child and it is no longer a container.)

Comment 50

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #48)
> If not, can we somehow set an attribute of a row so
> that it isn't visible in the list; and when the user undoes his/her deletion
> operation, we unset that attribute so that the rows are visible again.

I think you can set a property on a treerow and then style it in css like moz-tree-row(your-property) { display: none; } .

Comment 51

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #50)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #48)
> > If not, can we somehow set an attribute of a row so
> > that it isn't visible in the list; and when the user undoes his/her deletion
> > operation, we unset that attribute so that the rows are visible again.
> 
> I think you can set a property on a treerow and then style it in css like
> moz-tree-row(your-property) { display: none; } .

Will this also shift up the rows below the row being marked hidden ? Or will it leave a blank gap between them?

Comment 52

[Richard Marti (:Paenglab)]

This will shift them up.

Comment 53

[:aceman]

I hope it will just hide (collapse) them visually but they will be normally accessible from JS and the DOM.

Comment 54

[Jonathan Protzenko [:protz]]

Comment on attachment 8394921 [details] [diff] [review]
Patch (indexing) v2

Hi,

I currently have no time to properly review this patch. Rather than give an automatic r+, I prefer to un-assign myself from this review.

If no one performed the review in the meanwhile, please try again in 4 to 6 weeks to r? me, as things should have settled by then.

Thanks for understanding,

~ jonathan

Comment 55

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8394921 [details] [diff] [review]
Patch (indexing) v2

Sorry, thought I had indicated this previously on the bug and cleared the flag, but it looks like I didn't.  (I must have started a comment and gotten distracted.)  I'm no longer a Thunderbird or MailNews reviewer.  I try and provide feedback since I know a lot about pieces of Thunderbird (especally gloda), but I'm not able to do the required legwork time-wise to be confident about the side effects of changes and am definitely not planning to be there to help fix any bugs/regressions from landing new code.  Since :protz is swamped for time, the best option is likely to find an owner/peer for the parent module to review or have them defer to someone else interested in maybe being more involved in the code in question.

Comment 56

[Suyash Agarwal (:sshagarwal)]

(In reply to Jonathan Protzenko [:protz] from comment #54)
> I currently have no time to properly review this patch. Rather than give an
> automatic r+, I prefer to un-assign myself from this review.

(In reply to Andrew Sutherland (:asuth) from comment #55)
> I'm no longer a Thunderbird or MailNews reviewer.  I try and
> provide feedback since I know a lot about pieces of Thunderbird (especally
> gloda), but I'm not able to do the required legwork time-wise to be
> confident about the side effects of changes and am definitely not planning
> to be there to help fix any bugs/regressions from landing new code.

Fine, thanks for your time.

Comment 57

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8394921 [details] [diff] [review]
Patch (indexing) v2

Will you please review this?

Comment 58

[[e-mail]]

Comment on attachment 8394921 [details] [diff] [review]
Patch (indexing) v2

Sorry, I know nothing about gloda.

Comment 59

[Suyash Agarwal (:sshagarwal)]

Will you please review the gloda patch for this bug?

Thanks.

Comment 60

[Kent James (:rkent)]

"Will you please review the gloda patch for this bug?"

Really protz is the main person competent to do that these days. My gloda work has been quite minimal, and I'm afraid that my review would be less competent than the cursory review that protz decided not to do. I really think you have little choice but to wait until he is free.

Comment 61

[Jonathan Protzenko [:protz]]

Ok, I'll find some time to perform a proper review of this by the end of the week. Please don't r? me, I have a reminder already.

Thanks,

~ jonathan

Comment 62

[Jonathan Protzenko [:protz]]

Comment on attachment 8394921 [details] [diff] [review]
Patch (indexing) v2

Review of attachment 8394921 [details] [diff] [review]:
-----------------------------------------------------------------

Hi,

First of all, I strongly suggest you move the Gloda part of your patch onto a separate bug; it really is a nightmare to separate the Gloda-related comments from the rest of the discussion :-).

From what I gathered, this patch is still more of a sketch rather than a final, polished patch, so I'm going to stay at a high-level and focus on the tasks that remain to be done, rather than nitpick details (but rest assured that I _will_ do that once the patch moves forward ;-)).

1. Proper deletion in Gloda (comment #26, #29, #34, tangentially #43).

The first concern that I have is that in databind.js:objDelete, you're still hardcoding the fact that you're deleting based on a path. As Andrew and I pointed out, the path column is specific to IM code, so I really do believe that this is something that ought to live in IM code. Plus, your objDelete function is bizarre.

a) You wrote:

  objDelete: function(aValues, aTableName, aColumnName = null) {

But what your implementation does, albeit in a much more convoluted way, is:

  objDelete: function(aValues, aTableName, aColumnName = "path") {
    objDeleteByColumn(aValues, aTableName, aColumnName);
  }

Also there's a lot of code duplication between objDelete and objDeleteByColumn.

I suggest you get rid of objDelete _completely_ and only expose objDeleteByColumn. Indeed,

b) You only ever do:

  Gloda.deleteNounItem(IMConversationNoun, aData.split(","), "path");

which then calls onto:

  aItemNounDef.objDelete.call(aItemNounDef.datastore,
		aValues, aItemNounDef.tableName, aColumnName);

Meaning that your semantics of "default-to-path-for-the-column-name" are never actually used in practice.

Just delete objDelete, use objDeleteByColumn everywhere, you're providing the column name anyway.

c) Regarding the actual deletion, you seem to have done a proper job of defining a new deleteNounItem function in Gloda. While this seems like a solid approach, have you:
- made absolutely sure that you can't first get the ids of the conversations from their paths? because
- it may be expensive to iterate on every possible conversation repeatedly (this is in a for loop!) just to delete one value among all possible ones. Have you considered a better approach?

d) You seem to have added a trigger, but I recall Andrew mentioning that these are actually lies and that they're not actually implemented. Can you explain to me how your addition is supposed to make triggers work?


I'm posting this comment, to make sure that it does not end up being lost. I'll add a second one for the topic of observers and how to update search results. Please by all means create a new bug and quote this comment at the beginning.

Thanks,

jonathan

Comment 63

[Jonathan Protzenko [:protz]]

Comment on attachment 8394921 [details] [diff] [review]
Patch (indexing) v2

Review of attachment 8394921 [details] [diff] [review]:
-----------------------------------------------------------------

This is the second part of the review.

2. Properly observing changes and deletions (comment #46, comment #29, comment #43)

I'm going to mostly expand on Andrew's comment #46 and try to express in a lengthier way what I think he meant :).

There are two issues at stake here. Whether you want to be able to undo deletion (the "deferred" deletion model), and whether you want to update search results (the "faceted view" in real-time). It somehow seems to me that the two issues are not clearly separated in the subsequent comments.

a) Rollback deletion?

What this means is that when you delete a message, you merely mark it as "deleted", by changing a column in the _database_. This is what Gloda does in markMessagesDeletedByIDs in mailnews/db/gloda/modules/datastore.js. Then, you should probably read msg_search.js: on line 89, you can see that the search query is explicitly searching for messages which have not been marked as "deleted".

      " AND +messages.deleted = 0" +

Later on, messages are _actually_ deleted from the database; the database "rows" are removed with a "DELETE" query.

If you want to implement such a model, then please take inspiration from whatever exists and reuse as much code as possible.


b) Proper updating of search results.

It seems to me that you're trying to achieve another, orthogonal goal, that is, updating the search results (the user interface) by removing "lines" (as you call them) when messages are deleted.

(Disclaimer: I'm not familiar with the IM part of Thunderbird as I've never actually used it. I kind of assume from your patch that searching for IM logs uses the same "faceted" search results UI as the regular, gloda-backed email search?)

I think you should just forget about it. Gloda already does not update the faceted UI when a message is deleted (from a quick testing), so please don't add extra work to a patch that is already pretty big, and just leave it as is. This is minor.


I think that is pretty much all for this round of review. I hope that I've provided enough context for you to move on. Please let me know in the soon-to-be-created separate bug if you have any more specific questions, and I'll do my best to reply given the time constraints that I have.

I think this is going in the right direction and there's mostly cleanup / reorganizing left to do. If you manage to get everything right for the next patch, I'll do a more nitpicky review and I'll also tell you about tests :).

Good luck!

~ jonathan

Comment 64

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.8

Okay, so confirmed that we can't hide rows using CSS.
So, I have tried to remove the faults in this approach.
There is still a major issue that exists:
After we delete and undo the deletion (on a log group,
individual logs don't produce this issue) and the tree gets
back to almost the same state,
hovering on the tree produces tons of errors in the error
console, from the functions:
isContainer, getCellProperties, getLevel, getRowProperties etc.

I couldn't figure out the cause for this so far.

Thanks.

Comment 65

[[e-mail]]

(In reply to Suyash Agarwal from comment #64)
> After we delete and undo the deletion (on a log group,
> individual logs don't produce this issue) and the tree gets
> back to almost the same state,
> hovering on the tree produces tons of errors in the error
> console, from the functions:
> isContainer, getCellProperties, getLevel, getRowProperties etc.
Your code only seems to save one row, so that when you undelete a group it doesn't look as if you restore the correct number of rows.

Comment 66

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.9

(In reply to [e-mail] from comment #65)
> (In reply to Suyash Agarwal from comment #64)
> > hovering on the tree produces tons of errors in the error
> > console, from the functions:
> > isContainer, getCellProperties, getLevel, getRowProperties etc.
> Your code only seems to save one row, so that when you undelete a group it
> doesn't look as if you restore the correct number of rows.

Oh ya, thanks.
But I still get those errors in the console.

Comment 67

[aleth [:aleth]]

Comment on attachment 8417796 [details] [diff] [review]
Patch v3.9

Review of attachment 8417796 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/components/src/logger.js
@@ +723,5 @@
> +    try {
> +      OS.File.remove(aLogPath);
> +    } catch (ex) {
> +      Components.utils.reportError("Log deletion failed " + ex);
> +    }

This won't work as OS.File is async. OS.File returns promises, you have to handle these to catch errors.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +1203,5 @@
>  }
>  chatLogTreeView.prototype = {
>    __proto__: new PROTO_TREE_VIEW(),
>  
> +  hideDeletedLogs: function hideLogsMarkedForDeleted() {

I think I asked before for the ability to hide entries in a tree to be abstracted so it can be reused. Probably the right place to put it is jsTreeView, but if you don't get permission to modify that file you can do it here. It doesn't seem like it would take too many changes (the main one is that the methods of this new API should not include the word "deleted"). hideDeletedLogs() in chatLogTreeView would then use this new functionality.

Comment 68

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.93

Addressing OS.File.remove() part of comment #67.
Also, as aleth proposed for moving the hiding of rows functionality to jsTreeView, can you please tell me if it is under the scope of this bug that we move it there?

Thanks.

Comment 69

[:aceman]

I'll be needing the functionality of temporarily hiding treerows in bug 479767 (in the folder tree). So if it can be implemented universally I would be great. But I do not say it must be done in this bug.

Comment 70

[aleth [:aleth]]

(In reply to :aceman from comment #69)
> I'll be needing the functionality of temporarily hiding treerows in bug
> 479767 (in the folder tree). So if it can be implemented universally I would
> be great. But I do not say it must be done in this bug.

I would propose spinning this out into a separate bug, blocking this one.

Comment 71

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4.5

I have tailored this sufficiently.
So I am unable to decide if there's still anything
that can be made generic and put in jsTreeView.js

Thanks.

Comment 72

[:aceman]

Comment on attachment 8434047 [details] [diff] [review]
Patch v4.5

Review of attachment 8434047 [details] [diff] [review]:
-----------------------------------------------------------------

OK, so the hiding via just some css properties or collapsed=true didn't work? You now remove the rows from the rowMap? That may work in this scenario. But I would be afraid to use it for my use case in the folder pane.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +1195,5 @@
>  
> +  hideDeletedLogs: function hideLogsMarkedForDeletion() {
> +    let aIndex, level, row;
> +    let count = 1;
> +    for (let index = 0; index < this.hiddenLogs.length; index++) {

If this.hiddenLogs is an array, you can do 
for (let aIndex of this.hiddenLogs) ...

But do not call it aIndex as it is not an argument to the function.

Comment 73

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #72)
> OK, so the hiding via just some css properties or collapsed=true didn't
> work? You now remove the rows from the rowMap? That may work in this
> scenario. But I would be afraid to use it for my use case in the folder pane.

Ya I figured out that I can only remove rows from the tree shown.
I cannot hide them.
So the way to hide them was to scoop out the rows when hiding and put
them back (it was hard to put them back at the place they were removed from,
so I just rebuilt the tree).

Rebuilding the tree will be inexpensive for your folderpane too as what amount
of folders we can expect.. ~1000 at most?

Actually I wanted to stick to overridden recursivelyAddToMap() but then I wasn't
able to keep hold of the set "hidden" attributes as I was blowing the nodes at the
first place. So if you can do that it'll be great too.

These are just novice inputs rest is all on you :)

Thanks.

Comment 74

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4.7

Made the suggested change.
Removed a warning.
Fixed Undo action (was failing when there was an
ongoing conversation with the contact).
Added deletion using delete key.

Comment 75

[aleth [:aleth]]

Comment on attachment 8434745 [details] [diff] [review]
Patch v4.7

Review of attachment 8434745 [details] [diff] [review]:
-----------------------------------------------------------------

Some questions and some problems. Sorry for the delay!

::: mail/components/im/content/chat-messenger-overlay.js
@@ +10,5 @@
>  Components.utils.import("resource://gre/modules/FileUtils.jsm");
> +Components.utils.import("resource://gre/modules/PluralForm.jsm");
> +
> +// A global set holding the log files marked for deletion.
> +var logsMarkedForDeletion = new Set();

Why is this global?

@@ +14,5 @@
> +var logsMarkedForDeletion = new Set();
> +
> +// Notify the user of deletion of logs, giving a chance to undo
> +// the action.
> +var logDeletionTimeoutID;

ditto

@@ +15,5 @@
> +
> +// Notify the user of deletion of logs, giving a chance to undo
> +// the action.
> +var logDeletionTimeoutID;
> +function showDeletionNotification() {

Shouldn't this be in the same object as _showLogList?

@@ +38,5 @@
> +      let item = document.getElementById("contactlistbox").selectedItem;
> +      if (!item)
> +        return;
> +      if (item.localName == "imconv")
> +        chatHandler._showLogList(imServices.logs.getLogsForConversation(item.conv), false);

Why is the third parameter false?

@@ +40,5 @@
> +        return;
> +      if (item.localName == "imconv")
> +        chatHandler._showLogList(imServices.logs.getLogsForConversation(item.conv), false);
> +      else if (item.localName == "imcontact") {
> +        item = item.contact;

Just use item.contact in the next line.

@@ +41,5 @@
> +      if (item.localName == "imconv")
> +        chatHandler._showLogList(imServices.logs.getLogsForConversation(item.conv), false);
> +      else if (item.localName == "imcontact") {
> +        item = item.contact;
> +        chatHandler._showLogList(imServices.logs.getLogsForContact(item), false);

Why false?

@@ +46,5 @@
> +      }
> +    }
> +  }];
> +
> +  if (notificationBox) {

Is there a reason why there wouldn't be a notificationbox?

@@ +63,5 @@
> +  }
> +}
> +
> +// Actually delete the logs marked for deletion.
> +function removeLogsMarkedForDeletion() {

Should also live in chatHandler I think.

@@ +65,5 @@
> +
> +// Actually delete the logs marked for deletion.
> +function removeLogsMarkedForDeletion() {
> +  chatHandler._treeView.clearHiddenLogsList();
> +  if (logsMarkedForDeletion.size == 0)

nit: use !logsMarkedForDeletion.size

@@ +75,5 @@
> +  if (notificationBox && notification)
> +    notificationBox.removeNotification(notification);
> +
> +  // Copy logsMarkedForDeletion to avoid immediate deletion of
> +  // any new logs the user attempts to delete while we are here.

I don't understand this bit. "While we are here" where?

@@ +428,5 @@
>    _showLog: function(aConversation, aPath, aSearchTerm) {
>      if (!aConversation)
>        return;
>      this._showLogPanel();
> +    if (this._displayedLog && this._displayedLog == aPath)

When does it happen that this._displayLog isn't set? Maybe add a comment.

@@ +480,5 @@
>     */
>    _showLogList: function(aLogs, aShouldSelect) {
>      let logTree = document.getElementById("logTree");
>      let treeView = this._treeView = new chatLogTreeView(logTree, aLogs);
> +    treeView.hideDeletedLogs();

When can it happen that _showLogList is called and deleted logs already exist?

@@ +542,5 @@
> +    let logGroup = this._treeView._rowMap[currentIndex];
> +    if (!logGroup)
> +      return;
> +
> +    let rowMarkedForDeletion = currentIndex;

Why do we need a separate variable here?

@@ +545,5 @@
> +
> +    let rowMarkedForDeletion = currentIndex;
> +    // Check whether the selected row is a group and if it has children.
> +    if (logGroup.level == 0 && logGroup.children.length != 0) {
> +      for each (let logs in logGroup.children)

for ... of

@@ +549,5 @@
> +      for each (let logs in logGroup.children)
> +        logsMarkedForDeletion.add(logs.log.path);
> +    }
> +    else {
> +      logsMarkedForDeletion.add(logGroup.log.path);

This won't always work. JSON logs are grouped by day in the log tree which means that the path here is only the path of the first log file for that day. You either have to get all the other paths for the same day and add them to your list here, or you have to add a groupByDay parameter to your removeLog function in logger.js.

@@ +1199,5 @@
>  }
>  chatLogTreeView.prototype = {
>    __proto__: new PROTO_TREE_VIEW(),
>  
> +  hideDeletedLogs: function hideLogsMarkedForDeletion() {

Why doesn't this call reflectDeletionChanges? Or should reflectDeletionChanges call this? Or should they both call a helper function? There appears to be some duplication.

@@ +1218,5 @@
> +        this._tree.invalidateRow(index);
> +      }
> +    }
> +    // This is necessary so that we restore the tree back to the state it
> +    // should look like and not that like a fully redesigned tree.

Can you reformulate this comment, I'm not sure what you mean here. E.g. give an example of what could go wrong.

@@ +1222,5 @@
> +    // should look like and not that like a fully redesigned tree.
> +    this._restoreOpenStates();
> +  },
> +
> +  reflectDeletionChanges: function reflectDeletionChanges(aIndex) {

Just checking: Will this work if the selection is a group header (e.g. a month)? Why don't we need to add more than one entry to hiddenLogs in that case?

::: mail/components/im/content/chat-messenger-overlay.xul
@@ +45,5 @@
> +    <menupopup id="logTreeContext"
> +               onpopupshowing="if (document.getElementById('logTree').view.rowCount == 0 ||
> +                                   document.getElementById('logTree').view.selection.count == 0)
> +                                 return false;
> +                               else return true;">

Please make this a function in chat-messenger-overlay.js.

Comment 76

[aleth [:aleth]]

Comment on attachment 8434745 [details] [diff] [review]
Patch v4.7

Review of attachment 8434745 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/im/content/chat-messenger-overlay.xul
@@ +35,5 @@
>      <stringbundle id="bundle_places" src="chrome://places/locale/places.properties"/>
>    </stringbundleset>
>  
> +  <keyset>
> +    <key id="deleteLogKey" keycode="VK_DELETE" oncommand="chatHandler.deleteConversationLog();"/>

Please add an actual command element and have the key and the menuitem use it.

Don't forget to check that the logtree is actually focused before having this key do anything.

Comment 77

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4.9

Made the suggested changes.
Thanks.

Comment 78

[aleth [:aleth]]

Comment on attachment 8443362 [details] [diff] [review]
Patch v4.9

Review of attachment 8443362 [details] [diff] [review]:
-----------------------------------------------------------------

Please update the logger.js changes after discussing how to implement it with nhnt11. removeLog should take an aGroupByDay parameter for consistency with the rest of the API.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +226,5 @@
>  
>    saveTabState: function(aTab) { }
>  };
>  
> +function ShouldShowDeletionContext() {

Let's call this isLogDeletionPossible()

Shouldn't this live in chatHandler?

@@ +228,5 @@
>  };
>  
> +function ShouldShowDeletionContext() {
> +  if (document.getElementById('logTree').view.rowCount == 0 ||
> +      document.getElementById('logTree').view.selection.count == 0)

document...view is duplicated.

Is view.selection always defined?

@@ +234,5 @@
> +  else
> +    return true;
> +}
> +
> +function DeleteLog()

Shouldn't this live in chatHandler?

@@ +236,5 @@
> +}
> +
> +function DeleteLog()
> +{
> +  let activeElement = document.activeElement;

This is only used once, inline it.

@@ +240,5 @@
> +  let activeElement = document.activeElement;
> +
> +  // If logTree has logs and a log is selected and
> +  // the focus is on the logTree, delete the selected log.
> +  if (ShouldShowDeletionContext() && activeElement.id == "logTree")

Does this actually work? i.e. is the active element the log tree or the focused listitem?

@@ +414,5 @@
>                                aDate.getMonth() + 1, aDate.getDate());
>      return bundle.getFormattedString("dateTime", [date, time]);
>    },
> +  // Notify the user of deletion of logs, giving a chance to undo
> +  // the action.

This comment is in the wrong place

@@ +469,5 @@
> +  },
> +
> +  // Actually delete the logs marked for deletion.
> +  removeLogsMarkedForDeletion: function() {
> +    this._treeView.clearHiddenLogsList();

Does this also redisplay the log tree or is that not needed?

@@ +492,5 @@
>     *
>     * @param aLogs An nsISimpleEnumerator of imILog.
>     * @param aShouldSelect Either a boolean (true means select the first log
>     * of the list, false or undefined means don't mess with the selection) or a log
>     * item that needs to be selected.

add you explanation of the third parameter here

@@ +500,3 @@
>      let logTree = document.getElementById("logTree");
>      let treeView = this._treeView = new chatLogTreeView(logTree, aLogs);
> +    if (aIsRestore) {

call this aKeepState

@@ +503,5 @@
> +      // This is necessary so that we restore the tree back to the state it
> +      // was and not display a fully redesigned tree i.e., we persist the opened
> +      // and closed chat group items that were there just before the deletion is
> +      // undone and not present the user with some different opened and closed
> +      // containers.

You can shorten this to "Keep the tree in its current state, i.e. persist the previously opened/closed groups."

@@ +761,5 @@
>            document.getElementById("noPreviousConvScreen");
>        }
> +
> +      // If the user switched to another buddy, delete the logs marked for deletion.
> +      chatHandler.removeLogsMarkedForDeletion();

why not this.removeLogs...? Is there a reason?

@@ +1221,5 @@
>  }
>  chatLogTreeView.prototype = {
>    __proto__: new PROTO_TREE_VIEW(),
>  
> +  reflectDeletionChanges: function reflectDeletionChanges(aIndex) {

Why not call this hideEntry(aIndex) ?

::: mail/components/im/content/chat-messenger-overlay.xul
@@ +46,5 @@
>    <popupset id="mainPopupSet">
>      <tooltip id="imTooltip" type="im"/>
>  
> +    <menupopup id="logTreeContext"
> +               onpopupshowing="ShouldShowDeletionContext();">

You really haven't tested this, have you? ;)

::: mail/components/im/content/chat.css
@@ +117,5 @@
>  #button-chat[showingBadge="true"] > stack > .badgeButton-badge {
>    display: block;
>  }
> +
> +.messageImage[value="logDeletionNotification"] {

Why are there two different styles for this in two different chat.css files?

Comment 79

[aleth [:aleth]]

You should also make sure that it's not possible to delete the log for an ongoing conversation.

Comment 80

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #79)
> You should also make sure that it's not possible to delete the log for an
> ongoing conversation.

Actually it would be nice to support that edge case too, but you then have to be careful to close the existing log writer before deleting the file.

Comment 81

[Suyash Agarwal (:sshagarwal)]

This issue is still blocked on bug 955014. Do we have an update on that?

Comment 82

[aleth [:aleth]]

Forwarding to nhnt11, I am unsure of the status of those patches.

Comment 83

[:aceman]

Looks like that bug is mostly done, just needs some updates to the patch. We should ping the author:)

Comment 84

[harl]

> For example: profile\logs\gtalk\[e-mail]

I've deleted all data from profile\logs folder, but search results show my old conversations.

Windows 7 (64-bit)
Thunderbird 52.9.1 (32-bit)

Comment 85

[(inactive) Nihanth Subramanya [:nhnt11]]

I'm not working on this anymore.