1383733 - Activity Stream: Transform top sites into pager with 2-row grid

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

(See linked mocks)

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1383733 - Show two rows of top sites.

Review commit: https://reviewboard.mozilla.org/r/165730/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165730/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Follow-up needed for partially showing next page.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: tworowsites.png

Rebased on top of the other changes that looks like this.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8894571 [details]
Bug 1383733 - Show two rows of top sites.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165730/diff/1-2/

Comment 5

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

Comment on attachment 8894571 [details]
Bug 1383733 - Show two rows of top sites.

https://reviewboard.mozilla.org/r/165730/#review170930

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java:108
(Diff revision 2)
>      protected void onSizeChanged(int w, int h, int oldw, int oldh) {
>          super.onSizeChanged(w, h, oldw, oldh);
>  
>          int tiles = (w - tileMargin) / (desiredTileWidth + tileMargin);
>  
> -        if (tiles < MINIMUM_TILES) {
> +        if (tiles < TOP_SITES_COLUMNS) {

I think there should be a way to simplify this logic - I'm struggling to understand what the intent of this code is. Perhaps it's something like:

```java
int tiles = TOP_SITES_COLUMNS;
int actualTileSize = // compute
if (actualTileSize < desiredTileSize) {
  setPadding(0, 0, 0, 0;
} else {
  // calculate & set padding
}

// continue...
```

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesPage.java:26
(Diff revision 2)
>  
>          setLayoutManager(new GridLayoutManager(context, 1));
>      }
>  
>      public void setTiles(int tiles) {
> -        setLayoutManager(new GridLayoutManager(getContext(), tiles));
> +        setLayoutManager(new GridLayoutManager(getContext(), ActivityStreamPanel.TOP_SITES_COLUMNS));

If these values are constant and the args are unused, do we need the `setTiles` method? Can we set this in the constructor?

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8894571 [details]
Bug 1383733 - Show two rows of top sites.

https://reviewboard.mozilla.org/r/165730/#review170930

> I think there should be a way to simplify this logic - I'm struggling to understand what the intent of this code is. Perhaps it's something like:
> 
> ```java
> int tiles = TOP_SITES_COLUMNS;
> int actualTileSize = // compute
> if (actualTileSize < desiredTileSize) {
>   setPadding(0, 0, 0, 0;
> } else {
>   // calculate & set padding
> }
> 
> // continue...
> ```

I simplified the code a bit and added comments.

> If these values are constant and the args are unused, do we need the `setTiles` method? Can we set this in the constructor?

Yeah, this is not needed anymore.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8894571 [details]
Bug 1383733 - Show two rows of top sites.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165730/diff/2-3/

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8894571 [details]
Bug 1383733 - Show two rows of top sites.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165730/diff/3-4/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9f1712d723d
Show two rows of top sites. r=mcomella

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9f1712d723d

Comment 11

[Bogdan Surd, QA [:bsurd, NI]]

Top sites is properly displayed as a 2 row grid. Marking as verified.