Closed Bug 695307 Opened 9 years ago Closed 8 years ago

Awesome bar

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: lucasr, Assigned: lucasr)

Details

(Keywords: feature, Whiteboard: [birch] [ux needed] [Product Approved][QA+])

Attachments

(4 files)

* Needs UI design
Assignee: nobody → lucasr.at.mozilla
Whiteboard: [birch] [ux needed] [Product Approve]
Whiteboard: [birch] [ux needed] [Product Approve] → [birch] [ux needed] [Product Approved]
Attachment #569149 - Flags: review?(mark.finkle)
Attachment #569150 - Flags: review?(mark.finkle)
Those patches are an initial implementation of the current awesome bar features (frecency + bookmarks + history). There's a lot of details to work on though.

Possible follow-up patches:
- Show favicon on all entries in awesomebar
- Handle search entry focus properly when switching tabs
- Dismiss keyboard when it makes sense to do so
- Every other bug that we haven't seen yet ;-)
Whiteboard: [birch] [ux needed] [Product Approved] → [birch] [ux needed] [Product Approved][QA+]
Attachment #569146 - Flags: review?(mark.finkle) → review+
Comment on attachment 569149 [details] [diff] [review]
(3/4) Implement Bookmarks tab in AwesomeBar

I have seen devs using AsyncQueryHandler, instead of AsyncTask for queries, but it looks like you have things covered OK:
http://developer.android.com/reference/android/content/AsyncQueryHandler.html
http://kurtchen.com/blog/2010/05/18/use-asyncqueryhandler-to-do-async-query/

As long as we don't block on the main UI thread.
Attachment #569149 - Flags: review?(mark.finkle) → review+
Comment on attachment 569145 [details] [diff] [review]
(1/4) Factor out awesome bar search into AwesomeBarTabs

This looks OK, but I have a question about the query: Why not run the query in an AsyncTask? Does this code block the UI?

I'd still be OK with landing this, even if we need to file a followup to get the All Pages query in an AsyncTask. Getting the basic structure in-place is pretty important.
Attachment #569145 - Flags: review?(mark.finkle) → review+
Comment on attachment 569150 [details] [diff] [review]
(4/4) Implement history tab in AwesomeBar

My remedial knowledge of expandable lists and adpaters says this is OK, but I am alittle worried about the amount of work happening in onPostExecute. We need to make sure we don't block the main thread too long, since onPostExecute is executed on the main UI thread. Check logcat for any "strict mode" violations from this code.

We could always lower the MAX_RESULTS a bit if we run into trouble too.
Attachment #569150 - Flags: review?(mark.finkle) → review+
(In reply to Lucas Rocha (:lucasr) from comment #5)
> Those patches are an initial implementation of the current awesome bar
> features (frecency + bookmarks + history). There's a lot of details to work
> on though.
> 
> Possible follow-up patches:
> - Show favicon on all entries in awesomebar
> - Handle search entry focus properly when switching tabs
> - Dismiss keyboard when it makes sense to do so
> - Every other bug that we haven't seen yet ;-)

Please file the followup bugs after landing these patches
(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 569145 [details] [diff] [review] [diff] [details] [review]
> (1/4) Factor out awesome bar search into AwesomeBarTabs
> 
> This looks OK, but I have a question about the query: Why not run the query
> in an AsyncTask? Does this code block the UI?
> 
> I'd still be OK with landing this, even if we need to file a followup to get
> the All Pages query in an AsyncTask. Getting the basic structure in-place is
> pretty important.

CursorAdapter implements Filterable interface and uses Filter to filter list items. From the documentation, calls to filter.filter(String constraint), which is used in CursorAdapter in this case, run on a separate thread. See:

http://developer.android.com/reference/android/widget/Filter.html
(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 569150 [details] [diff] [review] [diff] [details] [review]
> (4/4) Implement history tab in AwesomeBar
> 
> My remedial knowledge of expandable lists and adpaters says this is OK, but
> I am alittle worried about the amount of work happening in onPostExecute. We
> need to make sure we don't block the main thread too long, since
> onPostExecute is executed on the main UI thread. Check logcat for any
> "strict mode" violations from this code.
> 
> We could always lower the MAX_RESULTS a bit if we run into trouble too.

I think it's perfectly possible to move this code to the doInBackground method as we're only dealing with the cursor data and no UI is involved. I'll do it in a follow-up patch.
Forgot to resolve the bug, sorry for the spam.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111026 Firefox/10.0a1 Fennec/10.0a1
Status: RESOLVED → VERIFIED
Keywords: feature
You need to log in before you can comment on or make changes to this bug.