1061648 - Mailing list display does not refresh correctly after addresses are deleted

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Stephen Schaub]

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.

Comment 1

[Stephen Schaub]

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.

Comment 2

[Stephen Schaub]

Attachment: bug23.patch

Comment 3

[Stephen Schaub]

Attachment: bug23.patch

Refresh view only if OK button is clicked

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Thanks for your patch! I'll have a look soon - just been having a heck of a time building comm-central lately. :/

Comment 5

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 6

[Stephen Schaub]

Attachment: bug1061648.patch

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.

Comment 7

[Mike Conley (: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.

Comment 8

[Stephen Schaub]

Attachment: bug1061648.patch

Revised according to feedback from mconley

Comment 9

[Mike Conley (: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.

Comment 10

[Patrick Cloke [:clokep]]

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

Comment 11

[Thomas D. (:thomas8)]

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! :)

Comment 12

[Stephen Schaub]

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!

Comment 13

[Thomas D. (:thomas8)]

:)

(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.