Closed
Bug 1066905
Opened 10 years ago
Closed 10 years ago
Merge TabRow.java and TabsLayoutItemView.java
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: mhaigh, Assigned: mhaigh)
References
Details
Attachments
(1 file, 1 obsolete file)
21.10 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=93f273899816
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8495998 -
Flags: review?(lucasr.at.mozilla)
Comment 3•10 years ago
|
||
Comment on attachment 8495998 [details] [diff] [review] Merge TabRow and TabsLayoutItemView Review of attachment 8495998 [details] [diff] [review]: ----------------------------------------------------------------- Looking good overall but still needs some important tweaks. ::: mobile/android/base/tabs/TabsGridLayout.java @@ +58,2 @@ > item.thumbnail.setImageDrawable(null); > item.close.setVisibility(View.VISIBLE); Wondering: why do we need to set the visibility of the close button here? Is it really necessary? @@ -103,5 @@ > @Override > public void bindView(View view, Tab tab) { > super.bindView(view, tab); > > - view.setOnClickListener(mSelectClickListener); Not needed anymore? Why? @@ +165,4 @@ > if (view == null) > return; > > + TabsLayoutItemView item = (TabsLayoutItemView) view; nit: Maybe cast the 'view' variable to TabsLayoutItemView directly, instead of having 2 local variables? ::: mobile/android/base/tabs/TabsLayoutAdapter.java @@ +87,5 @@ > return view; > } > > void bindView(View view, Tab tab) { > + if (view instanceof TabsLayoutItemView) { This check seems unnecessary. ::: mobile/android/base/tabs/TabsLayoutItemView.java @@ +53,5 @@ > } > > + @Override > + public void setChecked(boolean checked) { > + if (mChecked != checked) { nit: use early return instead: if (mChecked == checked) { return; } mChecked = checked; refreshDrawableState(); ... @@ +58,5 @@ > + mChecked = checked; > + refreshDrawableState(); > + > + int count = getChildCount(); > + for (int i=0; i < count; i++) { nit: space before and after '=' @@ +62,5 @@ > + for (int i=0; i < count; i++) { > + final View child = getChildAt(i); > + if (child instanceof Checkable) { > + ((Checkable) child).setChecked(checked); > + } nit: remove trailing space. @@ +73,5 @@ > + mChecked = !mChecked; > + } > + > + // this is a big nasty, but we'll get rid of it in bug 1058574 > + public void populateChildReferences() { This is a bit too smelly. You're relying on the caller to initialize the view references. This is not necessarily in the scope of bug 1058574. Just use onFinishInflate() instead. I'm generally not a fan of onFinishInflate() as it assumes new instances will always be created from inflation and I have seen device-specific bugs around it in the past. In this case though, doing a proper compound view would involve creating extra resources to handle the different variations between phones and new/old tablets. Anyway, onFinishInflate() should do the job here. ::: mobile/android/base/tabs/TabsListLayout.java @@ +96,4 @@ > mOnClickListener = new Button.OnClickListener() { > @Override > public void onClick(View v) { > + TabsLayoutItemView item = (TabsLayoutItemView) v.getTag(); Forgot to update this bit here, no? The view *is a* TabsLayoutItemView now. No tags needed anymore. @@ +361,4 @@ > else > animator.attach(view, Property.WIDTH, 1); > > + TabsLayoutItemView tab = (TabsLayoutItemView)view; nit: space before 'view'. @@ +398,4 @@ > public void onPropertyAnimationStart() { } > @Override > public void onPropertyAnimationEnd() { > + TabsLayoutItemView tab = (TabsLayoutItemView) view; tab -> item @@ +496,4 @@ > mSwipeView.setPressed(false); > > if (!mSwiping) { > + TabsLayoutItemView tab = (TabsLayoutItemView) mSwipeView; tab -> item @@ +584,4 @@ > mSwiping = true; > TabsListLayout.this.requestDisallowInterceptTouchEvent(true); > > + TabsLayoutItemView tab = (TabsLayoutItemView) mSwipeView; tab -> item
Attachment #8495998 -
Flags: review?(lucasr.at.mozilla) → feedback+
Assignee | ||
Comment 4•10 years ago
|
||
Nits fixed and some code simplification done around where we're casting to a variable which is only used once
Attachment #8495998 -
Attachment is obsolete: true
Attachment #8498177 -
Flags: review?(lucasr.at.mozilla)
Comment 5•10 years ago
|
||
Comment on attachment 8498177 [details] [diff] [review] Merge TabRow and TabsLayoutItemView Review of attachment 8498177 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: mobile/android/base/tabs/TabsLayoutAdapter.java @@ +19,3 @@ > // Adapter to bind tabs into a list > public class TabsLayoutAdapter extends BaseAdapter { > + public static final String LOGTAG = "Gecko" + TabsLayoutAdapter.class.getSimpleName();; nit: add empty line here. @@ +86,3 @@ > } > > void bindView(View view, Tab tab) { Follow-up: maybe change the TabsLayoutAdapter API to use TabsLayoutItemView instead of the generic View now that we have consolidated the child view type? This would remove the need for all these casts.
Attachment #8498177 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9206bbc5454f
Assignee | ||
Updated•10 years ago
|
Whiteboard: [fixed-larch]
Assignee | ||
Updated•10 years ago
|
Whiteboard: [fixed-larch]
https://hg.mozilla.org/mozilla-central/rev/9206bbc5454f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•