997211 - Implement removal of deleted chat logs from Gloda

Status: NEW

(Thunderbird :: Instant Messaging, defect)

Description

[Suyash Agarwal (:sshagarwal)]

+++ This bug was initially created as a clone of Bug #788145 +++

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

Bug 788145 talks about adding the ability to delete chat logs.
This bug is about deleting them from Gloda so that they don't show up in the search results.

(Quoting Jonathan Protzenko [:protz] from bug 788145 comment #62)
> 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 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

Comment 2

[Suyash Agarwal (:sshagarwal)]

(Jonathan Protzenko [:protz]'s review of attachment 8407583 [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 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.4

(In reply to Jonathan Protzenko [:protz] from bug 788145 comment #62)
> I suggest you get rid of objDelete _completely_ and only expose
> objDeleteByColumn. Indeed,
Done.

> 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? 
Ya, I asked the IM people and they confirmed that we don't have an ID attribute
accessible for logs (imILog)
> - 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?
Maybe we can add ID attribute to each log? Otherwise, path is the only unique identifier I could find for a log.
> 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?
No idea, I just put it there because he said we will need it. So, if you can please tell me how to put them to action, I'll do it.

(Jonathan Protzenko [:protz]'s review of attachment 8407583 [details] [diff] [review][review])
> This is the second part of the review.
> 
> a) Rollback deletion?
> If you want to implement such a model, then please take inspiration from
> whatever exists and reuse as much code as possible.
I don't think we'd be needing it, as I am triggering the gloda deletion only after
the timer to allow the user to undo the action expires. So, there is no going back, we
have already deleted the actual log file from the disk.
> 
> b) Proper updating of search results.
> 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.
Or, maybe, we can just use the listener areas I proposed in the patch with "TODOs"
to skip the marked for deletion logs from the search results returned?

Please let me know if this goes in the right direction and the changes I need to make.

Thanks.

Comment 4

[Patrick Cloke [:clokep]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #3)
> > b) Proper updating of search results.
> > 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.
> Or, maybe, we can just use the listener areas I proposed in the patch with
> "TODOs"
> to skip the marked for deletion logs from the search results returned?

Just want to short circuit a misconception here. Logs are not being deleted from the gloda search UI it's from the IM log tree UI. I think removing things from this UI is outside the scope of touching gloda things.

Comment 5

[Jonathan Protzenko [:protz]]

> Just want to short circuit a misconception here. Logs are not being deleted
> from the gloda search UI it's from the IM log tree UI. I think removing
> things from this UI is outside the scope of touching gloda things.

Thanks for the clarification Patrick, much appreciated.

(Indeed, the disclaimer from comment #2 reveals that I was thinking about the wrong thing).

Comment 6

[Jonathan Protzenko [:protz]]

1. About the sqlite database-side deletion
> > 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? 
> Ya, I asked the IM people and they confirmed that we don't have an ID
> attribute
> accessible for logs (imILog)
> > - 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?
> Maybe we can add ID attribute to each log? Otherwise, path is the only
> unique identifier I could find for a log.
Let's leave it like that. In other words, let's keep deleting using the path column. However, please answer the following questions:
- are you going to be deleting many logs at the same time or is it just going to be a log that gets deleted once in a while?
- do you have an index on the path column? (this has been discussed in the previous bug but not answered, please read http://en.wikipedia.org/wiki/Database_index or https://www.sqlite.org/lang_createindex.html if you're unsure what a database index is, per bug 788145 comment #27)
- do you plan on adding an index in newly created databases?
- do you plan on performing a schema migration?
See bug 788145 comment #26 for the original concerns.
> > 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?
> No idea, I just put it there because he said we will need it. So, if you can
> please tell me how to put them to action, I'll do it.
bug 788145 comment #26 also contains the original info.

I am not familiar with this particular part of Gloda and I am currently writing up my PhD dissertation, so my review bandwidth is seriously limited. I'm not sure I'll find time in the near future to dig into the code and actually figure out what happens with the triggers; your best bet is probably to read the sqlite documentation about triggers and read the gloda code as well to actually understand what happens. I don't have a ready-made answer that would tell you exactly what you have to do and how you have to implement it, sorry.

2. UI-Related concerns
> 
> (Jonathan Protzenko [:protz]'s review of attachment 8407583 [details] [diff] [review]
> [review][review])
> > This is the second part of the review.
> > 
> > a) Rollback deletion?
> > If you want to implement such a model, then please take inspiration from
> > whatever exists and reuse as much code as possible.
> I don't think we'd be needing it, as I am triggering the gloda deletion only
> after
> the timer to allow the user to undo the action expires. So, there is no
> going back, we
> have already deleted the actual log file from the disk.
Good.
> > 
> > b) Proper updating of search results.
> > 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.
> Or, maybe, we can just use the listener areas I proposed in the patch with
> "TODOs"
> to skip the marked for deletion logs from the search results returned?
> 
> Please let me know if this goes in the right direction and the changes I
> need to make.
It seems like you are _not_ talking about the faceted UI but something else (per comment #4). I'm fine with your adding a notification for actual log deletion. However, you'd probably need to ask someone else to review the IM log tree stuff.

Again, I'll perform a review whenever I can find some time, but please do try to figure out the trigger-related issues, that will certainly be quicker than waiting for my to dig into the code.

Thanks for the good work,

~ jonathan

Comment 7

[aleth [:aleth]]

(In reply to Jonathan Protzenko [:protz] from comment #6)
> 2. UI-Related concerns
> > > a) Rollback deletion?
> > > If you want to implement such a model, then please take inspiration from
> > > whatever exists and reuse as much code as possible.
> > I don't think we'd be needing it, as I am triggering the gloda deletion only
> > after
> > the timer to allow the user to undo the action expires. So, there is no
> > going back, we
> > have already deleted the actual log file from the disk.
> Good.

Just to clarify: There is no equivalent to compaction as the UI will only allow the deletion of entire log files (corresponding to entire chat sessions), as opposed to say individual messages from a conversation.

> > > b) Proper updating of search results.
> > > 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.
> > Or, maybe, we can just use the listener areas I proposed in the patch with
> > "TODOs"
> > to skip the marked for deletion logs from the search results returned?
> > 
> > Please let me know if this goes in the right direction and the changes I
> > need to make.
> It seems like you are _not_ talking about the faceted UI but something else
> (per comment #4). I'm fine with your adding a notification for actual log
> deletion. However, you'd probably need to ask someone else to review the IM
> log tree stuff.

I believe there are two separate issues here:
* Making the IM log tree visually reflect the fact that a log has been deleted immediately, despite the fact that the log file still exists. This is wanted and part of the other bug.

* The edge case that existing faceted UI gloda search results include messages from logs that have been deleted. As suggested by protz, I think we can safely ignore this for now if that is what happens for mails. (Any improvements made in follow-ups should probably provide the same behaviour for both email and IM.)

Comment 8

[Suyash Agarwal (:sshagarwal)]

(In reply to Jonathan Protzenko [:protz] from comment #6)
> > Ya, I asked the IM people and they confirmed that we don't have an ID
> > accessible for logs (imILog)
> Let's leave it like that. In other words, let's keep deleting using the path
> column. However, please answer the following questions:
> - are you going to be deleting many logs at the same time or is it just
Ya, sometimes. That's why I am using "," as the separator for different logs in
Gloda.deleteNounItem(IMConversationNoun, aData.split(","), "path");

> - do you have an index on the path column? (this has been discussed in the
> previous bug but not answered, please read
> http://en.wikipedia.org/wiki/Database_index or
> https://www.sqlite.org/lang_createindex.html if you're unsure what a
> database index is, per bug 788145 comment #27)
> - do you plan on adding an index in newly created databases?

I added path to indices in the schema section in index_im.js
Will that suffice?

> - do you plan on performing a schema migration?
I think so, otherwise that'd be slow isn't it? Or maybe we can go without it;
the users will experience some performance issues in the beginning if they
are deleting many *old* logs at a time. Because from now onwards we'll be indexing 
based on the path column. Does it work this way? I mean, without touching the old
records (entries in the gloda index tables) we now index the path column for the
new entries.

> See bug 788145 comment #26 for the original concerns.
> > > d) You seem to have added a trigger, but I recall Andrew mentioning that
> > No idea, I just put it there because he said we will need it. So, if you can
> > please tell me how to put them to action, I'll do it.
> bug 788145 comment #26 also contains the original info.
> 
> probably read the sqlite documentation about triggers and read the gloda
> code as well to actually understand what happens. I don't have a ready-made
> answer that would tell you exactly what you have to do and how you have to
> implement it, sorry.
Okay.

> 2. UI-Related concerns
> > need to make.
> It seems like you are _not_ talking about the faceted UI but something else
> (per comment #4). I'm fine with your adding a notification for actual log
> deletion. However, you'd probably need to ask someone else to review the IM
> log tree stuff.
Okay.

> Again, I'll perform a review whenever I can find some time, but please do
> try to figure out the trigger-related issues, that will certainly be quicker
> than waiting for my to dig into the code.

Okay, thanks :)

(In reply to aleth from comment #7)
> I believe there are two separate issues here:
> * Making the IM log tree visually reflect the fact that a log has been
> deleted immediately, despite the fact that the log file still exists. This
> is wanted and part of the other bug.
Ya, bug 788145.

> * The edge case that existing faceted UI gloda search results include
> messages from logs that have been deleted. As suggested by protz, I think we
> can safely ignore this for now if that is what happens for mails. (Any
> improvements made in follow-ups should probably provide the same behaviour
> for both email and IM.)
Okay :)

Comment 9

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8407700 [details] [diff] [review]
Patch v1.4

Removing review request as this patch needs to be updated and currently
waits on bug 788145.

Thanks.