1022105 - Support a menu and settings

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

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

In discussions we have said we'd give the user control of default search providers and control of some of the starter cards that show up on initial activity launch. I'm sure other configuration will be needed in the future.

Let's get the infrastructure in place to handle a 3-dot menu and a Settings activity.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_search_menu_mock1.png

Attaching a WIP..

I've mocked up what it would look like to have the "3 dot menu" in the search bar but I think it's starting to get crowded and I would really like this to be reserved for search related activities so that we can keep it simple and elegant. Also, if we consider the possibility that we might add even more items to it in the future, we might have to revisit this sooner rather than later.

So, I've also gone one step further in an effort to design for future iterations of this menu that would probably require space for more than 1/2 items. Currently I'm experimenting with the idea of a bottom "tab" interface (not unlike what we have right now in the History panel on about:home). I think this leaves us a lot more options in the future and also helps create a great mental model of this application. Better reach and visual spacing is also a nice plus.

Thoughts?

Comment 2

[Eric Edens [:eedens]]

Another Android UI pattern is the navigation drawer (https://developer.android.com/design/patterns/navigation-drawer.html).

Regarding the three dots, I like that it is consistent with Android's UI/UX patterns. We could explore ways to hide it -- for example, when the user taps on the search bar, we could animate the search bar to occupy the full width of the screen.

For bottom toolbars, I find these claustrophobic; Safari on iOS is a good example of a viewport that got squeezed too much. Instead of being fixed to the bottom of the viewport, it could be pushed to the bottom of the page. This would free screen space, but also require that a user scrolls down to find a button to the settings menu.

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Eric Edens [:eedens] from comment #2)
> Another Android UI pattern is the navigation drawer
> (https://developer.android.com/design/patterns/navigation-drawer.html).
> 
> Regarding the three dots, I like that it is consistent with Android's UI/UX
> patterns. We could explore ways to hide it -- for example, when the user
> taps on the search bar, we could animate the search bar to occupy the full
> width of the screen.

That's true, but we'd still be jamming more functionality into the top than I'd like. As we move forward and start demanding more organization within the app experience itself, I can see this quickly getting complicated.

> For bottom toolbars, I find these claustrophobic; Safari on iOS is a good
> example of a viewport that got squeezed too much. Instead of being fixed to
> the bottom of the viewport, it could be pushed to the bottom of the page.
> This would free screen space, but also require that a user scrolls down to
> find a button to the settings menu.

Yeah I agree, I'm experimenting with both right now and trying to maybe even do a "pull up to discover" at any time depending on the navigational/structural importance of this bar

Comment 4

[:Margaret Leibovic]

Eric said he would start working on this.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: XXHDPI - Mobile - Pre typing 3.png

Attaching newer mocks to show position and general updated-ness

Comment 6

[Eric Edens [:eedens]]

Attachment: bug-1022105-wip.patch

Chenxia --

Since we're not doing a tablet layout, this uses PreferenceActivity and not PreferenceFragment. Does that sound reasonable?

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8461141 [details] [diff] [review]
bug-1022105-wip.patch

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

Looks good, you should get rid of ConfirmPreference and use an AlertDialog instead.

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ +96,5 @@
> +
> +        mainView.findViewById(R.id.settings_button).setOnClickListener(new View.OnClickListener() {
> +            @Override
> +            public void onClick(View v) {
> +                startActivity(new Intent(getActivity(), SearchPreferencesActivity.class));

I wasn't sure if this was the right use of Context, which led me on a little chase to find this useful comment on using contexts by Brian. Good to read!

https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-February/000518.html

::: mobile/android/search/java/org/mozilla/search/SearchPreferencesActivity.java
@@ +17,5 @@
> +
> +/**
> + * TODO: Change this to PreferenceFragment when we stop supporting devices older than SDK 3.
> + */
> +public class SearchPreferencesActivity extends PreferenceActivity {

Nit: Drop the plural so it matches its parent class - SearchPreferenceActivity.

@@ +38,5 @@
> +
> +    /**
> +     * The new way of handling preferences is the PreferenceFragment, which helps for
> +     * managing complex pref systems (such as master/detail flows on tablets).
> +     */

Nit: This shouldn't be a javadoc-style comment, because it doesn't really describe what the method is doing. Maybe a note in the class javadoc comment, or even a //-style comment in the method?
(I know, this is really nit-picky.)

@@ +43,5 @@
> +    @SuppressWarnings("deprecation")
> +    private void setupPrefsScreen() {
> +        addPreferencesFromResource(R.xml.search_preferences);
> +
> +        DialogPreference pref = (DialogPreference) findPreference("clear_search_history_button");

Nit: I might suggest lifting this out as a private static final String constant.

@@ +44,5 @@
> +    private void setupPrefsScreen() {
> +        addPreferencesFromResource(R.xml.search_preferences);
> +
> +        DialogPreference pref = (DialogPreference) findPreference("clear_search_history_button");
> +        pref.setOnPreferenceChangeListener(new Preference.OnPreferenceChangeListener() {

Instead of making this a DialogPreference (which implies that it has a set of preferences to be saved), it would make more sense to use a (plain) Preference that has an onPreferenceClickListener that creates and shows an AlertDialog. AlertDialog has setPositiveButton() and setNegativeButton() listeners, which is exactly what you're trying to do here.

That means you'd build the AlertDialog in the listener that you attach here, and you wouldn't need the ConfirmPreference (which isn't really a preference anyways, right?).

http://developer.android.com/reference/android/app/AlertDialog.Builder.html

@@ +56,5 @@
> +        });
> +    }
> +
> +    private void clearHistory() {
> +        final AsyncTask<Void, Void, Boolean> clearHistoryTask = new AsyncTask<Void, Void, Boolean>() {

I'm not sure if you would just want to use UiAsyncTask for this - it's the same thing, but does things not on the UI thread. Since you're making DB calls, that might make sense.

@@ +70,5 @@
> +                if (success) {
> +                    Toast.makeText(SearchPreferencesActivity.this, SearchPreferencesActivity.this.getResources()
> +                            .getString(R.string.search_pref_clear_history_confirmation), Toast.LENGTH_SHORT).show();
> +                } else {
> +                    Log.e(SearchPreferencesActivity.class.getSimpleName(), "Error clearing search history.");

We generally use a static final string, but this might be okay - as long as the string doesn't exceed 23 characters, which will cause Log to throw. It might just be easier to hard code the logtag.

See Log.isLoggable():
http://developer.android.com/reference/android/util/Log.html#isLoggable(java.lang.String,%20int)

::: mobile/android/search/java/org/mozilla/search/prefs/ConfirmPreference.java
@@ +11,5 @@
> +/**
> + * The {@link ConfirmPreference} is a preference to show a dialog with OK and Cancel
> + * buttons.
> + * <p>
> + * This preference does not store information in shared prefs.

See comment in SearchPreferencesActivity - you don't need this class and should use an AlertDialog instead.

::: mobile/android/search/manifests/SearchAndroidManifest_activities.xml.in
@@ +14,5 @@
>                  <category android:name="android.intent.category.DEFAULT"/>
>              </intent-filter>
>          </activity>
> +
> +

Nit: Remove extra newline

@@ +21,5 @@
> +            android:label="@string/search_pref_title"
> +            android:parentActivityName="org.mozilla.search.MainActivity"
> +            android:theme="@style/SettingsTheme" >
> +            <meta-data
> +                android:name="android.support.PARENT_ACTIVITY"

Nit: I would at least put android:name on the same line as the meta tag.

::: mobile/android/search/res/layout/search_fragment_pre_search.xml
@@ +1,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<FrameLayout

Is this part of the mocks? I would wonder if it makes more sense to have this 3-dot menu to the right of the search bar (like we do for Fennec). But, I don't feel strongly about it; the menu just sticks out a bit.

::: mobile/android/search/res/values/search_styles.xml
@@ +12,3 @@
>  
>      <style name="BorderLessButton">
>      </style>

Maybe consider making this one a /> terminated style too?

::: mobile/android/search/res/xml/search_preferences.xml
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<PreferenceScreen xmlns:android="http://schemas.android.com/apk/res/android">

I wonder if "search_preferences" is confusing because we already have a "preferences_search".

@@ +6,5 @@
> +    <org.mozilla.search.prefs.ConfirmPreference
> +        android:dialogMessage="@string/search_pref_clear_history_title_dialog_message"
> +        android:dialogTitle="@string/search_pref_clear_history_title_dialog_title"
> +        android:key="clear_search_history_button"
> +        android:negativeButtonText="@string/search_pref_clear_history_cancel"

Android has some built-in strings for these sorts of things, so you might want to consider using R.android.string.cancel and R.android.string.ok instead.

http://developer.android.com/reference/android/R.string.html

...but you should actually not even need this preference - see other comments :)

::: mobile/android/search/strings/search_strings.dtd
@@ +15,5 @@
> +<!ENTITY search_pref_clear_history_title_dialog_message 'Delete all search history from this device?'>
> +<!ENTITY search_pref_clear_history_title_dialog_title 'Clear search history'>
> +<!ENTITY search_pref_clear_history_cancel 'Cancel'>
> +<!ENTITY search_pref_clear_history_ok 'OK'>
> +<!ENTITY search_pref_clear_history_title 'Clear search history'>

I feel a little strange seeing the same string three times in one file, but I think there's a benefit to having smaller granularity for strings, so this is probably okay.

Comment 8

[Eric Edens [:eedens]]

Attachment: bug-1022105-fix.patch

Thanks Chenxia! 

I made all of the changes, excepting one small XML style item. I find XML attributes easier to read when they're stacked -- but if you have a strong preference on this I can make it inline :)

> @@ +21,5 @@
> > +            android:label="@string/search_pref_title"
> > +            android:parentActivityName="org.mozilla.search.MainActivity"
> > +            android:theme="@style/SettingsTheme" >
> > +            <meta-data
> > +                android:name="android.support.PARENT_ACTIVITY"
> 
> Nit: I would at least put android:name on the same line as the meta tag.

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8461709 [details] [diff] [review]
bug-1022105-fix.patch

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

Great, nice cleanup. Just a few nits, but r+ with those fixed.

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ +76,5 @@
>      }
>  
>      @Override
>      public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedState) {
> +        View mainView = inflater.inflate(R.layout.search_fragment_pre_search, container, false);

Nit: make this final.

@@ +77,5 @@
>  
>      @Override
>      public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedState) {
> +        View mainView = inflater.inflate(R.layout.search_fragment_pre_search, container, false);
> +        listView = (ListView) mainView.findViewById(R.id.list_view);

Nit: Newline, and then a comment that you're setting the listview here. There's a lot of stuff going on in this onCreateView, so I'd like to see some comments, just for clarity.

@@ +93,5 @@
>                  }
>              }
>          });
> +
> +        mainView.findViewById(R.id.settings_button).setOnClickListener(new View.OnClickListener() {

Comment here "// Settings menu button" or something simple.

::: mobile/android/search/java/org/mozilla/search/SearchPreferenceActivity.java
@@ +19,5 @@
> +
> +/**
> + * This activity allows users to modify the settings for the search activity.
> + *
> + * A note on implementation. At the moment, we don't have tablet-specific designs.

Tiny tiny nit - "A note on implementation: " Colon instead of period.

@@ +24,5 @@
> + * Therefore, this implementation uses the old-style PreferenceActivity. When
> + * we start optimizing for tablets, we can migrate to Fennec's PreferenceFragment
> + * implementation.
> + *
> + * TODO: Change this to PreferenceFragment when we stop supporting devices older than SDK 3.

You probably mean either SDK 9 or 3.0 - the revisions and the releases have a different numbering scheme.

@@ +29,5 @@
> + */
> +public class SearchPreferenceActivity extends PreferenceActivity {
> +
> +    private static final String CLEAR_SEARCH_HISTORY_BUTTON_KEY = "clear_search_history_button";
> +    private static final String LOG_TAG = SearchPreferenceActivity.class.getSimpleName();

This might still be longer than 23 characters - it's probably better to be consistent with the rest of our code base and just hard code it. (Also nit, put it before the other string declarations to be consistent.)

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#132

::: mobile/android/search/res/layout/search_fragment_pre_search.xml
@@ +1,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<FrameLayout

Have you tried using a LinearLayout instead? I think FrameLayout can be confusing if you start adding more things to it, because the items you add end up stacking the views on top of each other.

One the other hand, if you did a LinearLayout, you'd need a vertical one that also holds a nested horizontal one that contains the menu button (with the correct gravity). It may be fine to just use what you have.

Comment 11

[Eric Edens [:eedens]]

Attachment: bug-1022105-fix.v2.patch

Fixed all items; for the FrameLayout vs LinearLayout:

> Have you tried using a LinearLayout instead? I think FrameLayout can be
> confusing if you start adding more things to it, because the items you add
> end up stacking the views on top of each other.
> 
> One the other hand, if you did a LinearLayout, you'd need a vertical one
> that also holds a nested horizontal one that contains the menu button (with
> the correct gravity). It may be fine to just use what you have.

I think for this particular case, the FrameLayout will work fine. But looking forward, if things get more complex, we might have to switch to a LinearLayout.

Adding margaret for review and (hopefully) check-in.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8462889 [details] [diff] [review]
bug-1022105-fix.v2.patch

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

Looks good, I can land this patch. I just have a few concerns we could address in follow-ups.

::: mobile/android/search/res/layout/search_fragment_pre_search.xml
@@ +20,5 @@
> +        android:layout_height="wrap_content"
> +        android:background="@android:color/transparent"
> +        android:padding="15dp"
> +        android:src="@drawable/ic_action_overflow"
> +        android:text="@string/search_settings_icon"

Why do we need this text if we also have a drawable set?

In any case, we should add a human-readable content description for accessibility. Can you file a follow-up for that?

::: mobile/android/search/strings/search_strings.dtd
@@ +9,5 @@
>  <!ENTITY search_for_something 'Search for something'>
> +
> +<!ENTITY search_pref_title 'Settings'>
> +<!ENTITY search_pref_clear_history_confirmation 'History cleared'>
> +<!ENTITY search_pref_clear_history_dialog_message 'Delete all search history from this device?'>

Did you get UX feedback on this string? It sounds a bit misleading, since you're really just deleting search history for this Fennec instance (one would hope users are smart enough to figure this out, but hey, people don't always understand how things work).

Comment 13

[:Margaret Leibovic]

Landed on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/72cf0099adb6

Comment 14

[Eric Edens [:eedens]]

> ::: mobile/android/search/res/layout/search_fragment_pre_search.xml
> @@ +20,5 @@
> > +        android:layout_height="wrap_content"
> > +        android:background="@android:color/transparent"
> > +        android:padding="15dp"
> > +        android:src="@drawable/ic_action_overflow"
> > +        android:text="@string/search_settings_icon"
> 
> Why do we need this text if we also have a drawable set?

Good catch! Initially it was a button using a unicode vertical ellipsis. After switching to an ImageButton with a drawable, I overlooked removing the text. I'll open a followup.

> In any case, we should add a human-readable content description for
> accessibility. Can you file a follow-up for that?
> 
> ::: mobile/android/search/strings/search_strings.dtd
> @@ +9,5 @@
> >  <!ENTITY search_for_something 'Search for something'>
> > +
> > +<!ENTITY search_pref_title 'Settings'>
> > +<!ENTITY search_pref_clear_history_confirmation 'History cleared'>
> > +<!ENTITY search_pref_clear_history_dialog_message 'Delete all search history from this device?'>
> 
> Did you get UX feedback on this string? It sounds a bit misleading, since
> you're really just deleting search history for this Fennec instance (one
> would hope users are smart enough to figure this out, but hey, people don't
> always understand how things work).

I wanted to get this landed, with the assumption there would be word tweaking coming from the nightly feedback. (Now that you mention it, perhaps it's misleading since we're only clearing search history for the search activity and Fennec, but not every other app on their phone too...)

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/72cf0099adb6

Comment 16

[:Margaret Leibovic]

https://github.com/ericedens/FirefoxSearch/commit/8d97eea2e3ef423551efcf229a41153f8d2339f1

Comment 17

[Flaviu Cos]

Verified as fixed in build 35.0a1 (2014-09-08);
Device: Google Nexus 7 (Android 4.4.4).