500083 - delete message from Search Messages results doesn't update view

Status: VERIFIED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[David :Bienvenu]

Attachment: proposed fix

If I do an advanced search, and delete an entry from the search results, the view doesn't update. This is also true for gloda search results, which use a search results view as well.

The attached patch fixes it for me - I'm going to work on a mozmill test for this, but I wanted to get asuth's opinion on the basic approach.

Comment 1

[David Ascher (:davida)]

I'm pretty sure even in en-UK it'd be curiosity, not curiousity.

Comment 2

[David :Bienvenu]

heh, ok, changed locally. There is already a mozmill test for search results, so I just need to add a delete test to that.

Putting in b3.

Comment 3

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

Comment on attachment 384776 [details] [diff] [review]
proposed fix

In response to https://bugzilla.mozilla.org/show_bug.cgi?id=474701#c163:

Since the gloda synthetic view is basically force-feeding the headers, it could make sure the registrations happens for the folders in its results view.  It's a toss-up on simplicity versus fake view rebuilds, but since the advanced search window is not commonly used, I'll say simplicity is the right thing.

The down-side to this implementation is that view rebuilds (which includes new searches) will happen on:
- any folder being moved
- any folder being deleted
- any folder being compacted (the search view will actually go away until the compaction completes)

The gloda search case has it easy because view rebuilds are low-cost for it.  It just crams the headers in again.  Arguably the gloda search won't be crazy about the compaction making us go away, but I rather suspect things won't be usable while that is happening anyways.

on file: mailnews/base/src/dbViewWrapper.js line 268
>       dump("view wrapper got delete completed\n");

please remove debug dump

Comment 4

[David :Bienvenu]

Attachment: patch addressing asuth's comments

carrying forward asuth's r+, requesting sr.

In the case of compact, at least for local folders, rebuilding the view is needed. I don't think advanced search is going to be that common, so right now I like the implementation simplicity trade-off.

Comment 5

[David :Bienvenu]

Attachment: add delete test to search window test case

I have not been able to test this since mozmill broke overnight for me - it fails cleaning up the mozmill extension dir in the objdir, at the start of the test run. I'm not sure why that started happening...but I'm clobbering and rebuilding.

Comment 7

[David :Bienvenu]

it turns out to be very difficult to write a test case that fails - basically, the failure is that we're missing some invalidation calls on the view, and it's pretty hard to tell if a paint has happened. It's a lot easier to write tests for the gloda search results because we can tell if the next message gets loaded...

Comment 8

[Mark Banner (:standard8)]

I've pushed the patch in time for today's nightly: http://hg.mozilla.org/comm-central/rev/06ef7c212993

We still need to sort out the testcase though.

Comment 9

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

The mozmill test is unhappy; was the patch refreshed before submission?  The use of "swc.dbView" would require that a getter helper is added to the bottom of test-window-helpers.js.  When that is added, press_delete is unhappy by way of wait_for_message_display_completion because it assumes all kinds of things about messageDisplays and docshells.  press_delete/wait_for_message_dispay_completion should either be more aware of these things or the MessageDisplayWidget subclass used for the search view may need to change its behaviour slightly to not be misleading.

Comment 10

[David :Bienvenu]

shoot, I didn't mean for the mozmill test part of the patch to land - that part should just be reverted.

Comment 11

[Wayne Mery (:wsmwk)]

v.fixed
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1pre) Gecko/20090626 Shredder/3.0b3pre

Comment 13

[Wayne Mery (:wsmwk)]

filed similar new Bug 515400.
noting it here as a reminder about needing a test

Comment 14

[Ludovic Hirlimann [:Usul]]

Added https://litmus.mozilla.org/show_test.cgi?id=9509 in BFT and FFT.