Closed Bug 1066905 Opened 5 years ago Closed 5 years ago

Merge TabRow.java and TabsLayoutItemView.java

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: mhaigh, Assigned: mhaigh)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attachment #8495998 - Flags: review?(lucasr.at.mozilla)
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+
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 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+
Whiteboard: [fixed-larch]
Whiteboard: [fixed-larch]
https://hg.mozilla.org/mozilla-central/rev/9206bbc5454f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.