Closed Bug 754265 Opened 9 years ago Closed 7 years ago
Add a dedicated API to provide a final complete value different from the matching one
in bug 720081 I'm using a temporary hack that we can backport to Aurora, though we should modify the result API to provide a way to do this properly. The idea is that the defaultComplete value can differ from the final value we push to textValue when the user confirms a match.
I tried to write tests covering the various code points I touched, existing tests in autocomplete and Places are passing (I'm going to make a Try run though, just in case). This allows to enter a different value when the user confirms a selection, so a result can suggest that a different final value is used. This is useful for inline completion (we were already supporting this through the comment hack) but also in future for searches, since we want to "mask" all of the pointless affilate code in autocomplete. Neil, any chance you may review this along this week? I need it for bug 959573, eventually if that's problematic I may try to ask gavin to find a different reviewer.
Attachment #8403899 - Flags: review?(enndeakin)
Comment on attachment 8403899 [details] [diff] [review] patch v1 >+ nsresult GetResultLabelAt(int32_t aIndex, nsAString & _retval); > private: > nsresult GetResultValueLabelAt(int32_t aIndex, bool aValueOnly, > bool aGetValue, nsAString & _retval); You changed the argument names in the C++ file but not here in the header. >diff --git a/toolkit/components/autocomplete/nsIAutoCompleteController.idl b/toolkit/components/autocomplete/nsIAutoCompleteController.idl >--- a/toolkit/components/autocomplete/nsIAutoCompleteController.idl >+++ b/toolkit/components/autocomplete/nsIAutoCompleteController.idl >@@ -1,17 +1,17 @@ >+ * Get the final value that should be completed when the user confirms >+ * the match at the given index. >+ */ >+ AString getFinalCompleteValueAt(in long index); >+ >+ /* > * Get / set the current search string. Note, setting will not start searching > */ > attribute AString searchString; > }; >diff --git a/toolkit/components/autocomplete/nsIAutoCompleteResult.idl b/toolkit/components/autocomplete/nsIAutoCompleteResult.idl >--- a/toolkit/components/autocomplete/nsIAutoCompleteResult.idl >+++ b/toolkit/components/autocomplete/nsIAutoCompleteResult.idl >@@ -77,14 +77,20 @@ interface nsIAutoCompleteResult : nsISup > > /** >+ * Get the final value that should be completed when the user confirms >+ * the match at the given index. >+ */ >+ AString getFinalCompleteValueAt(in long index); >+ A little confusing to have the same comment on both getFinalCompleteValueAt methods even though the index means something different I think.
Attachment #8403899 - Flags: review?(enndeakin) → review+
Summary: Add a dedicated API to provide a final defaultComplete value different from the matching one → Add a dedicated API to provide a final complete value different from the matching one
addressed comments, proceeding towards SR
looks like I should fix class nsFileResult MOZ_FINAL : public nsIAutoCompleteResult and do another try run, there is some failure in a recent try run I did that seem related to this
I indeed had forgotten to update some nsIAutocompleteResult instances in satchel and passwordmgr and this takes care of those. The changes are pretty much mechanical (adding the method and forwarding it to getValueAt) so not going to re-ask for review. Though if you have any comments feel free to post them. Now I'll push this to Try.
fix a typo, the tests I re-ran locally that were failing are now passing. Doing another try run.
yep, much better now: https://tbpl.mozilla.org/?tree=Try&rev=4352fd5d40f8
Comment on attachment 8408216 [details] [diff] [review] patch v4 (I only reviewed the interface changes)
Attachment #8408216 - Flags: superreview?(gavin.sharp) → superreview+
Target Milestone: --- → mozilla31
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Hi Marco, can you provide a point estimate.
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa?]
Hi Liz, can you review this bug to determine if it requires further QA verification [qa+] or not [qa-].
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa?] → p=0 s=it-31c-30a-29b.3 [qa-]
Whiteboard: p=0 s=it-31c-30a-29b.3 [qa-] → p=3 s=it-31c-30a-29b.3 [qa-]
Any thoughts about bug 1043584, it is the expected behavior?
(In reply to YF (Yang) from comment #13) > Any thoughts about bug 1043584, it is the expected behavior? Any behavioral change is not expected here, so I don't think it can be the expected behavior. We should fix that bug.
You need to log in before you can comment on or make changes to this bug.