Closed
Bug 465535
Opened 16 years ago
Closed 7 years ago
Cannot delete a contact from a mailing list within the quick search results
Categories
(MailNews Core :: Address Book, defect)
MailNews Core
Address Book
Tracking
(seamonkey2.43 affected, seamonkey2.44 affected, seamonkey2.45 affected, seamonkey2.46 affected, seamonkey2.47 affected)
RESOLVED
FIXED
Thunderbird 60.0
People
(Reporter: standard8, Assigned: helladise)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
1) Set up a mailing list with several contacts
2) Do a quick search within the contact list
3) Attempt to delete a card.
Result: Doesn't work
Expected Result: Delete the card.
Vague description of actual problem:
The locally stored prefs id isn't initialised for a nsAbMDBDirectory that is a search query. When we try and keep a reference to the nsIAddrDatabase in nsAbMDBDirectory::DeleteCards, it fails, because we haven't got the prefs, and hence we can't get the nsIAddrDatabase. We can't even get the non-search database, because mailing lists still don't know about the locally stored prefs.
Flags: wanted-thunderbird3+
Comment 1•15 years ago
|
||
Probably I understand the question but here seems that work fine.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091110 Lightning/1.0pre Shredder/3.0pre ID:20091110032315
I have create a tesing mailing list with 5 cards (new cards for testing) into one address book that contains others cards: it is this the right STR?
Reporter | ||
Updated•15 years ago
|
Assignee: bugzilla → nobody
Target Milestone: Thunderbird 3.0b2 → ---
Updated•14 years ago
|
Severity: normal → minor
I was able to reproduce the problem using her data on a sandbox system. Go into a sub-address book (a "list"), search on a name, select the name, click the delete button or right-click > delete, and nothing happens.
Works fine in the main address book, but not in the sub-address books or "lists".
I have a user I support that has hundreds of contacts in a dozens of lists and relied on the search function to find them quickly for deletion or changes. Now to do deletes, she has to scroll up and down manually to find them, which takes considerable time. This is impacting her efficiency and is complaining to me to fix it.
This used to work in Thunderbird 2.x and I just migrated her recently to 7 through the sequential updates. She was quite surprised to find that it no longer worked and is quite vocal about it...
I see that this problem is quite old and not assigned to anyone. I would be very appreciative if you could give this problem some of your time.
Sorry for my disjointed entry... by "her data" I am referring to the user who is reporting the problem. Couldn't find a way to edit my entry so I'm making my "correction" by adding this additional comment.(In reply to emeadows from comment #2)
> I was able to reproduce the problem using her data on a sandbox system. Go
> into a sub-address book (a "list"), search on a name, select the name, click
> the delete button or right-click > delete, and nothing happens.
>
> Works fine in the main address book, but not in the sub-address books or
> "lists".
>
> I have a user I support that has hundreds of contacts in a dozens of lists
> and relied on the search function to find them quickly for deletion or
> changes. Now to do deletes, she has to scroll up and down manually to find
> them, which takes considerable time. This is impacting her efficiency and
> is complaining to me to fix it.
>
> This used to work in Thunderbird 2.x and I just migrated her recently to 7
> through the sequential updates. She was quite surprised to find that it no
> longer worked and is quite vocal about it...
>
> I see that this problem is quite old and not assigned to anyone. I would be
> very appreciative if you could give this problem some of your time.
Cannot delete a contact from a mailing LIST within the quick search results.
Works fine in the main address book, but not in the sub-address books or "lists".
Go into a sub-address book (a "list"), search on a name, select the name, click the delete button or right-click > delete, and nothing happens.
Comment 6•8 years ago
|
||
Still REPRODUCIBLE with English SeaMonkey (unofficial by frg) 2.47a1 (NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0 Build 20160628114904 (Default Classic Theme) on German WIN7 64bit:
1. Launch Addressbook-Editor via icon in Application Bar (Status Bar)
2. Click arbitrary address Book → New Lisr (icon) → Name = Testlist →
Add 3 email-addresses to list like 1@1.org, 2@2.org, 3@w.org → [ok]
3. Click Testlist → into quick search bar above contacts pane type "@"
» Nothing changes, all items contain "@"
4. Click "2@2.org" → rightclick → Delete → Convirm Warning with [ok]
Bug: deletion will fail
a) already REPRODUCIBLE with DE SeaMonkey 2.0 (Windows NT 6.1; WOW64;
rv:1.9.1.4) Gecko/20091017 Build 20091017081335 (Classic Theme)
on German WIN7 64bit
a1) so definitely different to "Bug 1270651 - contacts cannot be deleted when
they have been found through a search", what started with SM 2.42
(Version 45)
b) <del> key does not work, either.
status-seamonkey2.43:
--- → affected
status-seamonkey2.44:
--- → affected
status-seamonkey2.45:
--- → affected
status-seamonkey2.46:
--- → affected
status-seamonkey2.47:
--- → affected
Assignee | ||
Comment 7•7 years ago
|
||
Still reproducible at changeset 0d0988b6c3a1 on Windows 7
I tracked down the bug to a problem in this part of the mailnews module:
mailnews\addrbook\src\nsAbMDBDirectory.cpp(420):NS_IMETHODIMP nsAbMDBDirectory::DeleteCards(nsIArray *aCards)
When the mIsQueryURI==true branch is taken with a m_IsMailList==true the GetDatabase(...) call on line 431 fails with NS_ERROR_NOT_INITIALIZED
Using GetAbDatabase() instead resolves the issue. However, this has the side effect of initializing the mDatabase member for the lifespan of the search query directory object.
If this is not a problem - see the attached card-delete-fix.diff file for a possible solution.
Otherwise, either of these could be done:
1. Instead of initilizing mDatabase directly, modify GetAbDatabase() so it accepts a database output parameter like GetDatabase() does.This way it could be used instead of the GetDatabase(...) call on line 431 and limit the lifespan of the database object to the mIsQueryURI scope.
2. Merge the parent directory database search code from GetAbDatabase() into GetDatabase() when using mail lists. All use places have to be carefully reviewed.
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8957535 -
Flags: review?(mconley)
Comment 9•7 years ago
|
||
Redirecting to Magnus - I'm afraid I don't have the cycles to do Thunderbird reviews these days.
Updated•7 years ago
|
Attachment #8957535 -
Flags: review?(mconley) → review?(mkmelin+mozilla)
Comment 10•7 years ago
|
||
Comment on attachment 8957535 [details] [diff] [review]
A possible solution to the issue
I'll take the review.
Attachment #8957535 -
Flags: review?(mkmelin+mozilla) → review?(jorgk)
Comment 11•7 years ago
|
||
Here's the same patch ignoring white-space just so we can focus on the changes better.
Comment 12•7 years ago
|
||
Comment on attachment 8957792 [details] [diff] [review]
ignore-white-space.patch
Review of attachment 8957792 [details] [diff] [review]:
-----------------------------------------------------------------
::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +425,5 @@
> + if (!mDatabase)
> + rv = GetAbDatabase();
> +
> + if (!NS_SUCCEEDED(rv) || !mDatabase)
> + return rv;
Please make put the error checking into a block following
if (!mDatabase)
...
@@ +432,5 @@
> // if this is a query, delete the cards from the directory (without the query)
> // before we do the delete, make this directory (which represents the search)
> // a listener on the database, so that it will get notified when the cards are deleted
> // after delete, remove this query as a listener.
> + mDatabase->AddListener(this);
Hmm, looking at nsAbMDBDirectory::GetAbDatabase(), that already attaches a listener. Looks like we're doing it twice here then, no?
@@ +440,5 @@
> do_GetService(NS_ABMANAGER_CONTRACTID, &rv);
> NS_ENSURE_SUCCESS(rv, rv);
>
> + nsCOMPtr<nsIAbDirectory> directoryNoQuery;
> + rv = abManager->GetDirectory(mURINoQuery, getter_AddRefs(directoryNoQuery));
Please revert change of variable name, it creates needless noise.
Comment 13•7 years ago
|
||
Comment on attachment 8957535 [details] [diff] [review]
A possible solution to the issue
Review of attachment 8957535 [details] [diff] [review]:
-----------------------------------------------------------------
I've already reviewed the other patch. The patch needs more work, so cancelling the review for now.
To avoid confusion: Yes, we need a patch taking white-space/indentation changes into account, for if you re-indent large blocks, it's sometimes easier to review with a patch that ignores white-space.
::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +445,2 @@
> NS_ENSURE_SUCCESS(rv, rv);
> +
Please avoid introducing trailing spaces.
@@ +448,2 @@
> NS_ENSURE_SUCCESS(rv, rv);
> +
And here.
Attachment #8957535 -
Flags: review?(jorgk)
Assignee | ||
Comment 14•7 years ago
|
||
Well, I agree the Add/RemoveListener from the old code is no longer necessary as this will be handled by the GetAbDatabase() and the ~nsAbMDBDirectory() destructor.
This is my first contribution to the community, so I'm sorry if I didn't manage to follow all coding style rules.
You may find attached an updated version with 2 diffs - with and without whitespace.
(if you submit code block re-idents on separate commits)
Feel free to use it in any way you like.
I've run the mozmill interactive test suite, tested by hand in some common sense use cases and all seems fine.
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Comment 16•7 years ago
|
||
Updated•7 years ago
|
Attachment #8957535 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8957792 -
Attachment is obsolete: true
Comment 17•7 years ago
|
||
Comment on attachment 8957835 [details] [diff] [review]
card-delete-fix-r2-ignore-ws.diff
This looks very clear now ;-)
Aceman, what do you think about this?
Attachment #8957835 -
Flags: feedback?(acelists)
Comment 18•7 years ago
|
||
Comment on attachment 8957835 [details] [diff] [review]
card-delete-fix-r2-ignore-ws.diff
Review of attachment 8957835 [details] [diff] [review]:
-----------------------------------------------------------------
Nice, this works for me.
But why is the mDatabase change done? In the mIsQueryURI branch the mDatabase isn't used. The problem was that database couldn't be get via GetDatabase but users of that were was removed now. Instead you set up the database listener via GetAbDatabase.
I don't know whether we need the database locally-scoped only, as Konstantin discusses in comment 7.
::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +424,5 @@
>
> + if (!mDatabase)
> + {
> + rv = GetAbDatabase();
> + if (!NS_SUCCEEDED(rv) || !mDatabase)
NS_FAILED(rv)
Attachment #8957835 -
Flags: feedback?(acelists) → feedback+
Comment 19•7 years ago
|
||
I've removed some trailing spaces which had crept in again and I'm using NS_FAILED() now.
Let's see whether this breaks anything:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=99a4fa8236d46701fe2754000836bd21b31d9b65
Attachment #8957834 -
Attachment is obsolete: true
Attachment #8957835 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8957851 -
Flags: review?(jorgk)
Comment 20•7 years ago
|
||
Comment on attachment 8957851 [details] [diff] [review]
card-delete-fix-r2.diff - traling spaces removed and NS_FAILED() used
Thanks again, this didn't break any test and works for the deletion from a mailing list after a search.
Attachment #8957851 -
Flags: review?(jorgk) → review+
Updated•7 years ago
|
Comment 21•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d6eb1caf9e2d
Fix deletion of contact from a mailing list within the quick search results. r=jorgk
Updated•7 years ago
|
Target Milestone: --- → Thunderbird 60.0
You need to log in
before you can comment on or make changes to this bug.
Description
•