1142171 - (splitpanehistory) Use "split pane styling" for History Panel on Tablets in landscape

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Related to bug 1129171.

It makes sense to see if we can use this in our other Panels like Bookmarks.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ And History too?

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Anthony Lam (:antlam) from comment #1)
> ^ And History too?

I'm re-purposing this to be History only.  We can file a new one for Bookmarks.

Comment 3

[Vivek Balakrishnan[:vivek]]

Attachment: part1.patch

* Moved the HistoryAdapter to a separate class 
* Extracted the common features in HistoryPanel such as the Loader callbacks to the base class
* Renamed HistoryPanel to HistoryListPanel

Comment 4

[Vivek Balakrishnan[:vivek]]

Attachment: part2.patch

* Container Panel defined to hold the history list views.

Comment 5

[Vivek Balakrishnan[:vivek]]

Attachment: part3.patch

* Split list view created which split the cursor based on the date ranges
* Enabled split pane list for Tablets in landscape mode.

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

As per IRC: 

"I was picturing the left side to be “Today, Yesterday, Last 7 days, April, March, Feb..” like you get when you click “View all history” in desktop firefox"

Comment 7

[Nick Alexander :nalexander [he/him]]

vivek posted a screenshot: https://www.dropbox.com/s/zkuedkxmjo6bisg/history_landscape.png?dl=0 and here's a cap of desktop: https://www.dropbox.com/s/si9y134l04hj1vo/Screenshot%202015-05-08%2014.45.10.png?dl=0

Honestly, this /looks/ great to me.

How about, rather than some number of named months, we go "Last month", "Last year" (and maybe something else for "everything")?  vivek, how hard would it be to make 6 months (dynamic) and then "older than 6 months"?  (That may not localize well.)

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8601721 [details] [diff] [review]
part1.patch

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

License, comments, and some thoughts about how the base vs. derived classes interact, please.  But this is pretty mechanical, so r+ and I'll look at a fresher version later.

::: mobile/android/base/home/HistoryAdapter.java
@@ +1,2 @@
> +package org.mozilla.gecko.home;
> +

nit: license.

@@ +10,5 @@
> +import org.mozilla.gecko.db.BrowserContract;
> +
> +import java.util.Date;
> +
> +public class HistoryAdapter extends MultiTypeCursorAdapter {

nit: descriptive comment!  What are the types?

@@ +22,5 @@
> +    private static final long MS_PER_DAY = 86400000;
> +    private static final long MS_PER_WEEK = MS_PER_DAY * 7;
> +
> +    // The time ranges for each section
> +    private static enum MostRecentSection {

Ah, I see this is not very configurable :(  And I see why you had questions about adding sections and cursors for the split-pane implementation.  I think one cursor and manual ranges is acceptable even in split-pane.  Simplicity!

::: mobile/android/base/home/HistoryBasePanel.java
@@ +1,1 @@
> +package org.mozilla.gecko.home;

nit: license.

@@ +32,5 @@
> +import org.mozilla.gecko.TelemetryContract;
> +import org.mozilla.gecko.db.BrowserDB;
> +
> +/**
> + * Fragment backed by <code>HistoryAdapter<code> to displays history tabs sorted by date ranges.

Nit: update comment to include discussion of this being a base class specialized by a vanilla list and a split-pane implementation.  What does the base class handle?  What does the derived class handle?

@@ +129,5 @@
> +            return mDB.getRecentHistory(cr, HISTORY_LIMIT);
> +        }
> +    }
> +
> +    protected void updateUiFromCursor(Cursor c) {

I feel like this isn't quite right -- it's not clear how a base class is intended to override this to actually do its UI update.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8601725 [details] [diff] [review]
part2.patch

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

A question, but it's looking fine.

::: mobile/android/base/home/HistoryContainerPanel.java
@@ +36,5 @@
> +
> +    @Override
> +    protected void loadIfVisible() {
> +        Fragment currentFragment;
> +        // Force reload fragment only in tablets when the orientation has changed.

Remind me why we're doing this only on tablets?

@@ +41,5 @@
> +        if (canLoad() && HardwareUtils.isTablet() && (currentFragment = getChildFragmentManager().findFragmentByTag(FRAGMENT_TAG)) != null) {
> +            if (currentFragment.getArguments().getInt(FRAGMENT_ORIENTATION) != GeckoScreenOrientation.getInstance().getAndroidOrientation()) {
> +                // As the fragment becomes visible only after onStart callback, we can safely remove it from the back-stack.
> +                // If a portrait fragment is in the back-stack and then a landscape fragment should be shown, there can
> +                // be a brief flash as the fragment are replaced.

nit: fragments.

::: mobile/android/base/resources/layout/home_history_container_panel.xml
@@ +7,5 @@
> +              android:layout_width="match_parent"
> +              android:layout_height="match_parent"
> +              android:orientation="vertical">
> +
> +    <FrameLayout android:id="@+id/history_panel_container"

You have history_container_panel in a bunch of places, and then history_panel_container here.  Can we make this just "@+id/container"?

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8601726 [details] [diff] [review]
part3.patch

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

Just a few notes.  I didn't review the tricky cursor handling 'cuz you have a work in progress patch.  Looking good!  Sorry for the slow review.

::: mobile/android/base/home/HistoryAdapter.java
@@ +21,5 @@
>      // For the time sections in history
>      private static final long MS_PER_DAY = 86400000;
>      private static final long MS_PER_WEEK = MS_PER_DAY * 7;
>  
>      // The time ranges for each section

protected?  And fold these in to the earlier patch.

::: mobile/android/base/home/HistorySplitListPanel.java
@@ +99,5 @@
> +                mUrlOpenListener.onUrlOpen(url, EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));
> +            }
> +        });
> +
> +        mItemList.setContextMenuInfoFactory(new HomeContextMenuInfo.Factory() {

I feel like we should be able to share some of this between the list and the splitpane implementations.

@@ +170,5 @@
> +            mRangeAdapter.clear();
> +            mItemAdapter.swapCursor(mAdapter.getCursor());
> +            selected = null;
> +
> +            final SparseArray<MostRecentSection> mostRecentSections = mAdapter.getmMostRecentSections();

nit: getMostRecentSections.

::: mobile/android/base/moz.build
@@ +306,5 @@
>      'home/HistoryAdapter.java',
>      'home/HistoryBasePanel.java',
>      'home/HistoryContainerPanel.java',
>      'home/HistoryListPanel.java',
> +    'home/HistorySplitListPanel.java',

Try to be consistent with RemoteTabs -- HistorySplitPaneFragment.

::: mobile/android/base/resources/layout/home_history_split_plane_panel.xml
@@ +20,5 @@
> +        android:orientation="horizontal">
> +
> +        <org.mozilla.gecko.home.HomeListView
> +            android:id="@+id/range_list"
> +            style="@style/Widget.RemoteTabsListView"

Either extract a common style, or duplicate this one so they can be styled differently.

@@ +23,5 @@
> +            android:id="@+id/range_list"
> +            style="@style/Widget.RemoteTabsListView"
> +            android:layout_width="0dp"
> +            android:layout_height="match_parent"
> +            android:layout_weight="0.32"/>

Can we extract these weights into a shared file?  Maybe into dimens.xml?

@@ +28,5 @@
> +
> +        <View
> +            android:layout_width="1dp"
> +            android:layout_height="match_parent"
> +            android:background="#D7D9DB"/>

And this color -- I'm sure it's re-used.

@@ +39,5 @@
> +            android:layout_weight="0.68"/>
> +
> +    </LinearLayout>
> +
> +    <LinearLayout android:layout_width="match_parent"

This button seems odd.  I wonder if this is where antlam will want it.

Comment 11

[Vivek Balakrishnan[:vivek]]

Attachment: part1.patch

Comments, license, review nits addressed.

Comment 12

[Vivek Balakrishnan[:vivek]]

Attachment: part2.patch

Review nits addressed

Comment 13

[Vivek Balakrishnan[:vivek]]

Attachment: part3.patch

Styles and split dimensions added to xml files

Comment 14

[Vivek Balakrishnan[:vivek]]

Attachment: part4.patch

History sections updated to be similar to that in FF desktop. Section string are localized.

Comment 15

[Vivek Balakrishnan[:vivek]]

Try server run:https://treeherder.mozilla.org/#/jobs?repo=try&revision=25adc7adb33f

Comment 16

[Vivek Balakrishnan[:vivek]]

(In reply to Nick Alexander :nalexander from comment #9)
> Comment on attachment 8601725 [details] [diff] [review]
> part2.patch
> 
> Review of attachment 8601725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A question, but it's looking fine.
> 
> ::: mobile/android/base/home/HistoryContainerPanel.java
> @@ +36,5 @@
> > +
> > +    @Override
> > +    protected void loadIfVisible() {
> > +        Fragment currentFragment;
> > +        // Force reload fragment only in tablets when the orientation has changed.
> 
> Remind me why we're doing this only on tablets?

In tablets when the orientation changes the fragment backstack would have the wrong list view. we are overriding loadIfVisible() here to pop that fragment iff it is not the right one from the backstack and load a new fragment for the current configuration.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8611303 [details] [diff] [review]
part4.patch

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

Strings LGTM.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +49,5 @@
>  <!ENTITY bookmark_options "Options">
>  
>  <!ENTITY history_today_section "Today">
>  <!ENTITY history_yesterday_section "Yesterday">
> +<!ENTITY history_week_section3 "Last 7 days">

Desktop does this, so this is almost certainly fine, but FYSA: many style guides have numerals below a certain value spelled out, so "Last seven days", "Last 30 days". If you want to be triple-sure, check with UX.

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8611303 [details] [diff] [review]
part4.patch

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

I've asked for a pretty big rework of this patch, but it's definitely coming along.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +690,5 @@
>      }
>  
>      @Override
>      public Cursor getRecentHistory(ContentResolver cr, int limit) {
> +        return cr.query((limit > 0) ? combinedUriWithLimit(limit) : mCombinedUriWithProfile,

Ah, I see now.  This is dangerous.  I don't know what the right thing to do is, but it's not fetching all history.  I think rnewman will have a good opinion.

::: mobile/android/base/home/HistoryAdapter.java
@@ +30,5 @@
>  
>      // For the time sections in history
>      private static final long MS_PER_DAY = 86400000;
>      private static final long MS_PER_WEEK = MS_PER_DAY * 7;
> +    private static final Calendar FIRST_DAY_OF_CURRENT_MONTH = Calendar.getInstance();

You're not using this in a static way, so it shouldn't be static.

I'm quite confused by what you're trying to accomplish here, but let's attack this in stages.  First, I think a history adapter should just keep its current time in an instance variable.  Each time the cursor updates, the current time should roll forward.

Second, can we make the section logic simpler?  How about storing the "current time" the adapter knows about like I suggest, and then include a "milliseconds from first time" value for each section, using an enum with values (like http://stackoverflow.com/a/8811838)?

So we'd have TODAY(0), YESTERDAY(one day in ms, or seconds, or days), WEEK(seven days in unit), ONE_MONTH_AGO(31 days in ...).  Oh, urgh, that doesn't account for variable months.

Different approach: we include an array of (earliest time, section header string).  Each time the cursor is updated, we recreate the array relative to the new time.  Then we walk backwards checking times as needed to determine sections.  Reasonable?

@@ +45,5 @@
>          TODAY,
>          YESTERDAY,
>          WEEK,
> +        THIS_MONTH,
> +        MONTH_AGO,

ONE_MONTH_AGO (might as well be consistent).

@@ +152,5 @@
>      }
>  
> +    private String getMonthString(final int monthsBefore) {
> +        // Shift back calendar month.
> +        FIRST_DAY_OF_CURRENT_MONTH.add(Calendar.MONTH, -monthsBefore);

Nifty, but don't use the static (or the member) for this temporary calculation.

@@ +191,5 @@
>          if (delta < MS_PER_WEEK) {
>              return MostRecentSection.WEEK;
>          }
>  
> +        int i = 0;

Don't hardcode the 5.  I'd like to see this data driven, meaning calculated from some data structure, rather thana encoding days/weeks/months specially.

::: mobile/android/base/home/HistoryBaseFragment.java
@@ +114,5 @@
>      }
>  
>      protected static class HistoryCursorLoader extends SimpleCursorLoader {
>          // Max number of history results
> +        private static final int HISTORY_LIMIT = -1;

This /definitely/ needs a comment explaining the meaning of things.

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8611297 [details] [diff] [review]
part1.patch

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

Rubber stamp.  Looks okay to me!

Comment 20

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8611299 [details] [diff] [review]
part2.patch

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

Rubber stamp.  Looks okay to me!

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8611302 [details] [diff] [review]
part3.patch

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

Keeping the r? 'cuz I need to get back to this.

::: mobile/android/base/home/HistoryAdapter.java
@@ +190,5 @@
>              }
>          } while (c.moveToNext());
>      }
>  
> +    public SparseArray<MostRecentSection> getMostRecentSections() {

Fold this into the original patch.

::: mobile/android/base/home/HistoryBaseFragment.java
@@ +129,5 @@
>              return mDB.getRecentHistory(cr, HISTORY_LIMIT);
>          }
>      }
>  
> +    protected HomeContextMenuInfo getHomeContextMenuInfo(final View view, final int position, final long id, final Cursor cursor) {

And this fold this into the earlier patch.

::: mobile/android/base/home/HistorySplitPlaneFragment.java
@@ +195,5 @@
> +            mItemAdapter.swapCursor(mAdapter.getCursor());
> +            selected = null;
> +
> +            final SparseArray<MostRecentSection> mostRecentSections = mAdapter.getMostRecentSections();
> +            for (int i = 0; i < mostRecentSections.size(); i++) {

This is very confusing.  I will need to look at this more closely, but I'm quite tired right now so I'm going to leave it for a while.

Comment 22

[Vivek Balakrishnan[:vivek]]

Attachment: part1.patch

A new patch with common functionsrealted to history parsing pulled to base fragment

Comment 23

[Vivek Balakrishnan[:vivek]]

Attachment: part2.patch

Added container fragment and placeholder code for split plane fragments

Comment 24

[Vivek Balakrishnan[:vivek]]

Attachment: part3.patch

Final part of the patch: 
Split plane fragment,
As discussed over irc, each split plane segments issues separate query 
localization strings for the sections.

Comment 25

[Nick Alexander :nalexander [he/him]]

I'm not going to get to this before Whistler, and having the flags in my queue is stressing me out.  Let's revisit in July.

Comment 26

[Jenn Chaulk (:jchaulk)]

Is this something that might be feasible for 43?

Comment 27

[Vivek Balakrishnan[:vivek]]

Agreed with Sebastian, we are targeting 43

Comment 28

[Sebastian Kaspari (:sebastian; :pocmo)]

The patches do not compile using 'mach'. After a quick look it seems like home_history_split_plane_panel.xml accidentally ended up in the crashreporter's resource directory:

mobile/android/base/crashreporter/res/layout/home_history_split_plane_panel.xml

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: history_tablet.png

@Anthony: I feel like the touch targets for the list are very small. Especially compared to the list under "Synced tabs" - what do you think?

Maybe we need to change the wording of the empty panel too? "Websites you visited *most recently* show up here." is not really correct if "Older than 6 months" is selected.

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8616378 [details] [diff] [review]
part1.patch

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

This split pane is so much more fun to use on tablets then just having a simple list! :)

There are some NITs but it seems like most of them have already been in HistoryPanel before. So I guess there's no pressing need to get rid of them. However if you agree and if some further iterations are needed then feel free to fix them too, now that we split and move the code around.

::: mobile/android/base/home/HistoryBaseFragment.java
@@ +44,5 @@
> +    // For the time sections in history
> +    private static final long MS_PER_DAY = 86400000;
> +    private static final long MS_PER_WEEK = MS_PER_DAY * 7;
> +
> +    // Logging tag name

NIT: I feel like this comment is not adding much additional information. I usually tend to pick a very descriptive name for the class member and only add comments if additional clarification is needed.

@@ +55,5 @@
> +    protected static final String FORMAT_S1 = "%1$s";
> +    protected static final String FORMAT_S2 = "%2$s";
> +
> +    // The button view for clearing browsing history.
> +    protected View mClearHistoryButton;

NIT: Same here: You picked the perfect name for the button and it's absolutely clear what it should do. I don't think the comment is needed.

@@ +245,5 @@
> +    }
> +
> +    protected static class HistoryCursorLoader extends SimpleCursorLoader {
> +        // Max number of history results
> +        private static final int HISTORY_LIMIT = 100;

If we just have a simple history list - like before - then limiting the list makes sense. However now that we split them into specific groups this will hide some data in between. For example "Last 7 days" might be cut-off after 100 entries but we might show the full list of let's say 75 entries for 'May'. This creates some 'invisible' gaps. I wonder if this is a problem.

Regardless of this I feel like 100 is a quite small number of items for a scrolling list in general.

Comment 31

[Vivek Balakrishnan[:vivek]]

Attachment: part1.patch

Re-based against the latest central and comment nits corrected

Comment 32

[Vivek Balakrishnan[:vivek]]

Attachment: part2.patch

Re-base and reviewer changed in comment line

Comment 33

[Vivek Balakrishnan[:vivek]]

Attachment: part3.patch

Moved the layout to res/layout.
@Sebastian: Can you please try now if it compiles for you.

Comment 34

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Sebastian Kaspari from comment #29)
> Created attachment 8636514 [details]
> history_tablet.png
> 
> @Anthony: I feel like the touch targets for the list are very small.
> Especially compared to the list under "Synced tabs" - what do you think?
> 
> Maybe we need to change the wording of the empty panel too? "Websites you
> visited *most recently* show up here." is not really correct if "Older than
> 6 months" is selected.

I agree, but we should try to handle the split pane issue first. We can file a follow up related to bug 1091826 for the empty state.

As for the touch targets, I think we'll need to make them the same size (and look similar) as bug 1129171 and also inherit the tail-less arrow. I don't have my tablet on my right now but I think we even enlarged it to match the height of the links on the right hand side. 

Can we see how that looks?

Comment 35

[Vivek Balakrishnan[:vivek]]

Attachment: part3.patch

Patch updated with Antlam's UI changes

Sample screenshots:
https://www.dropbox.com/s/idbichrgrrbe5dh/history_split.png?dl=0

Comment 36

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8636612 [details] [diff] [review]
part1.patch

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

LGTM!

::: mobile/android/base/home/HistoryListFragment.java
@@ +204,5 @@
>  
>              mContext = context;
>  
>              // Initialize map of history sections
> +            mMostRecentSections = new SparseArray<>();

I'm keen to see if this will compile on the builders as <> is a feature of Java 7.

Comment 37

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8636613 [details] [diff] [review]
part2.patch

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

I feel like the HistoryContainerPanel might be overkill. Already looking at patch 3: Do you think the HistorySplitPlaneFragment could handle both cases: There's always a history list and there's an optional range list. It's more or less two different layouts that need to be loaded depending on device. If there's a range list then clicks on it will change the history list. If there's no range list then there's only a history list. So yeah, do you think the functionality could be shrunk into a single Fragment using Range/History components? It resembles the behavior you'd implement with non-"support fragments" and resource qualifiers: http://developer.android.com/guide/practices/tablets-and-handsets.html#Fragments

I'd like to hear your opinion on that. But I also don't want to make everything more complicated just before the finish line! :)

::: mobile/android/base/home/HistoryContainerPanel.java
@@ +17,5 @@
> +
> +/**
> + * A <code>HomeFragment</code> that, holds the History list fragment for the current device and orientation.
> + */
> +public class HistoryContainerPanel extends HomeFragment {

To be consistent with the other names, we should name this HistoryContainerFragment.

@@ +18,5 @@
> +/**
> + * A <code>HomeFragment</code> that, holds the History list fragment for the current device and orientation.
> + */
> +public class HistoryContainerPanel extends HomeFragment {
> +    private static final String FRAGMENT_TAG = "backstack_tag";

This is a bit confusing name (backstack_tag) because the tag is not directly related to the backstack but identifying the fragment.

@@ +72,5 @@
> +                .commitAllowingStateLoss();
> +    }
> +
> +    private Fragment getFragmentForOrientation() {
> +        // TODO: Create splitpane here based on orientation for tablets.

I see that this TODO is handled in patch 3. Feel free to squash them to a single commit to avoid reviewing intermediate states.

Comment 38

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8636760 [details] [diff] [review]
part3.patch

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

LGTM, just some nits.

::: mobile/android/base/home/HistorySplitPlaneFragment.java
@@ +34,5 @@
> + * Fragment that displays other date ranges and tabs in two separate <code>ListView<code> instances.
> + * <p/>
> + * This is intended to be used in landscape mode on tablets.
> + */
> +public class HistorySplitPlaneFragment extends HistoryBaseFragment {

Should this be named Pane instead of Plane?

::: mobile/android/base/resources/layout/home_history_split_plane_panel.xml
@@ +42,5 @@
> +    </LinearLayout>
> +
> +    <LinearLayout android:layout_width="match_parent"
> +                  android:layout_height="wrap_content"
> +                  android:background="@color/home_button_bar_bg">

Does this LinearLayout serve any purpose? We could stretch the button to match_parent, couldn't we?

::: mobile/android/base/resources/values/dimens.xml
@@ +215,5 @@
>      <item name="match_parent" type="dimen">-1</item>
>      <item name="wrap_content" type="dimen">-2</item>
>  
> +    <item name="split_plane_left_pane_weight" format="float" type="dimen">0.32</item>
> +    <item name="split_plane_right_pane_weight" format="float" type="dimen">0.68</item>

I'm curious to know how you came up with these numbers. I'd probably use a fixed size for the 'range' pane and let the 'history list' pane use whatever is left.

::: mobile/android/base/resources/values/styles.xml
@@ +556,5 @@
>          <item name="android:drawSelectorOnTop">true</item>
>      </style>
>  
> +    <style name="Widget.HistoryListView" parent="Widget.HomeListView">
> +        <item name="android:childDivider">#E7ECF0</item>

Now that we have at least two widgets using this color (with bookmarks maybe following too), let's define it in colors.xml

Comment 39

[Sebastian Kaspari (:sebastian; :pocmo)]

This is looking great! Let's just discuss the fragment 'fusion' mentioned above. Either here or on IRC.

Let's also take care of filing follow-up bugs for things like:
* Empty state for things like 'Older than 6 months' (see above)
* Maybe we should move "Clear history" below the right pane instead of below both (debatable)
* Let's try to use just the time limits instead of a hard limit to avoid history gaps
* Let's do this split pane thing for bookmarks too! (As mentioned in the first bug)

Comment 40

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh, and one small thing: Please use r=sebastian instead of r=s.kaspari :)

Comment 41

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8636760 [details] [diff] [review]
part3.patch

The screenshot is looking good. + for progress! Can we use the same type spec as the device name? 

Also, what's the padding to the left currently spec'd at?

Comment 42

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1142171 - Pre: Refactor HistoryAdapter to a separate class and defined section ranges r?sebastian.

Bug 1142171 - Pre: Refactor HistoryAdapter to a separate class and defined section ranges  r?sebastian.

Comment 43

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1142171: Split history List view for tablet landscape mode r?sebastian.

Bug 1142171: Split history List view for tablet landscape mode r?sebastian.

Comment 44

[Vivek Balakrishnan[:vivek]]

screenshot for updated layout:
https://www.dropbox.com/s/9mcog74bpzmcgen/newlayout.png?dl=0

@antlam:
I have used margin 16dp. should center the text?

Comment 45

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Looks good! Can we do 15dp? that would match our padding elsewhere.

Vivek, could I get a build to test this on my device please? the screenshot looks a bit low res :)

Thanks!

Comment 46

[Vivek Balakrishnan[:vivek]]

Hi Anthony,

Please find the apk at https://www.dropbox.com/s/up2ww7m36xdpxod/fennec-43.0a1.en-US.android-arm.apk?dl=0

I have changed margin to 15dp and centered the text

Comment 47

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Thanks Vivek,

I just checked it out!I think we need to keep these labels left aligned, with 15dp padding on the left and centered vertically (the arrow icon as well needs to be centered vertically). I notice the padding on top and beneath are not even in your build (tested on my N9).

Finally, can we also follow the type specifications in the "Synced Tabs" panel? I.e. the states for "selected" and "unselected" (for the dates on the left) in your build are not the same as the "Synced Tabs" panel.

I think "selected" should be Roboto Light, 14sp, #363B40 (Text and tabs tray grey in our palette). And "unselected" would be Roboto Light, 14sp, #BFBFBF (disabled grey I think). You can probably double check this from our work in the synced tabs panel split pane :)

Thanks vivek!

I think that should do it :)

Comment 48

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8647236 [details]
MozReview Request: Bug 1142171 - Pre: Refactor HistoryAdapter to a separate class and defined section ranges  r?sebastian.

https://reviewboard.mozilla.org/r/15955/#review14423

Ship It!

Comment 49

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8647237 [details]
MozReview Request: Bug 1142171: Split history List view for tablet landscape mode r?sebastian.

https://reviewboard.mozilla.org/r/15957/#review14425

Awesome refactoring! The code looks much simpler and is easier to follow. :)

I added some comments/questions to the review but that's nothing that should stop this from landing, so r+.

::: mobile/android/base/home/HistoryItemAdapter.java:18
(Diff revision 1)
> + * A Cursor adapter implementation that always iterate over the start to start + length - 1 items.

Isn't this Adapter independent from any "start" and just always creates views for everything in the cursor?

::: mobile/android/base/home/HistoryItemAdapter.java:35
(Diff revision 1)
> +        // Return view from position relative to the start.

Same question: What start?

::: mobile/android/base/home/HistoryPanel.java:127
(Diff revision 1)
> +    }

I'd let the Android system make this decision using resource qualifiers: Naming both files the same, putting the default in res/layout and the split pane version into res/layout-sw600dp-land.

::: mobile/android/base/home/HistoryPanel.java:137
(Diff revision 1)
> +        if(mRangeList != null) {

NIT: Space: if_(

Also: If this is used more often in this class then I'd create a boolean like isSplitPane to make the code more readable.

::: mobile/android/base/resources/values/dimens.xml:227
(Diff revision 1)
> +    <item name="split_plane_right_pane_weight" format="float" type="dimen">0.68</item>

I guess I've asked this before: How did you come up with these numbers? My instinct would be to use a fixed size for the left (range) pane and let the right pane use everything that's left.

Comment 50

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Could I see a screenshot after the changes, Vivek? :)

Comment 51

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8647237 [details]
MozReview Request: Bug 1142171: Split history List view for tablet landscape mode r?sebastian.

Bug 1142171: Split history List view for tablet landscape mode r?sebastian.

Comment 52

[Vivek Balakrishnan[:vivek]]

@Antlam : can you please try this apk
https://www.dropbox.com/s/up2ww7m36xdpxod/fennec-43.0a1.en-US.android-arm.apk?dl=0

Comment 53

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Vivek Balakrishnan[:vivek] from comment #52)
> @Antlam : can you please try this apk
> https://www.dropbox.com/s/up2ww7m36xdpxod/fennec-43.0a1.en-US.android-arm.
> apk?dl=0

Thanks Vivek, I'll not be in the office next week so I'll have to give this build a try when I return. I'll try to locate a tablet where I'll be as well. Stay tuned!

Comment 54

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Sorry for the delay Vivek,

But, this looks great! Two points of feedback and then we can get it in! :)

1) Apply the same "About page header grey" to the backgrounds of the left column for "Today", "Yesterday", etc... 

2) Keep the same font _weight_ and _size_ (Roboto light, 14sp ) for active and inactive (just like the "Device name" in the Synced tabs panel where only color changes) 

Thanks a lot vivek, appreciate it! this will be a great improvement

Comment 55

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8647236 [details]
MozReview Request: Bug 1142171 - Pre: Refactor HistoryAdapter to a separate class and defined section ranges  r?sebastian.

Bug 1142171 - Pre: Refactor HistoryAdapter to a separate class and defined section ranges  r?sebastian.

Comment 56

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1142171: Split history List view for tablet landscape mode r?sebastian.

Bug 1142171: Split history List view for tablet landscape mode r?sebastian.

Comment 57

[Vivek Balakrishnan[:vivek]]

@Antlam: I have addressed your feedback in the latest. Can you please let me know if any other task is pending.

The apks are at https://www.dropbox.com/s/up2ww7m36xdpxod/fennec-43.0a1.en-US.android-arm.apk?dl=0

Comment 58

[Vivek Balakrishnan[:vivek]]

@Sebastian: I had rebased the patch on top of latest fx-team and I would be grateful if you can give a quick re-review.

Comment 59

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Looks good!

Comment 60

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8656161 [details]
MozReview Request: Bug 1142171: Split history List view for tablet landscape mode r?sebastian.

https://reviewboard.mozilla.org/r/18101/#review16279

Great! Tested it on my tablet too. Works great.

We recently added code to hide "clear history" if a specific restriction is enabled. This code survived the rebase too. Let's ship it. :)

Comment 61

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/78b356dd104c

Comment 62

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/65c98a523cf3

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78b356dd104c
https://hg.mozilla.org/mozilla-central/rev/65c98a523cf3

Comment 64

[Liz Henry (:lizzard) (relman/hg->git project)]

Release Note Request (optional, but appreciated)
[Why is this notable]: user-facing new feature, suggested by dev team
[Suggested wording]: New split pane styling for History panel on tablets in landscape mode
[Links (documentation, blog post, etc)]:

Comment 65

[Sebastian Kaspari (:sebastian; :pocmo)]

All follow-ups we talked about are now filed and should be blocking this bug.