[Android] Allow setting recently visited search engines as default search engine

chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.preferences;
 
 import android.content.Context;
 import android.content.Intent;
 import android.content.SharedPreferences;
 import android.content.res.Resources;
 import android.os.Build;
 import android.os.Bundle;
 import android.text.SpannableString;
 import android.text.style.ForegroundColorSpan;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.View.AccessibilityDelegate;
 import android.view.View.OnClickListener;
 import android.view.ViewGroup;
 import android.view.accessibility.AccessibilityEvent;
 import android.view.accessibility.AccessibilityNodeInfo;
 import android.widget.BaseAdapter;
 import android.widget.RadioButton;
 import android.widget.TextView;
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.ContextUtils;
+import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.locale.LocaleManager;
 import org.chromium.chrome.browser.omnibox.geo.GeolocationHeader;
 import org.chromium.chrome.browser.preferences.website.ContentSetting;
 import org.chromium.chrome.browser.preferences.website.GeolocationInfo;
 import org.chromium.chrome.browser.preferences.website.SingleWebsitePreferences;
 import org.chromium.chrome.browser.preferences.website.WebsitePreferenceBridge;
 import org.chromium.chrome.browser.search_engines.TemplateUrlService;
 import org.chromium.chrome.browser.search_engines.TemplateUrlService.LoadListener;
 import org.chromium.chrome.browser.search_engines.TemplateUrlService.TemplateUrl;
 import org.chromium.components.location.LocationUtils;
 import org.chromium.ui.text.SpanApplier;
 import org.chromium.ui.text.SpanApplier.SpanInfo;
 
+import java.util.ArrayList;
 import java.util.List;
 
 /**
 * A custom adapter for listing search engines.
 */
 public class SearchEngineAdapter extends BaseAdapter implements LoadListener, OnClickListener {
     /**
      * A callback for reporting progress to the owner.
      */
     public interface SelectSearchEngineCallback {
         /**
          * Called when the search engine data has loaded and we've determined the currently active
          * one.
          * @param name Provides the name of it (with a simplified URL in parenthesis).

[Ian Wen, 2016/10/19 00:03:31]

You should update the javadoc for this function.

[ltian, 2016/10/19 18:56:31]

Interface will be removed and this class will directly update the default search
provider.

          */
-        void currentSearchEngineDetermined(int selectedIndex);
+        void currentSearchEngineDetermined(String keyword);
     }
 
     // The current context.
     private Context mContext;
 
     // The layout inflater to use for the custom views.
     private LayoutInflater mLayoutInflater;
 
     // The callback to use for notifying caller of progress.
     private SelectSearchEngineCallback mCallback;
 
-    // The list of available search engines.
-    private List<TemplateUrl> mSearchEngines;
-    // The position (index into mSearchEngines) of the currently selected search engine. Can be -1
+    // The list of prepopluated and default search engines.
+    private List<TemplateUrl> mPrepopulatedSearchEngines = new ArrayList<TemplateUrl>();
+    // The list of recently visited search engines.
+    private List<TemplateUrl> mRecentVisitedSearchEngines = new ArrayList<TemplateUrl>();

[Ian Wen, 2016/10/19 00:03:30]

How about renaming it to mRecentSearchEngines?

[ltian, 2016/10/19 18:56:31]

Done.

+    // The position (index into mPrepopulatedSearchEngines) of the currently selected search engine.
+    // Can be -1
     // if current search engine is managed and set to something other than the pre-populated values.

[Ian Wen, 2016/10/19 00:03:31]

Please reformat the comments.

[ltian, 2016/10/19 18:56:31]

Done.

     private int mSelectedSearchEnginePosition = -1;
 
     // The position of the default search engine before user's action.
     private int mInitialEnginePosition = -1;
 
+    private static final int VIEW_TYPE_ITEM = 0;
+    private static final int VIEW_TYPE_DIVIDER = 1;

[Ian Wen, 2016/10/19 00:03:31]

Like what I said in last patchset, public members should be listed above
protected and private members; static members should show above nonstatic
members; final member should show above non final members.

you should move #83-#84 to #61.

[ltian, 2016/10/19 18:56:31]

Done.

+
     /**
      * Construct a SearchEngineAdapter.
      * @param context The current context.
      * @param callback The callback to use to communicate back.
      */
     public SearchEngineAdapter(Context context, SelectSearchEngineCallback callback) {
         mContext = context;
         mLayoutInflater = (LayoutInflater) mContext.getSystemService(
                 Context.LAYOUT_INFLATER_SERVICE);
         mCallback = callback;
 
         initEntries();
     }
 
-    /**
-     * @return The index of the selected engine before user's action.
-     */
-    public int getInitialSearchEnginePosition() {
-        return mInitialEnginePosition;
-    }
-
     // Used for testing.
 
     String getValueForTesting() {
         return Integer.toString(mSelectedSearchEnginePosition);
     }
 
-    void setValueForTesting(String value) {
-        searchEngineSelected(Integer.parseInt(value));
+    String setValueForTesting(String value) {
+        return searchEngineSelected(Integer.parseInt(value));
+    }
+
+    String getKeywordForTesting(int index) {
+        return toKeyword(index);
     }
 
     /**
      * Initialize the search engine list.
      */
     private void initEntries() {
         TemplateUrlService templateUrlService = TemplateUrlService.getInstance();
         if (!templateUrlService.isLoaded()) {
             templateUrlService.registerLoadListener(this);
             templateUrlService.load();
             return;  // Flow continues in onTemplateUrlServiceLoaded below.
         }
 
-        // Fetch all the search engine info and the currently active one.
-        mSearchEngines = templateUrlService.getLocalizedSearchEngines();
-        int searchEngineIndex = templateUrlService.getDefaultSearchEngineIndex();
-        // Convert the TemplateUrl index into an index into mSearchEngines.
+        int defaultSearchEngineIndex = templateUrlService.getDefaultSearchEngineIndex();
+        // Fetch all the search engine info and the currently active one, based on their types,
+        // add into different lists.

[Ian Wen, 2016/10/19 00:03:31]

I would simply remove the comments between 126 and 127.

[ltian, 2016/10/19 18:56:31]

Done.

+        for (TemplateUrl templateUrl : templateUrlService.getLocalizedSearchEngines()) {

[Ian Wen, 2016/10/19 00:03:31]

I would rename getLocalizedSearchEngines() to getSearchEngines(), as it is no
longer just localized any more.

[ltian, 2016/10/19 18:56:31]

Done.

+            if (templateUrl.getType() == TemplateUrlService.TYPE_PREPOPULATED
+                    || templateUrl.getType() == TemplateUrlService.TYPE_DEFAULT) {
+                mPrepopulatedSearchEngines.add(templateUrl);
+            } else {
+                mRecentVisitedSearchEngines.add(templateUrl);
+            }
+        }
+
+        // Convert the TemplateUrl index into an index of mSearchEngines.
         mSelectedSearchEnginePosition = -1;
-        for (int i = 0; i < mSearchEngines.size(); ++i) {
-            if (mSearchEngines.get(i).getIndex() == searchEngineIndex) {
+        for (int i = 0; i < mPrepopulatedSearchEngines.size(); ++i) {
+            if (mPrepopulatedSearchEngines.get(i).getIndex() == defaultSearchEngineIndex) {
                 mSelectedSearchEnginePosition = i;
             }
         }
+
+        for (int i = 0; i < mRecentVisitedSearchEngines.size(); ++i) {
+            if (mRecentVisitedSearchEngines.get(i).getIndex() == defaultSearchEngineIndex) {
+                mSelectedSearchEnginePosition = i + mPrepopulatedSearchEngines.size() + 1;

[Ian Wen, 2016/10/19 00:03:31]

Please explain why you need to add one here.

"// Add one to offset the title for recent search engine list"

[ltian, 2016/10/19 18:56:31]

Done.

+            }
+        }
+
         mInitialEnginePosition = mSelectedSearchEnginePosition;
 
         // Report back what is selected.
-        mCallback.currentSearchEngineDetermined(toIndex(mSelectedSearchEnginePosition));
+        mCallback.currentSearchEngineDetermined(toKeyword(mSelectedSearchEnginePosition));
     }
 
-    private int toIndex(int position) {
-        return mSearchEngines.get(position).getIndex();
+    private String toKeyword(int position) {
+        if (position < mPrepopulatedSearchEngines.size()) {
+            return mPrepopulatedSearchEngines.get(position).getKeyword();
+        } else {
+            position -= mPrepopulatedSearchEngines.size() + 1;
+            return mRecentVisitedSearchEngines.get(position).getKeyword();
+        }
     }
 
     // BaseAdapter:
 
     @Override
     public int getCount() {
-        return mSearchEngines == null ? 0 : mSearchEngines.size();
+        return mPrepopulatedSearchEngines == null
+                ? 0
+                : mPrepopulatedSearchEngines.size() + mRecentVisitedSearchEngines.size() + 1;
     }
 
     @Override
     public Object getItem(int pos) {
-        TemplateUrl templateUrl = mSearchEngines.get(pos);
-        return templateUrl.getShortName();
+        if (pos < mPrepopulatedSearchEngines.size()) {
+            return mPrepopulatedSearchEngines.get(pos);
+        } else if (pos > mPrepopulatedSearchEngines.size()) {
+            pos -= mPrepopulatedSearchEngines.size() + 1;
+            return mRecentVisitedSearchEngines.get(pos);
+        }
+        return null;
     }
 
     @Override
     public long getItemId(int position) {
         return position;
     }
 
     @Override
+    public int getItemViewType(int position) {
+        if (position == mPrepopulatedSearchEngines.size()) {
+            return VIEW_TYPE_DIVIDER;
+        } else {
+            return VIEW_TYPE_ITEM;
+        }
+    }
+
+    @Override
     public View getView(int position, View convertView, ViewGroup parent) {
         View view = convertView;
-        if (convertView == null) {
+        TemplateUrl templateUrl = (TemplateUrl) getItem(position);
+        int itemViewType = getItemViewType(position);
+        if (itemViewType == VIEW_TYPE_DIVIDER) {

[Ian Wen, 2016/10/19 00:03:31]

Oops this is a mistake.

The reason why you need to have convertView is that you could recycle the item
view that is scrolled off the screen. In your current approach you are creating
new Views, even if convertView is already inflated.

Check this out:
http://lucasr.org/2012/04/05/performance-tips-for-androids-listview/.

Also you should understand the code in bookmarks:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[ltian, 2016/10/19 18:56:31]

Done.

+            view = mLayoutInflater.inflate(R.layout.search_engine_recent_title, null);
+        } else {
             view = mLayoutInflater.inflate(R.layout.search_engine, null);
         }
+        if (itemViewType == VIEW_TYPE_DIVIDER) {
+            view.setOnClickListener(null);
+            view.setEnabled(false);
+            return view;
+        }
 
         view.setOnClickListener(this);
         view.setTag(position);
 
         // TODO(finnur): There's a tinting bug in the AppCompat lib (see http://crbug.com/474695),
         // which causes the first radiobox to always appear selected, even if it is not. It is being
         // addressed, but in the meantime we should use the native RadioButton instead.
         RadioButton radioButton = (RadioButton) view.findViewById(R.id.radiobutton);
         // On Lollipop this removes the redundant animation ring on selection but on older versions
         // it would cause the radio button to disappear.
         // TODO(finnur): Remove the encompassing if statement once we go back to using the AppCompat
         // control.
         final boolean selected = position == mSelectedSearchEnginePosition;
         if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
             radioButton.setBackgroundResource(0);
         }
         radioButton.setChecked(selected);
 
         TextView description = (TextView) view.findViewById(R.id.description);
-        TemplateUrl templateUrl = mSearchEngines.get(position);
         Resources resources = mContext.getResources();
         description.setText(templateUrl.getShortName());
 
+        TextView url = (TextView) view.findViewById(R.id.url);
+        url.setText(templateUrl.getUrl());
+        if (templateUrl.getType() == TemplateUrlService.TYPE_PREPOPULATED
+                || templateUrl.getType() == TemplateUrlService.TYPE_DEFAULT
+                || templateUrl.getUrl().length() == 0) {
+            url.setVisibility(View.GONE);
+        }
+
         // To improve the explore-by-touch experience, the radio button is hidden from accessibility
         // and instead, "checked" or "not checked" is read along with the search engine's name, e.g.
         // "google.com checked" or "google.com not checked".
         radioButton.setImportantForAccessibility(View.IMPORTANT_FOR_ACCESSIBILITY_NO);
         description.setAccessibilityDelegate(new AccessibilityDelegate() {
             @Override
             public void onInitializeAccessibilityEvent(View host, AccessibilityEvent event) {
                 super.onInitializeAccessibilityEvent(host, event);
                 event.setChecked(selected);
             }
@@ skipping 44 matching lines @@
 
     @Override
     public void onClick(View view) {
         if (view.getTag() == null) {
             onLocationLinkClicked();
         } else {
             searchEngineSelected((int) view.getTag());
         }
     }
 
-    private void searchEngineSelected(int position) {
+    private String searchEngineSelected(int position) {
         // First clean up any automatically added permissions (if any) for the previously selected
         // search engine.
         SharedPreferences sharedPreferences =
                 ContextUtils.getAppSharedPreferences();
         if (sharedPreferences.getBoolean(PrefServiceBridge.LOCATION_AUTO_ALLOWED, false)) {
             if (locationEnabled(mSelectedSearchEnginePosition, false)) {
                 String url = TemplateUrlService.getInstance().getSearchEngineUrlFromTemplateUrl(
-                        toIndex(mSelectedSearchEnginePosition));
+                        toKeyword(mSelectedSearchEnginePosition));
                 WebsitePreferenceBridge.nativeSetGeolocationSettingForOrigin(
                         url, url, ContentSetting.DEFAULT.toInt(), false);
             }
             sharedPreferences.edit().remove(PrefServiceBridge.LOCATION_AUTO_ALLOWED).apply();
         }
 
         // Record the change in search engine.
         mSelectedSearchEnginePosition = position;
 
+        String keyword = toKeyword(mSelectedSearchEnginePosition);
         // Report the change back.
-        mCallback.currentSearchEngineDetermined(toIndex(mSelectedSearchEnginePosition));
+        mCallback.currentSearchEngineDetermined(keyword);
 
+        // If the user has manually set the default search engine, disable auto switching.
+        boolean manualSwitch = mSelectedSearchEnginePosition != mInitialEnginePosition;
+        if (manualSwitch) {
+            RecordUserAction.record("SearchEngine_ManualChange");
+            LocaleManager.getInstance().setSearchEngineAutoSwitch(false);
+        }
         notifyDataSetChanged();
+        return keyword;
     }
 
     private void onLocationLinkClicked() {
         if (!LocationUtils.getInstance().isSystemLocationSettingEnabled()) {
             mContext.startActivity(LocationUtils.getInstance().getSystemLocationSettingsIntent());
         } else {
             Intent settingsIntent = PreferencesLauncher.createIntentForSettingsPage(
                     mContext, SingleWebsitePreferences.class.getName());
             String url = TemplateUrlService.getInstance().getSearchEngineUrlFromTemplateUrl(
-                    toIndex(mSelectedSearchEnginePosition));
+                    toKeyword(mSelectedSearchEnginePosition));
             Bundle fragmentArgs = SingleWebsitePreferences.createFragmentArgsForSite(url);
             fragmentArgs.putBoolean(SingleWebsitePreferences.EXTRA_LOCATION,
                     locationEnabled(mSelectedSearchEnginePosition, true));
             settingsIntent.putExtra(Preferences.EXTRA_SHOW_FRAGMENT_ARGUMENTS, fragmentArgs);
             mContext.startActivity(settingsIntent);
         }
     }
 
     private boolean locationEnabled(int position, boolean checkGeoHeader) {
         if (position == -1) return false;
 
         String url = TemplateUrlService.getInstance().getSearchEngineUrlFromTemplateUrl(
-                toIndex(position));
+                toKeyword(position));
         GeolocationInfo locationSettings = new GeolocationInfo(url, null, false);
         ContentSetting locationPermission = locationSettings.getContentSetting();
         // Handle the case where the geoHeader being sent when no permission has been specified.
         if (locationPermission == ContentSetting.ASK && checkGeoHeader) {
             return GeolocationHeader.isGeoHeaderEnabledForUrl(mContext, url, false);
         }
         return locationPermission == ContentSetting.ALLOW;
     }
 }