Closed Bug 1022100 Opened 6 years ago Closed 6 years ago

Display 'Search History' card to initial activity launch

Categories

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

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: mfinkle, Assigned: eedens)

References

Details

Attachments

(3 files, 4 obsolete files)

Part of the basic mockups.

Ian & Anthony: Should we use the PDF deck's "History" section as a starting point?
Flags: needinfo?(ibarlow)
Flags: needinfo?(alam)
Summary: Display 'search history' card to initial activity launch → Display 'Search History' card to initial activity launch
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.
Flags: needinfo?(alam)
Blocks: search
OS: Linux → Android
Hardware: x86_64 → All
We can iterate on the UX of this with fake data, but we'll need bug 1017078 in order to do this for real.
Depends on: 1017078
(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
Flags: needinfo?(ibarlow)
Assignee: nobody → eedens
Attached patch bug-1022100-wip.patch (obsolete) — Splinter Review
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
Attachment #8458910 - Flags: feedback?(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.
Attachment #8458910 - Flags: feedback?(margaret.leibovic) → feedback+
Attached patch bug-1022100-wip.v2.patch (obsolete) — Splinter Review
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
Attachment #8458910 - Attachment is obsolete: true
Attachment #8458991 - Flags: feedback?(margaret.leibovic)
Attached patch bug-1022100-wip.v3.patch (obsolete) — Splinter Review
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.
Attachment #8458991 - Attachment is obsolete: true
Attachment #8458991 - Flags: feedback?(margaret.leibovic)
Attachment #8459056 - Flags: feedback?(margaret.leibovic)
Attachment #8459056 - Flags: feedback?(lucasr.at.mozilla)
Some pointers:

 PreSearchFragment.java -- contains the CursorAdapter and ListView
 SearchHistoryProvider.java -- contains the ContentProvider
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.
Attachment #8459056 - Flags: feedback?(margaret.leibovic) → feedback+
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.
Attachment #8459056 - Flags: feedback?(margaret.leibovic)
Attachment #8459056 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8459056 - Flags: feedback+
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)
Attachment #8459056 - Flags: feedback?(margaret.leibovic) → feedback+
Pulled Fennec changes into its own patch.
Attachment #8459827 - Flags: review?(margaret.leibovic)
Attached patch p2.bug-1041641.v4.patch (obsolete) — Splinter Review
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
Attachment #8459056 - Attachment is obsolete: true
Attachment #8459833 - Flags: review?(margaret.leibovic)
Attachment #8459827 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8459833 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8459827 [details] [diff] [review]
p1.bug-1022100.v4.patch

Patch name referenced the wrong bug.
Attachment #8459827 - Attachment description: p1.bug-1041641.v4.patch → p1.bug-1022100.v4.patch
Attachment #8459827 - Attachment filename: p1.bug-1041641.v4.patch → p1.bug-1022100.v4.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.
Attachment #8459833 - Attachment is obsolete: true
Attachment #8459882 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d49cfaebea9d
https://hg.mozilla.org/mozilla-central/rev/c7872ffb3adc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.