Ian Barlow (:ibarlow) Reporter
	Description • 11 years ago

A few things are coming up as we work through implementation on the visited panel: "visited" seems to be confusing people, and the vertical tabs on the bottom of the screen feel heavy and are also inconsistent with what we're proposing on tablets, where we use tabs on the side.

After some discussion with Lucas, we ended up with a rough sketch like this: http://cl.ly/image/0v2N071q1X1y

* Three tabs along the bottom, for Top Sites / History / Tabs from last time
* Change panel title to History

	Ian Barlow (:ibarlow) Reporter
	Comment 1 • 11 years ago

Icons are in progress, but this can be used as a reference to start the UI work.

	Ian Barlow (:ibarlow) Reporter
	Comment 2 • 11 years ago

Note: the bottom tabs can only be toggled by tapping them. Swiping only affects the top panel tabs.

	Shilpan Bhagat Assignee
	Comment 3 • 11 years ago

A very rough patch with a lot of work pending on the look and feel of things. But I wanted to put a patch through for some feedback on the direction I am heading. This patch uses nested fragments.

I will also need some drawables from Ian to replace the current selection bar on the tab widget (currently I am using the selection bar used by icontabwidget). And the images to be used in the tab.

	Lucas Rocha (:lucasr)
	Comment 4 • 11 years ago

Comment on attachment 780268 [details] [diff] [review]
[WIP] Adding tab at the bottom in visited page

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

Looks pretty good. Looks like a simple approach that could work well (even with the nested fragments).

Let's change the class names to better match the UI design. So, rename:
VisitedPage -> MostVisitedPage
HistoryPage -> MostRecentPage
VisitedPageContainer -> HistoryPage
Keep LastTabsPage as is.

Note that you'll have to update/add new strings for this new UI. For instance, the 'Visited' page will now be called 'History'. Also, rename the Page.VISITED enum to match the new UI (i.e. Page.History).

::: mobile/android/base/home/HomeFragment.java
@@ +44,5 @@
>      private static final String REGEX_URL_TO_TITLE = "^([a-z]+://)?(www\\.)?";
>  
>      protected void showSubPage(Fragment subPage) {
> +        getChildFragmentManager().beginTransaction()
> +                .addToBackStack(null).replace(R.id.visited_page_container, subPage)

This method does not belong to HomeFragment anymore as it's specific to the visited page now. Move this to VisitedPageContainer.

::: mobile/android/base/home/LastTabsPage.java
@@ +101,5 @@
>      @Override
>      public void onViewCreated(View view, Bundle savedInstanceState) {
>          final TextView title = (TextView) view.findViewById(R.id.title);
>          title.setText(R.string.home_last_tabs_title);
> +        title.setVisibility(View.VISIBLE);

Title is always visible, no?

::: mobile/android/base/home/VisitedPage.java
@@ -103,5 @@
>          });
>  
>          registerForContextMenu(mList);
> -
> -        mEmptyMessage = view.findViewById(R.id.empty_message);

You will still need this, no?

@@ +185,5 @@
>                  // flashing the empty message before loading.
>                  mList.setEmptyView(mEmptyMessage);
>  
> +                final int tabVisibility = (c.getCount() == 0 ? View.GONE : View.VISIBLE);
> +                final View parent = (View) getView().getParent();

What is this trying to accomplish exactly? Hide the tabs at the bottom when history is empty? With the new UI, this page will not have to care about the tabs directly. Simply show the empty message and that's all.

::: mobile/android/base/home/VisitedPageContainer.java
@@ +28,5 @@
> +    private int mSelectedTab;
> +
> +    @Override
> +    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
> +        return inflater.inflate(R.layout.home_visited_container_page, container, false);

You forgot to include this layout file in the patch I guess?

@@ +35,5 @@
> +    @Override
> +    public void onViewCreated(View view, Bundle savedInstanceState) {
> +        super.onViewCreated(view, savedInstanceState);
> +
> +        ImageButton button;

nit: Move this button variable declaration down. Just before its first use in the method.

@@ +36,5 @@
> +    public void onViewCreated(View view, Bundle savedInstanceState) {
> +        super.onViewCreated(view, savedInstanceState);
> +
> +        ImageButton button;
> +        Resources resources = view.getContext().getResources();

final

@@ +49,5 @@
> +
> +        button = mTabWidget.addTab(R.drawable.tabs_synced);
> +        button.setContentDescription(resources.getString(R.string.tabs_synced));
> +
> +        showVisitedPage();

This implies that the default tab is the first one, right? I'd be more explicit about it, just in case.

@@ +52,5 @@
> +
> +        showVisitedPage();
> +
> +        mTabWidget.setTabSelectionListener(this);
> +        mTabWidget.setCurrentTab(mSelectedTab);

Is mSelectedTab actually needed?

@@ +58,5 @@
> +
> +    @Override
> +    public void onTabChanged(int index) {
> +        if (index != mSelectedTab) {
> +            if (index == 0)

nit: Add {}'s on all if's, even if they are one-liners.

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

Where is this file used?

::: mobile/android/base/resources/drawable/page_title_background.xml
@@ +5,5 @@
> +          android:top="-1dp">
> +
> +        <shape android:shape="rectangle" >
> +            <stroke android:width="1dp"
> +                    android:color="@color/doorhanger_divider_dark" />

Is this to add a background color with a divider at the bottom?

::: mobile/android/base/resources/layout/home_history_page.xml
@@ +10,5 @@
>                android:background="@android:color/white">
>  
>      <include layout="@layout/home_list_with_title"/>
>  
> +</LinearLayout>

Unrelated?

::: mobile/android/base/resources/layout/home_list_with_title.xml
@@ +15,3 @@
>      <TextView android:id="@+id/title"
> +              style="@style/Widget.Home.PageTitle"
> +              android:visibility="gone"/>

The title is always visible, right? Maybe it will be different on tablets but, at least for phones, there's no need to hide the title view here.

	Ian Barlow (:ibarlow) Reporter
	Comment 5 • 11 years ago

Assets are included here, and these are the colours for the tab background and active tab indicator: http://cl.ly/image/0G0z1O3H1B2e

	Shilpan Bhagat Assignee
	Comment 6 • 11 years ago

This patch implements the tabs on the bottom of the history page

The mdpi pngs are causing trouble with draw9. I am looking into it. However, the rest is good to go.

	Shilpan Bhagat Assignee
	Comment 7 • 11 years ago

good to review* :)

	Shilpan Bhagat Assignee
	Comment 8 • 11 years ago

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 780268 [details] [diff] [review]
> [WIP] Adding tab at the bottom in visited page
> 
> Review of attachment 780268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good. Looks like a simple approach that could work well (even
> with the nested fragments).
> 
> Let's change the class names to better match the UI design. So, rename:
> VisitedPage -> MostVisitedPage
> HistoryPage -> MostRecentPage
> VisitedPageContainer -> HistoryPage
> Keep LastTabsPage as is.
> 
> Note that you'll have to update/add new strings for this new UI. For
> instance, the 'Visited' page will now be called 'History'. Also, rename the
> Page.VISITED enum to match the new UI (i.e. Page.History).
> 
Oops, I missed this comment. Will put up another patch with these changes.
> ::: mobile/android/base/home/VisitedPage.java
> @@ -103,5 @@
> >          });
> >  
> >          registerForContextMenu(mList);
> > -
> > -        mEmptyMessage = view.findViewById(R.id.empty_message);
> 
> You will still need this, no?
> 
It will be needed and I have this setup in home_list_with_title and is being set in VisitedPage.
This will also be useful later on for bug 895867.
> ::: mobile/android/base/home/VisitedPageContainer.java
> @@ +28,5 @@
> > +    private int mSelectedTab;
> > +
> > +    @Override
> > +    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
> > +        return inflater.inflate(R.layout.home_visited_container_page, container, false);
> 
> You forgot to include this layout file in the patch I guess?
> 

> @@ +49,5 @@
> > +
> > +        button = mTabWidget.addTab(R.drawable.tabs_synced);
> > +        button.setContentDescription(resources.getString(R.string.tabs_synced));
> > +
> > +        showVisitedPage();
> 
> This implies that the default tab is the first one, right? I'd be more
> explicit about it, just in case.
> 
> @@ +52,5 @@
> > +
> > +        showVisitedPage();
> > +
> > +        mTabWidget.setTabSelectionListener(this);
> > +        mTabWidget.setCurrentTab(mSelectedTab);
> 
> Is mSelectedTab actually needed?
> 
Yes, it is needed to store the current selected tab so that, if the tab is pressed again, the view doesn't reload
> ::: mobile/android/base/resources/drawable/empty_divider.xml
> @@ +1,1 @@
> > +<?xml version="1.0" encoding="utf-8"?>
> 
> Where is this file used?
> 
It's not used anymore, I was experimenting something. 
> ::: mobile/android/base/resources/drawable/page_title_background.xml
> @@ +5,5 @@
> > +          android:top="-1dp">
> > +
> > +        <shape android:shape="rectangle" >
> > +            <stroke android:width="1dp"
> > +                    android:color="@color/doorhanger_divider_dark" />
> 
> Is this to add a background color with a divider at the bottom?
> 
Yes, this adds a divider at the bottom of the title.

	Shilpan Bhagat Assignee
	Comment 9 • 11 years ago

This patch adds bottom tabs on history page and tries to get as close to the new naming convention without making any major class name changes.

It uses nested fragments for the three subpages and IconTabWidget for the tab.

	Shilpan Bhagat Assignee
	Comment 10 • 11 years ago

This patch changes the naming conventions of the strings in the locales and other files depending on the names to make the naming convention more coherent

Apply after Part 1. :)

	Shilpan Bhagat Assignee
	Comment 11 • 11 years ago

This patch makes major changes in terms of class names. 

VisitedPageContainer -> HistoryPage
VisitedPage -> MostVisitedPage
HistoryPage -> MostRecentPage

	Lucas Rocha (:lucasr)
	Comment 12 • 11 years ago

Comment on attachment 781509 [details] [diff] [review]
Part 1: Adding tabs at the bottom in history page

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

Looks excellent. I just want to see a new version of this patch with the suggested changes before giving r+. Please share a test build so that we can try it.

::: mobile/android/base/home/VisitedPageContainer.java
@@ +60,5 @@
> +    }
> +
> +    @Override
> +    public void onTabChanged(int index) {
> +        if (index != mSelectedTab) {

I tend to prefer early returns over these if's that enclose the whole method's code. So, change this to

if (index == mSelected) {
    return;
}

// ...

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +7,5 @@
>  <!ENTITY  error_loading_file "An error occurred when trying to load files required to run &brandShortName;">
>  
>  <!ENTITY  all_pages_title "Top Sites">
>  <!ENTITY  bookmarks_title "Bookmarks">
> +<!ENTITY  history_title "Most recent">

When you change an existing string, you have to rename its identifier too. Double-check with mfinkle if that is still the case. If you have to rename, name it home_history_title and place it together with the other about:home strings below.

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

Rename to home_history_tabs_indicator

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

Rename this to home_page_title_background for consistency with the other about:home resources.

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

Rename to home_history_tabs_indicator

::: mobile/android/base/resources/layout/home_visited_container_page.xml
@@ +9,5 @@
> +              android:orientation="vertical">
> +
> +    <FrameLayout android:id="@+id/visited_page_container"
> +                 android:layout_width="fill_parent"
> +                 android:layout_height="fill_parent"

Use 0dp for layout_height when you use layout_weight=1. Just a recommended optimization on Android.

::: mobile/android/base/resources/layout/tabs_panel_header.xml
@@ +9,5 @@
>                                              android:layout_width="wrap_content"
>                                              android:layout_height="fill_parent"
>                                              android:tabStripEnabled="false"
> +                                            android:divider="@drawable/tab_indicator_divider"
> +                                            android:layout="@layout/tabs_panel_indicator"/>

This change go in a separate patch that adds the android:layout param to IconTabWidget.

::: mobile/android/base/resources/values/attrs.xml
@@ +194,5 @@
>      </declare-styleable>
>  
> +    <declare-styleable name="IconTabWidget">
> +        <attr name="android:layout"/>
> +    </declare-styleable>

Ditto. This should go on a separate patch that adds this param to IconTabWidget.

::: mobile/android/base/resources/values/dimens.xml
@@ +71,5 @@
>      <dimen name="validation_message_margin_top">6dp</dimen>
>      <dimen name="forward_default_offset">-13dip</dimen>
>      <dimen name="url_bar_offset_left">32dp</dimen>
>      <dimen name="toast_button_padding">8dp</dimen>
> +    <dimen name="visited_tabs_height">50dp</dimen>

Shouldn't this be 48dp for consistency with the main toolbar? Maybe just use the existing browser_toolbar_height dimen?

::: mobile/android/base/widget/IconTabWidget.java
@@ +21,5 @@
>      public static interface OnTabChangedListener {
>          public void onTabChanged(int tabIndex);
>      }
>  
>      public IconTabWidget(Context context, AttributeSet attrs) {

The changes in IconTabWidget should be in a separate patch btw.

@@ +27,5 @@
> +
> +        TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.IconTabWidget);
> +
> +        try {
> +            mButtonLayout = a.getResourceId(R.styleable.IconTabWidget_android_layout, 0);

It seems like this parameter will become a hard requirement with this change. You should probably throw an IllegalArgumentException if mButtonLayout is 0 here.

@@ +28,5 @@
> +        TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.IconTabWidget);
> +
> +        try {
> +            mButtonLayout = a.getResourceId(R.styleable.IconTabWidget_android_layout, 0);
> +        } finally {

This 'finally' is not necessary btw. If we throw for some reason on getResourceId(), it means something is terribly wrong and the finally part won't help much :-)

	Lucas Rocha (:lucasr)
	Comment 13 • 11 years ago

Comment on attachment 781512 [details] [diff] [review]
Part 2: Making changes to strings based on new naming convention

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

You should probably fold this patch together with the main one before pushing.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +11,2 @@
>  <!ENTITY  switch_to_tab "Switch to tab">
> +<!ENTITY  history_title "History">

Rename this to home_history_title and move it below.

	Lucas Rocha (:lucasr)
	Comment 14 • 11 years ago

Comment on attachment 781514 [details] [diff] [review]
Part 3: Making changes to class name based on new naming convention

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

Nice. Fold this patch together with the main one before pushing. Hopefully, mercurial will do the right thing :-P

	Shilpan Bhagat Assignee
	Comment 15 • 11 years ago

Test build: https://dl.dropboxusercontent.com/u/11916346/mozilla/fennec-25.0a1.en-US.android-arm.apk

Will put up another patch soon.

	Shilpan Bhagat Assignee
	Comment 16 • 11 years ago

This patch adds the layout attribute to IconTabWidget and throws a runtime exception if not supplied.

	Lucas Rocha (:lucasr)
	Comment 17 • 11 years ago

Comment on attachment 781744 [details] [diff] [review]
Part 1: Adding layout attribute to IconTabWidget

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

Nice.

::: mobile/android/base/widget/IconTabWidget.java
@@ +15,5 @@
>  import android.widget.TabWidget;
>  
>  public class IconTabWidget extends TabWidget {
>      private OnTabChangedListener mListener;
> +    private final int mButtonLayout;

Rename this to mButtonLayoutId for clarity.

@@ +25,5 @@
>      public IconTabWidget(Context context, AttributeSet attrs) {
>          super(context, attrs);
> +
> +        TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.IconTabWidget);
> +

nit: remove this empty line here.

	Shilpan Bhagat Assignee
	Comment 18 • 11 years ago

Made changes as mentioned above

	Shilpan Bhagat Assignee
	Comment 19 • 11 years ago

This patch adds a tab to the bottom.

	Lucas Rocha (:lucasr)
	Comment 20 • 11 years ago

Comment on attachment 781775 [details] [diff] [review]
Part 2: Adding tabs at the bottom in history page, renaming class names and strings.

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

Awesome. Please file follow-ups for the cross-fade, the color of the 'open all tabs from last time' button, and any other obvious issues/missing bits you found while working on this.

	Shilpan Bhagat Assignee
	Comment 21 • 11 years ago

Small changes before checking in.

	Shilpan Bhagat Assignee
	Comment 22 • 11 years ago

Missed out on a minor change.

	:Margaret Leibovic
	Comment 23 • 11 years ago

Since we're landing this on fig, let's just get someone from our team to push it for you. People who look for checkin-needed will try to land this on inbound, which won't work out. You should ask on IRC if anyone is going to push to fig soon, and they can do it for you (I can also do it for you if no one else does).

	Shilpan Bhagat Assignee
	Comment 24 • 11 years ago

Resolving a conflict

	Shilpan Bhagat Assignee
	Comment 25 • 11 years ago

Conflicts make me sad

	Sriram Ramasubramanian [:sriram]
	Comment 26 • 11 years ago

Folded both the patches and pushed.
http://hg.mozilla.org/projects/fig/rev/1491af6fc2ae

	:Margaret Leibovic
	Comment 27 • 11 years ago

I backed this out for causing rc2 to fail:
https://hg.mozilla.org/projects/fig/rev/c2b5919558d3

See bug 898887 for more details. Let's get a fix for that before re-landing this patch.

	Chenxia Liu [:liuche]
	Comment 28 • 11 years ago

Shilpan, there's a bug in your strings, and "History" is used in places other than the about:home screen.

	Chenxia Liu [:liuche]
	Comment 29 • 11 years ago

While this was on nightly, the patches for this bug also seems to have introduced bug 895837.

	Shilpan Bhagat Assignee
	Comment 30 • 11 years ago

Fixing the bug which makes the test fail.

	Chenxia Liu [:liuche]
	Comment 31 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=835d489b880d

	Chenxia Liu [:liuche]
	Comment 32 • 11 years ago

(In reply to Chenxia Liu [:liuche] from comment #29)
> While this was on nightly, the patches for this bug also seems to have
> introduced bug 895837.

Whoops, I meant bug 898555.

	Chenxia Liu [:liuche]
	Comment 33 • 11 years ago

https://hg.mozilla.org/projects/fig/rev/a0890fdc1346

	Ed Morley [:emorley]
	Comment 35 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/a0890fdc1346

	cajbir (:cajbir)
	Comment 36 • 11 years ago

Since this bug landed I'm getting the following compile error:

mobile/android/base/home/HistoryPage.java:80: cannot find symbol
symbol  : method getChildFragmentManager()
location: class org.mozilla.gecko.home.HistoryPage
        getChildFragmentManager().beginTransaction()
        ^
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

Any thoughts on what I might have that is wrong?

	Lucas Rocha (:lucasr)
	Comment 37 • 11 years ago

(In reply to Chris Double (:doublec) from comment #36)
> Since this bug landed I'm getting the following compile error:
> 
> mobile/android/base/home/HistoryPage.java:80: cannot find symbol
> symbol  : method getChildFragmentManager()
> location: class org.mozilla.gecko.home.HistoryPage
>         getChildFragmentManager().beginTransaction()
>         ^
> Note: Some input files use or override a deprecated API.
> Note: Recompile with -Xlint:deprecation for details.
> 
> Any thoughts on what I might have that is wrong?

On a local build? You probably need to update your Android SDK and support library.

	cajbir (:cajbir)
	Comment 38 • 11 years ago

(In reply to Lucas Rocha (:lucasr) from comment #37)
> 
> On a local build? You probably need to update your Android SDK and support
> library.

According to the Fennec build instructions page [1] you need at least SDK 16 which I'm using. If that's changed it might be worth updating.

[1] https://wiki.mozilla.org/Mobile/Fennec/Android#Linux

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 39 • 11 years ago

(In reply to Chris Double (:doublec) from comment #38)

> According to the Fennec build instructions page [1] you need at least SDK 16
> which I'm using. If that's changed it might be worth updating.
> 
> [1] https://wiki.mozilla.org/Mobile/Fennec/Android#Linux

Those docs mention installing the "Extras / Android Support Library" in a small section of notes. We only need v4 of the support library in case that matters:

android-sdk-linux_x86/tools/android
# install "Android SDK Tools"
# install "Android SDK Platform-tools"
# install "API 17 / SDK Platform"
# install "Extras / Android Support Library"

	cajbir (:cajbir)
	Comment 40 • 11 years ago

(In reply to Mark Finkle (:mfinkle) from comment #39)
> Those docs mention installing the "Extras / Android Support Library" in a
> small section of notes. We only need v4 of the support library in case that
> matters:

Thanks, updating "Extras / Android Support Library" fixed it.