Create grid base layout for tabs

RESOLVED FIXED in Firefox 35

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mhaigh, Assigned: mhaigh)

Tracking

unspecified
Firefox 35
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-larch])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
We need to make a grid based layout for the tabs
(Assignee)

Comment 1

4 years ago
Created attachment 8488265 [details] [diff] [review]
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.
Attachment #8488265 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8488265 - Flags: review?(lucasr.at.mozilla) → feedback+
(Assignee)

Comment 3

4 years ago
Created attachment 8493155 [details] [diff] [review]
Bug 1056920 - Create grid based layout for tabs (r=lucasr)

Nits fixed and code cleaned up a bit
Attachment #8488265 - Attachment is obsolete: true
Attachment #8493155 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8493155 - Flags: review?(lucasr.at.mozilla) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/8329fcecefdc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35

Updated

4 years ago
Blocks: 1072862
Does this need any manual testing from QE?
Flags: qe-verify?
You need to log in before you can comment on or make changes to this bug.