1056920 - Create grid base layout for tabs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

We need to make a grid based layout for the tabs

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Create a grid based layout for tabs

This is WIP patch to land.  It introduces a grid layout which will be needed as a base for other bugs.  

This bug will be revisited later for further refinement such as adding the tab menu items back to the tabs panel in landscape mode and UX changes.

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8488265 [details] [diff] [review]
Create a grid based layout for tabs

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

Looking good. Just need to get rid of all list-specific animation bits from TabsListLayout.

::: mobile/android/base/resources/values/styles.xml
@@ +182,5 @@
> +        <item name="android:layout_height">match_parent</item>
> +        <item name="android:paddingTop">0dp</item>
> +        <item name="android:stretchMode">columnWidth</item>
> +        <item name="android:numColumns">auto_fit</item>
> +        <item name="android:columnWidth">@dimen/panel_grid_view_column_width</item>

Hmm, create a separate dimension for this. The panel UI has different constraints than the tabs panel. Let's not mix them together. Having a fixed dimen resource should be fine for now as long as it doesn't look too broken.

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +47,5 @@
> +    private TabsLayoutAdapter mTabsAdapter;
> +
> +    private List<View> mPendingClosedTabs;
> +    private int mCloseAnimationCount;
> +    private int mCloseAllAnimationCount;

Get rid of these for now. We don't know how this is going to work yet. Let's land the very minimal grid widget and iterate from there.

@@ +50,5 @@
> +    private int mCloseAnimationCount;
> +    private int mCloseAllAnimationCount;
> +
> +    // Time to animate non-flinged tabs of screen, in milliseconds
> +    private static final int ANIMATION_DURATION = 250;

Ditto.

@@ +53,5 @@
> +    // Time to animate non-flinged tabs of screen, in milliseconds
> +    private static final int ANIMATION_DURATION = 250;
> +
> +    // Time between starting successive tab animations in closeAllTabs.
> +    private static final int ANIMATION_CASCADE_DELAY = 75;

Ditto.

@@ +55,5 @@
> +
> +    // Time between starting successive tab animations in closeAllTabs.
> +    private static final int ANIMATION_CASCADE_DELAY = 75;
> +
> +    private int mOriginalSize;

Ditto.

@@ +81,5 @@
> +        });
> +    }
> +
> +    private class TabsGridLayoutAdapter extends TabsLayoutAdapter {
> +        private Button.OnClickListener mOnClickListener;

Missed this in my TabsListLayout review: rename this member to something more meaningful like mCloseClickListener or something.

nit: add empty line here.

@@ +90,5 @@
> +                @Override
> +                public void onClick(View v) {
> +                    TabsLayoutItemView tab = (TabsLayoutItemView) v.getTag();
> +                    final int pos = (isVertical() ? tab.info.getWidth() : 0 - tab.info.getHeight());
> +                    animateClose(tab.info, pos);

Simply call Tabs.getInstance().closeTab(tab) for now.

@@ +110,5 @@
> +
> +        @Override
> +        public void bindView(View view, Tab tab) {
> +            super.bindView(view, tab);
> +            view.setOnClickListener(new View.OnClickListener() {

Create this listener only once and share across all child views, just like you're doing with the close button.

@@ +243,5 @@
> +            ViewHelper.setHeight(view, mOriginalSize);
> +        }
> +    }
> +
> +    private boolean isVertical() {

Remove, not necessary in the grid.

@@ +248,5 @@
> +        return true;
> +    }
> +
> +    @Override
> +    public void closeAll() {

Replace this with something that simply calls closeTab in each tab instance.

@@ +312,5 @@
> +            cascadeDelay += ANIMATION_CASCADE_DELAY;
> +        }
> +    }
> +
> +    private void animateClose(final View view, int pos) {

Remove.

@@ +344,5 @@
> +
> +        animator.start();
> +    }
> +
> +    private void animateFinishClose(final View view) {

Remove.

@@ +375,5 @@
> +
> +        animator.start();
> +    }
> +
> +    private void animateCancel(final View view) {

Remove.

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: Bug 1056920 - Create grid based layout for tabs (r=lucasr)

Nits fixed and code cleaned up a bit

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8493155 [details] [diff] [review]
Bug 1056920 - Create grid based layout for tabs (r=lucasr)

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

Nice.

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +63,5 @@
> +
> +    private class TabsGridLayoutAdapter extends TabsLayoutAdapter {
> +
> +        private Button.OnClickListener mCloseClickListener;
> +        private View.OnClickListener mSelectClickListener;

Both should be final.

@@ +212,5 @@
> +        mTabsAdapter.setTabs(tabData);
> +        updateSelectedPosition();
> +    }
> +
> +    public void resetTransforms(View view) {

Does this really need to be public?

Comment 5

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/89abe3df46af

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android bustage.
https://hg.mozilla.org/integration/fx-team/rev/fac90451603f

Please verify that this is green on Try before re-landing.

Comment 7

[Martyn Haigh (:mhaigh)]

https://tbpl.mozilla.org/?tree=Try&rev=6d667d73f10a

and

https://hg.mozilla.org/integration/fx-team/rev/8329fcecefdc

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8329fcecefdc

Comment 9

[u279076]

Does this need any manual testing from QE?