882365 - Add top bookmarks to the about:home bookmarks page

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

Show a list of top bookmarks and pinned sites in the about:home's bookmarks page.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: Big huge TopBookmarksView

Apologies for the super huge patch. I was cleaning up things here and there -- and didn't really see what size it was until I did a | hg qref | :(

This adds a TopBookmarksView.java (Note: This is not added to the BookmarksPage yet. That'll be a separate patch. This just adds it to the code base :P). This is a height constrained view.
The items of this view are of type TopBookmarkItemView.java. The thumbnail used in this is a height constrained one, and also can show a dominant color filter (from the favicon), if there is no thumbnail. That view is BookmarkThumbnailView.

Changes over existing TopSitesView:
1. Split all views into individual classes. (oooh. so cleaan)
2. TopSitesView was using a SimpleCursorAdapter. We don't need that as we don't use its functionality of using a layout and it doing work for us. So, changed it to CursorAdapter. The "getView()" of CursorAdapter is implemented. We need to supply a "newView()" and a "bindView()". That's done based on old "getView()".
3. There are few private methods used in just one another place in same file. They are now merged.
4. LoadBookmarksTask will load bookmarks. This would call LoadThumbnailsTask, if there are bookmarks. These are UiAsyncTasks, put in a separate private class. This cleans up some of the old code.
5. onMeasure() logic of TopBookmarksView is now completely different. It's based on the usual GridView style. The TopSitesView's onMeasure() didn't really ask its children to measure. Instead was constraining the thumbnail size and using it. However, the same approach will fail here as the thumbnail size and the TopBookmarkItemView's size are different. The text is now below (and not over) the thumbnail. Hence, moved the logic of height constraining into BookmarkThumbnailView. TopBookmarkItemView will do its usual "measure all my children" part.
6. TopBookmarkItemView is basically a view holder. It takes care of setting an "empty" state, when there is no valid url. This is used for showing "Add a bookmark".
7. BookmarkThumbnailView is a w:h ratio view. Additionally, it shows a dominant color background, when supplied with a color. The filter values are made as constants -- used only once in entire application. (why 27.34%? should ask Ian! :( ).
8. That's all folks!

Need to do:
1. Attach this to BookmarksPage inside the ListView. (wiping off the sweat in my forehead)
2. Add context menu capabilites.
3. Add pinning site capabilities (mostly a followup).
4. Change the db to only return "bookmarks" and not "top sites".

Additional Note:
The height constrained TopBookmarksView is a good first step in making a GridView of our own. I have code for creating an AdapterView that is backed by an adapter. We can use it by flipping the parent of this view anytime later.

Comment 2

[Sriram Ramasubramanian [:sriram]]

The changes in BookmarksPage and FadedTextView was due to eclipse plugin! :( I'll remove it later.

Comment 3

[:Margaret Leibovic]

Comment on attachment 763859 [details] [diff] [review]
Part 1: Big huge TopBookmarksView

I saw Sriram ping me about this on IRC, so adding to my queue. I easily forget things if there aren't flags to remind me ;)

Comment 4

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #1)

> Apologies for the super huge patch. I was cleaning up things here and there
> -- and didn't really see what size it was until I did a | hg qref | :(

For future reference, you can use qcrecord to deal with this (if you really do want to split the patch up):
https://developer.mozilla.org/en-US/docs/Mercurial_Queues#Wanted

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: Add a border to thumbnails

This patch adds a border to thumbnails. The paint is a static object, initialized only once (as recommended by android). The stroke width is unfortunately dependent on the density. Hence setting it whenever we draw it.

And weirdly, 1dp stroke on ShapeDrawable is thicker than 1dp on Paint. Hence had to double the density value! (why android, why?)

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3: Center crop the thumbnails

Center cropping the thumbnails (default), and center-ing the favicons.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 4: Wrapper list adapter

There is a check in Android's ListView that doesnt allow adding header views on the fly. This throws an exception if an adapter is already set and it is not of type HeaderViewListAdapter.

Why android, why?

Instead, we can set a HeaderViewListAdapter to start with and add headers on the fly. This will add headers happily. And when a new adapter is set, (from Android code):


        if (mHeaderViewInfos.size() > 0|| mFooterViewInfos.size() > 0) {
            mAdapter = new HeaderViewListAdapter(mHeaderViewInfos, mFooterViewInfos, adapter);
        } else {
            mAdapter = adapter;
        }

We are still happy :D

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 4: Wrapper list adapter

Unwanted headers in previous patch. Oh Eclipse!

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 5: Header recalibration

We add a "header" view for "folders", when are in opened state. Now, we are adding one more "header" for GridViews. So the existing check for "getHeadersCount == 1" fails :P

Cleaning up that to take care of all the headers we might actually have.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 6: Scrolling ListView

Since the GridView is scrollable, it eats all ACTION_MOVE events. This makes the ListView not scroll, when scrolled from GridView (which is height restricted).

This is a hack (ya! until we implement our own AdapterView), that will pass the events to GridView, until/unless its an ACTION_MOVE. Also, the ListView sets itself up and ready to process the next event. If the next event was anything but ACTION_MOVE, GridView will consume it. If it was a move, ListView will consume it, and thereby we would scroll (happilly ever after -- until next ACTION_UP).

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 763859 [details] [diff] [review]
Part 1: Big huge TopBookmarksView

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

Looks pretty good. Just want to see a cleanup version and get some questions answered before giving my r+.

::: mobile/android/base/home/BookmarkThumbnailView.java
@@ +20,5 @@
> +public class BookmarkThumbnailView extends ImageView {
> +    private static final String LOGTAG = "GeckoBookmarkThumbnailView";
> +
> +    // 27.34% opacity filter for the dominant color.
> +    private static final int FILTER = 0x46FFFFFF;

COLOR_FILTER?

::: mobile/android/base/home/BookmarksPage.java
@@ -7,5 @@
>  
>  import org.mozilla.gecko.R;
>  import org.mozilla.gecko.home.HomePager.OnUrlOpenListener;
>  
> -import android.app.Activity;

Unrelated, put this in a separate patch.

::: mobile/android/base/home/FadedTextView.java
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko.home;
>  
> +import org.mozilla.gecko.R;

Unrelated, put this in a separate patch.

::: mobile/android/base/home/TopBookmarkItemView.java
@@ +39,5 @@
> +
> +    // Child views.
> +    private TextView mTitleView;
> +    private ImageView mThumbnailView;
> +    private ImageView mPinView;

Maket these final.

@@ +49,5 @@
> +    // Pinned state.
> +    private boolean mIsPinned = false;
> +
> +    // Empty state.
> +    private boolean mIsEmpty = true;

Isn't it empty when title and url are null? Seems a bit redudant at first sight. Besides, you don't seem to be actually using it for anything meaningul.

@@ +84,5 @@
> +        return drawableState;
> +    }
> +
> +    /**
> +     * Returns the title shown by this view.

No need for the extra description here. The @return is enough.

@@ +93,5 @@
> +        return (!TextUtils.isEmpty(mTitle) ? mTitle : mUrl);
> +    }
> +
> +    /**
> +     * Returns the url shown by this view.

Ditto.

@@ +102,5 @@
> +        return mUrl;
> +    }
> +
> +    /**
> +     * Returns the pinned state of this view.

Ditto.

@@ +111,5 @@
> +        return mIsPinned;
> +    }
> +
> +    /**
> +     * Sets a title for this view.

Same.

@@ +125,5 @@
> +        updateTitleView();
> +    }
> +
> +    /**
> +     * Sets an url for this view.

Same.

@@ +182,5 @@
> +     */
> +    public void displayFavicon(Bitmap favicon) {
> +        if (favicon == null) {
> +            // Should show default favicon.
> +            displayThumbnail(0);

It's not obvious how calling displayThumbnail(0) implies showing default favicon here. Am I missing something?

@@ +192,5 @@
> +    }
> +
> +    /**
> +     * (non-javadoc)
> +     * 

nit: there's a trailing space in all javadocs btw.

@@ +211,5 @@
> +        refreshDrawableState();
> +    }
> +
> +    /**
> +     * (non-javadoc)

This is just noise :-) The method is private, it's enough to know this javadoc is not public.

::: mobile/android/base/home/TopBookmarksView.java
@@ +79,5 @@
> +
> +        // Initialize the adapter.
> +        mAdapter = new TopBookmarksAdapter(getContext(), null);
> +        setAdapter(mAdapter);
> +        

nit: remove trailing spaces here.

@@ +93,5 @@
> +            }
> +        });
> +
> +        // Load the bookmarks.
> +        new LoadBookmarksTask().execute();

We shouldn't be using these AsyncTasks anymore. Please file a follow-up to port this code to use SimpleCursorLoaders (for the bookmarks query and the thumbnails refresh).

@@ +194,5 @@
> +    }
> +
> +    /**
> +     * Returns the url open listener this view uses to open urls.
> +     * 

nit: please remove all trailing spaces.

@@ +240,5 @@
> +                Bitmap bitmap = (thumbnails != null ? thumbnails.get(url) : null);
> +                if (bitmap != null) {
> +                    view.displayThumbnail(bitmap);
> +                } else {
> +                    // This runs in the background thread.

This is not true, is it? Both updateThumbnails() calls in TopBookmarksView happen in the UI thread. Or am I missing something?

@@ +285,5 @@
> +            String title = "";
> +            boolean pinned = false;
> +
> +            // Cursor is already moved to required position.
> +            if (!cursor.isAfterLast()) {

I don't follow. How check isAfterLast() tell me that this is "already in the required position"?

@@ +286,5 @@
> +            boolean pinned = false;
> +
> +            // Cursor is already moved to required position.
> +            if (!cursor.isAfterLast()) {
> +                url = cursor.getString(cursor.getColumnIndex(URLColumns.URL));

getColumnIndexOrThrow() so that we can catch weird DB bugs if they happen.

@@ +287,5 @@
> +
> +            // Cursor is already moved to required position.
> +            if (!cursor.isAfterLast()) {
> +                url = cursor.getString(cursor.getColumnIndex(URLColumns.URL));
> +                title = cursor.getString(cursor.getColumnIndex(URLColumns.TITLE));

Ditto.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 764382 [details] [diff] [review]
Part 2: Add a border to thumbnails

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

Looks good but needs an important fix in the onDraw() implementation.

::: mobile/android/base/home/BookmarkThumbnailView.java
@@ +40,5 @@
> +    // Initializing the static border paint.
> +    static {
> +        sBorderPaint = new Paint(Paint.ANTI_ALIAS_FLAG);
> +        sBorderPaint.setColor(0xFFCFD9E1);
> +        sBorderPaint.setStyle(Paint.Style.STROKE); 

nit: trailing space.

@@ +55,5 @@
>      public BookmarkThumbnailView(Context context, AttributeSet attrs, int defStyle) {
>          super(context, attrs, defStyle);
> +
> +        // A border will be drawn if needed.
> +        setWillNotDraw(false);

This is one of my favourite examples of how *not* to name a setter :-)

@@ +85,5 @@
> +        super.onDraw(canvas);
> +
> +        if (mShowBorder) {
> +            sBorderPaint.setStrokeWidth(mStrokeWidth);
> +            canvas.drawRect(0, 0, canvas.getWidth(), canvas.getHeight(), sBorderPaint);

You should never assume canvas.getWidth()/canvas.getHeight() match the width/height of the view. Use the View's getWidth()/getHeight() instead. Drawing always happens after the view has been measured so it's safe to rely on getWidth()/getHeight() here.

Comment 13

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #12)
> Comment on attachment 764382 [details] [diff] [review]
> Part 2: Add a border to thumbnails
> 
> Review of attachment 764382 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but needs an important fix in the onDraw() implementation.
> 
> ::: mobile/android/base/home/BookmarkThumbnailView.java
> @@ +40,5 @@
> > +    // Initializing the static border paint.
> > +    static {
> > +        sBorderPaint = new Paint(Paint.ANTI_ALIAS_FLAG);
> > +        sBorderPaint.setColor(0xFFCFD9E1);
> > +        sBorderPaint.setStyle(Paint.Style.STROKE); 
> 
> nit: trailing space.
> 
> @@ +55,5 @@
> >      public BookmarkThumbnailView(Context context, AttributeSet attrs, int defStyle) {
> >          super(context, attrs, defStyle);
> > +
> > +        // A border will be drawn if needed.
> > +        setWillNotDraw(false);
> 
> This is one of my favourite examples of how *not* to name a setter :-)
> 
> @@ +85,5 @@
> > +        super.onDraw(canvas);
> > +
> > +        if (mShowBorder) {
> > +            sBorderPaint.setStrokeWidth(mStrokeWidth);
> > +            canvas.drawRect(0, 0, canvas.getWidth(), canvas.getHeight(), sBorderPaint);
> 
> You should never assume canvas.getWidth()/canvas.getHeight() match the
> width/height of the view. Use the View's getWidth()/getHeight() instead.
> Drawing always happens after the view has been measured so it's safe to rely
> on getWidth()/getHeight() here.

Aah True. I missed that.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: Add a border to thumbnails

Fixed whitespace. Changed the getWidth() and getHeight() calls.

Comment 15

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #11)
> Comment on attachment 763859 [details] [diff] [review]
> Part 1: Big huge TopBookmarksView
> 
> Review of attachment 763859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good. Just want to see a cleanup version and get some questions
> answered before giving my r+.
> 

> 
> @@ +49,5 @@
> > +    // Pinned state.
> > +    private boolean mIsPinned = false;
> > +
> > +    // Empty state.
> > +    private boolean mIsEmpty = true;
> 
> Isn't it empty when title and url are null? Seems a bit redudant at first
> sight. Besides, you don't seem to be actually using it for anything
> meaningul.

Marking it "true" just for a reference sake (for readability). This is used to change the color of the mTitleView. When we have a title/url that we can show with the thumbnail, this is marked as false, and a refreshDrawableState() is called. Otherwise, the color is with 50% opacity.

> @@ +182,5 @@
> > +     */
> > +    public void displayFavicon(Bitmap favicon) {
> > +        if (favicon == null) {
> > +            // Should show default favicon.
> > +            displayThumbnail(0);
> 
> It's not obvious how calling displayThumbnail(0) implies showing default
> favicon here. Am I missing something?

Good catch. That was a bug :P Fixed it.

> 
> @@ +240,5 @@
> > +                Bitmap bitmap = (thumbnails != null ? thumbnails.get(url) : null);
> > +                if (bitmap != null) {
> > +                    view.displayThumbnail(bitmap);
> > +                } else {
> > +                    // This runs in the background thread.
> 
> This is not true, is it? Both updateThumbnails() calls in TopBookmarksView
> happen in the UI thread. Or am I missing something?

Oops. I referred to something else in DB. This is not true. Removed the comment. Is it fine for the call to be in UI thread? I can file a followup to move it an async task may be.

> 
> @@ +285,5 @@
> > +            String title = "";
> > +            boolean pinned = false;
> > +
> > +            // Cursor is already moved to required position.
> > +            if (!cursor.isAfterLast()) {
> 
> I don't follow. How check isAfterLast() tell me that this is "already in the
> required position"?

The old code used to move the cursor to the required position. I changed the CursorAdapter, whose getView() does this by default, before calling bindView(). I wanted to mention that here :P I can change the comment to make it convey this fact. (But what comment??)

Comment 16

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: Big huge TopBookmarksView

Fixed everything as per review comments.

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 764388 [details] [diff] [review]
Part 3: Center crop the thumbnails

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

I originally wrote ThumbnailView.java to handle this, because center_crop can cut off the left or right edges in order to make the image fit. Things work here because we actually requrest thumbnails at the right aspect ratio for these, but I wonder if we're better off to inherit BookmarkThumbnailView from ThumbnailView and have it fix this?

Comment 18

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #17)
> Comment on attachment 764388 [details] [diff] [review]
> Part 3: Center crop the thumbnails
> 
> Review of attachment 764388 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I originally wrote ThumbnailView.java to handle this, because center_crop
> can cut off the left or right edges in order to make the image fit. Things
> work here because we actually requrest thumbnails at the right aspect ratio
> for these, but I wonder if we're better off to inherit BookmarkThumbnailView
> from ThumbnailView and have it fix this?

Extending is just an extra work when we have thumbnails of size we want. And even if I extend it, this patch still holds good, as I need to change the scale type from center to centerCrop based on if its a thumbnail or an icon. So I guess extending (considering its an extra work for about:home, while needed on tabs tray), could be separate patch or follow up.

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 764415 [details] [diff] [review]
Part 4: Wrapper list adapter

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

Giving r- to get more background information.

::: mobile/android/base/home/BookmarksListView.java
@@ +72,5 @@
>          mCursorAdapter = new BookmarksListAdapter(getContext());
>  
> +        // Adding a wrapper list adapter that supports headers, by default.
> +        // This allows us to add/remove headers and change the actual adapter on demand.
> +        HeaderViewListAdapter adapter = new HeaderViewListAdapter(null, null, mCursorAdapter);

Not sure I follow. I need more background information to be able to review this change. Is this just to be able to add a header view at any time? As opposed to being forced to do so before setting adapter?

@@ -157,5 @@
>          }
>  
>          // This will update the cursorAdapter to use the new one if it already exists.
>          mCursorAdapter.changeCursor(cursor);
> -        setAdapter(mCursorAdapter);

Why was this call needed in the first place?

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 764483 [details] [diff] [review]
Part 6: Scrolling ListView

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

::: mobile/android/base/home/BookmarksListView.java
@@ +110,5 @@
>          }
>      }
>  
>      @Override
> +    public boolean onInterceptTouchEvent(MotionEvent event) {

Argh, this obviously looks really hacky. Suggestion: try making TopBookmarksView's requestDisallowInterceptTouchEvent() a no-op instead. This is what the AbsListView uses when it knows that the user is trying to scroll and should avoid its parent to grab any touch events from then on. I'm not entirely sure this will work but it's a potentially simpler/less-scary hack.

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 764962 [details] [diff] [review]
Part 2: Add a border to thumbnails

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

Nice.

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 764972 [details] [diff] [review]
Part 1: Big huge TopBookmarksView

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

Looks great. But the favicon loading call should not run in the UI thread. Two options:
1) Remove the favicon loading code from the patch, push it, and file a follow-up to handle it off main thread (this will force us to implement this properly later)
2) Fix this straight away in this patch

::: mobile/android/base/home/TopBookmarkItemView.java
@@ +175,5 @@
> +            return;
> +        }
> +
> +        mThumbnailView.setImageBitmap(favicon);
> +        mThumbnailView.setBackgroundColor(Favicons.getInstance().getFaviconColor(favicon, mUrl));

I assume it's fine to run this color computation in the UI thread?

::: mobile/android/base/home/TopBookmarksView.java
@@ +167,5 @@
> +            }
> +        }
> +
> +        // Find the minimum of bookmarks we need to show, and the one given by the cursor.
> +        int total = Math.min(count > 0 ? count : Integer.MAX_VALUE, mMaxBookmarks);

final

@@ +170,5 @@
> +        // Find the minimum of bookmarks we need to show, and the one given by the cursor.
> +        int total = Math.min(count > 0 ? count : Integer.MAX_VALUE, mMaxBookmarks);
> +
> +        // Number of rows required to show these bookmarks.
> +        int rows = (int) Math.ceil((double) total / mNumColumns);

final

@@ +171,5 @@
> +        int total = Math.min(count > 0 ? count : Integer.MAX_VALUE, mMaxBookmarks);
> +
> +        // Number of rows required to show these bookmarks.
> +        int rows = (int) Math.ceil((double) total / mNumColumns);
> +        int childrenHeight = childHeight * rows;

final

@@ +197,5 @@
> +     * Returns the url open listener this view uses to open urls.
> +     *
> +     * @return A valid url open listener, null if none exists.
> +     */
> +    public OnUrlOpenListener getOnUrlOpenListener() {

Are you using this somewhere? If not, just remove it?

@@ +225,5 @@
> +            // adapter refreshes (e.g. on device rotation).
> +            if (child == null)
> +                continue;
> +
> +            TopBookmarkItemView view = (TopBookmarkItemView) child;

final

@@ +238,5 @@
> +                Bitmap bitmap = (thumbnails != null ? thumbnails.get(url) : null);
> +                if (bitmap != null) {
> +                    view.displayThumbnail(bitmap);
> +                } else {
> +                    bitmap = BrowserDB.getFaviconForUrl(getContext().getContentResolver(), url);

This shouldn't run on the main thread, should it? This will probably cause UI jank once the thumbnails are updated. I'd prefer to get this fixed before pushing. Otherwise it will get lost in the sea of bugs we have to work on in fig.

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 764417 [details] [diff] [review]
Part 5: Header recalibration

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

::: mobile/android/base/home/BookmarksListView.java
@@ +113,5 @@
>      public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
>          final ListView list = (ListView) parent;
>          final int headerCount = list.getHeaderViewsCount();
>  
> +        if (mIsHeaderAdded) {

If wonder if you could simply check if view (the argument) equals mFolderView here. Then you wouldn't need the boolean flag anymore (less error-prone, simpler).

Comment 24

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #19)
> Comment on attachment 764415 [details] [diff] [review]
> Part 4: Wrapper list adapter
> 
> Review of attachment 764415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Giving r- to get more background information.
> 
> ::: mobile/android/base/home/BookmarksListView.java
> @@ +72,5 @@
> >          mCursorAdapter = new BookmarksListAdapter(getContext());
> >  
> > +        // Adding a wrapper list adapter that supports headers, by default.
> > +        // This allows us to add/remove headers and change the actual adapter on demand.
> > +        HeaderViewListAdapter adapter = new HeaderViewListAdapter(null, null, mCursorAdapter);
> 
> Not sure I follow. I need more background information to be able to review
> this change. Is this just to be able to add a header view at any time? As
> opposed to being forced to do so before setting adapter?
> 
> @@ -157,5 @@
> >          }
> >  
> >          // This will update the cursorAdapter to use the new one if it already exists.
> >          mCursorAdapter.changeCursor(cursor);
> > -        setAdapter(mCursorAdapter);
> 
> Why was this call needed in the first place?

refreshListWithCursor() after an AysncTask runs a db query (basically to refresh bookmarks). Once this is done, the ListView removes the existing adapter, by calling setAdapter(null) and then changes the cursor, and adds a new one. This is because, there is a check in ListView that checks whether the supplied adapter is an instance of HeaderViewListAdapter.

But our TopBookmarksView (a header) will be added even before db query. So, setting the adapter as null -- removes the header -- and then adds an adapter -- re-adds it. This causes a flash on the screen, as the space for the TopBookmarksView shrinks and expands -- which doesnt look quite right.

My patch does a simple thing. Adds a HeaderViewListAdapter to start with. So, when TopBookmarksView is added to the list, it stays as-is. The db query can refresh anytime, and the list can refresh the wrapped adapter "mCursorAdapter". Listview will still be happy as the base adapter for it is the wrapper adapter, a HeaderViewListAdapter.

Comment 25

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #23)
> Comment on attachment 764417 [details] [diff] [review]
> Part 5: Header recalibration
> 
> Review of attachment 764417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/BookmarksListView.java
> @@ +113,5 @@
> >      public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
> >          final ListView list = (ListView) parent;
> >          final int headerCount = list.getHeaderViewsCount();
> >  
> > +        if (mIsHeaderAdded) {
> 
> If wonder if you could simply check if view (the argument) equals
> mFolderView here. Then you wouldn't need the boolean flag anymore (less
> error-prone, simpler).

Though we could make that change there, we cannot do that here: https://bugzilla.mozilla.org/attachment.cgi?id=764417&action=diff#a/mobile/android/base/home/BookmarksListView.java_sec4
Hence we need a boolean to track if we have added something or not. (Or, inflate and make mFolderView null on demand -- which is expensive than tracking a boolean).

Comment 26

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #20)
> Comment on attachment 764483 [details] [diff] [review]
> Part 6: Scrolling ListView
> 
> Review of attachment 764483 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/BookmarksListView.java
> @@ +110,5 @@
> >          }
> >      }
> >  
> >      @Override
> > +    public boolean onInterceptTouchEvent(MotionEvent event) {
> 
> Argh, this obviously looks really hacky. Suggestion: try making
> TopBookmarksView's requestDisallowInterceptTouchEvent() a no-op instead.
> This is what the AbsListView uses when it knows that the user is trying to
> scroll and should avoid its parent to grab any touch events from then on.
> I'm not entirely sure this will work but it's a potentially
> simpler/less-scary hack.

I dont understand how a "request..Event()" would help here. I was reading about it. A child can make it true, so that parents cannot intercept on them -- for the duration of that touch. But in our case, we need the parent (ListView) to intercept on the child (GridView). And that particular value is false by default, so we are good.

Comment 27

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #22)
> Comment on attachment 764972 [details] [diff] [review]
> Part 1: Big huge TopBookmarksView
> 
> Review of attachment 764972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great. But the favicon loading call should not run in the UI thread.
> Two options:
> 1) Remove the favicon loading code from the patch, push it, and file a
> follow-up to handle it off main thread (this will force us to implement this
> properly later)
> 2) Fix this straight away in this patch

Moving it to a separate patch, as it might need more work than expected.

> 
> ::: mobile/android/base/home/TopBookmarkItemView.java
> @@ +175,5 @@
> > +            return;
> > +        }
> > +
> > +        mThumbnailView.setImageBitmap(favicon);
> > +        mThumbnailView.setBackgroundColor(Favicons.getInstance().getFaviconColor(favicon, mUrl));
> 
> I assume it's fine to run this color computation in the UI thread?

This is safe as we usually cache the colors for favicons that are already seen. So, when the list view scrolls, we get the colors faster. Normally, calculating it can always happen in UI thread.

> 
> @@ +238,5 @@
> > +                Bitmap bitmap = (thumbnails != null ? thumbnails.get(url) : null);
> > +                if (bitmap != null) {
> > +                    view.displayThumbnail(bitmap);
> > +                } else {
> > +                    bitmap = BrowserDB.getFaviconForUrl(getContext().getContentResolver(), url);
> 
> This shouldn't run on the main thread, should it? This will probably cause
> UI jank once the thumbnails are updated. I'd prefer to get this fixed before
> pushing. Otherwise it will get lost in the sea of bugs we have to work on in
> fig.

Moving it to a separate patch.

Comment 28

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: Big huge TopBookmarksView

Made changes as per review comment.

Comment 29

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 7: Flip the switch

This adds the TopBookmarksView as a header to BookmarksListView.

The other complicated approach would be to create mTopBookmarksView in BookmarksPage and manually add it as a header. Now, when we work on tablets, this would have weird if (isTablet()) clauses and such. Instead, it went the other way of adding the view in XML. Now, android will complain for trying to do an "addView()" on an adapter view and will throw an exception. (There was some code back in the days, where anything inside that XML would be treated as header views. I couldn't find it now. Anyways.. ). So, I override addView() to add the views as headers instead. This keeps BookmarksPage clean.

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 764417 [details] [diff] [review]
Part 5: Header recalibration

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

Looks good (after discussion on IRC).

::: mobile/android/base/home/BookmarksListView.java
@@ +48,5 @@
>      // Folder title for the currently shown list of bookmarks.
>      private BookmarkFolderView mFolderView;
>  
> +    // Check for adding a header view, if needed.
> +    private boolean mIsHeaderAdded = false;

This looks a bit too generic. Maybe mHasTitleHeader? Anyway, something more specific to the intent of this boolean.

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 765489 [details] [diff] [review]
Part 1: Big huge TopBookmarksView

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

Excellent.

::: mobile/android/base/home/TopBookmarksView.java
@@ +213,5 @@
> +            final View child = getChildAt(i);
> +
> +            // The grid view might get temporarily out of sync with the
> +            // adapter refreshes (e.g. on device rotation).
> +            if (child == null)

nit: add {}'s on all one-line if's

@@ +332,5 @@
> +        @Override
> +        protected Map<String, Bitmap> doInBackground(Cursor... params) {
> +            // TopBookmarksAdapter's cursor.
> +            final Cursor adapterCursor = params[0];
> +            if (adapterCursor == null || !adapterCursor.moveToFirst())

Ditto.

@@ +341,5 @@
> +                final String url = adapterCursor.getString(adapterCursor.getColumnIndexOrThrow(URLColumns.URL));
> +                urls.add(url);
> +            } while(adapterCursor.moveToNext());
> +
> +            if (urls.size() == 0)

Ditto.

@@ +353,5 @@
> +            try {
> +                do {
> +                    final String url = cursor.getString(cursor.getColumnIndexOrThrow(Thumbnails.URL));
> +                    final byte[] b = cursor.getBlob(cursor.getColumnIndexOrThrow(Thumbnails.DATA));
> +                    if (b == null)

Ditto.

@@ +357,5 @@
> +                    if (b == null)
> +                        continue;
> +
> +                    Bitmap thumbnail = BitmapUtils.decodeByteArray(b);
> +                    if (thumbnail == null)

Ditto.

@@ +363,5 @@
> +
> +                    thumbnails.put(url, thumbnail);
> +                } while (cursor.moveToNext());
> +            } finally {
> +                if (cursor != null)

Ditto.

Comment 32

[Lucas Rocha (:lucasr)]

Comment on attachment 765510 [details] [diff] [review]
Part 7: Flip the switch

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

Hmm, I feel slightly uncomfortable with having an AdapterView that implements addView() as this goes a bit against the whole concept of adapter-based views (children are managed by the adapter, not added/removed directly). I understand the nice practical advantage of being able to add the top bookmarks grid directly in the layout file but I would prefer an approach where we make it clear that the grid is added as a header view in the listview. It's less "magic" and more explicit.

Comment 33

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #32)
> Comment on attachment 765510 [details] [diff] [review]
> Part 7: Flip the switch
> 
> Review of attachment 765510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm, I feel slightly uncomfortable with having an AdapterView that
> implements addView() as this goes a bit against the whole concept of
> adapter-based views (children are managed by the adapter, not added/removed
> directly). I understand the nice practical advantage of being able to add
> the top bookmarks grid directly in the layout file but I would prefer an
> approach where we make it clear that the grid is added as a header view in
> the listview. It's less "magic" and more explicit.

I have that patch too :P Will post that.

Comment 34

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 7: (option 2) Flip the switch

This goes the traditional way. Add a mTopBookmarksView in BookmarksPage and add it as a header to the ListView.

But somehow I don't like this approach. Now when we land the tablet specific stuff, where the GridView won't be a header, this code will look more like:

.. onCreateView() {
  if (((BrowserApp) getActivity().isTablet()) {
       // just inflate.
  } else {
       // do this patch.
  }

and in onViewCreated() {
   if (isTablet() {
       mTopBookmarks = findViewById(R.id.top_bookmarks);
   } else {
       // we already have it.
   }

Do we want to go through this?
I guess we can add comments to my old patch in the XML that says,

<!-- Note: This will be added as a header. -->

And that would make the code look cleaner. And we aren't going to re-use BookmarksListView. So, overriding addView() doesn't seem scary to me.

Comment 35

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 8: Minor cleanup

From android code I figured out that changeCursor() already does what we need. swapCursor() doesnt close old cursor, but changeCursor() does. [copy/paste error :P]

Comment 36

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 765699 [details] [diff] [review]
Part 8: Minor cleanup

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

::: mobile/android/base/home/TopBookmarksView.java
@@ +302,5 @@
>          }
>  
>          @Override
> +        public void onPostExecute(Cursor cursor) {
> +            // Change to newCursor.

This should have been "new cursor". I can correct it.

Comment 37

[Lucas Rocha (:lucasr)]

Comment on attachment 765699 [details] [diff] [review]
Part 8: Minor cleanup

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

Nice. You should probably merge this one with the patch that introduces TopBookmarksView.

Comment 38

[Wesley Johnston (:wesj)]

Comment on attachment 764388 [details] [diff] [review]
Part 3: Center crop the thumbnails

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

I think the (tiny bit of) work extending ThumbnailView is probably worth it. I'd love it if we could just make the special scale we're using work with setScaleType

view.setScaleType(CrazyGeckoScaling)

but we can't, so maybe just a flag?

view.useGeckoScaling(false);
view.setScaleType(ScaleType.CENTER);

::: mobile/android/base/home/BookmarkThumbnailView.java
@@ +96,5 @@
>       * @param color the color filter to apply over the drawable.
>       */
>      @Override
>      public void setBackgroundColor(int color) {
> +        setScaleType(ScaleType.CENTER);

This shouldn't be in setBackgroundColor. You should probably just call this separately when you set the view to a favicon.

Comment 39

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #38)
> Comment on attachment 764388 [details] [diff] [review]
> Part 3: Center crop the thumbnails
> 
> Review of attachment 764388 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the (tiny bit of) work extending ThumbnailView is probably worth it.
> I'd love it if we could just make the special scale we're using work with
> setScaleType
> 
> view.setScaleType(CrazyGeckoScaling)
> 
> but we can't, so maybe just a flag?
> 
> view.useGeckoScaling(false);
> view.setScaleType(ScaleType.CENTER);
> 
> ::: mobile/android/base/home/BookmarkThumbnailView.java
> @@ +96,5 @@
> >       * @param color the color filter to apply over the drawable.
> >       */
> >      @Override
> >      public void setBackgroundColor(int color) {
> > +        setScaleType(ScaleType.CENTER);
> 
> This shouldn't be in setBackgroundColor. You should probably just call this
> separately when you set the view to a favicon.

Thanks, moving it to bug 885821 for discussions.

Comment 40

[Sriram Ramasubramanian [:sriram]]

Patch 1 & 8 merged and pushed:
http://hg.mozilla.org/projects/fig/rev/f154de5f5ef7

Path 2:
http://hg.mozilla.org/projects/fig/rev/e2a28f0025f1

Patch 3 tracked by bug 885821.

Patch 4 tracked by bug 885481.

Patch 5 pushed with required changes: (mHasFolderHeader)
http://hg.mozilla.org/projects/fig/rev/42c9ee1b0a15

Patch 6 tracked by bug 885485.

Note: This doesn't add the top bookmarks to the list view yet. That's tracked by patch 7. Until then, users (on fig???) won't see any changes.

Comment 41

[Lucas Rocha (:lucasr)]

Comment on attachment 765623 [details] [diff] [review]
Part 7: (option 2) Flip the switch

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

Yeah, I prefer this approach. Just want to see another pass before giving r+.

::: mobile/android/base/home/BookmarksPage.java
@@ +30,5 @@
>      @Override
>      public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
>          super.onCreateView(inflater, container, savedInstanceState);
> +
> +        mTopBookmarks = new TopBookmarksView(getActivity());

I'd reorder this code a bit to make it easier to follow. Something like:

BookmarksListView list = (BookmarksListView) inflater.inflate(R.layout.home_bookmarks_page, container, false);

mTopBookmarks = new TopBookmarksView(getActivity());
list.addHeaderView(mTopBookmarks);

return list;

@@ +31,5 @@
>      public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
>          super.onCreateView(inflater, container, savedInstanceState);
> +
> +        mTopBookmarks = new TopBookmarksView(getActivity());
> +        BookmarksListView list = (BookmarksListView) inflater.inflate(R.layout.home_bookmarks_page, null);

Don't just always pass null for the container here. It will force the inflated view to use default layout params which is not necessarily what we want here. So, use inflater.inflate(R.layout.home_bookmarks_page, container, false) to be safe.

For more informaiton: http://www.doubleencore.com/2013/05/layout-inflation-as-intended/

Also, although I see the point of adding this code to onCreateView(), it seems it would be better to add it to onViewCreated() instead. This way you'd just use mList instead of having to hold a temp ref to it in onCreateView() to add the header view. Less code, win.

Comment 42

[Lucas Rocha (:lucasr)]

Comment on attachment 764483 [details] [diff] [review]
Part 6: Scrolling ListView

Moved to follow-up bug.

Comment 43

[Lucas Rocha (:lucasr)]

Comment on attachment 764415 [details] [diff] [review]
Part 4: Wrapper list adapter

Moved to a follow-up bug.

Comment 44

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 7: (option 2) Flip the switch

Flipped the lines and made the inflation happen with "container" as the parent.

Comment 45

[Lucas Rocha (:lucasr)]

Comment on attachment 766779 [details] [diff] [review]
Part 7: (option 2) Flip the switch

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

Nice.

Comment 46

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/3cedae0f17c3

Comment 47

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f154de5f5ef7
https://hg.mozilla.org/mozilla-central/rev/e2a28f0025f1
https://hg.mozilla.org/mozilla-central/rev/42c9ee1b0a15
https://hg.mozilla.org/mozilla-central/rev/3cedae0f17c3