857063 - Use GridLayout instead of GridView in about:home's Top Sites

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

The use of GridView inside a ScrollView is a hack that we should probably avoid. The reason I used GridView in the original implementation was because GridLayout was not available in the Android support library back then. But GridLayout is available the support library v7 nowadays.

Apparently, Support Library v7 needs to be imported a library project (as opposed to the traditional jar file). We'd need to figure out the build machinery to make this work for us.

Maybe the new design will allow us to get rid of the nesting of GridView inside ScrollView. Just filing this bug to properly track this issue.

Comment 1

[Sriram Ramasubramanian [:sriram]]

GridLayout cannot have an adapter attached to it. It's a "grid" based layout, where you can have a 4x3 grid or a 5x7 grid and adding elements will align it to the grid. It's more like web designing. Anyways, I have a better approach for the new about:home, where the GridView will be a header view of the ListView. This way each one can have its own adapter. The views will recycle individual rows.

Comment 2

[Lucas Rocha (:lucasr)]

"Where the GridView will be a header view of the ListView"

FWIW, this sounds even worse than what we have now :-) The whole point of this bug report is remove the nesting of scrollable views from about:home.

Top Sites doesn't need to be an AdapterView because: 1. it's a short list that will never be scrolled (in itself) anyway 2. Because of 1, we can simply fill the GridLayout with the results of a DB query and forget about it.

Comment 3

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #2)
> "Where the GridView will be a header view of the ListView"
> 
> FWIW, this sounds even worse than what we have now :-) The whole point of
> this bug report is remove the nesting of scrollable views from about:home.
> 
> Top Sites doesn't need to be an AdapterView because: 1. it's a short list
> that will never be scrolled (in itself) anyway 2. Because of 1, we can
> simply fill the GridLayout with the results of a DB query and forget about
> it.

If that's the case, we don't need any cursors or DB queries. We don't need to monitor for OnActivityContentChanged() and worry about refreshing it on rotation. Right?
ScrollView in about:home is the problem. http://cl.ly/image/0D2w3o3n1D0s <-- It extends far more than what the phone size is. But if we add a GridView as a header for ListView, it's not going to do any harm. I've tried and tested it.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Two scrollable views are problematic only if both can scroll. Think of a case where there are 25 thumbnails in GridView and it's enclosed inside a ListView with 50 entries. That's clearly a problem. But when the height of GridView is fixed so that its content can never scroll, its not a problem. I didn't feel any lag in scrolling even.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> (In reply to Lucas Rocha (:lucasr) from comment #2)
> > "Where the GridView will be a header view of the ListView"
> > 
> > FWIW, this sounds even worse than what we have now :-) The whole point of
> > this bug report is remove the nesting of scrollable views from about:home.
> > 
> > Top Sites doesn't need to be an AdapterView because: 1. it's a short list
> > that will never be scrolled (in itself) anyway 2. Because of 1, we can
> > simply fill the GridLayout with the results of a DB query and forget about
> > it.
> 
> If that's the case, we don't need any cursors or DB queries. We don't need
> to monitor for OnActivityContentChanged() and worry about refreshing it on
> rotation. Right?

No idea what you mean here. I just wrote above that we'd still use a DB query. The way we update top sites now is as simple as re-querying the DB and changing the cursor on an adapter. It would be trivial to change this to simply fill a GridLayout. BrowserApp's OnActivityContentChanged simply triggers an update() call on AboutHomeContent.

The way onMeasure() is implemented on TopSitesView right now is pretty much a hack (I take the blame here) to avoid GridView to be scrollable. Using a simpler layout would likely allow us to remove this code (or at least part of it).

> ScrollView in about:home is the problem. http://cl.ly/image/0D2w3o3n1D0s <--
> It extends far more than what the phone size is. But if we add a GridView as
> a header for ListView, it's not going to do any harm. I've tried and tested
> it.

I don't follow. How is "extends far more than what the phone size is" a problem? Not sure I understand how you're planning to use a ListView in there. We might be talking about different issues here. Maybe it would help if I could see a WIP patch or something.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We updated top sites with Activity Stream.