858340 - Domain autocompletion

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We should support domain autocompletion like desktop to make typing even faster.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Mostly working. This looks through the top 10 results of your current query and finds a domain (or if there is none, a domain+subdomain) that matches the current text. Still need to test with a more IMEish keyboard like swipe, and work on styling a bit (right now I just select everything after the cursor, but I think I can do something more "composition" like). Build at:

http://people.mozilla.com/~wjohnston/domainComplete.apk

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 733654 [details] [diff] [review]
WIP Patch


>+    public abstract static class AutocompleteHandler {
>+        public AutocompleteHandler() { }
>+        abstract void onAutocomplete(String res);

"res" ? use more characters!

>+            public void afterTextChanged(final Editable s) {
>+                final String text = s.toString();
>+                // return early if we're backspacing through the string, or have no autocomplete results

Line break before the comment

>+                AutocompleteHandler handler = new AutocompleteHandler() {
>+                    public void onAutocomplete(final String res) {

"res"

>+                                if (text.equals(res))
>+                                    return;

I assume text can't be null?

Looks pretty nice. LucasR or JChen might be good reviewers.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #1)

> the current text. Still need to test with a more IMEish keyboard like swipe,
> and work on styling a bit (right now I just select everything after the
> cursor, but I think I can do something more "composition" like).

I don't know. Selection seems more straight forward for users. Composition can be pretty goofy.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v1

I think this is pretty good. Fixed a little bug where host == null, and put in a little optimization where we don't search the path in all cases.

JChen sounds like a good reviewer to me.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 734117 [details] [diff] [review]
Patch v1

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

Great work so far!

At the work week, we also talked about (possibly) autocompleting top Alexa sites, how easy can this code be extended in the future to implement that?

::: mobile/android/base/AwesomeBar.java
@@ +77,5 @@
> +    public abstract static class AutocompleteHandler {
> +        public AutocompleteHandler() { }
> +        abstract void onAutocomplete(String res);
> +    }
> +

Maybe an interface?

@@ +194,4 @@
>              @Override
> +            public void afterTextChanged(final Editable s) {
> +                final String text = s.toString();
> +

I think we don't want to autocomplete searches? So if the user is entering a search, we can bail early. You can use StringUtils.isSearchQuery for checking that.

@@ +197,5 @@
> +
> +                // return early if we're backspacing through the string, or have no autocomplete results
> +                AutocompleteHandler handler = new AutocompleteHandler() {
> +                    public void onAutocomplete(final String result) {
> +                        ThreadUtils.postToUiThread(new Runnable() {

Is AutocompleteHandler always going to run on UI thread? If so, I'd rather have the caller post to UI thread rather than the implementer.

@@ +216,5 @@
> +                        });
> +                    }
> +                };
> +
> +                Log.i(LOGTAG, "Completing " + text + " from " + prevString + " and " + autoComplete);

Nit: can you put this inside 'if (DEBUG)' and add a 'final int DEBUG = false' at the very top?

@@ +217,5 @@
> +                    }
> +                };
> +
> +                Log.i(LOGTAG, "Completing " + text + " from " + prevString + " and " + autoComplete);
> +                // if you're hitting backspace (the string is getting smaller or unchanged), don't autocomplete

Nit: limit line to 100 chars

@@ +220,5 @@
> +                Log.i(LOGTAG, "Completing " + text + " from " + prevString + " and " + autoComplete);
> +                // if you're hitting backspace (the string is getting smaller or unchanged), don't autocomplete
> +                if (prevString != null && (prevString.length() >= text.length())) {
> +                    handler = null;
> +                }

Hmm maybe the other way? i.e. set handler to not null if these conditions are false. Otherwise you'd be creating handlers unnecessarily.

@@ +226,5 @@
> +                // if this is the autocomplete text being set, don't run the filter
> +                if (autoComplete == null || !autoComplete.equals(text)) {
> +                    mAwesomeTabs.filter(text, handler);
> +                    prevString = text;
> +                }

See last comment. If we don't run the filter, we don't have to create the handler at all.

@@ +232,5 @@
>                  // If the AwesomeBar has a composition string, don't call updateGoButton().
>                  // That method resets IME and composition state will be broken.
>                  if (!hasCompositionString(s)) {
>                      updateGoButton(text);
>                  }

I'm a bit concerned about how autocomplete will interfere with IMEs that use composition strings. Maybe we don't autocomplete when there are compositions? See the example here.

::: mobile/android/base/AwesomeBarTabs.java
@@ +326,5 @@
>  
>          // The tabs should only be visible if there's no on-going search
>          mSearching = searchTerm.length() != 0;
>          // reset the pager adapter to force repopulating the cache
> +        //mViewPager.setAdapter(mPagerAdapter);

Why did you comment this out?

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +166,5 @@
>          mHandler.removeMessages(MESSAGE_LOAD_FAVICONS);
>          mHandler = null;
>      }
>  
> +    AwesomeBar.AutocompleteHandler mAutocompleteHandler = null;

Move to top?

@@ +189,5 @@
> +            return;
> +
> +        synchronized (mAutocompleteHandler) {
> +            if (mAutocompleteHandler == null)
> +                return;

If 'mAutocompleteHandler == null', you will get a NPE at synchronized() above.

@@ +199,5 @@
> +            if (res != null) {
> +                mAutocompleteHandler.onAutocomplete(res);
> +                mAutocompleteHandler = null;
> +            }
> +        }

I'm not sure the synchronization here will work as intended. Can you review it more? What if the text changes on the UI thread just as we are autocompleting? Are we going to end up deleting the new text?

@@ +205,5 @@
> +
> +    private String searchHosts(String searchTerm, Cursor cursor, boolean searchPath) {
> +        int i = 0;
> +        if (cursor.moveToFirst()) {
> +            int urlIndex = cursor.getColumnIndexOrThrow(URLColumns.URL);

Is the column guaranteed to exist?

@@ +217,5 @@
> +                // In contrast to desktop, we also strip mobile subdomains,
> +                // since its unlikely users are intentionally typing them
> +                if (host.startsWith("www.")) host = host.substring(4);
> +                else if (host.startsWith("mobile.")) host = host.substring(7);
> +                else if (host.startsWith("m.")) host = host.substring(2);

Can you put 'strip subdomains' as a utility function in StringUtils?

@@ +226,5 @@
> +
> +                if (searchPath) {
> +                    List<String> path = url.getPathSegments();
> +                    for (String seg : path) {
> +                        host += "/" + seg;

This screams 'use StringBuilder!' :)

@@ +227,5 @@
> +                if (searchPath) {
> +                    List<String> path = url.getPathSegments();
> +                    for (String seg : path) {
> +                        host += "/" + seg;
> +                        if (host.startsWith(searchTerm)) {

StringBuilder doesn't have startsWith() but it does have indexOf()

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Need some feedback on threading and IME composition stuff. 

(In reply to Jim Chen [:jchen :nchen] from comment #5)
> At the work week, we also talked about (possibly) autocompleting top Alexa
> sites, how easy can this code be extended in the future to implement that?

I put together a rough implementation of that in bug 858829. Super easy, but requires a separate file read (once). Thinking about ways to improve that, but open to suggestions.
 
> Maybe an interface?
Oh yeah. I meant to do this that way and then forgot. I had to add this in its own file which feels silly :( Not sure if there's a better way to handle it.

> I think we don't want to autocomplete searches? So if the user is entering a
> search, we can bail early. You can use StringUtils.isSearchQuery for
> checking that.
Good idea. I also added a code path to re-use the old autocomplete value if the string still matches. That gets rid of some small flashes as we destroy/overwrite the selected text on each key press and should feel a bit faster.

> Is AutocompleteHandler always going to run on UI thread? If so, I'd rather
> have the caller post to UI thread rather than the implementer.

Done.

> Nit: can you put this inside 'if (DEBUG)' and add a 'final int DEBUG =
> false' at the very top?
I just killed the logging.

> Hmm maybe the other way? i.e. set handler to not null if these conditions
> are false. Otherwise you'd be creating handlers unnecessarily.

Moved all of this to just call Awesomebar, which now implements TextWatcher and AutocompleteResultHandler. i.e. we never have to create a new handler.
 
> I'm a bit concerned about how autocomplete will interfere with IMEs that use
> composition strings. Maybe we don't autocomplete when there are
> compositions? See the example here.

Is there an example? I tried this with a few "swype-like" keyboards and since we don't receive the afterTextChanged call (with these at least) until the user is done typing, we have no problems. But I can imagine there are IME's that do update the text as you go...

> Why did you comment this out?

Accident.

> If 'mAutocompleteHandler == null', you will get a NPE at synchronized()
> above.
I removed this synchronization. mAutocompleteHandler is only written to from the UI thread. With a few little fixes in here, and haven't been able to trigger any problems. I'm checking the results from autocomplete before I apply them to the text (i.e. we shouldn't be able to suddenly change the string completely underneith you).

My main concern is what happens if you type and then backspace quickly. The UI thread should set mAutocompleteHandler = null and we should just not return the result... if we do return a result before the backspace is sent, afterTextChanged should fire after and remove the new autocompleted text. Hoping your experiene with threads and IME can help here a bit.

> Is the column guaranteed to exist?
Yes? If it doesn't we've broken something horribly. I think this should probably throw and fail.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 734825 [details] [diff] [review]
Patch v2

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

Just a few more nits :)

(In reply to Wesley Johnston (:wesj) from comment #6)
> Created attachment 734825 [details] [diff] [review]
> Patch v2
> 
> My main concern is what happens if you type and then backspace quickly. The
> UI thread should set mAutocompleteHandler = null and we should just not
> return the result... if we do return a result before the backspace is sent,
> afterTextChanged should fire after and remove the new autocompleted text.
> Hoping your experiene with threads and IME can help here a bit.

Okay, I think threading is fine then. There may be some edge cases but we can sort that out later :)

::: mobile/android/base/AutocompleteHandler.java
@@ +6,5 @@
> +package org.mozilla.gecko;
> +
> +public interface AutocompleteHandler {
> +    abstract void onAutocomplete(String res);
> +}

Package-level interface as discussed on IRC.

::: mobile/android/base/AwesomeBar.java
@@ +51,3 @@
>  
> +public class AwesomeBar extends GeckoActivity implements AutocompleteHandler,
> +                                                         TextWatcher {

Nit: 'implements ...' on another line, aligned to extends

@@ +75,5 @@
>      private ContextMenuSubject mContextMenuSubject;
>      private boolean mIsUsingSwype;
>      private boolean mDelayRestartInput;
> +    private String mAutoComplete = "";
> +    private String mPrevString = null;

Now these names seem a bit unclear. I think mAutoComplete can be mAutoCompleteResult.

But I'm not sure what mPrevString should be. I was thinking mAutoCompletePrefix or mAutoCompleteBase to indicate that it's the part of the result that the user entered. What do you think? In any case a comment would be helpful.

@@ +735,5 @@
> +    @Override
> +    public void afterTextChanged(final Editable s) {
> +        final String text = s.toString();
> +        int actionBits = mText.getImeOptions() & EditorInfo.IME_MASK_ACTION;
> +        boolean useHandler = true;

boolean useHandler = false?

@@ +737,5 @@
> +        final String text = s.toString();
> +        int actionBits = mText.getImeOptions() & EditorInfo.IME_MASK_ACTION;
> +        boolean useHandler = true;
> +        boolean reuseAutocomplete = false;
> +        if (!StringUtils.isSearchQuery(text, actionBits == EditorInfo.IME_ACTION_SEARCH)) {

So 'actionBits == EditorInfo.IME_ACTION_SEARCH' will happen when the user enters a search, and then deletes the search. In that case we are still in "search" mode even if the user then enters something that can be a URL.

e.g. User enters 'search term'; we switch to search mode; then user deletes that and enters 'goog'. In this case, isSearchQuery will return true. If we still want autocomplete in this case, we would pass in 'false' instead of 'actionBits == EditorInfo.IME_ACTION_SEARCH' to isSearchQuery.

@@ +766,5 @@
> +        // If the AwesomeBar has a composition string, don't call updateGoButton().
> +        // That method resets IME and composition state will be broken.
> +        if (!hasCompositionString(s)) {
> +            updateGoButton(text);
> +        }

This is what I meant for the IME composition example. Here we only call updateGoButton() when there's no composition. I think we should only autocomplete when there's no composition as well. You can try 'if (!hasCompositionString(s) && !StringUtils.isSearchQuery(...' above.

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +78,5 @@
>      private boolean mAnimateSuggestions;
>      private View mSuggestionsOptInPrompt;
>      private Handler mHandler;
>      private ListView mListView;
> +    private AutocompleteHandler mAutocompleteHandler = null;

You should make mAutocompleteHandler volatile since you're using it on more than one thread and you're not using explicit synchronization.

@@ +213,5 @@
> +                final Uri url = Uri.parse(cursor.getString(urlIndex));
> +                String host = StringUtils.stripCommonSubdomains(url.getHost());
> +                // host may be null for about pages
> +                if (host == null)
> +                    continue;

You should make StringUtils.stripCommonSubdomains handle null strings.

@@ +216,5 @@
> +                if (host == null)
> +                    continue;
> +
> +                StringBuilder hostBuilder = new StringBuilder(host);
> +                Log.i(LOGTAG, "Builder: " + hostBuilder.toString());

Remove log before landing.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Thanks for the good reviews. I think I've got everything in here.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 735599 [details] [diff] [review]
Patch v3

I should note, this applies on top of the patch I have in bug 857165 where I also needed these stripSubdomain, removeScheme methods. That's not up for review, but I didn't want to reorganize my queue (i.e. can you look at the entire method?)

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 735599 [details] [diff] [review]
Patch v3

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

Thanks; great work!

You want to run this through try first because cpeterson added some Awesomebar input tests recently.

::: mobile/android/base/AwesomeBar.java
@@ +46,5 @@
>  
>  import java.net.URLEncoder;
>  import java.util.Arrays;
>  import java.util.Collection;
> +import java.lang.StringBuilder;

No need for import

@@ +51,3 @@
>  
> +interface AutocompleteHandler {
> +    abstract void onAutocomplete(String res);

No need for abstract

@@ +715,5 @@
> +
> +    @Override
> +    public void afterTextChanged(final Editable s) {
> +        final String text = s.toString();
> +        int actionBits = mText.getImeOptions() & EditorInfo.IME_MASK_ACTION;

actionBits is not used now; you can remove the line.

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +224,5 @@
> +
> +                    for (String seg : path) {
> +                        hostBuilder.append("/").append(seg);
> +                        if (hostBuilder.indexOf(searchTerm) == 0) {
> +                            return hostBuilder.append("/").toString();

Does path include the filename part? e.g. if "page.html" is the last segment, we don't want to append a '/', but we can fix that in follow up.

Comment 11

[Wesley Johnston (:wesj)]

I had this based on top of some other things that are taking too long to get landed. Rebased and pushed to try finally:

https://tbpl.mozilla.org/?tree=Try&rev=d8a10492e9bc

Comment 12

[Wesley Johnston (:wesj)]

Looked good!
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a10853e457

Comment 13

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Backed out for build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/86ab0edf0dcb

https://tbpl.mozilla.org/php/getParsedLog.php?id=21918804&tree=Mozilla-Inbound

Comment 14

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Possibly a needs-clobber issue. If that's the case, don't forget to push an update to CLOBBER along with your other changes.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch for checkin

Comment 16

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c124a12d219

Comment 17

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/9c124a12d219

Comment 18

[Aaron Train [:aaronmt]]

How does this work? I'm on Nightly (04/20), and I don't see this functionality at all.

Comment 19

[Aaron Train [:aaronmt]]

(In reply to Aaron Train [:aaronmt] from comment #18)
> How does this work? I'm on Nightly (04/20), and I don't see this
> functionality at all.

Nevermind, I see this now. This feature is only for visited URLs.

Comment 20

[u421692]

Test Case added in Moztrap:
https://moztrap.mozilla.org/manage/case/8081/