877870 - Implement search results in editing mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Bring back the frecency search UI in the new editing mode (fig repo).

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Rename bookmark_item_row.xml to home_item_row.xml

Not entirely sure about the new name, but we should definitely not have more ethan one layout with the very same TwoLinePageRow. Suggestions for names are welcome.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Make favicon loading optional in updateFromCursor()

Favicon is not necessarily in the cursor when we use TwoLinePageRow in search results.

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Only call filtering listener if toolbar is in editing mode

The filtering is only meant to happen while we're in editing mode.

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Add finer-grained APIs to update TwoLinePageRow

Just a quick addition to the TwoLinePageRow view. This patch is optional.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Use favicon mem cache as fallback in TwoLinePageRow

This is required to use TwoLinePageRow in the search results.

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Initial implementation of new browser search fragment

This only implements the very basic filtering of results. Favicons and search suggestions are coming in separate patches.

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Favicons async-loading in the new browser search fragment

Comment 8

[Lucas Rocha (:lucasr)]

I'll post patches for the search suggestion bits in bug 871650.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 759182 [details]
Rename bookmark_item_row.xml to home_item_row.xml

I was thinking of doing this when I pull out the HomeListView while implementing ContextMenu. You beat me :P

Comment 10

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 759185 [details] [diff] [review]
Make favicon loading optional in updateFromCursor()

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

True. Thanks for catching it.

Comment 11

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 759188 [details]
Add finer-grained APIs to update TwoLinePageRow

If these methods aren't going to be called from anywhere else outside, I would like to make them private. My motivation was that, TwoLinePageRow is always backed by a cursor data and didn't want to split them. Feels like extra method calls.

Also, the url currently doesn't support "switch to tab" and "bookmark/reading list" icons. They are going to be compound drawables. I was thinking of implementing them the same way as MenuItemDefault.

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: Factor out code to open url and cancel editing mode

So that we can use the same code on for homepager and browsersearch.

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: Change BookmarkPage to use a listener to open URLs

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: Remove obsolete AboutHomeLoadCompleteListener from BrowserApp

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Initial implementation of new browser search fragment

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 759801 [details] [diff] [review]
Initial implementation of new browser search fragment

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

Lucas, One thing I wonder about is that we're not (at a glance at least) sharing anything between different fragments. Does that mean we're planning to re-implement context menus, switch to tab, whatever other features for each and every fragment independently?

::: mobile/android/base/BrowserApp.java
@@ +391,5 @@
> +
> +            mBrowserSearch.setOnUrlOpenListener(new BrowserSearch.OnUrlOpenListener() {
> +                public void onUrlOpen(String url) {
> +                    openUrl(url);
> +                }

Instead of doing something like this, can we do something more like what's recommended here: http://developer.android.com/guide/components/fragments.html#EventCallbacks

I hate passing around these listeners.

Comment 17

[Brian Nicholson (:bnicholson)]

Comment on attachment 759801 [details] [diff] [review]
Initial implementation of new browser search fragment

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

Looks good overall; r+ with comments addressed.

::: mobile/android/base/BrowserApp.java
@@ +389,5 @@
> +            mBrowserSearch = BrowserSearch.newInstance();
> +            mBrowserSearch.setUserVisibleHint(false);
> +
> +            mBrowserSearch.setOnUrlOpenListener(new BrowserSearch.OnUrlOpenListener() {
> +                public void onUrlOpen(String url) {

Nit: Use @Override

@@ +391,5 @@
> +
> +            mBrowserSearch.setOnUrlOpenListener(new BrowserSearch.OnUrlOpenListener() {
> +                public void onUrlOpen(String url) {
> +                    openUrl(url);
> +                }

I agree with Wes that we might want to use the activity-based callback approach -- this is the officially recommended way to do it, and it's what we did with AboutHome. It enforces the contract by requiring clients to handle this event instead of these setX() calls post-creation.

Also, there's a bug here since you only set the listener when mBrowserSearch is null. If "Don't Keep Activities" is enabled, the fragment will be automatically recreated with the activity, but it won't be assigned a UrlOpenListener.

::: mobile/android/base/BrowserSearch.java
@@ +36,5 @@
> +    // Cursor loader ID for search query
> +    private static final int SEARCH_LOADER_ID = 0;
> +
> +    // Holds the current search term to use in the query
> +    private String mSearchTerm;

This should be declared volatile since it's used in multiple threads.

@@ +107,5 @@
> +    public void onLoaderReset(Loader<Cursor> loader) {
> +        mAdapter.swapCursor(null);
> +    }
> +
> +    public void onItemClick(AdapterView<?> parent, View view, int position, long id) {

Nit: @Override

@@ +114,5 @@
> +            return;
> +        }
> +
> +        final String url = c.getString(c.getColumnIndexOrThrow(URLColumns.URL));
> +        if (mUrlOpenListener != null) {

You won't need this null check anymore if you move to the activity-based event approach mentioned above.

@@ +121,5 @@
> +    }
> +
> +    public void filter(String searchTerm) {
> +        if (TextUtils.isEmpty(searchTerm)) {
> +            return;

Shouldn't we clear results if the user types a letter and then hits backspace? Or is that handled somewhere else?

::: mobile/android/base/SimpleCursorLoader.java
@@ +47,5 @@
> +     * Registers an observer to get notifications from the content provider
> +     * when the cursor needs to be refreshed.
> +     */
> +    private void registerContentObserver(Cursor cursor, ContentObserver observer) {
> +        cursor.registerContentObserver(mObserver);

s/mObserver/observer/

I would just delete this method though and use cursor.registerContentObserver() directly below.

@@ +123,5 @@
> +        }
> +    }
> +
> +    @Override
> +    protected void onReset() {

It's not mentioned in the comments here like it is in the other methods, but hopefully this must also be called from the UI thread since it uses mCursor?

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 759199 [details] [diff] [review]
Favicons async-loading in the new browser search fragment

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

::: mobile/android/base/BrowserSearch.java
@@ +205,5 @@
>          }
>      }
>  
> +    private static class FaviconsCursorLoader extends SimpleCursorLoader {
> +        private ArrayList<String> mUrls;

Nit: Add 'final' modifier.

Comment 19

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #16)
> Comment on attachment 759801 [details] [diff] [review]
> Initial implementation of new browser search fragment
> 
> Review of attachment 759801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Lucas, One thing I wonder about is that we're not (at a glance at least)
> sharing anything between different fragments. Does that mean we're planning
> to re-implement context menus, switch to tab, whatever other features for
> each and every fragment independently?

No, the idea is to have a common parent class that implements the common functionality for HomePager/BrowserSearch. This will be done as part of bug 878136. See:

  https://bugzilla.mozilla.org/show_bug.cgi?id=878136#c3 

> ::: mobile/android/base/BrowserApp.java
> @@ +391,5 @@
> > +
> > +            mBrowserSearch.setOnUrlOpenListener(new BrowserSearch.OnUrlOpenListener() {
> > +                public void onUrlOpen(String url) {
> > +                    openUrl(url);
> > +                }
> 
> Instead of doing something like this, can we do something more like what's
> recommended here:
> http://developer.android.com/guide/components/fragments.html#EventCallbacks
> 
> I hate passing around these listeners.

Fair enough. Will do it.

Comment 20

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #17)
> Comment on attachment 759801 [details] [diff] [review]
> Initial implementation of new browser search fragment
> 
> Review of attachment 759801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good overall; r+ with comments addressed.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +389,5 @@
> > +            mBrowserSearch = BrowserSearch.newInstance();
> > +            mBrowserSearch.setUserVisibleHint(false);
> > +
> > +            mBrowserSearch.setOnUrlOpenListener(new BrowserSearch.OnUrlOpenListener() {
> > +                public void onUrlOpen(String url) {
> 
> Nit: Use @Override
> 
> @@ +391,5 @@
> > +
> > +            mBrowserSearch.setOnUrlOpenListener(new BrowserSearch.OnUrlOpenListener() {
> > +                public void onUrlOpen(String url) {
> > +                    openUrl(url);
> > +                }
> 
> I agree with Wes that we might want to use the activity-based callback
> approach -- this is the officially recommended way to do it, and it's what
> we did with AboutHome. It enforces the contract by requiring clients to
> handle this event instead of these setX() calls post-creation.

No problem, done.

> Also, there's a bug here since you only set the listener when mBrowserSearch
> is null. If "Don't Keep Activities" is enabled, the fragment will be
> automatically recreated with the activity, but it won't be assigned a
> UrlOpenListener.

Nice catch. This code is now gone anyway.

> ::: mobile/android/base/BrowserSearch.java
> @@ +36,5 @@
> > +    // Cursor loader ID for search query
> > +    private static final int SEARCH_LOADER_ID = 0;
> > +
> > +    // Holds the current search term to use in the query
> > +    private String mSearchTerm;
> 
> This should be declared volatile since it's used in multiple threads.
> 
> @@ +107,5 @@
> > +    public void onLoaderReset(Loader<Cursor> loader) {
> > +        mAdapter.swapCursor(null);
> > +    }
> > +
> > +    public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> 
> Nit: @Override

Fixed.

> @@ +114,5 @@
> > +            return;
> > +        }
> > +
> > +        final String url = c.getString(c.getColumnIndexOrThrow(URLColumns.URL));
> > +        if (mUrlOpenListener != null) {
> 
> You won't need this null check anymore if you move to the activity-based
> event approach mentioned above.

Indeed, fixed.

> @@ +121,5 @@
> > +    }
> > +
> > +    public void filter(String searchTerm) {
> > +        if (TextUtils.isEmpty(searchTerm)) {
> > +            return;
> 
> Shouldn't we clear results if the user types a letter and then hits
> backspace? Or is that handled somewhere else?

If the text entry is empty, we simply hide the browser search UI entirely. See where hideBrowserSearch() is called. This isEmpty() check is not strictly necessary.

> ::: mobile/android/base/SimpleCursorLoader.java
> @@ +47,5 @@
> > +     * Registers an observer to get notifications from the content provider
> > +     * when the cursor needs to be refreshed.
> > +     */
> > +    private void registerContentObserver(Cursor cursor, ContentObserver observer) {
> > +        cursor.registerContentObserver(mObserver);
> 
> s/mObserver/observer/
> 
> I would just delete this method though and use
> cursor.registerContentObserver() directly below.

Done.

> @@ +123,5 @@
> > +        }
> > +    }
> > +
> > +    @Override
> > +    protected void onReset() {
> 
> It's not mentioned in the comments here like it is in the other methods, but
> hopefully this must also be called from the UI thread since it uses mCursor?

Yep, it is called from the UI thread. The only method that is called from a worker thread is loadInBackground().

Comment 21

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #11)
> Comment on attachment 759188 [details]
> Add finer-grained APIs to update TwoLinePageRow
> 
> If these methods aren't going to be called from anywhere else outside, I
> would like to make them private. My motivation was that, TwoLinePageRow is
> always backed by a cursor data and didn't want to split them. Feels like
> extra method calls.
> 
> Also, the url currently doesn't support "switch to tab" and
> "bookmark/reading list" icons. They are going to be compound drawables. I
> was thinking of implementing them the same way as MenuItemDefault.

Ok, I'll make the new method private.

Comment 22

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 759188 [details]
Add finer-grained APIs to update TwoLinePageRow

r+ with methods made private.

Comment 23

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 759782 [details] [diff] [review]
Change BookmarkPage to use a listener to open URLs

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

::: mobile/android/base/home/BookmarksPage.java
@@ +74,5 @@
> +        } catch (ClassCastException e) {
> +            throw new ClassCastException(activity.toString()
> +                    + " must implement HomePager.OnUrlOpenListener");
> +        }
> +

I somehow don't like making the activity an OnUrlOpenListener. This is kind of restricting an activity to do it, and will not allow having other url-bar listeners (and the need for an interface becomes obsolete). Could we add this when we add a Page?

Comment 24

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #23)
> Comment on attachment 759782 [details] [diff] [review]
> Change BookmarkPage to use a listener to open URLs
> 
> Review of attachment 759782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/BookmarksPage.java
> @@ +74,5 @@
> > +        } catch (ClassCastException e) {
> > +            throw new ClassCastException(activity.toString()
> > +                    + " must implement HomePager.OnUrlOpenListener");
> > +        }
> > +
> 
> I somehow don't like making the activity an OnUrlOpenListener. This is kind
> of restricting an activity to do it, and will not allow having other url-bar
> listeners (and the need for an interface becomes obsolete). Could we add
> this when we add a Page?

Not sure I follow. Have a look at what is recommended here:

http://developer.android.com/guide/components/fragments.html#EventCallbacks

I actually started with a listener setters but Wes and Brian suggested the approach above (and I agree). The tricky thing with listeners on fragments is that we don't control their life-cycle directly and a lot of how fragments a managed happens asynchronously. So, figuring out when it's safe to set a listener on a fragment is not that simple.

The approach recommended above is the most reliable way to set/unset listeners on a fragment because it's based on the fragment's own life-cycle. Besides, in practice, the only listener we actually want here is the host activity anyway (BrowserApp, in our case). See, for example, how search suggestions is implemented in bug 871650.

If later we ever realize that we need to listen to these events somewhere else (other than the host activity), we can think of an alternative more flexible way. For now, this approach covers what we need. Let's keep it.

Comment 25

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/projects/fig/rev/2e9bcd695801
http://hg.mozilla.org/projects/fig/rev/8b24cd7f46ae
http://hg.mozilla.org/projects/fig/rev/06bd6a216022
http://hg.mozilla.org/projects/fig/rev/33236a7d3ef5
http://hg.mozilla.org/projects/fig/rev/dc61bbe74149
http://hg.mozilla.org/projects/fig/rev/7875f965bb4c
http://hg.mozilla.org/projects/fig/rev/0496e60d608c
http://hg.mozilla.org/projects/fig/rev/0807f3d71b68
http://hg.mozilla.org/projects/fig/rev/68cd82adf040
http://hg.mozilla.org/projects/fig/rev/72e003d38d00

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2e9bcd695801
https://hg.mozilla.org/mozilla-central/rev/8b24cd7f46ae
https://hg.mozilla.org/mozilla-central/rev/06bd6a216022
https://hg.mozilla.org/mozilla-central/rev/33236a7d3ef5
https://hg.mozilla.org/mozilla-central/rev/dc61bbe74149
https://hg.mozilla.org/mozilla-central/rev/7875f965bb4c
https://hg.mozilla.org/mozilla-central/rev/0496e60d608c
https://hg.mozilla.org/mozilla-central/rev/0807f3d71b68
https://hg.mozilla.org/mozilla-central/rev/68cd82adf040
https://hg.mozilla.org/mozilla-central/rev/72e003d38d00