Implement search results in editing mode

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 26
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-fig)

Attachments

(10 attachments, 1 obsolete attachment)

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
Assignee

Description

6 years ago
Bring back the frecency search UI in the new editing mode (fig repo).
Assignee

Comment 1

6 years ago
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)
Assignee

Comment 2

6 years ago
Favicon is not necessarily in the cursor when we use TwoLinePageRow in search results.
Attachment #759185 - Flags: review?(sriram)
Assignee

Comment 3

6 years ago
The filtering is only meant to happen while we're in editing mode.
Attachment #759186 - Flags: review?(mark.finkle)
Assignee

Comment 4

6 years ago
Just a quick addition to the TwoLinePageRow view. This patch is optional.
Attachment #759188 - Flags: review?(sriram)
Assignee

Comment 5

6 years ago
This is required to use TwoLinePageRow in the search results.
Attachment #759192 - Flags: review?(sriram)
Assignee

Comment 6

6 years ago
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)
Assignee

Comment 8

6 years ago
I'll post patches for the search suggestion bits in bug 871650.
Assignee

Updated

6 years ago
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+
Assignee

Comment 12

6 years ago
So that we can use the same code on for homepager and browsersearch.
Attachment #759781 - Flags: review?(bnicholson)
Assignee

Updated

6 years ago
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+
Assignee

Comment 19

6 years ago
(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+
Assignee

Comment 20

6 years ago
(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().
Assignee

Comment 21

6 years ago
(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?
Assignee

Comment 24

6 years ago
(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+
Assignee

Updated

6 years ago
Blocks: 881774
Assignee

Updated

6 years ago
Blocks: 881776
Assignee

Updated

6 years ago
Blocks: 881777
Assignee

Updated

6 years ago
Blocks: 881779
Assignee

Updated

6 years ago
Blocks: 881780
Assignee

Updated

6 years ago
Blocks: 881783
Assignee

Updated

6 years ago
Blocks: 882073
Assignee

Updated

6 years ago
Blocks: 882081
Assignee

Updated

6 years ago
Blocks: 882735
You need to log in before you can comment on or make changes to this bug.