754265 - Add a dedicated API to provide a final complete value different from the matching one

Status: VERIFIED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1

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.

Comment 2

[Neil Deakin]

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.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v2

addressed comments, proceeding towards SR

Comment 4

[Marco Bonardo [:mak]]

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

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v3

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.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v4

fix a typo, the tests I re-ran locally that were failing are now passing. Doing another try run.

Comment 7

[Marco Bonardo [:mak]]

yep, much better now: https://tbpl.mozilla.org/?tree=Try&rev=4352fd5d40f8

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8408216 [details] [diff] [review]
patch v4

(I only reviewed the interface changes)

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/669dff46362f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/669dff46362f

Comment 11

[Marco Mucci [:MarcoM]]

Hi Marco, can you provide a point estimate.

Comment 12

[Marco Mucci [:MarcoM]]

Hi Liz, can you review this bug to determine if it requires further QA verification [qa+] or not [qa-].

Comment 13

[YF (Yang)]

Any thoughts about bug 1043584, it is the expected behavior?

Comment 14

[Marco Bonardo [:mak]]

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