Closed Bug 877870 Opened 7 years ago Closed 7 years ago

Implement search results in editing mode

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

(Whiteboard: fixed-fig)

Attachments

(10 files, 1 obsolete file)

2.74 KB, text/plain
sriram
: review+
Details
1.60 KB, patch
sriram
: review+
Details | Diff | Splinter Review
1.12 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
3.10 KB, text/plain
sriram
: review+
Details
965 bytes, patch
sriram
: review+
Details | Diff | Splinter Review
7.23 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
2.08 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
6.00 KB, patch
sriram
: review+
Details | Diff | Splinter Review
1.94 KB, patch
sriram
: review+
Details | Diff | Splinter Review
19.14 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
Bring back the frecency search UI in the new editing mode (fig repo).
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.
Attachment #759182 - Flags: review?(sriram)
Favicon is not necessarily in the cursor when we use TwoLinePageRow in search results.
Attachment #759185 - Flags: review?(sriram)
The filtering is only meant to happen while we're in editing mode.
Attachment #759186 - Flags: review?(mark.finkle)
Just a quick addition to the TwoLinePageRow view. This patch is optional.
Attachment #759188 - Flags: review?(sriram)
This is required to use TwoLinePageRow in the search results.
Attachment #759192 - Flags: review?(sriram)
This only implements the very basic filtering of results. Favicons and search suggestions are coming in separate patches.
Attachment #759196 - Flags: review?(bnicholson)
Attachment #759196 - Flags: feedback?(mark.finkle)
I'll post patches for the search suggestion bits in bug 871650.
Blocks: 871650
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
Attachment #759182 - Flags: review?(sriram) → review+
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.
Attachment #759185 - Flags: review?(sriram) → review+
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.
Attachment #759192 - Flags: review?(sriram) → review+
So that we can use the same code on for homepager and browsersearch.
Attachment #759781 - Flags: review?(bnicholson)
Attachment #759196 - Attachment is obsolete: true
Attachment #759196 - Flags: review?(bnicholson)
Attachment #759196 - Flags: feedback?(mark.finkle)
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 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?
Attachment #759801 - Flags: review?(bnicholson) → review+
Attachment #759781 - Flags: review?(bnicholson) → review+
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.
Attachment #759199 - Flags: review?(bnicholson) → review+
(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.
Attachment #759186 - Attachment is patch: true
Attachment #759186 - Flags: review?(mark.finkle) → review+
(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().
(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 on attachment 759188 [details]
Add finer-grained APIs to update TwoLinePageRow

r+ with methods made private.
Attachment #759188 - Flags: review?(sriram) → review+
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?
(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.
Attachment #759782 - Flags: review?(sriram) → review+
Attachment #759783 - Flags: review?(sriram) → review+
Blocks: 881774
Blocks: 881776
Blocks: 881777
Blocks: 881779
Blocks: 881780
Blocks: 881783
Blocks: 882073
Blocks: 882081
Blocks: 882735
You need to log in before you can comment on or make changes to this bug.