1107209 - setupAutocomplete() function hides/shows columns and is called at each keypress

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[:aceman]

The code of the function is like this:
  // if the pref is set to turn on the comment column, honor it here.
  // this element then gets cloned for subsequent rows, so they should
  // honor it as well
  //
  // TODO: do we really need to do this at each keypress?
  try
  {
    if (getPref("mail.autoComplete.highlightNonMatches"))
      autoCompleteWidget.highlightNonMatches = true;

    if (getPref("mail.autoComplete.commentColumn"))
      autoCompleteWidget.showCommentColumn = true;
  } catch (ex)
  {
      // if we can't get this pref, then don't show the columns (which is
      // what the XUL defaults to)
  }

Is this needed to be run at each keypress (called from onRecipientsChanged())? It looks expensive. Do we expect those prefs to change while compose is open and do we want to support dynamic changes to it?

Comment 1

[Wayne Mery (:wsmwk)]

"It looks expensive."  <> trivial

https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Reporting_a_Thunderbird_Performance_Problem_with_G

Comment 2

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #0)

> Is this needed to be run at each keypress (called from
> onRecipientsChanged())?

I don't think we need this at every keypress.

> It looks expensive.

Indeed, too expensive for every keypress compared to the low probability of needing to change this dynamically during typing of recipient.

> Do we expect those prefs to
> change while compose is open and do we want to support dynamic changes to it?

Could we make it so that we check those prefs ONCE for onFocus event of each recipient input field?
This would allow addons to change it at any time, which would probably require focus elsewhere like on a "Show comments" checkbox, then when focus returns to recipient input, we update *once* but we no longer do it at every keypress.

Comment 3

[:aceman]

Seamonkey seems to use the same pattern so maybe they know the history of this.

Comment 4

[Magnus Melin [:mkmelin]]

Looks like those setupAutocomplete calls are not needed. Someone would need to test it yes.

Comment 5

[Ian Neal]

Neil is more likely to know the history.

Comment 6

[[e-mail]]

(In reply to aceman from comment #0)
>   // TODO: do we really need to do this at each keypress?
We don't, do we? Where's your code path showing this happens?

>     if (getPref("mail.autoComplete.highlightNonMatches"))
>       autoCompleteWidget.highlightNonMatches = true;
This is fairly cheap, it's setting an attribute to the value it currently is.

>     if (getPref("mail.autoComplete.commentColumn"))
>       autoCompleteWidget.showCommentColumn = true;
This is even cheaper because it checks the current value in the setter.

Comment 7

[:aceman]

(In reply to [e-mail] from comment #6)
> (In reply to aceman from comment #0)
> >   // TODO: do we really need to do this at each keypress?
> We don't, do we? Where's your code path showing this happens?
https://hg.mozilla.org/comm-central/file/b36d991075f1/mail/components/compose/content/messengercompose.xul -> onRecipientsChanged() -> setupAutocomplete()

Yes, if you looked at the SM version, it may not have this problem. It is possible I made this "on each keypress" behaviour in https://hg.mozilla.org/comm-central/rev/d9a1835283be for TB only.

> >     if (getPref("mail.autoComplete.highlightNonMatches"))
> >       autoCompleteWidget.highlightNonMatches = true;
> This is fairly cheap, it's setting an attribute to the value it currently is.
> 
> >     if (getPref("mail.autoComplete.commentColumn"))
> >       autoCompleteWidget.showCommentColumn = true;
> This is even cheaper because it checks the current value in the setter.
Even if it is cheap, why do it if not necessary. Those optimizations could be accidentally removed.

Comment 8

[:aceman]

Attachment: WIP patch

In addition to only setting the properties once per compose window, instead of at each keypress, this also seems to fix problem observed in bug 1290839:

STR 2:
- Create a new message
- Open contacts sidebar
- add a known address from the sidebar using "Add to To:"
- add an unknown recipient by hand into the second line that opened automatically.
Result: The unknown recipient will show in *black*.

With this patch, highlightnonmatches is set on first row immediatelly on startup. Then it is cloned to all rows. Thus, whenever autocomplete does not find matches on typing and sets nomatch=true, the recipient becomes red (in any row), which seems to be the expected behaviour.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8777135 [details] [diff] [review]
WIP patch

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

Oops, bug 1290839 suddenly caused some movement here ;-)
I'll look at it in the morning.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ -1005,5 @@
>        awMenulistKeyPress(event, event.target);
>    } catch (e) { }
>  }
>  
> -function awRecipientInputCommand(event, inputElement)

How do we arrange for this to run on every keypress? Do we need to cancel that arrangement?

@@ -1007,5 @@
>  }
>  
> -function awRecipientInputCommand(event, inputElement)
> -{
> -  gContentChanged=true;

Is this not needed any more?

Comment 10

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #9)
> ::: mail/components/compose/content/addressingWidgetOverlay.js
> @@ -1005,5 @@
> >        awMenulistKeyPress(event, event.target);
> >    } catch (e) { }
> >  }
> >  
> > -function awRecipientInputCommand(event, inputElement)
> 
> How do we arrange for this to run on every keypress? Do we need to cancel
> that arrangement?

This patch is mainly about removing setupAutocomplete() from onRecipientsChanged, which is called from
 <textbox id="addressCol2#1"> (and each recipient row) via onchange="onRecipientsChanged();" and oninput="onRecipientsChanged();" (see messengercompose.xul) .

> @@ -1007,5 @@
> >  }
> >  
> > -function awRecipientInputCommand(event, inputElement)
> > -{
> > -  gContentChanged=true;
> 
> Is this not needed any more?

I see no callers in base TB or addons.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8777135 [details] [diff] [review]
WIP patch

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

I will try this now. It has status WIP, so what is missing?

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ -1029,5 @@
>    if (addresses.length > 0)
>    {
>      // we need to set up our own autocomplete session and search for results
> -
> -    setupAutocomplete(); // be safe, make sure we are setup

Are you sure this is safe to remove?

@@ -1068,5 @@
>  
>      this.recipientType = recipientType;
>  
>      // set up the auto complete sessions to use
> -    setupAutocomplete();

and this?

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8777135 [details] [diff] [review]
WIP patch

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

I like that "highlightnonmatches" is set before you start typing. This reduces the amount of magic.
Fixes STR 2 (comment #8, originally from bug 1290839 comment #5) nicely.

Two questions remain:
- Why WIP, what else is there to come?
- Are the two removals of setupAutocomplete() OK? Would it hurt to leave them in?

Comment 13

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #12)
> Two questions remain:
> - Why WIP, what else is there to come?

Nothing, if it works. I just wasn't sure yet, you could find something while testing it together with 1290839.

> - Are the two removals of setupAutocomplete() OK? Would it hurt to leave
> them in?

It would probably not hurt to leave them in. It just seems redundant. The function always sets the first row. Is there any case that the attributes could get lost on the first row? I do not see any code in whole c-c or m-c that would set it to false or remove it (except a test). So can it get lost?
Or may those parseAndAddAddresses() or AutomatedAutoCompleteHandler.init() be called before ComposeLoad() where we now run setupAutocomplete() ?

Also why is AutomatedAutoCompleteHandler.init() calling setupAutocomplete() when parseAndAddAddresses() did it just before it? Something seems redundant.

But if you are not sure, I can take this AutomatedAutoCompleteHandler block of functions to a new bug.

Comment 14

[:aceman]

Attachment: patch

Removed the autocompletehandler parts. I'll look at them more in a new bug.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8777135 [details] [diff] [review]
WIP patch

I think we can dare to take it all out ;-)

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8777515 [details] [diff] [review]
patch

Choose the patch you want, you have my blessing.
I'm prepared to wear the blame if we took too much out ;-)

Comment 17

[:aceman]

Comment on attachment 8777515 [details] [diff] [review]
patch

OK, thanks, pushed the full patch. Let's see what happens. I watched it a bit and the parseAndAddAddresses is called when compose is ready, e.g. when dragging recipients from Contacts panel to the recipient rows. And then the attributes are already set.

Comment 18

[:aceman]

https://hg.mozilla.org/comm-central/rev/11fc4645db0101251d10f66343ba08d9698b282b

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8777135 [details] [diff] [review]
WIP patch

[Approval Request Comment]
Regression caused by (bug #): Looks like this has been wrong for a while, most
likely due to switch to toolkit/autocomplete in TB 31.
User impact if declined: Wrong black colour for unknown recipients.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
This one is a little risky, but if we don't push it to a larger audience, we'll never know ;-)

Comment 20

[Jorg K (CEST = GMT+2)]

Aurora (TB 50):
https://hg.mozilla.org/releases/comm-aurora/rev/5be94e438080

Comment 21

[Jorg K (CEST = GMT+2)]

Beta (TB 49):
https://hg.mozilla.org/releases/comm-beta/rev/b73f895ef4ce

Comment 22

[Kent James (:rkent)]

I'm not seeing any good reason to take this in TB45 , the effects are not that critical, and "This one is a little risky, but if we don't push it to a larger audience, we'll never know" does not exactly inspire confidence.