Closed Bug 1056920 Opened 10 years ago Closed 10 years ago

Create grid base layout for tabs

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: mhaigh, Assigned: mhaigh)

References

Details

(Whiteboard: [fixed-larch])

Attachments

(1 file, 1 obsolete file)

We need to make a grid based layout for the tabs
Attached patch Create a grid based layout for tabs (obsolete) (deleted) — Splinter Review
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+
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Blocks: 1072862
Does this need any manual testing from QE?
Flags: qe-verify?
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: