Closed Bug 1142171 (splitpanehistory) Opened 5 years ago Closed 4 years ago

Use "split pane styling" for History Panel on Tablets in landscape

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed
relnote-firefox --- 43+

People

(Reporter: antlam, Assigned: vivek)

References

Details

Attachments

(6 files, 12 obsolete files)

713.32 KB, image/png
Details
28.31 KB, patch
sebastian
: review+
Details | Diff | Splinter Review
6.68 KB, patch
sebastian
: feedback+
Details | Diff | Splinter Review
35.71 KB, patch
sebastian
: review+
antlam
: feedback+
Details | Diff | Splinter Review
40 bytes, text/x-review-board-request
sebastian
: review+
Details
40 bytes, text/x-review-board-request
sebastian
: review+
Details
Related to bug 1129171.

It makes sense to see if we can use this in our other Panels like Bookmarks.
^ And History too?
Summary: Use "split pane styling" for Bookmarks Panel on Tablets in landscape → Use "split pane styling" for Bookmarks + History Panel on Tablets in landscape
(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.
Alias: splitpanehistory
Assignee: nobody → vivekb.balakrishnan
Status: NEW → ASSIGNED
Hardware: x86 → All
Summary: Use "split pane styling" for Bookmarks + History Panel on Tablets in landscape → Use "split pane styling" for History Panel on Tablets in landscape
Attached patch part1.patch (obsolete) — Splinter Review
* 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
Attachment #8601721 - Flags: review?(nalexander)
Attached patch part2.patch (obsolete) — Splinter Review
* Container Panel defined to hold the history list views.
Attachment #8601725 - Flags: review?(nalexander)
Attached patch part3.patch (obsolete) — Splinter Review
* Split list view created which split the cursor based on the date ranges
* Enabled split pane list for Tablets in landscape mode.
Attachment #8601726 - Flags: review?(nalexander)
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"
Flags: needinfo?(vivekb.balakrishnan)
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 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.
Attachment #8601721 - Flags: review?(nalexander) → review+
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"?
Attachment #8601725 - Flags: review?(nalexander) → review+
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.
Attachment #8601726 - Flags: review?(nalexander) → feedback+
Attached patch part1.patch (obsolete) — Splinter Review
Comments, license, review nits addressed.
Attachment #8601721 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8611297 - Flags: review?(nalexander)
Attached patch part2.patch (obsolete) — Splinter Review
Review nits addressed
Attachment #8601725 - Attachment is obsolete: true
Attachment #8611299 - Flags: review?(nalexander)
Attached patch part3.patch (obsolete) — Splinter Review
Styles and split dimensions added to xml files
Attachment #8601726 - Attachment is obsolete: true
Attachment #8611302 - Flags: review?(nalexander)
Attached patch part4.patch (obsolete) — Splinter Review
History sections updated to be similar to that in FF desktop. Section string are localized.
Attachment #8611303 - Flags: review?(nalexander)
(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 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.
Attachment #8611303 - Flags: review+
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.
Attachment #8611303 - Flags: review?(nalexander) → feedback+
Comment on attachment 8611297 [details] [diff] [review]
part1.patch

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

Rubber stamp.  Looks okay to me!
Attachment #8611297 - Flags: review?(nalexander) → review+
Comment on attachment 8611299 [details] [diff] [review]
part2.patch

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

Rubber stamp.  Looks okay to me!
Attachment #8611299 - Flags: review?(nalexander) → review+
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.
Attachment #8611302 - Flags: feedback+
Attached patch part1.patch (obsolete) — Splinter Review
A new patch with common functionsrealted to history parsing pulled to base fragment
Attachment #8611297 - Attachment is obsolete: true
Attachment #8611299 - Attachment is obsolete: true
Attachment #8611302 - Attachment is obsolete: true
Attachment #8611303 - Attachment is obsolete: true
Attachment #8611302 - Flags: review?(nalexander)
Attachment #8616378 - Flags: review?(nalexander)
Attached patch part2.patch (obsolete) — Splinter Review
Added container fragment and placeholder code for split plane fragments
Attachment #8616379 - Flags: review?(nalexander)
Attached patch part3.patch (obsolete) — Splinter Review
Final part of the patch: 
Split plane fragment,
As discussed over irc, each split plane segments issues separate query 
localization strings for the sections.
Attachment #8616380 - Flags: review?(nalexander)
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.
Is this something that might be feasible for 43?
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8616378 - Flags: review?(s.kaspari)
Attachment #8616379 - Flags: review?(s.kaspari)
Attachment #8616380 - Flags: review?(s.kaspari)
Agreed with Sebastian, we are targeting 43
Flags: needinfo?(jchaulk)
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
Flags: needinfo?(vivekb.balakrishnan)
Attached image 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.
Flags: needinfo?(alam)
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.
Attached patch part1.patchSplinter Review
Re-based against the latest central and comment nits corrected
Attachment #8616378 - Attachment is obsolete: true
Attachment #8616378 - Flags: review?(s.kaspari)
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8636612 - Flags: review?(s.kaspari)
Attached patch part2.patchSplinter Review
Re-base and reviewer changed in comment line
Attachment #8616379 - Attachment is obsolete: true
Attachment #8616379 - Flags: review?(s.kaspari)
Attachment #8636613 - Flags: review?(s.kaspari)
Attached patch part3.patch (obsolete) — Splinter Review
Moved the layout to res/layout.
@Sebastian: Can you please try now if it compiles for you.
Attachment #8616380 - Attachment is obsolete: true
Attachment #8616380 - Flags: review?(s.kaspari)
Attachment #8636615 - Flags: review?(s.kaspari)
(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?
Flags: needinfo?(alam) → needinfo?(s.kaspari)
Attached patch part3.patchSplinter Review
Patch updated with Antlam's UI changes

Sample screenshots:
https://www.dropbox.com/s/idbichrgrrbe5dh/history_split.png?dl=0
Attachment #8636615 - Attachment is obsolete: true
Attachment #8636615 - Flags: review?(s.kaspari)
Attachment #8636760 - Flags: review?(s.kaspari)
Attachment #8636760 - Flags: feedback?(alam)
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.
Attachment #8636612 - Flags: review?(s.kaspari) → review+
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.
Attachment #8636613 - Flags: review?(s.kaspari) → feedback+
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
Attachment #8636760 - Flags: review?(s.kaspari) → review+
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)
Flags: needinfo?(s.kaspari)
Oh, and one small thing: Please use r=sebastian instead of r=s.kaspari :)
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?
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8636760 - Flags: feedback?(alam) → feedback+
Bug 1142171 - Pre: Refactor HistoryAdapter to a separate class and defined section ranges  r?sebastian.
Attachment #8647236 - Flags: review?(s.kaspari)
Bug 1142171: Split history List view for tablet landscape mode r?sebastian.
Attachment #8647237 - Flags: review?(s.kaspari)
screenshot for updated layout:
https://www.dropbox.com/s/9mcog74bpzmcgen/newlayout.png?dl=0

@antlam:
I have used margin 16dp. should center the text?
Flags: needinfo?(vivekb.balakrishnan) → needinfo?(alam)
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!
Flags: needinfo?(alam) → needinfo?(vivekb.balakrishnan)
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
Flags: needinfo?(vivekb.balakrishnan)
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 :)
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8647236 - Flags: review?(s.kaspari) → review+
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 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.
Attachment #8647237 - Flags: review?(s.kaspari) → review+
Could I see a screenshot after the changes, 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.
@Antlam : can you please try this apk
https://www.dropbox.com/s/up2ww7m36xdpxod/fennec-43.0a1.en-US.android-arm.apk?dl=0
Flags: needinfo?(vivekb.balakrishnan)
(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!
Flags: needinfo?(alam)
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
Flags: needinfo?(alam) → needinfo?(vivekb.balakrishnan)
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.
Attachment #8647237 - Attachment is obsolete: true
Bug 1142171: Split history List view for tablet landscape mode r?sebastian.
Attachment #8656161 - Flags: review?(s.kaspari)
@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
Flags: needinfo?(vivekb.balakrishnan) → needinfo?(alam)
@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.
Flags: needinfo?(s.kaspari)
Looks good!
Flags: needinfo?(alam)
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. :)
Attachment #8656161 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/mozilla-central/rev/78b356dd104c
https://hg.mozilla.org/mozilla-central/rev/65c98a523cf3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
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)]:
All follow-ups we talked about are now filed and should be blocking this bug.
Flags: needinfo?(s.kaspari)
Depends on: 1208519
Depends on: 1209898
Flags: needinfo?(jchaulk)
You need to log in before you can comment on or make changes to this bug.