1055595 - Extract the adapter from the TabsTray

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

Extract the adapter from the tabs tray so we can reuse the code for the grid view

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: 0001-bug-1055595-Extract-the-adapter-from-the-TabsTray.patch

Adapter extracted and some work done to setup for the gridview to be implemented

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 8475930 [details] [diff] [review]
0001-bug-1055595-Extract-the-adapter-from-the-TabsTray.patch

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

Looks like a pretty good start. This is ok as an intermediary step towards a cleaner adapter base class. Just let me know how you're planning to change this to accommodate the grid view.

::: mobile/android/base/tabs/TabsAdapter.java
@@ +53,5 @@
> +    abstract boolean isVertical();
> +    abstract void animateClose(final View view, int pos);
> +    abstract boolean isPrivate();
> +    abstract int getOriginalSize();
> +    abstract void setItemChecked(int pos, boolean isSelected);

It seems to me that all these abstract methods should actually stay in the TabsTray view. This is coupling the adapter with the host view a bit too much. Are you planning to clean those up once you implement the grid? I can live with these until then if this is just an intermediary step.

Generally speaking, I'd like to keep the TabsAdapter as dumb as possible. This means it should only be responsible for generating views that can be bound to Tabs and a simple API do add/remove tabs and refresh the list. This means it doesn't need to track tab events itself. Most of the tab events are directly coupled with the the view-level representation. So these should be handled in the view and redirected to the underlying adapted whenever necessary. Have a look at what the TabStripAdapter looks like in bug 1014987.

It would be nice if we could align the tabs adapter implementations now so that we can possibly consolidate them into a single base class later if appropriate. Not a hard requirement but nice to have.

::: mobile/android/base/tabs/TabsTray.java
@@ +126,4 @@
>      }
>  
>      // ViewHolder for a row in the list
> +    public static class TabRow {

We should probably create an interface for the views that can be bound to tab instances. Something similar to what we do in our panels system. For reference, see:

http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelViewAdapter.java
http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelItemView.java

No need to implement such system here in this bug. We can do this in a follow-up. I'm just giving some useful pointers for now.

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: 0001-Removing-dependencies-from-adapter.patch

This is a WIP patch for general review.

I've readdressed the adapter issue and dumbed it down considerably.  I'm left having to pass a few (badly named) interfaces around from the TabsTray (as it's still known in this patch) through the TabsAdapter to the TabRow implementations.

Thoughts and comments appreciated.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8477412 [details] [diff] [review]
0001-Removing-dependencies-from-adapter.patch

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

For the record, here's what we've agreed on yesterday:
- Remove animation bits from the TabsLayoutItemView base class.
- Get rid of the inner interfaces (TabListCloseAnimator and TabLayoutProperties)
- In TabsListLayout, subclass TabsLayoutItemView in an inner class and add the animation bits that are specific to the list layout there.
- Cast container argument in the adapter's getView() to TabsListLayout so that you can get orientation there.

That should keep TabsLayoutItemView free from animation code that can't and shouldn't really be shared between the list layout and the upcoming grid.

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: Part 1 - Extract code out of adapter and in to parent class

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Part 2 - abstract itemview, create concrete implementation in list view, introduce factory and implement in to adapter view

Comment 7

[Martyn Haigh (:mhaigh)]

Attachment: Part 3 - move TabsAdapter code out in to own class and slight refactoring to accommodate

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8480632 [details] [diff] [review]
Part 3 - move TabsAdapter code out in to own class and slight refactoring to accommodate

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

It seems this patch has new code that was implemented in a separate patch?

::: mobile/android/base/tabs/TabsListLayout.java
@@ -243,5 @@
>  
> -
> -    public static interface TabsLayoutFactory<T> {
> -        public T createItemView(View view, ViewGroup parent);
> -        public Button.OnClickListener createOnClickListener();

I don't remember having seen this interface before. Is it defined in a separate patch? This createOnClickListener looks a bit smelly.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8480629 [details] [diff] [review]
Part 1 - Extract code out of adapter and in to parent class

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

I'm assuming you want to review this? Looks good.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8480631 [details] [diff] [review]
Part 2 - abstract itemview, create concrete implementation in list view, introduce factory and implement in to adapter view

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

Ok, let's try to simplify this a bit. What we need is an adapter that can generate views that are bound to tabs. The interaction with the views generated by the adapter depends on the host TabsListLayout. For example, in TabsListLayout, tapping on the tab view's close button will trigger a certain animation. On the upcoming TabsGridLayout it might do something completely different. For now, let's only focus on the adapter side. We can work on abstracting TabsLayoutItemView a bit when you start implementing TabsGridLayout. So, here's what I suggest:

1. In part 2, simply move assignValues() to TabsLayoutItemView.
2. In part 3, refactor TabsAdapter to follow a newView/bindView format (like, say, our MultiTypeCursorAdapter). The getView() should look like:

    @Override
    public final View getView(int position, View convertView, ViewGroup parent) {
        final Context context = parent.getContext();
        final Tab tab = mTabs.get(position);

        final TabsLayoutItemView item;
        if (convertView == null) {
            item = newView(context, tab, parent);
        } else {
            item = (TabsLAyoutItemView) convertView;
        }

        bindView(item, tab);
        return convertView;
    }

Where newView() will simply call mInflater.inflate(R.layout.tabs_row, null). (Please file a follow-up to rename tab_row to tabs_layout_item_view btw). And bindView() will simply call item.assignValues(tab).

3. In part 4, extract the adapter by:
 - Rename TabsAdapter to TabsLayoutAdapter for consistency.
 - In TabsListLayout, create TabsLayoutAdapter subclass called TabsListLayoutAdapter that overrides newView() and bindView() and does something like:

    @Override
    public TabsLayoutItemView newView(Context context, Tab tab, ViewGroup parent) {
        TabsLayoutItemView item = super.newView(context, tab, parent);
        item.close.setOnClickListener(mOnCloseClickListener);
    }

    @Override
    public void bindView(TabsLayoutItemView item, Tab tab) {
        super.bindView(item, tab);
        resetTransforms();
    }

Comment 11

[Martyn Haigh (:mhaigh)]

Attachment: Part 2 - abstract itemview, create concrete implementation in list view, introduce factory and implement in to adapter view

Updated how we deal with the close button listener, moving it out of the adapter and associated TabsLayoutFactory and in to the abstract TabsLayoutItemView class. 

Note: TabsLayoutFactory is created in this patch but put in the TabsListLayout class, alongside the adapter, due to restrictions of the language.  It'll move out of TabsListLayout and in to the adapter in part 3 of this patch.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 8481336 [details] [diff] [review]
Part 2 - abstract itemview, create concrete implementation in list view, introduce factory and implement in to adapter view

Cancelling review based on my feedback in comment #10.

Comment 13

[Martyn Haigh (:mhaigh)]

Attachment: Part 1 - simplify adapter

Comment 14

[Martyn Haigh (:mhaigh)]

Attachment: Part 2 - move code from Layout to item view

Comment 15

[Martyn Haigh (:mhaigh)]

Attachment: Part 3 - introduce newView/bindView constructs in to adapter

Comment 16

[Martyn Haigh (:mhaigh)]

Attachment: Part 4 - rename and extract adapter, make abstract and create concrete implementation within the list view

Review flag meant for all patches

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8482314 [details] [diff] [review]
Part 1 - simplify adapter

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

Nice.

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 8482317 [details] [diff] [review]
Part 2 - move code from Layout to item view

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

Great.

::: mobile/android/base/tabs/TabsLayoutItemView.java
@@ +4,4 @@
>  
>  package org.mozilla.gecko.tabs;
>  
> +import org.mozilla.gecko.animation.ViewHelper;

Unused?

@@ +13,3 @@
>  import android.view.View;
>  import android.view.ViewGroup;
> +import android.widget.Button;

Unused?

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 8482318 [details] [diff] [review]
Part 3 - introduce newView/bindView constructs in to adapter

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

Looks good, just needs some tweaking in the API.

::: mobile/android/base/tabs/TabsListLayout.java
@@ +258,3 @@
>  
>              if (convertView == null) {
>                  convertView = mInflater.inflate(R.layout.tabs_row, null);

Hmmm, actually, let's keep newView()/bindView() as generic as possible for now i.e. they should take/return a generic View instead of TabsLayoutItemView. We can tweak this to be specific to TabsLayoutItemView once we get rid of the weird TabRow/TabsLayoutItemView distinction. So, here's how newView()/bindView() should look like:

View getView(int position, View convertView, ViewGroup parent) {
   final View view;
   if (convertView == null) {
       view = newView();
   } else {
       view = convertView;
   }

   final Tab tab = mTabs.get(position);
   bindView(view, tab);

   return view;
}

void View newView(int position, ViewParent parent) {
   final View view = mInflater.inflate(R.layout.tabs_row, parent, false);

   final TabsLayoutItemView item = new TabsLayoutItemView(view);
   item.close.setOnClickListener(mCloseClickListener);

   view.setTag(item);
   return view;
}

void bindView(View view, Tab tab) {
    TabsLayoutItemView item = (TabsLayoutItemView) view.getTag();
    item.assignValues(tab);
}

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 8482329 [details] [diff] [review]
Part 4 - rename and extract adapter, make abstract and create concrete implementation within the list view

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

Looks good with the suggested changes.

::: mobile/android/base/tabs/TabsListLayout.java
@@ +42,4 @@
>  
>      final private boolean mIsPrivate;
>  
> +    private TabsLayoutAdapter mTabsLayoutAdapter;

I'd keep the member name (mTabsAdapter) for conciseness.

@@ +94,5 @@
> +
> +        @Override
> +        public TabsLayoutItemView newView(View convertView) {
> +            TabsLayoutItemView item = super.newView(convertView);
> +            item.close.setOnClickListener(new Button.OnClickListener() {

I'd keep the original idea of a common mOnCloseClickListener that is created only once in the adapter's constructor. No need to create a new click listener for each new child view.

Comment 21

[Martyn Haigh (:mhaigh)]

Attachment: Part 2: move code from layout to item view

fixed nits

Comment 22

[Martyn Haigh (:mhaigh)]

Attachment: Part 2: move code from layout to item view

Was a bit overzealous with the pruning of imports - reinstated ViewHelper

Comment 23

[Martyn Haigh (:mhaigh)]

Attachment: Part 3: introduce newView/bindView constructs in to adapter

Refactored accordingly - kept the resetTransforms in the getView method for the moment to keep the bindView method as generic as possible - this will move in the the list specific implementation of bindView eventually.

Comment 24

[Martyn Haigh (:mhaigh)]

Attachment: Part 4: rename and extract adapter, make abstract and create concrete implementation within the list view

merged previous changes, reworked click listener to only instantiate once and renamed adapter variable.

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 8483463 [details] [diff] [review]
Part 4: rename and extract adapter, make abstract and create concrete implementation within the list view

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

::: mobile/android/base/tabs/TabsListLayout.java
@@ +103,5 @@
> +            };
> +        }
> +
> +        @Override
> +        View newView(int position, ViewGroup parent) {

public for consistency. This is a private class anyway.

Comment 26

[Lucas Rocha (:lucasr)]

Comment on attachment 8483411 [details] [diff] [review]
Part 3: introduce newView/bindView constructs in to adapter

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

Nice.

Comment 27

[Lucas Rocha (:lucasr)]

Backed out due to build failure. I think you forgot to include TabsLayoutAdapter.java in the last patch.

https://hg.mozilla.org/projects/larch/rev/1e06b0f7ef49

Comment 28

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/projects/larch/rev/ed73890a70a7
https://hg.mozilla.org/projects/larch/rev/b6e089bc11e8
https://hg.mozilla.org/projects/larch/rev/d98b84aa7856
https://hg.mozilla.org/projects/larch/rev/daf3f3f77cbf

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed73890a70a7
https://hg.mozilla.org/mozilla-central/rev/b6e089bc11e8
https://hg.mozilla.org/mozilla-central/rev/d98b84aa7856
https://hg.mozilla.org/mozilla-central/rev/daf3f3f77cbf