Closed Bug 891183 Opened 11 years ago Closed 11 years ago

[FIG] Change Title strip to Tabs strip for tablets

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect, P1)

ARM
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: shilpanbhagat, Assigned: shilpanbhagat)

References

Details

(Whiteboard: fixed-fig)

Attachments

(1 file, 8 obsolete files)

Currently the title strip is scrollable with the active page's title at the center. This is currently implemented across all devices. Change the title strip, on tablets, to match the design as shown here: http://cl.ly/image/3S3H1J1S3x0D
Did a bit of code refactoring
Attachment #772420 - Flags: review?(sriram)
Comment on attachment 772420 [details] [diff] [review]
Patch 1/2: Refactoring code before adding the tab strip

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

Overall looks good. Want to see another version with the changes. Hence f+ for now.

::: mobile/android/base/Makefile.in
@@ +511,5 @@
>    $(NULL)
>  
>  RES_LAYOUT_LARGE_V11 = \
>    res/layout-large-v11/browser_toolbar.xml \
> +  res/layout-large-v11/home_pager.xml \

Put this in a separate patch. Not needed until you override something.

::: mobile/android/base/resources/layout-large-v11/home_pager.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

Should include the license boilerplate.

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +25,5 @@
>                          android:layout_height="fill_parent"
>                          android:layout_above="@+id/find_in_page">
>  
>              <include layout="@layout/shared_ui_components"/>
> +            <include layout="@layout/home_pager"/>

New line before this line.

::: mobile/android/base/resources/layout/home_pager.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

Put this in a separate patch. Not needed until you override something.
Attachment #772420 - Flags: review?(sriram) → feedback+
This patch adds a custom tabs strip for tablets, replacing HomePagerTabStrip as per design. 

TODO: replace the current selection drawable with a thinner strip matching the ratio of HomePagerTabStrip (Need images for that. Will ask ibarlow for them)
Attachment #772420 - Attachment is obsolete: true
Attachment #773798 - Flags: feedback?(sriram)
Attached image Screenshot of the tablet tabs strip (obsolete) —
This is currently what it looks like. I think we should decrease the thickness of the selection drawable below? If so, I would need some draw9's from you. Or maybe just PNGs, I could do the rest.
Attachment #773801 - Flags: feedback?(ibarlow)
I can do that, but first check with Sriram on how he did this on phones. I don't remember making any images for him, so he may have used a different approach.
Comment on attachment 773798 [details] [diff] [review]
[WIP] custom tabs strip for about:home

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

There should be a change in the approach. ViewPager should know its decor view and call methods on it, when its own adapter changes -- like removeAllViews(), addView() and so on. TabWidget shouldn't be doing this job. HomePager can change the current tab too, as it would know the decor View. Would like to see another version with this major change.

::: mobile/android/base/Makefile.in
@@ +474,5 @@
>    res/layout/launch_app_listitem.xml \
>    res/layout/menu_action_bar.xml \
>    res/layout/menu_item_action_view.xml \
>    res/layout/menu_popup.xml \
> +  res/layout/menu_strip_tabs.xml \

Rename it to "tab_menu_strip".

::: mobile/android/base/home/HomePager.java
@@ +42,5 @@
>      public interface OnUrlOpenListener {
>          public void onUrlOpen(String url);
>      }
>  
> +    public interface OnAdapterChangedListener {

Why do we need this? Can't HomePager just do what is needed?

@@ +46,5 @@
> +    public interface OnAdapterChangedListener {
> +        public void onAdapterChanged(PagerAdapter oldAdapter, PagerAdapter newAdapter);
> +    }
> +
> +    public interface Decor {}

Add the comment from android's ViewPager source. I see we use it for same purpose.
Make the interface have package access only.

::: mobile/android/base/home/TabMenuStrip.java
@@ +20,5 @@
> +import org.mozilla.gecko.home.HomePager;
> +import org.mozilla.gecko.R;
> +
> +public class TabMenuStrip extends TabWidget implements HomePager.Decor {
> +    private String LOGTAG = "GeckoTabMenuStrip";

"private static final" String

@@ +22,5 @@
> +
> +public class TabMenuStrip extends TabWidget implements HomePager.Decor {
> +    private String LOGTAG = "GeckoTabMenuStrip";
> +
> +    private Context mContext;

Not needed.

@@ +23,5 @@
> +public class TabMenuStrip extends TabWidget implements HomePager.Decor {
> +    private String LOGTAG = "GeckoTabMenuStrip";
> +
> +    private Context mContext;
> +    private TabWidget mTabWidget;

Not needed. It extends TabWidget and so you can call all its methods.

@@ +25,5 @@
> +
> +    private Context mContext;
> +    private TabWidget mTabWidget;
> +    private HomePager mPager;
> +    private PageListener mPageListener;

private final.

@@ +35,5 @@
> +
> +        mPageListener = new PageListener();
> +    }
> +
> +    void updateStrip(int currentItem, PagerAdapter adapter) {

Make it "private".

@@ +61,5 @@
> +    @Override
> +    protected void onAttachedToWindow() {
> +        super.onAttachedToWindow();
> +
> +        final ViewParent parent = getParent();

This is not a good way of doing it -- if this view has to be extensible. This should be done in HomePager.

@@ +68,5 @@
> +        }
> +
> +        mPager = (HomePager) parent;
> +        mPager.setOnPageChangeListener(mPageListener);
> +        mPager.setOnAdapterChangedListener(mPageListener);

HomePager will know its "Decor" view. Cache it and make changes to it, by making HomePager expose an interface.

---
TabMenuStrip extends TabWidget {
  ... usual stuff ...
}

... 

HomePager {
  addView() {
     if (child instanceof TabWidget) {
         // Store this child as decor view.
     }
  }

  onPageChanged() {
     // if the decor view is tab-widget, change the current tab.
  }

  setAdapter() {
     mDecorView.removeAllViews();
  }

  // inside the adapter.
  mDecorView.addView();
};

@@ +92,5 @@
> +        }
> +
> +        @Override
> +        public void onPageSelected(int position) {
> +            mTabWidget.setCurrentTab(position);

setCurrentTab() would do.

::: mobile/android/base/resources/layout-large-v11/home_pager.xml
@@ +2,5 @@
> +<!-- This file is used to include the home pager in gecko app
> +     layout based on screen size -->
> +
> +<merge xmlns:android="http://schemas.android.com/apk/res/android"
> +       xmlns:gecko="http://schemas.android.com/apk/res-auto">

res-auto is not needed. We don't use gecko namespace here.

::: mobile/android/base/resources/layout/menu_strip_tabs.xml
@@ +2,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<Button xmlns:android="http://schemas.android.com/apk/res/android"

Usually a TabWidget has TextView and not buttons. The reason is due to the clickable behavior I guess. Probably change it based on what's recommended by Android.

@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<Button xmlns:android="http://schemas.android.com/apk/res/android"
> +        android:layout_width="@dimen/tabs_strip_button_width"

This could be a "minWidth" if UX wants. But not a layout_width. That will shrink text in other locales.

@@ +5,5 @@
> +
> +<Button xmlns:android="http://schemas.android.com/apk/res/android"
> +        android:layout_width="@dimen/tabs_strip_button_width"
> +        android:layout_height="fill_parent"
> +        android:textSize="12sp"

This should go inside TextAppearance. And 12sp is too small.
Attachment #773798 - Flags: feedback?(sriram) → feedback-
Based on IRC discussions,

It's ok to have TabMenuStrip use a parent adapter. However please mention that this strip is intended to be used only with a ViewPager and nothing else.

I'm still not sure about TabMenuStrip listening to page changes. Could you post a version with HomePager informing its HomePagerDecorView (which TabMenuStrip implements), to change the current item? Also, HomePager should be listening to TabClickListener and changing the page.
(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> HomePager {
>   addView() {
>      if (child instanceof TabWidget) {
>          // Store this child as decor view.
>      }
>   }

Is this just pseudocode? I really don't like doing this. The current patch does basically the same thing:

if (child instanceof Decor) {
  // Store as a decor view
}

I'm not sure what I think of having the ViewPager drive the Decor directly either. It seems like it would work, but it doesn't match what Android's doing, which means its likely going to be harder to interact with any external code (and there are some neat title strips out there!). Is there some advantage to doing things this way?
Implemented the Decor interface as per Sriram's comments. I still am not sure about implementing the tabclicklistener on the HomePager because HomePager can never be certain about the kind of Decor which is being applied inside it. In out case, it could either be HomePagerTitleStrip or TabMenuStrip. This also means that if we ever plan to remove the TabMenuStrip completely, HomePager is affected. I would like some opinions on this.

I also have a 'onRemovePagerView' method for Decor incase we plan to dynamically add/remove the currently hardcoded Pages
Attachment #775020 - Flags: feedback?(wjohnston)
Attachment #775020 - Flags: feedback?(sriram)
Comment on attachment 775020 [details] [diff] [review]
[WIP] Patch for new tabs menu for about:home for Tablets

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

This is looking pretty good. Almost exactly what I wanted.
I want to kill "mParent" inside the TabMenuStrip. Could you please try something with adapter and HomePager to avoid it?
I would like to see a version with it.

::: mobile/android/base/home/HomePager.java
@@ +47,5 @@
>  
> +    /**
> +     * Used internally to tag special types of child views that should be added as
> +     * pager decorations by default.
> +     */

"Special type of child views that could be added as pager decorations by default."

@@ +68,5 @@
> +    @Override
> +    public void setAdapter(PagerAdapter pagerAdapter) {
> +        super.setAdapter(pagerAdapter);
> +
> +        if (mDecor != null) {

I would recommend moving this block to the adapter of this view. This can be done in addTab(), when a tab is added. It is guaranteed that the DecorView won't have any child views at that point.

::: mobile/android/base/home/TabMenuStrip.java
@@ +23,5 @@
> +/**
> + * This View is only meant to be used inside HomePager View.
> + */
> +public class TabMenuStrip extends TabWidget implements HomePager.Decor,
> +                                                       ViewPager.OnPageChangeListener {

I would like the ViewPager.OnPagerChangeListener to be supported by the adapter in HomePager. Being the "model" for the HomePager, the adapter can handle changes to the ViewPager and do the cleanups with the Decor. Since the addTab() is now going to take care of adding views to Decor, the adapter can look for this change too.

@@ +43,5 @@
> +        return button;
> +    }
> +
> +    @Override
> +    protected void onAttachedToWindow() {

When adapter listens for changes, this method can be removed.

@@ +101,5 @@
> +
> +        @Override
> +        public void onClick(View view) {
> +            if (mPager != null)
> +                mPager.setCurrentItem(mIndex, true);

Try removing the mPager here.

How about something like:

HomePager {
  interface Decor {
    setOnTitleClickListener(OnTitleClickListener xyz);
  }

  class Adapter implements OnTitleClickListener {
     onDecorTitleClicked(int index) {
        setCurrentItem(index); // This is on HomePager.
     }
  }
};

And the above onClick() will actually call the OnTitleClickListener if one exists.

::: mobile/android/base/resources/layout-large-v11/home_pager.xml
@@ +6,5 @@
> +<!-- This file is used to include the home pager in gecko app
> +     layout based on screen size -->
> +
> +<merge xmlns:android="http://schemas.android.com/apk/res/android"
> +       xmlns:gecko="http://schemas.android.com/apk/res-auto">

This namespace is not needed.

::: mobile/android/base/resources/values/styles.xml
@@ +244,5 @@
>          <item name="android:textColor">?android:attr/textColorHint</item>
>      </style>
>  
> +    <style name="TextAppearance.Widget.HomePagerTabMenuStrip" parent="TextAppearance.Small">
> +        <item name="android:textColor">?android:attr/textColorHint</item>

Why a hint color?
Attachment #775020 - Flags: feedback?(sriram) → feedback-
(In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> Comment on attachment 775020 [details] [diff] [review]
> [WIP] Patch for new tabs menu for about:home for Tablets
> 
> Review of attachment 775020 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking pretty good. Almost exactly what I wanted.
> I want to kill "mParent" inside the TabMenuStrip. Could you please try
> something with adapter and HomePager to avoid it?
> I would like to see a version with it.
> 
> ::: mobile/android/base/home/HomePager.java
> @@ +47,5 @@
> >  
> > +    /**
> > +     * Used internally to tag special types of child views that should be added as
> > +     * pager decorations by default.
> > +     */
> 
> "Special type of child views that could be added as pager decorations by
> default."
> 
Done

> @@ +68,5 @@
> > +    @Override
> > +    public void setAdapter(PagerAdapter pagerAdapter) {
> > +        super.setAdapter(pagerAdapter);
> > +
> > +        if (mDecor != null) {
> 
> I would recommend moving this block to the adapter of this view. This can be
> done in addTab(), when a tab is added. It is guaranteed that the DecorView
> won't have any child views at that point.
> 
Umm, yes but addTab() does add TabInfo to the adapter. decorviews are different from views added by the adapter. This means that when all the views from the from the adapter are removed, the views inside the decorview still remains. Hence, we need to remove and repopulate the decorview as needed. Previously I was doing this when an adapter changed. However now I am cleaning up the decorview on the instantiation of a TabsAdapter. This means that the decor view is dependent on the particular adapter for its cleaning up. And we will need to implement something like removeAllDecorViews() to clean the decorView.

Also, instantiation != setting an adapter. Instantiation does not mean it will be set as an adapter. So semantically this seems wrong. This works in our case since we instantiate a new adapter on show(). But since we are making most of these changes based on design decision I think this should be brought up. If you clean the decorview on setAdapter(), the interface methods are called by the adapter and the pagerView, which seems messy and not the best way of doing this.

This also, brings me to another point where OnPageListener methods are unnecessarily replicated by the DecorView which makes the code more complicated and a bit round about.

Finally, this implementation does not support multiple decorviews. We can do that be creating an arraylist of decorviews. However, this can be taken care of in a more smarter manner as I did in the first implementation.

The current way show by you will add the tabs
> ::: mobile/android/base/home/TabMenuStrip.java
> @@ +23,5 @@
> > +/**
> > + * This View is only meant to be used inside HomePager View.
> > + */
> > +public class TabMenuStrip extends TabWidget implements HomePager.Decor,
> > +                                                       ViewPager.OnPageChangeListener {
> 
> I would like the ViewPager.OnPagerChangeListener to be supported by the
> adapter in HomePager. Being the "model" for the HomePager, the adapter can
> handle changes to the ViewPager and do the cleanups with the Decor. Since
> the addTab() is now going to take care of adding views to Decor, the adapter
> can look for this change too.
> 
> @@ +43,5 @@
> > +        return button;
> > +    }
> > +
> > +    @Override
> > +    protected void onAttachedToWindow() {
> 
> When adapter listens for changes, this method can be removed.
> 
> @@ +101,5 @@
> > +
> > +        @Override
> > +        public void onClick(View view) {
> > +            if (mPager != null)
> > +                mPager.setCurrentItem(mIndex, true);
> 
> Try removing the mPager here.
> 
> How about something like:
> 
> HomePager {
>   interface Decor {
>     setOnTitleClickListener(OnTitleClickListener xyz);
>   }
> 
>   class Adapter implements OnTitleClickListener {
>      onDecorTitleClicked(int index) {
>         setCurrentItem(index); // This is on HomePager.
>      }
>   }
> };
> 
> And the above onClick() will actually call the OnTitleClickListener if one
> exists.
> 
> ::: mobile/android/base/resources/layout-large-v11/home_pager.xml
> @@ +6,5 @@
> > +<!-- This file is used to include the home pager in gecko app
> > +     layout based on screen size -->
> > +
> > +<merge xmlns:android="http://schemas.android.com/apk/res/android"
> > +       xmlns:gecko="http://schemas.android.com/apk/res-auto">
> 
> This namespace is not needed.
> 
Done

> ::: mobile/android/base/resources/values/styles.xml
> @@ +244,5 @@
> >          <item name="android:textColor">?android:attr/textColorHint</item>
> >      </style>
> >  
> > +    <style name="TextAppearance.Widget.HomePagerTabMenuStrip" parent="TextAppearance.Small">
> > +        <item name="android:textColor">?android:attr/textColorHint</item>
> 
> Why a hint color?
That was the color being used by the homepagertabstrip.
Made changes as asked.
Attachment #775020 - Attachment is obsolete: true
Attachment #775020 - Flags: feedback?(wjohnston)
Attachment #776582 - Flags: review?(sriram)
Comment on attachment 776582 [details] [diff] [review]
Patch for new tabs menu for about:home for Tablets

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

This looks good. Pretty solid. Few changes are needed here and there. Mostly cosmetic. I would wait for designs and changes before giving an r+.

::: mobile/android/base/home/HomePager.java
@@ +30,5 @@
>  
>      private final Context mContext;
>      private volatile boolean mLoaded;
> +    private Decor mDecor;
> +    private PagerAdapter mAdapter;

This is not needed.

@@ +46,5 @@
>      public interface OnUrlOpenListener {
>          public void onUrlOpen(String url);
>      }
>  
> +    public static interface OnTitleClickListener {

interface OnTitleClickListener is enough. This needn't be known outside the package.

@@ +47,5 @@
>          public void onUrlOpen(String url);
>      }
>  
> +    public static interface OnTitleClickListener {
> +        public void onDecorViewClicked(int index);

onTitleClicked(int index);

@@ +54,5 @@
> +    /**
> +     * Special type of child views that could be added as pager decorations by default.
> +     */
> +    interface Decor {
> +        public void onAddPagerView(String title);

onAddTitle(String title);

@@ +55,5 @@
> +     * Special type of child views that could be added as pager decorations by default.
> +     */
> +    interface Decor {
> +        public void onAddPagerView(String title);
> +        public void onRemovePagerView(int position);

We don't use this. We can remove this.

@@ +56,5 @@
> +     */
> +    interface Decor {
> +        public void onAddPagerView(String title);
> +        public void onRemovePagerView(int position);
> +        public void removeAllPagerViews();

And this too.

@@ +57,5 @@
> +    interface Decor {
> +        public void onAddPagerView(String title);
> +        public void onRemovePagerView(int position);
> +        public void removeAllPagerViews();
> +        public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels);

Could you please add this if-and-only-if we support active scrolling? And remove from this patch.

@@ +59,5 @@
> +        public void onRemovePagerView(int position);
> +        public void removeAllPagerViews();
> +        public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels);
> +        public void onPageSelected(int position);
> +        public void onPageScrollStateChanged(int state);

Same here.

@@ +74,5 @@
>      public HomePager(Context context, AttributeSet attrs) {
>          super(context, attrs);
>          mContext = context;
> +
> +        setOnPageChangeListener(mPageListener);

I don't like this here. Set this if and only if mDecor is available.

@@ +80,5 @@
> +
> +    @Override
> +    public void setAdapter(PagerAdapter pagerAdapter) {
> +        super.setAdapter(pagerAdapter);
> +        mAdapter = pagerAdapter;

Do we use mAdapter anywhere? I think you can remove it.

@@ +87,5 @@
> +    @Override
> +    public void addView(View child, int index, ViewGroup.LayoutParams params) {
> +        if (child instanceof Decor) {
> +            ((ViewPager.LayoutParams) params).isDecor = true;
> +            mDecor = (Decor) child;

Set the page change listener here. And in that case, don't create a private member. Do something like,

setOnPageChangeListener(new PageListener());

@@ +151,5 @@
>      }
>  
> +    private class PageListener implements ViewPager.OnPageChangeListener {
> +
> +        public PageListener() {}

Not needed.

@@ +155,5 @@
> +        public PageListener() {}
> +
> +        @Override
> +        public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) {
> +            if (mDecor != null) {

Based on above suggestion, since the page listener is going to added only when there is a decor view for this home pager, remove all the (mDecor != null) checks please :D

@@ +175,5 @@
> +            }
> +        }
> +    }
> +
> +    class TabsAdapter extends FragmentStatePagerAdapter implements OnTitleClickListener {

This could be a private class. Does anyone else use this?

@@ +196,5 @@
>          public TabsAdapter(FragmentManager fm) {
>              super(fm);
> +
> +            if (mDecor != null) {
> +                mDecor.removeAllPagerViews();

This call isn't needed.

::: mobile/android/base/home/TabMenuStrip.java
@@ +15,5 @@
> +
> +import org.mozilla.gecko.home.HomePager;
> +import org.mozilla.gecko.R;
> +
> +public class TabMenuStrip extends TabWidget implements HomePager.Decor {

Fold the lines as in BrowserApp.

@@ +24,5 @@
> +    public TabMenuStrip(Context context, AttributeSet attrs) {
> +        super(context, attrs);
> +    }
> +
> +    public TextView addTab(String title) {

This doesn't need to return anything. And this entire method can be moved into onAddPagerView (now onAddTitle(); ).
Attachment #776582 - Flags: review?(sriram) → feedback+
(In reply to Shilpan Bhagat from comment #4)
> Created attachment 773801 [details]
> Screenshot of the tablet tabs strip
> 
> This is currently what it looks like. I think we should decrease the
> thickness of the selection drawable below? If so, I would need some draw9's
> from you. Or maybe just PNGs, I could do the rest.

un-needinfoing myself, as I think Shilpan has everything he needs here from me
Comment on attachment 773801 [details]
Screenshot of the tablet tabs strip

replace with the newer assets I provided last week and we should be good here.
Attachment #773801 - Flags: feedback?(ibarlow) → feedback-
(In reply to Ian Barlow (:ibarlow) from comment #14)
> (In reply to Shilpan Bhagat from comment #4)
> > Created attachment 773801 [details]
> > Screenshot of the tablet tabs strip
> > 
> > This is currently what it looks like. I think we should decrease the
> > thickness of the selection drawable below? If so, I would need some draw9's
> > from you. Or maybe just PNGs, I could do the rest.
> 
> un-needinfoing myself, as I think Shilpan has everything he needs here from
> me

Is there any mockup on the size of the title strip and the font sizes? I didn't want to r+ the patch until then :(
Assignee: nobody → sbhagat
This patch includes the animating strip at the bottom. TabWidget is no longer of use based on the design, hence changed to LinearLayout. Works like a charm :)
Attachment #773798 - Attachment is obsolete: true
Attachment #773801 - Attachment is obsolete: true
Attachment #776582 - Attachment is obsolete: true
Attachment #788734 - Flags: review?(sriram)
I still have some performance improvements I can do here but nothing extraordinary, can be done in a follow-up.
Just a heads up: bug 901432 is introducing a utility class to make it easier to write about:home UI tests. Right now, it assumes there's a TabWidget in the History page. If bug 901432 lands first, you'll have to update AboutHomeTest to fetch the tabs strip by ID instead of by View type.
how about you quickly review this Lucas? Might save us (me) some trouble :P
Comment on attachment 788734 [details] [diff] [review]
Patch for new tabs menu for about:home for Tablets

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

Mostly looks good. Needs couple of changes in the animation logic -- the logic is really good.
Please attach an mdpi file. Requires one more pass before giving r+.

::: mobile/android/base/Makefile.in
@@ +750,5 @@
>    res/drawable-hdpi/alert_camera.png \
>    res/drawable-hdpi/alert_mic.png \
>    res/drawable-hdpi/alert_mic_camera.png \
>    res/drawable-hdpi/arrow_popup_bg.9.png \
> +  res/drawable-hdpi/home_tab_menu_strip.9.png \

Where is the mdpi version for this?

::: mobile/android/base/home/HomePager.java
@@ +83,5 @@
> +    public void addView(View child, int index, ViewGroup.LayoutParams params) {
> +        if (child instanceof Decor) {
> +            ((ViewPager.LayoutParams) params).isDecor = true;
> +            mDecor = (Decor) child;
> +            setOnPageChangeListener(new PageListener());

I find this class to be used only once. Make this an anonymous class right here.

setOnPageChangeListener(new ViewPager.OnPageChangeListener() { ... });

@@ +138,5 @@
>      public boolean isVisible() {
>          return mLoaded;
>      }
>  
> +    private class PageListener implements ViewPager.OnPageChangeListener {

This should be made anonymous as mentioned above.

::: mobile/android/base/home/TabMenuStrip.java
@@ +39,5 @@
> +    public TabMenuStrip(Context context, AttributeSet attrs) {
> +        super(context, attrs);
> +
> +        TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.TabMenuStrip);
> +        int stripResId = a.getResourceId(R.styleable.TabMenuStrip_strip, -1);

final int stripResId.

@@ +49,5 @@
> +    }
> +
> +    @Override
> +    public void onAddPagerView(String title) {
> +        TextView button = (TextView) LayoutInflater.from(getContext()).inflate(R.layout.tab_menu_strip, this, false);

final TextView button.

@@ +67,5 @@
> +    public void onPageSelected(final int position) {
> +        mSelectedView = getChildAt(position);
> +
> +        // Callback to measure and draw the strip after the view is visible.
> +        ViewTreeObserver vto = mSelectedView.getViewTreeObserver();

Why do you need a view tree observer here? Isn't it easy enough to just set the bounds based on the selectedView?

@@ +84,5 @@
> +    }
> +
> +    @Override
> +    public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) {
> +        if (mStrip != null) {

Early return is better.

if (mStrip == null) {
    return;
}

@@ +86,5 @@
> +    @Override
> +    public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) {
> +        if (mStrip != null) {
> +            final ScrollingData scrollingData = getScrollingData(position, positionOffset);
> +            final Rect prevBounds = new Rect(scrollingData.ScrollingAwayFromTab.getLeft(),

Why not use "scrollingData.awayTab.getDrawingRect(prevBounds)". ?

@@ +96,5 @@
> +                                             scrollingData.ScrollingTowardsTab.getTop(),
> +                                             scrollingData.ScrollingTowardsTab.getRight(),
> +                                             scrollingData.ScrollingTowardsTab.getBottom());
> +
> +            mStrip.setBounds((int) (prevBounds.left + ((nextBounds.left - prevBounds.left) * scrollingData.Progress)),

Comment saying: Scaling the size based on the progress.

I would recommend pulling the lengthy statements as variables.
final int left = ...;

@@ +97,5 @@
> +                                             scrollingData.ScrollingTowardsTab.getRight(),
> +                                             scrollingData.ScrollingTowardsTab.getBottom());
> +
> +            mStrip.setBounds((int) (prevBounds.left + ((nextBounds.left - prevBounds.left) * scrollingData.Progress)),
> +                                   (int) (prevBounds.top + ((nextBounds.top - prevBounds.top) * scrollingData.Progress)),

This is all contained in a LinearLayout, whose top and bottom are fixed. Please you prevBounds.top. That should be enough.

@@ +99,5 @@
> +
> +            mStrip.setBounds((int) (prevBounds.left + ((nextBounds.left - prevBounds.left) * scrollingData.Progress)),
> +                                   (int) (prevBounds.top + ((nextBounds.top - prevBounds.top) * scrollingData.Progress)),
> +                                   (int) (prevBounds.right + ((nextBounds.right - prevBounds.right) * scrollingData.Progress)),
> +                                   (int) (prevBounds.bottom + ((nextBounds.bottom - prevBounds.bottom) * scrollingData.Progress)));

Just prevBounds.bottom.

@@ +103,5 @@
> +                                   (int) (prevBounds.bottom + ((nextBounds.bottom - prevBounds.bottom) * scrollingData.Progress)));
> +            invalidate();
> +        }
> +    }
> +

Add some comment on what this class is for.

@@ +105,5 @@
> +        }
> +    }
> +
> +    private class ScrollingData {
> +        public View ScrollingTowardsTab;

names should be camelCaseInJava. ;) Not CamelCase. :)
All these members needn't be public. Package access should suffice.

Also use, "toTab", "fromTab". shorter and simpler yet conveys the intention.

@@ +114,5 @@
> +    private ScrollingData getScrollingData(int position, float positionOffset) {
> +        int nextTabIndex;
> +        float currProgress = position + positionOffset;
> +        ScrollingData scrollingData = new ScrollingData();
> +

Add more comments on this logic.
Something like: // Moving left when prev progress > cur progress.

@@ +118,5 @@
> +
> +        if (mPrevProgress > currProgress) {
> +            nextTabIndex = (int) Math.floor((double) currProgress);
> +            scrollingData.ScrollingTowardsTab = getChildAt(nextTabIndex);
> +            scrollingData.ScrollingAwayFromTab = getChildAt((int) Math.ceil((double) currProgress));

Cant we do a simple check of "positionOffset < 0.5 then towardsTab == getChildAt(position); awayTab = getChildAt(position + 1);" ?

@@ +119,5 @@
> +        if (mPrevProgress > currProgress) {
> +            nextTabIndex = (int) Math.floor((double) currProgress);
> +            scrollingData.ScrollingTowardsTab = getChildAt(nextTabIndex);
> +            scrollingData.ScrollingAwayFromTab = getChildAt((int) Math.ceil((double) currProgress));
> +            scrollingData.Progress = 1 + nextTabIndex - currProgress;

progress = 1 - positionOffset.

@@ +124,5 @@
> +        } else {
> +            nextTabIndex = (int) Math.ceil((double) currProgress);
> +            scrollingData.ScrollingTowardsTab = getChildAt(nextTabIndex);
> +            scrollingData.ScrollingAwayFromTab = getChildAt((int) Math.floor((double) currProgress));
> +            scrollingData.Progress = 1 + currProgress - nextTabIndex;

Same changes as above here.
progress = 1 + positionOffset.

@@ +127,5 @@
> +            scrollingData.ScrollingAwayFromTab = getChildAt((int) Math.floor((double) currProgress));
> +            scrollingData.Progress = 1 + currProgress - nextTabIndex;
> +        }
> +
> +        mPrevProgress = currProgress;

I need to think about this variable name and purpose. But I can do that only when the above logic is cleaned up a bit (based on a need for "currProgress" variable at all. I guess you might be better off with just saving "positionOffset" as "prevOffset".

@@ +132,5 @@
> +        return scrollingData;
> +    }
> +
> +    @Override
> +    public void dispatchDraw(Canvas canvas) {

You shouldn't be overriding this guy. It should be onDraw() that should be overriden (from my memory).
And when that's done, please set "setWillNotDraw(false)" in your constructor.

@@ +147,5 @@
> +            mSelectedView.requestFocus();
> +            return;
> +        }
> +
> +        if (hasFocus) {

Early return please.

@@ +159,5 @@
> +                        // A view is focused so send an event to announce the menu strip state.
> +                        sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED);
> +                    }
> +                    break;
> +                }

New line after this.

::: mobile/android/base/resources/layout/home_pager.xml
@@ +14,5 @@
> +                                      android:layout_height="fill_parent"
> +                                      android:background="@android:color/white"
> +                                      android:visibility="gone">
> +
> +        <org.mozilla.gecko.home.HomePagerTabStrip android:layout_width="fill_parent"

Should be shifted left by 4 spaces. For the entire block.

::: mobile/android/base/resources/values/attrs.xml
@@ +211,5 @@
>          <attr name="android:verticalSpacing"/>
>      </declare-styleable>
>  
> +    <declare-styleable name="TabMenuStrip">
> +        <attr name="strip" format="integer"/>

This is a resource right? It's better to use "reference" as format here.
Also please change the name to "stripDrawable". That conveys the meaning clearly.
Attachment #788734 - Flags: review?(sriram) → review-
Here's a test build to play with while I get another patch up:
https://dl.dropboxusercontent.com/u/11916346/mozilla/tabstripbuild.apk
Flags: needinfo?(ibarlow)
Made changes as mentioned above + some performance improvements
Attachment #788734 - Attachment is obsolete: true
Attachment #789434 - Flags: review?(sriram)
(In reply to Sriram Ramasubramanian [:sriram] from comment #22)
> Comment on attachment 788734 [details] [diff] [review]
> Patch for new tabs menu for about:home for Tablets
> 
> Review of attachment 788734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly looks good. Needs couple of changes in the animation logic -- the
> logic is really good.
> Please attach an mdpi file. Requires one more pass before giving r+.
> 
> ::: mobile/android/base/Makefile.in
> @@ +750,5 @@
> >    res/drawable-hdpi/alert_camera.png \
> >    res/drawable-hdpi/alert_mic.png \
> >    res/drawable-hdpi/alert_mic_camera.png \
> >    res/drawable-hdpi/arrow_popup_bg.9.png \
> > +  res/drawable-hdpi/home_tab_menu_strip.9.png \
> 
> Where is the mdpi version for this?
> 
Mdpi version will be added as a follow up or another patch on this bug? I cant seem to make a 9png out of it.
> @@ +67,5 @@
> > +    public void onPageSelected(final int position) {
> > +        mSelectedView = getChildAt(position);
> > +
> > +        // Callback to measure and draw the strip after the view is visible.
> > +        ViewTreeObserver vto = mSelectedView.getViewTreeObserver();
> 
> Why do you need a view tree observer here? Isn't it easy enough to just set
> the bounds based on the selectedView?
> 
It is not in edge cases, where we set the view but is not yet rendered. This means that any of the method calls related to positioning we make, will return 0. Hence we should only do this when the view rendered

> @@ +86,5 @@
> > +    @Override
> > +    public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) {
> > +        if (mStrip != null) {
> > +            final ScrollingData scrollingData = getScrollingData(position, positionOffset);
> > +            final Rect prevBounds = new Rect(scrollingData.ScrollingAwayFromTab.getLeft(),
> 
> Why not use "scrollingData.awayTab.getDrawingRect(prevBounds)". ?
>
Because it sets the height and width of the rect, and a few other properties, none of which are help in figuring out positioning. We can do this in a more optimized way by using just four properties of the rect. Which I am doing currently. I did optimize rect initialization though. 

> > +                                             scrollingData.ScrollingTowardsTab.getRight(),
> > +                                             scrollingData.ScrollingTowardsTab.getBottom());
> > +
> > +            mStrip.setBounds((int) (prevBounds.left + ((nextBounds.left - prevBounds.left) * scrollingData.Progress)),
> > +                                   (int) (prevBounds.top + ((nextBounds.top - prevBounds.top) * scrollingData.Progress)),
> 
> This is all contained in a LinearLayout, whose top and bottom are fixed.
> Please you prevBounds.top. That should be enough.
>
I can, but keeping with the ideology of keeping things as extensible as possible, this will also allow vertical animations, should we ever choose to use orientation = vertical. With almost no incurring costs at all.
> @@ +99,5 @@
> > +
> > +            mStrip.setBounds((int) (prevBounds.left + ((nextBounds.left - prevBounds.left) * scrollingData.Progress)),
> > +                                   (int) (prevBounds.top + ((nextBounds.top - prevBounds.top) * scrollingData.Progress)),
> > +                                   (int) (prevBounds.right + ((nextBounds.right - prevBounds.right) * scrollingData.Progress)),
> > +                                   (int) (prevBounds.bottom + ((nextBounds.bottom - prevBounds.bottom) * scrollingData.Progress)));
> 
> Just prevBounds.bottom.
>
Same argument here 
> @@ +103,5 @@
> > +                                   (int) (prevBounds.bottom + ((nextBounds.bottom - prevBounds.bottom) * scrollingData.Progress)));
> > +            invalidate();
> > +        }
> > +    }
> > +
> 
> Add some comment on what this class is for.
> 
> @@ +105,5 @@
> > +        }
> > +    }
> > +
> > +    private class ScrollingData {
> > +        public View ScrollingTowardsTab;
> 
> names should be camelCaseInJava. ;) Not CamelCase. :)
> All these members needn't be public. Package access should suffice.
> 
My bad I kept things half done. I was planning to make this class static as an optimization as a follow up and this would have made complete sense. But I completely forgot to do that :P It's in now.
> Also use, "toTab", "fromTab". shorter and simpler yet conveys the intention.
> 
> @@ +114,5 @@
> > +    private ScrollingData getScrollingData(int position, float positionOffset) {
> > +        int nextTabIndex;
> > +        float currProgress = position + positionOffset;
> > +        ScrollingData scrollingData = new ScrollingData();
> > +
> 
> Add more comments on this logic.
> Something like: // Moving left when prev progress > cur progress.
> 
> @@ +118,5 @@
> > +
> > +        if (mPrevProgress > currProgress) {
> > +            nextTabIndex = (int) Math.floor((double) currProgress);
> > +            scrollingData.ScrollingTowardsTab = getChildAt(nextTabIndex);
> > +            scrollingData.ScrollingAwayFromTab = getChildAt((int) Math.ceil((double) currProgress));
> 
> Cant we do a simple check of "positionOffset < 0.5 then towardsTab ==
> getChildAt(position); awayTab = getChildAt(position + 1);" ?
> 
If do not really like using if blocks if I can avoid it plus this logic fails since it assumes that if position offset is less then 0.5 we move towards the left. which is not true, we can be moving towards the right page too. I went ahead and made the logic more clear though. 
> @@ +119,5 @@
> > +        if (mPrevProgress > currProgress) {
> > +            nextTabIndex = (int) Math.floor((double) currProgress);
> > +            scrollingData.ScrollingTowardsTab = getChildAt(nextTabIndex);
> > +            scrollingData.ScrollingAwayFromTab = getChildAt((int) Math.ceil((double) currProgress));
> > +            scrollingData.Progress = 1 + nextTabIndex - currProgress;
> 
> progress = 1 - positionOffset.
> 
> @@ +124,5 @@
> > +        } else {
> > +            nextTabIndex = (int) Math.ceil((double) currProgress);
> > +            scrollingData.ScrollingTowardsTab = getChildAt(nextTabIndex);
> > +            scrollingData.ScrollingAwayFromTab = getChildAt((int) Math.floor((double) currProgress));
> > +            scrollingData.Progress = 1 + currProgress - nextTabIndex;
> 
> Same changes as above here.
> progress = 1 + positionOffset.
> 
Won't work
> @@ +127,5 @@
> > +            scrollingData.ScrollingAwayFromTab = getChildAt((int) Math.floor((double) currProgress));
> > +            scrollingData.Progress = 1 + currProgress - nextTabIndex;
> > +        }
> > +
> > +        mPrevProgress = currProgress;
> 
> I need to think about this variable name and purpose. But I can do that only
> when the above logic is cleaned up a bit (based on a need for "currProgress"
> variable at all. I guess you might be better off with just saving
> "positionOffset" as "prevOffset".
> 
There is not true positionOffset in Android and kudos to their bad variable naming. position + positionOffset give you the progress position of where you currently are w.r.t the entire view pager. Think of the three pages in the view to be an entire view. Now progress goes from 0 to 2 i.e if progress is 1.5, it means you are halfway between the second and third page.
By skimming through: where is the camelCase?? It should "toTab" and "fromTab" not "From.. ", "To..". Please change and upload a new patch.
They are static variables, I guess they should all be UpperCase? I'll do that and put one up
Priority: -- → P1
This patch adds the camel casing
Attachment #789434 - Attachment is obsolete: true
Attachment #789434 - Flags: review?(sriram)
Attachment #789677 - Flags: review?(sriram)
(In reply to Shilpan Bhagat from comment #25)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #22)
> > Comment on attachment 788734 [details] [diff] [review]
> > Patch for new tabs menu for about:home for Tablets
> > 
> > Review of attachment 788734 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Mostly looks good. Needs couple of changes in the animation logic -- the
> > logic is really good.
> > Please attach an mdpi file. Requires one more pass before giving r+.
> > 
> > ::: mobile/android/base/Makefile.in
> > @@ +750,5 @@
> > >    res/drawable-hdpi/alert_camera.png \
> > >    res/drawable-hdpi/alert_mic.png \
> > >    res/drawable-hdpi/alert_mic_camera.png \
> > >    res/drawable-hdpi/arrow_popup_bg.9.png \
> > > +  res/drawable-hdpi/home_tab_menu_strip.9.png \
> > 
> > Where is the mdpi version for this?
> > 
> Mdpi version will be added as a follow up or another patch on this bug? I
> cant seem to make a 9png out of it.

We can't land this patch without this 9png. As there is no default in drawable/ folder, this is going to crash in tbpl for sure. Atleast copy hdpi resource in mdpi folder so that we dont crash. And, why do you need a 9 png anyways? Can't we just use a ShapeDrawable??

> > @@ +67,5 @@
> > > +    public void onPageSelected(final int position) {
> > > +        mSelectedView = getChildAt(position);
> > > +
> > > +        // Callback to measure and draw the strip after the view is visible.
> > > +        ViewTreeObserver vto = mSelectedView.getViewTreeObserver();
> > 
> > Why do you need a view tree observer here? Isn't it easy enough to just set
> > the bounds based on the selectedView?
> > 
> It is not in edge cases, where we set the view but is not yet rendered. This
> means that any of the method calls related to positioning we make, will
> return 0. Hence we should only do this when the view rendered

Mmmm. Then here's my question. What's the guarantee that mSelectedView is valid? If its not rendered yet, then it won't have a Handler. And I guess it's ViewTreeObserver won't be alive too. In which case this piece of code won't be hit. Please post an example scenario where this would be hit.

> 
> > @@ +86,5 @@
> > > +    @Override
> > > +    public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) {
> > > +        if (mStrip != null) {
> > > +            final ScrollingData scrollingData = getScrollingData(position, positionOffset);
> > > +            final Rect prevBounds = new Rect(scrollingData.ScrollingAwayFromTab.getLeft(),
> > 
> > Why not use "scrollingData.awayTab.getDrawingRect(prevBounds)". ?
> >
> Because it sets the height and width of the rect, and a few other
> properties, none of which are help in figuring out positioning. We can do
> this in a more optimized way by using just four properties of the rect.
> Which I am doing currently. I did optimize rect initialization though. 

Aah right. This is fine.

> 
> > > +                                             scrollingData.ScrollingTowardsTab.getRight(),
> > > +                                             scrollingData.ScrollingTowardsTab.getBottom());
> > > +
> > > +            mStrip.setBounds((int) (prevBounds.left + ((nextBounds.left - prevBounds.left) * scrollingData.Progress)),
> > > +                                   (int) (prevBounds.top + ((nextBounds.top - prevBounds.top) * scrollingData.Progress)),
> > 
> > This is all contained in a LinearLayout, whose top and bottom are fixed.
> > Please you prevBounds.top. That should be enough.
> >
> I can, but keeping with the ideology of keeping things as extensible as
> possible, this will also allow vertical animations, should we ever choose to
> use orientation = vertical. With almost no incurring costs at all.

Ideally we won't have vertical animations. So I recommend removing this code. We have the policy of having only the stuff we want and not having generic re-usuable code, unless there is a need. Please simply this part.

> > @@ +99,5 @@
> > > +
> > > +            mStrip.setBounds((int) (prevBounds.left + ((nextBounds.left - prevBounds.left) * scrollingData.Progress)),
> > > +                                   (int) (prevBounds.top + ((nextBounds.top - prevBounds.top) * scrollingData.Progress)),
> > > +                                   (int) (prevBounds.right + ((nextBounds.right - prevBounds.right) * scrollingData.Progress)),
> > > +                                   (int) (prevBounds.bottom + ((nextBounds.bottom - prevBounds.bottom) * scrollingData.Progress)));
> > 
> > Just prevBounds.bottom.
> >
> Same argument here 

Same reply here :P
Comment on attachment 789677 [details] [diff] [review]
Patch for new tabs menu for about:home for Tablets

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

Looks pretty good. Needs one more iteration. The demo looks beautiful too.
Once the recommended changes are done and I see a new patch, I would be happy to give an r+.

::: mobile/android/base/home/TabMenuStrip.java
@@ +33,5 @@
> +    private Drawable mStrip;
> +    private View mSelectedView;
> +
> +    private Rect prevBounds;
> +    private Rect nextBounds;

These could be made final.

@@ +35,5 @@
> +
> +    private Rect prevBounds;
> +    private Rect nextBounds;
> +
> +    // This variable is used to predict the dircetion of scroll.

direction.

@@ +38,5 @@
> +
> +    // This variable is used to predict the dircetion of scroll.
> +    private float mPrevProgress;
> +
> +    // We use a static class for saving scrolling data to save on object creation.

"Data associated with the scrolling of the strip drawable".

@@ +42,5 @@
> +    // We use a static class for saving scrolling data to save on object creation.
> +    private static class ScrollingData {
> +        public static View toTab;
> +        public static View fromTab;
> +        public static float progress;

Package access works fine for this.
I like the idea of enclosing this inside a class. However, its not much of a meta data to hold separately.
Instead of having a static class that's going to occupy memory always, I would recommend moving these 3 variables (just because its just 3 variables) out to the main class itself.

@@ +112,5 @@
> +
> +        nextBounds.left =  ScrollingData.toTab.getLeft();
> +        nextBounds.top =  ScrollingData.toTab.getTop();
> +        nextBounds.right = ScrollingData.toTab.getRight();
> +        nextBounds.bottom = ScrollingData.toTab.getBottom();

If you would have only horizontal scrolling, you could reduce this to 4 final int variables. That's better than a Rect class.

@@ +117,5 @@
> +
> +        mStrip.setBounds((int) (prevBounds.left + ((nextBounds.left - prevBounds.left) * ScrollingData.progress)),
> +                         (int) (prevBounds.top + ((nextBounds.top - prevBounds.top) * ScrollingData.progress)),
> +                         (int) (prevBounds.right + ((nextBounds.right - prevBounds.right) * ScrollingData.progress)),
> +                         (int) (prevBounds.bottom + ((nextBounds.bottom - prevBounds.bottom) * ScrollingData.progress)));

As I had mentioned, please use this only for horizontal scrolling. We can add changes if we make vertical scrolling.

@@ +131,5 @@
> +        if (position >= getChildCount() - 1) {
> +            return;
> +        }
> +
> +        float currProgress = position + positionOffset;

final.

Add a new line after this.

@@ +166,5 @@
> +            return;
> +        }
> +
> +        int i = 0;
> +        int numTabs = getChildCount();

final.

New line here.

@@ +176,5 @@
> +                    // A view is focused so send an event to announce the menu strip state.
> +                    sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED);
> +                }
> +                break;
> +            }

New line here.
Attachment #789677 - Flags: review?(sriram)
Attachment #789677 - Flags: review-
Attachment #789677 - Flags: feedback+
Made changes based on the above comments.
Attachment #789677 - Attachment is obsolete: true
Attachment #789912 - Flags: review?(sriram)
Comment on attachment 789912 [details] [diff] [review]
Patch for new tabs menu for about:home for Tablets

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

Perfect!! :)
Attachment #789912 - Flags: review?(sriram) → review+
(In reply to Shilpan Bhagat from comment #23)
> Here's a test build to play with while I get another patch up:
> https://dl.dropboxusercontent.com/u/11916346/mozilla/tabstripbuild.apk

Mind posting an updated build with the new changes? Thanks
Flags: needinfo?(ibarlow)
https://hg.mozilla.org/mozilla-central/rev/353b024859c1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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

Creator:
Created:
Updated:
Size: