Closed Bug 1107209 Opened 10 years ago Closed 8 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

31 Branch
defect
Not set
normal

Tracking

(thunderbird48 wontfix, thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr45- wontfix, thunderbird51 fixed)

RESOLVED FIXED
Thunderbird 51.0
Tracking Status
thunderbird48 --- wontfix
thunderbird49 --- fixed
thunderbird50 --- fixed
thunderbird_esr45 - wontfix
thunderbird51 --- fixed

People

(Reporter: aceman, Assigned: aceman)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

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?
See Also: → 1085674
(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.
Seamonkey seems to use the same pattern so maybe they know the history of this.
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(iann_bugzilla)
Looks like those setupAutocomplete calls are not needed. Someone would need to test it yes.
Flags: needinfo?(mkmelin+mozilla)
Neil is more likely to know the history.
Flags: needinfo?(iann_bugzilla) → needinfo?(neil)
(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.
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk 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.
Attached patch WIP patchSplinter Review
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.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #8777135 - Flags: feedback?(mozilla)
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?
(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 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 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?
Attachment #8777135 - Flags: feedback?(mozilla) → feedback+
(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.
Attached patch patch (obsolete) — Splinter Review
Removed the autocompletehandler parts. I'll look at them more in a new bug.
Attachment #8777135 - Attachment is obsolete: true
Attachment #8777515 - Flags: review?(mozilla)
Comment on attachment 8777135 [details] [diff] [review] WIP patch I think we can dare to take it all out ;-)
Attachment #8777135 - Attachment is obsolete: false
Attachment #8777135 - Flags: review+
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 ;-)
Attachment #8777515 - Flags: review?(mozilla) → review+
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.
Attachment #8777515 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
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 ;-)
Attachment #8777135 - Flags: approval-comm-esr45?
Attachment #8777135 - Flags: approval-comm-beta+
Attachment #8777135 - Flags: approval-comm-aurora+
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.
Attachment #8777135 - Flags: approval-comm-esr45? → approval-comm-esr45-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: