1022100 - Display 'Search History' card to initial activity launch

Status: RESOLVED FIXED

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

Description

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

Part of the basic mockups.

Ian & Anthony: Should we use the PDF deck's "History" section as a starting point?

Comment 1

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

Attachment: prev_searchhistory_mock1.png

Here's a look at a more card-like history view that I've been working on. I feel like this will be a good warm up (at least UX-wise) for the whole cards metaphor that we might want to carry forward.

I'm thinking the "Recent" section could easily be "Nearby" as well. We might want to discuss that a bit though because it could dramatically change the UX of this I think. So, I just went with "Recent History" for mocks sake. 

The general idea here is that as items get older, we might not want to be caching lots of assets and so we start to simplify these items.

The hopes and dreams for incorporating an 'x' is that it allows for quick user curation of this list, this makes it become just a bit more of a  powerful/useful feature while giving minimal visual affordance.

Still knocking out the kinks with Ian but thought I'd post a WIP for you here.

Comment 2

[:Margaret Leibovic]

We can iterate on the UX of this with fake data, but we'll need bug 1017078 in order to do this for real.

Comment 3

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #0)
> Part of the basic mockups.
> 
> Ian & Anthony: Should we use the PDF deck's "History" section as a starting
> point?

I think we still want the idea of a 'recommendations' section and a 'history' section on the start screen. The big question I have is how those things behave. 

I imagine the 'recommended' section would have specific cards that it opens, but I am less certain of how history should behave. Does it launch specific cards? Does it fire off a new multi search? We will probably want to try this both ways. I sketched out a few options here http://cl.ly/image/1K473Z1v0P3S/o

Comment 4

[Eric Edens [:eedens]]

Attachment: bug-1022100-wip.patch

Major changes:
 - Remove abstraction layers
 - Uses CursorAdapter for history entries
 - Added change notifications to mobile/android/base/db/SearchHistoryProvider.java

PR: https://github.com/ericedens/FirefoxSearch/pull/24

Comment 5

[:Margaret Leibovic]

Comment on attachment 8458910 [details] [diff] [review]
bug-1022100-wip.patch

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

Looking good! This is definitely heading in the right direction.

::: mobile/android/base/db/SearchHistoryProvider.java
@@ +87,5 @@
>          } finally {
>              c.close();
>          }
>  
> +        getContext().getContentResolver().notifyChange(uri, null);

I don't see a similar call in BrowserProvider. Shouldn't the ContentProvider implementation take care of this for us?

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +47,5 @@
> +        // Listen for broadcasts requesting to start a search.
> +        startSearchReceiver = new StartSearchReceiver();
> +        registerReceiver(startSearchReceiver, new IntentFilter(Constants.INTENT_START_SEARCH));
> +
> +        queryHandler = new HistoryQueryHandler(getContentResolver());

Instead of making an empty subclass, you could just use AsyncQueryHandler direclty like this:

queryHandler = new AsyncQueryHandler(getContentResolver()) {};

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ +40,5 @@
>      @Override
> +    public void onCreate(Bundle savedInstanceState) {
> +        super.onCreate(savedInstanceState);
> +        getLoaderManager().initLoader(0, null, this);
> +        cursorAdapter = new SimpleCursorAdapter(getActivity(), R.layout.search_card_history, null,

Using SimpleCursorAdapter is fine for now, but in the future we will probably need to make our own CursorAdapter implementation to add more functionality. Here's an example of a really simple CursorAdapter implementation:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PinSiteDialog.java#207

@@ +54,5 @@
> +            @Override
> +            public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> +                final TextView textView = (TextView) view.findViewById(R.id.site_name);
> +                if (textView != null) {
> +                    ((AcceptsSearchQuery) getActivity()).onSearch((textView.getText().toString()));

Instead of grabbing the text out of the view like this, you should just get it directly from the Cursor. This will make the code more robust if we ever change the view around.

You can follow a pattern like this:

final Cursor c = cursorAdapter.getCursor();
if (c == null || !c.moveToPosition(position)) {
  return;
}

final String query = c.getString(c.getColumnIndexOrThrow(SearchHistory.QUERY));

@@ +66,5 @@
>  
> +    @Override
> +    public void onResume() {
> +        super.onResume();
> +    }

No need to override this method if you're just calling super.

@@ +76,5 @@
> +            listView.setAdapter(null);
> +        }
> +        listView = null;
> +        cursorAdapter.swapCursor(null);
> +        cursorAdapter = null;

You should destroy cursorAdapter in onDestroy, not in onDestroyView (since you're creating it in onCreate).

@@ +82,5 @@
> +
> +    @Override
> +    public Loader<Cursor> onCreateLoader(int id, Bundle args) {
> +        return new CursorLoader(getActivity(), SearchHistory.CONTENT_URI,
> +                PROJECTION, null, null, SearchHistory.DATE_LAST_VISITED + " DESC");

We should probably set a limit here. You may need to add support for a query parameter in SearchHistoryProvider in order to do this. We can just file a follow-up bug about this, but we should fix it sooner rather than later.

::: mobile/android/search/res/layout/search_card_history.xml
@@ +14,5 @@
> +        android:layout_marginRight="10dp"
> +        android:layout_marginTop="20dp"
> +        android:fontFamily="sans-serif-thin"
> +        android:textSize="16sp"/>
> +</FrameLayout>

Let's just make this a TextView. No need for the FrameLayout wrapper.

::: mobile/android/search/res/layout/search_fragment_post_search.xml
@@ +12,5 @@
> +    <WebView
> +        android:layout_width="match_parent"
> +        android:layout_height="match_parent"
> +        android:id="@+id/webview"/>
> +</RelativeLayout>

And we can just make this a WebView. No need for the RelativeLayout wrapper.

::: mobile/android/search/res/layout/search_fragment_pre_search.xml
@@ +14,5 @@
> +        android:id="@+id/list_view"
> +        android:layout_width="match_parent"
> +        android:layout_height="wrap_content"/>
> +
> +</LinearLayout>

And let's just make this a ListView :)

::: mobile/android/search/res/layout/search_stream_header.xml
@@ -8,5 @@
> -           android:scaleType="centerCrop"
> -           android:src="@drawable/search_header"
> -           android:contentDescription="@string/search_header_image_content_description">
> -
> -</ImageView>

Looks like we'll have some conflicts with my work in bug 1022102, but it will be straightforward to resolve.

Comment 6

[Eric Edens [:eedens]]

Attachment: bug-1022100-wip.v2.patch

Thanks for the feedback! 

I've made all of the changes, excepting the query limit on the search history content provider. I'll open a bug for that. 
 
> I don't see a similar call in BrowserProvider. Shouldn't the ContentProvider
> implementation take care of this for us?

When using ContentProvider directly, the implementing class needs to call notifyChange [1]. Though, I checked our AbstractTransactionalProvider [2], and it handles notifyChange when calling insertInTransaction.

[1] https://developer.android.com/reference/android/content/ContentProvider.html#insert%28android.net.Uri,%20android.content.ContentValues%29
[2] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/AbstractTransactionalProvider.java#220

Comment 7

[Eric Edens [:eedens]]

Attachment: bug-1022100-wip.v3.patch

Hey Lucas!

I was hoping to get some feedback on using the CursorAdapter -- specifically whether it's configured properly to update the ListView when there's an insert on the ContentProvider.

Comment 8

[Eric Edens [:eedens]]

Some pointers:

 PreSearchFragment.java -- contains the CursorAdapter and ListView
 SearchHistoryProvider.java -- contains the ContentProvider

Comment 9

[:Margaret Leibovic]

Comment on attachment 8459056 [details] [diff] [review]
bug-1022100-wip.v3.patch

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

I have a few more comments, but this is looking good. I think we could land this soon.

::: mobile/android/search/java/org/mozilla/search/MainActivity.java
@@ +125,5 @@
> +    private class HistoryQueryHandler extends AsyncQueryHandler {
> +        public HistoryQueryHandler(ContentResolver cr) {
> +            super(cr);
> +        }
> +    }

You can get rid of this subclass now that you're not using it anymore.

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +35,2 @@
>  
>          webview = (WebView) mainView.findViewById(R.id.webview);

Now that search_fragment_post_search only contains a WebView, mainView should be this WebView, so you don't need the separate variables.

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ +27,2 @@
>   */
> +public class PreSearchFragment extends Fragment implements LoaderManager.LoaderCallbacks<Cursor> {

I think you should just make a separate subclass to implement LoaderCallbacks, since this is the pattern we generally follow in the rest of our code.

It also seems like it would still make sense to use a ListFragment here. Any reason in particular not to?

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

Now that the search_fragment_pre_search layout only has one view, isn't mainView actually this ListView?

In that case, you don't even need to store the ListView in a member variable, since you can just use getView() to get the view if you need it elsewhere.

@@ +77,5 @@
>  
> +    @Override
> +    public void onDestroyView() {
> +        super.onDestroyView();
> +        if (listView != null) {

Will listView ever be null here?

::: mobile/android/search/res/layout/search_card_history.xml
@@ +3,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<TextView
> +    android:id="@+id/site_name"
> +    xmlns:android="http://schemas.android.com/apk/res/android"

Nit: Make the xmlns attribute first in the list. Also, I don't think you need to give this view an id, since it's the only view in this layout file.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8459056 [details] [diff] [review]
bug-1022100-wip.v3.patch

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

When it comes to change notifications, the main things you have to keep an eye are:
- The content provider is calling ContentResolver.notifyChange() with the correct URIs.
- All cursors generated from a query in the content provider have correct notification URIs.
- The Loader associated with the returned cursors are properly setting ContentObservers.

You seem to be doing all these things here.

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ +65,5 @@
> +                    return;
> +                }
> +                final String query = c.getString(c.getColumnIndexOrThrow(SearchHistory.QUERY));
> +                if (!TextUtils.isEmpty(query)) {
> +                    ((AcceptsSearchQuery) getActivity()).onSearch(query);

Better fail fast on fragment creation here to avoid bugs that are hard to find. Set the listener from activity in onAttach/onDetach. See, for example, how TopSitesPanel does it.

@@ +70,5 @@
> +                }
> +            }
> +        });
> +        // Hide the default dividers.
> +        listView.setDivider(null);

This should be done in the style associated with this ListView.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8459056 [details] [diff] [review]
bug-1022100-wip.v3.patch

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

(Oops, looks like my flag got reset)

Comment 12

[Eric Edens [:eedens]]

Attachment: p1.bug-1022100.v4.patch

Pulled Fennec changes into its own patch.

Comment 13

[Eric Edens [:eedens]]

Attachment: p2.bug-1041641.v4.patch

Thanks Lucas and Margaret! I've made the updates, with some notes below:

> It also seems like it would still make sense to use a ListFragment here. Any
> reason in particular not to?

ListFragment supports ListAdapter, but not CursorAdapter :/

> you don't even need to store the ListView in a member variable,
> since you can just use getView() to get the view if you need it elsewhere.

Hmm -- you're definitely right that we could get away with not storing a reference to the listview. However, we'd have to cast getView every time we need to access the listview, and then those casts could become incorrect if we update the layout xml file. I'd vote we keep a reference to the listview.

> > +<TextView
> > +    android:id="@+id/site_name"
> > +    xmlns:android="http://schemas.android.com/apk/res/android"
> 
> Nit: Make the xmlns attribute first in the list. Also, I don't think you
> need to give this view an id, since it's the only view in this layout file.

The SimpleCursorAdapter uses the id of the textview when populating cards.


Associated PR: https://github.com/ericedens/FirefoxSearch/pull/24/files

Comment 14

[:Margaret Leibovic]

Comment on attachment 8459833 [details] [diff] [review]
p2.bug-1041641.v4.patch

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

Looks great! Just a few nits, then I can help land this on fx-team.

::: mobile/android/search/java/org/mozilla/search/PreSearchFragment.java
@@ +34,5 @@
> +    private ListView listView;
> +
> +    private final String[] PROJECTION = new String[]{SearchHistory.QUERY, SearchHistory._ID};
> +
> +    private static final int LOADER_CALLBACK_ID = 1;

Nit: This is the loader's id, not the callback's id, so I think just LOADER_ID or maybe LOADER_ID_SEARCH_HISTORY would be a better name.

@@ +100,5 @@
> +        listView.setAdapter(null);
> +        listView = null;
> +    }
> +
> +    private class LoaderCallback implements LoaderManager.LoaderCallbacks<Cursor> {

Nit: I think CursorLoaderCallbacks would be a better name for this class. Or even SearchHistoryLoaderCallbacks.

::: mobile/android/search/java/org/mozilla/search/stream/MockHistoryProvider.java
@@ +61,5 @@
> +        }
> +
> +        queries.add(0, query);
> +
> +        writeToSp(queries);

I didn't looked at this class closely before, since it's just temporary, but I want to confirm that this won't be used if the search activity is build with Fennec, right? I wouldn't want us to be unintentionally stuffing things into SharedPreferences.

Comment 15

[Eric Edens [:eedens]]

Comment on attachment 8459827 [details] [diff] [review]
p1.bug-1022100.v4.patch

Patch name referenced the wrong bug.

Comment 16

[Eric Edens [:eedens]]

Attachment: p2.bug-1022100.v5.patch

Carrying r+ from previous patch.

Good catch on the MockHistory cruft. That definitely shouldn't get committed to MC; I modified the Grunt export script to stop that from happening moving forward.

Comment 17

[:Margaret Leibovic]

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

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d49cfaebea9d
https://hg.mozilla.org/mozilla-central/rev/c7872ffb3adc