Closed Bug 1061648 Opened 6 years ago Closed 6 years ago

Mailing list display does not refresh correctly after addresses are deleted

Categories

(Thunderbird :: Address Book, defect)

31 Branch
x86_64
Linux
defect
Not set
normal

Tracking

(thunderbird36 fixed)

RESOLVED FIXED
Thunderbird 36.0
Tracking Status
thunderbird36 --- fixed

People

(Reporter: sschaub, Assigned: sschaub)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0
Build ID: 20140715214327

Steps to reproduce:

1. Double-click a non-empty mailing list. Mailing List dialog appears.
2. Remove an entry from the list.
3. Click OK.





Actual results:

Dialog remains open. User must click OK a second time to close the dialog. However, mailing list display is still not updated.


Expected results:

Dialog closes and mailing list display updates to indicate entry has been removed.
There are really two separate issues involved in the bug: 
1. When the user edits a mailing list, removes one or more entries, and clicks OK, an exception is thrown in GetListValue() (abMailListDialog.js) due to inadequate code checks to handle this case. There's a straightforward fix for this (decrement the oldTotal variable after removing an item from the list). 

2. After #1 is fixed, the list display doesn't refresh properly: extra items appear at the end of the list. The user has to manually refresh the display by selecting a separate list or address book, then selecting the list again. I think this happens because the nsAddrDatabase::EditMailList processing uses the observer mechanism to notify listeners (including nsAbView) about changes to the mailing list, but the current implementation does not correctly handle the case when the resulting list is shorter than the original. 

My thought about #2 is that there is probably no reasonable way to use the existing observer mechanism to properly refresh the list when the changed list is shorter than the original. I suggest that a better approach to updating mail list display after an edit is to force a complete refresh of the nsAbView. I think this would also neatly solve some other issues related to refresh of the display after a mailing list edit.
Attached patch bug23.patch (obsolete) — Splinter Review
Attachment #8483725 - Flags: review?(mconley)
Attached patch bug23.patch (obsolete) — Splinter Review
Refresh view only if OK button is clicked
Attachment #8483725 - Attachment is obsolete: true
Attachment #8483725 - Flags: review?(mconley)
Attachment #8484099 - Flags: review?(mconley)
Thanks for your patch! I'll have a look soon - just been having a heck of a time building comm-central lately. :/
Comment on attachment 8484099 [details] [diff] [review]
bug23.patch

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

This looks good Stephen! I just have one suggestion.

::: mail/components/addrbook/content/abCommon.js
@@ +480,5 @@
>  }
>  
>  function goEditListDialog(abCard, listURI)
>  {
> +  window.ok = false;  

I guess you're doing this so that we don't force a refresh if we've cancelled out of the dialog?

If that's the case, I approve of the idea, but I have a different suggestion for the technique. Instead of having abMailListDialog reach back into the opener to set a variable, we can pass something to openDialog as an "out param", and read that param once openDialog returns.

See https://developer.mozilla.org/en-US/docs/Web/API/Window.openDialog#Returning_values_from_the_dialog, for example.
Attachment #8484099 - Flags: review?(mconley) → review-
Attached patch bug1061648.patch (obsolete) — Splinter Review
Thanks for the great feedback, Mike, and the pointer to the technique to get dialog return values. I like that much better than my original approach. Let me know what you think about this attempt.
Attachment #8484099 - Attachment is obsolete: true
Attachment #8485028 - Flags: review?(mconley)
Comment on attachment 8485028 [details] [diff] [review]
bug1061648.patch

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

This looks really good! Just some style nits, and then I think we're good to go.

::: mail/components/addrbook/content/abCommon.js
@@ +480,5 @@
>  }
>  
>  function goEditListDialog(abCard, listURI)
>  {
> +  var params = {inn:{abCard:abCard, listURI:listURI}, out:{ok:false}};

We should use let instead of var now.

Nit: "in" instead of "inn".

Also, let's style it like this:

let params = {
  in: {
    abCard: abCard,
    listUri: listUri,
  },
  out: {
    ok: false,
  },
};

We should also document what "ok" represents.

::: mailnews/addrbook/content/abMailListDialog.js
@@ +247,5 @@
>  {
>    InitCommonJS();
>  
> +  gListCard = window.arguments[0].inn.abCard;
> +  var listUri  = window.arguments[0].inn.listURI;

Note that I changed listURI to listUri in my example above. This might be a nice opportunity to be consistent here.
Attachment #8485028 - Flags: review?(mconley) → review-
Attached patch bug1061648.patchSplinter Review
Revised according to feedback from mconley
Attachment #8485028 - Attachment is obsolete: true
Attachment #8485153 - Flags: review?(mconley)
Comment on attachment 8485153 [details] [diff] [review]
bug1061648.patch

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

Unfortunately, I can't run the test suite just yet, but this does look good. :) Thanks Stephen! Someone will land this once the tree re-opens.
Attachment #8485153 - Flags: review?(mconley) → review+
Stephen, thanks for providing this patch! It has now been checked into comm-central and should be available in the next nightly build!

http://hg.mozilla.org/comm-central/rev/c939d7b1b732
Assignee: nobody → sschaub
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
Depends on: 1083785
Blocks: 628035
Blocks: 758969
Blocks: 1106783
Stephen, anybody daring to fix stuff in the jungle of the current ("old") AB is a hero by definition, so thanks a lot for that, you rock!

Having said that, true heros should also suffer a bit, so this adventure here was obviously a bit too simple ;)

I was about to close Bug 434014 which is essentially the same as this bug, but something in that bug's description and your precise wording of current intended behaviour before your patch, to "notify listeners (including nsAbView)" made me think, "perhaps there's other consumers/listeners out there which we don't reach..." and indeed, current patch here, attachment 8485153 [details] [diff] [review], breaks things for composition's contacts side bar which also allows editing List Properties. And it's wired up so weirdly that doing so actually backfires (temporarily) into the list pane of the main AB window with the List selected... Welcome to the jungle! :)
Thomas, good catch! I failed to test this patch with the composition contacts side bar. I'll take a look. And thanks for the welcome!
:)

(In reply to Thomas D. from comment #11)
> current patch here, attachment 8485153 [details] [diff] [review], breaks things for composition's
> contacts side bar which also allows editing List Properties. And it's wired
> up so weirdly that doing so actually backfires (temporarily) into the list
> pane of the main AB window with the List selected... Welcome to the jungle!
> :)

Filed as followup bug 1106783...
Per my tentative reading of Bug 434014 Comment 0 it looks as if that notification/listening system which you deliberately skipped here so far should be part of the solution for this bug...
Per Bug 406921 Comment 9, there are at least 15 scenarios which you'll have to check...

No need to back out this bug (imo):

Symptoms of bug 1106783 are also weird but do not involve dataloss dangers afasics, so imo it's fine to keep this anyway even if we're not fast enough to fix bug 1106783.
Blocks: 434014
You need to log in before you can comment on or make changes to this bug.