This is the bug to track "enhanced" work on the new Reading List panel on about:home. We are looking at ways to display the Reading list in a more useful and editorial way, instead of the current title/URL implementation. The new list should display * Displaying a more editorial list of articles (using twitter cards, for example) ** Title ** Intro ** Picture, if available * Length of article, in minutes

I wonder if there are any quick wins that could get us this kind of layout, using the article content we store for offline reading? So instead of using something like twitter cards, just pulling in the title and the first paragraph or so of content and displaying it here? This would be a really nice enhancement for the reading list to include when we launch the new awesomescreen, if at all possible.

Bug 782406 is about storing descriptions for reading list items. The description could be the first paragraph of the article, for example.

Just pinging to keep this on our radar. I realize this probably won't make the cut for our initial relaunch of the Awesomescreen, but want to keep it prioritized for work in the very near future. Maybe we can hack on this during the work week?

(In reply to Ian Barlow (:ibarlow) from comment #3) > Just pinging to keep this on our radar. I realize this probably won't make > the cut for our initial relaunch of the Awesomescreen, but want to keep it > prioritized for work in the very near future. > > Maybe we can hack on this during the work week? Yeah, this shouldn't be too hard to implement.

patch for UI component. want to get some feedback

Comment on attachment 8360229 [details] [diff] [review] premilinary UI patch ># HG changeset patch ># User Sola Ogunsakin <oogunsakin@mozilla.com> ># Date 1389770349 28800 ># Tue Jan 14 23:19:09 2014 -0800 ># Node ID ab2aaae568d447170ce4551b8a50d46cc746629c ># Parent 88c7efc9d514409089595cecf0c0f53f1d2cd1c0 >Show excepts in about:home's Reading List panel. r=liuche > >diff --git a/mobile/android/base/home/ReadingListPage.java b/mobile/android/base/home/ReadingListPage.java >--- a/mobile/android/base/home/ReadingListPage.java >+++ b/mobile/android/base/home/ReadingListPage.java >@@ -5,17 +5,17 @@ > > package org.mozilla.gecko.home; > > import org.mozilla.gecko.R; > import org.mozilla.gecko.db.BrowserContract.Bookmarks; > import org.mozilla.gecko.db.BrowserDB; > import org.mozilla.gecko.db.BrowserDB.URLColumns; > import org.mozilla.gecko.home.HomePager.OnUrlOpenListener; >-import org.mozilla.gecko.home.TwoLinePageRow; >+import org.mozilla.gecko.home.ReadingListRow; > import org.mozilla.gecko.ReaderModeUtils; > > import android.app.Activity; > import android.content.Context; > import android.content.res.Configuration; > import android.database.Cursor; > import android.os.Bundle; > import android.support.v4.app.LoaderManager.LoaderCallbacks; >@@ -150,36 +150,16 @@ public class ReadingListPage extends Hom > } > > private void updateUiFromCursor(Cursor c) { > // We delay setting the empty view until the cursor is actually empty. > // This avoids image flashing. > if ((c == null || c.getCount() == 0) && mEmptyView == null) { > final ViewStub emptyViewStub = (ViewStub) mTopView.findViewById(R.id.home_empty_view_stub); > mEmptyView = emptyViewStub.inflate(); >- >- final TextView emptyHint = (TextView) mEmptyView.findViewById(R.id.home_empty_hint); >- String readingListHint = emptyHint.getText().toString(); >- >- // Use an ImageSpan to include the reader icon in the "Tip". >- int imageSpanIndex = readingListHint.indexOf("%I"); >- if (imageSpanIndex != -1) { >- final ImageSpan readingListIcon = new ImageSpan(getActivity(), R.drawable.reader_cropped, ImageSpan.ALIGN_BOTTOM); >- final SpannableStringBuilder hintBuilder = new SpannableStringBuilder(readingListHint); >- >- // Add additional spacing. >- hintBuilder.insert(imageSpanIndex + 2, " "); >- hintBuilder.insert(imageSpanIndex, " "); >- >- // Add icon. >- hintBuilder.setSpan(readingListIcon, imageSpanIndex + 1, imageSpanIndex + 3, Spanned.SPAN_INCLUSIVE_INCLUSIVE); >- >- emptyHint.setText(hintBuilder, TextView.BufferType.SPANNABLE); >- } >- > mList.setEmptyView(mEmptyView); > } > } > > /** > * Cursor loader for the list of reading list items. > */ > private static class ReadingListLoader extends SimpleCursorLoader { >@@ -198,23 +178,23 @@ public class ReadingListPage extends Hom > */ > private class ReadingListAdapter extends CursorAdapter { > public ReadingListAdapter(Context context, Cursor cursor) { > super(context, cursor, 0); > } > > @Override > public void bindView(View view, Context context, Cursor cursor) { >- final TwoLinePageRow row = (TwoLinePageRow) view; >+ final ReadingListRow row = (ReadingListRow) view; > row.updateFromCursor(cursor); > } > > @Override > public View newView(Context context, Cursor cursor, ViewGroup parent) { >- return LayoutInflater.from(parent.getContext()).inflate(R.layout.bookmark_item_row, parent, false); >+ return LayoutInflater.from(parent.getContext()).inflate(R.layout.reading_list_item_row, parent, false); > } > } > > /** > * LoaderCallbacks implementation that interacts with the LoaderManager. > */ > private class CursorLoaderCallbacks implements LoaderCallbacks<Cursor> { > @Override >diff --git a/mobile/android/base/home/ReadingListRow.java b/mobile/android/base/home/ReadingListRow.java >new file mode 100644 >--- /dev/null >+++ b/mobile/android/base/home/ReadingListRow.java >@@ -0,0 +1,107 @@ >+/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- >+ * This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+package org.mozilla.gecko.home; >+ >+import android.util.Log; >+import org.mozilla.gecko.favicons.Favicons; >+import org.mozilla.gecko.R; >+import org.mozilla.gecko.Tab; >+import org.mozilla.gecko.Tabs; >+import org.mozilla.gecko.db.BrowserContract.Combined; >+import org.mozilla.gecko.db.BrowserDB.URLColumns; >+import org.mozilla.gecko.favicons.OnFaviconLoadedListener; >+import org.mozilla.gecko.util.ThreadUtils; >+import org.mozilla.gecko.widget.FaviconView; >+ >+import android.content.Context; >+import android.database.Cursor; >+import android.graphics.Bitmap; >+import android.text.TextUtils; >+import android.util.AttributeSet; >+import android.view.Gravity; >+import android.view.LayoutInflater; >+import android.widget.LinearLayout; >+import android.widget.TextView; >+ >+import java.lang.ref.WeakReference; >+ >+public class ReadingListRow extends LinearLayout >+ implements Tabs.OnTabsChangedListener { >+ private static final int NO_ICON = 0; >+ >+ private final TextView mTitle; >+ private final TextView mPreview; >+ private final TextView mReadTime; >+ >+ public ReadingListRow(Context context) { >+ this(context, null); >+ } >+ >+ public ReadingListRow(Context context, AttributeSet attrs) { >+ super(context, attrs); >+ >+ setGravity(Gravity.CENTER_VERTICAL); >+ >+ LayoutInflater.from(context).inflate(R.layout.reading_list_row, this); >+ mTitle = (TextView) findViewById(R.id.title); >+ mPreview = (TextView) findViewById(R.id.preview); >+ mReadTime = (TextView) findViewById(R.id.read_time); >+ } >+ >+ @Override >+ protected void onAttachedToWindow() { >+ Tabs.registerOnTabsChangedListener(this); >+ } >+ >+ @Override >+ protected void onDetachedFromWindow() { >+ // Delay removing the listener to avoid modifying mTabsChangedListeners >+ // while notifyListeners is iterating through the array. >+ ThreadUtils.postToUiThread(new Runnable() { >+ @Override >+ public void run() { >+ // Tabs.unregisterOnTabsChangedListener(ReadingListRow.this); >+ } >+ }); >+ } >+ >+ @Override >+ public void onTabChanged(final Tab tab, final Tabs.TabEvents msg, final Object data) { >+ switch(msg) { >+ case ADDED: >+ case CLOSED: >+ case LOCATION_CHANGE: >+ // updateDisplayedUrl(); >+ break; >+ } >+ } >+ >+ private void setTitle(String title) { >+ mTitle.setText(title); >+ } >+ >+ private void setPreview(String preview) { >+ mPreview.setText(preview); >+ } >+ >+ private void setReadTime(int time) { >+ mReadTime.setText("12mins"); >+ } >+ >+ public void updateFromCursor(Cursor cursor) { >+ if (cursor == null) { >+ return; >+ } >+ >+ int titleIndex = cursor.getColumnIndexOrThrow(URLColumns.TITLE); >+ final String title = cursor.getString(titleIndex); >+ setTitle(title); >+ >+ // no data for these fields yet, put fakes >+ setReadTime(12); >+ setPreview("Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."); >+ } >+} >diff --git a/mobile/android/base/moz.build b/mobile/android/base/moz.build >--- a/mobile/android/base/moz.build >+++ b/mobile/android/base/moz.build >@@ -224,16 +224,17 @@ gbjar.sources += [ > 'home/HomePagerTabStrip.java', > 'home/LastTabsPage.java', > 'home/ListManager.java', > 'home/ListPage.java', > 'home/MostRecentPage.java', > 'home/MultiTypeCursorAdapter.java', > 'home/PinSiteDialog.java', > 'home/ReadingListPage.java', >+ 'home/ReadingListRow.java', > 'home/SearchEngine.java', > 'home/SearchEngineRow.java', > 'home/SearchLoader.java', > 'home/SimpleCursorLoader.java', > 'home/SuggestClient.java', > 'home/TabMenuStrip.java', > 'home/TopSitesGridItemView.java', > 'home/TopSitesGridView.java', >diff --git a/mobile/android/base/resources/layout/home_empty_reading_page.xml b/mobile/android/base/resources/layout/home_empty_reading_page.xml >--- a/mobile/android/base/resources/layout/home_empty_reading_page.xml >+++ b/mobile/android/base/resources/layout/home_empty_reading_page.xml >@@ -7,45 +7,27 @@ > android:layout_width="fill_parent" > android:layout_height="fill_parent" > android:orientation="vertical" > android:gravity="center"> > > <!-- Empty spacer view --> > <View android:layout_width="fill_parent" > android:layout_height="0dip" >- android:layout_weight="1"/> >+ android:layout_weight="3"/> > >- <ImageView android:layout_width="fill_parent" >- android:layout_height="wrap_content" >- android:gravity="top|center" >- android:scaleType="center" >- android:src="@drawable/icon_reading_list_empty" >- android:paddingBottom="10dp"/> >- >- <TextView android:layout_width="fill_parent" >+ <TextView android:layout_width="match_parent" > android:layout_height="0dip" > android:gravity="top|center" > android:text="@string/home_reading_list_empty" >- android:textAppearance="@style/TextAppearance.EmptyMessage" >- android:paddingLeft="50dp" >- android:paddingRight="50dp" >- android:layout_weight="3"/> >+ android:textColor="#777777" >+ android:fontFamily="sans-serif-light" >+ android:textSize="18sp" >+ android:layout_weight="1"/> > >- <ImageView android:src="@drawable/divider_horizontal" >- android:layout_width="fill_parent" >- android:layout_height="1dip" >- android:paddingLeft="25dp" >- android:paddingRight="25dp"/> >+ <!-- Empty spacer view --> >+ <View android:layout_width="fill_parent" >+ android:layout_height="0dip" >+ android:layout_weight="4"/> > >- <TextView android:id="@+id/home_empty_hint" >- android:layout_width="fill_parent" >- android:layout_height="wrap_content" >- android:gravity="bottom" >- android:text="@string/home_reading_list_hint" >- android:textAppearance="@style/TextAppearance.Small" >- android:contentDescription="@string/home_reading_list_hint_accessible" >- android:paddingTop="20dp" >- android:paddingBottom="15dp" >- android:paddingLeft="25dp" >- android:paddingRight="40dp"/> >+ > > </LinearLayout> >diff --git a/mobile/android/base/resources/layout/reading_list_item_row.xml b/mobile/android/base/resources/layout/reading_list_item_row.xml >new file mode 100644 >--- /dev/null >+++ b/mobile/android/base/resources/layout/reading_list_item_row.xml >@@ -0,0 +1,9 @@ >+<?xml version="1.0" encoding="utf-8"?> >+<!-- This Source Code Form is subject to the terms of the Mozilla Public >+ - License, v. 2.0. If a copy of the MPL was not distributed with this >+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> >+ >+<org.mozilla.gecko.home.ReadingListRow xmlns:android="http://schemas.android.com/apk/res/android" >+ android:layout_width="fill_parent" >+ android:layout_height="wrap_content" >+ android:minHeight="@dimen/page_row_height"/> >diff --git a/mobile/android/base/resources/layout/reading_list_row.xml b/mobile/android/base/resources/layout/reading_list_row.xml >new file mode 100644 >--- /dev/null >+++ b/mobile/android/base/resources/layout/reading_list_row.xml >@@ -0,0 +1,62 @@ >+<?xml version="1.0" encoding="utf-8"?> >+<!-- This Source Code Form is subject to the terms of the Mozilla Public >+ - License, v. 2.0. If a copy of the MPL was not distributed with this >+ - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> >+ >+<merge xmlns:android="http://schemas.android.com/apk/res/android" >+ xmlns:gecko="http://schemas.android.com/apk/res-auto"> >+ >+ <RelativeLayout android:layout_width="fill_parent" >+ android:layout_height="wrap_content" >+ android:layout_marginRight="10dp" >+ android:layout_marginLeft="10dp" >+ android:layout_marginTop="14dp" >+ android:layout_marginBottom="14dp"> >+ >+ <LinearLayout android:layout_width="wrap_content" >+ android:layout_height="fill_parent" >+ android:layout_marginRight="17dip" >+ android:orientation="vertical" >+ android:layout_alignParentLeft="true" >+ android:layout_toLeftOf="@+id/read_time"> >+ >+ <TextView >+ android:id="@+id/title" >+ android:layout_width="fill_parent" >+ android:layout_height="wrap_content" >+ android:maxLines="2" >+ android:ellipsize="end" >+ android:singleLine="false" >+ android:text="asdasd" >+ android:textColor="#222222" >+ android:textSize="18sp" >+ android:fontFamily="sans-serif-light" /> >+ >+ <TextView android:id="@+id/preview" >+ android:layout_width="fill_parent" >+ android:layout_height="0dp" >+ android:layout_weight="1" >+ android:drawablePadding="5dp" >+ android:singleLine="false" >+ android:maxLines="4" >+ android:textColor="#777777" >+ android:fontFamily="sans-serif" >+ android:ellipsize="end" >+ android:textSize="12sp"/> >+ </LinearLayout> >+ >+ <TextView >+ android:id="@+id/read_time" >+ android:layout_width="wrap_content" >+ android:layout_height="fill_parent" >+ android:layout_alignParentRight="true" >+ android:paddingTop="4dp" >+ android:fontFamily="sans-serif-condensed" >+ android:drawablePadding="5dp" >+ android:text="12mins" >+ android:textColor="#FF9400" >+ android:textSize="14sp" /> >+ >+ </RelativeLayout> >+ >+</merge>

description text clips when I set the row item height to 112dp. can't get the text to fill the space without clipping. It works ok when i make the row item height flexible and allow the text expand to a maximum of 4 lines.

row item without fixed height. maximum number of lines for description text set to 4.

Comment on attachment 8360229 [details] [diff] [review] premilinary UI patch Cool, just wanted to clean up the flags on this bug a bit. Will take a look tomorrow!

(In reply to Sola Ogunsakin [:sola] from comment #8) > Created attachment 8360236 [details] > Row-Without-Clipping.png > > row item without fixed height. maximum number of lines for description text > set to 4. Hey cool I didn't know we could do flexible row heights. This would definitely be preferable. :) Couple comments: * Let's remove the ellipsis at the end of the text. It's already clear that this isn't the entire text of the article, no need to call it out explicitly by adding an ellipsis. * Let's try Roboto Condensed *italic* for the article length. BTW - is this part actually functional now? Or are you just setting up the styling for it. Just curious. Looking really great overall!

Comment on attachment 8360229 [details] [diff] [review] premilinary UI patch Review of attachment 8360229 [details] [diff] [review]: ----------------------------------------------------------------- Looks nice. It seems this patch contains unrelated changes mixed in i.e. the empty view changes. Please split them into separate patches. You might want to consider using styles in case the layouts for phones and tablets end up being different. Double-check with ibarlow. ::: mobile/android/base/home/ReadingListPage.java @@ -156,5 @@ > final ViewStub emptyViewStub = (ViewStub) mTopView.findViewById(R.id.home_empty_view_stub); > mEmptyView = emptyViewStub.inflate(); > - > - final TextView emptyHint = (TextView) mEmptyView.findViewById(R.id.home_empty_hint); > - String readingListHint = emptyHint.getText().toString(); Why is this code being removed? ::: mobile/android/base/home/ReadingListRow.java @@ +29,5 @@ > +import java.lang.ref.WeakReference; > + > +public class ReadingListRow extends LinearLayout > + implements Tabs.OnTabsChangedListener { > + private static final int NO_ICON = 0; unused? @@ +32,5 @@ > + implements Tabs.OnTabsChangedListener { > + private static final int NO_ICON = 0; > + > + private final TextView mTitle; > + private final TextView mPreview; mExcerpt? @@ +59,5 @@ > + @Override > + protected void onDetachedFromWindow() { > + // Delay removing the listener to avoid modifying mTabsChangedListeners > + // while notifyListeners is iterating through the array. > + ThreadUtils.postToUiThread(new Runnable() { I realize you've copied this code from TwoLinePageRow but I find it really suspicious that we're having to explicitly post the unregisterOnTabsChangedListener() call in the UI thread given that View API is always called from the UI thread anyway. @@ +73,5 @@ > + switch(msg) { > + case ADDED: > + case CLOSED: > + case LOCATION_CHANGE: > + // updateDisplayedUrl(); Remove updateDisplayedUrl() entirely? Is onTabChanged necessary at all? ::: mobile/android/base/resources/layout/home_empty_reading_page.xml @@ +25,5 @@ > > + <!-- Empty spacer view --> > + <View android:layout_width="fill_parent" > + android:layout_height="0dip" > + android:layout_weight="4"/> How's this layout change related to the main intent of this patch? ::: mobile/android/base/resources/layout/reading_list_row.xml @@ +10,5 @@ > + android:layout_height="wrap_content" > + android:layout_marginRight="10dp" > + android:layout_marginLeft="10dp" > + android:layout_marginTop="14dp" > + android:layout_marginBottom="14dp"> Just make ReadingListRow inherit from RelativeLayout instead of LinearLayout and set margins directly in reading_list_item_row.xml. This way you can avoid this unnecessary RelativeLayout in reading_list_row.xml

Comment on attachment 8360229 [details] [diff] [review] premilinary UI patch Review of attachment 8360229 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a good start! ::: mobile/android/base/home/ReadingListPage.java @@ -156,5 @@ > final ViewStub emptyViewStub = (ViewStub) mTopView.findViewById(R.id.home_empty_view_stub); > mEmptyView = emptyViewStub.inflate(); > - > - final TextView emptyHint = (TextView) mEmptyView.findViewById(R.id.home_empty_hint); > - String readingListHint = emptyHint.getText().toString(); Agreed - the Reading list empty view displays a visual tip when there are no reading list items, so even if you don't see it, this should stay in. ::: mobile/android/base/home/ReadingListRow.java @@ +42,5 @@ > + > + public ReadingListRow(Context context, AttributeSet attrs) { > + super(context, attrs); > + > + setGravity(Gravity.CENTER_VERTICAL); Is something that can be moved to the xml layout file? ::: mobile/android/base/resources/layout/home_empty_reading_page.xml @@ +18,5 @@ > android:layout_height="0dip" > android:gravity="top|center" > android:text="@string/home_reading_list_empty" > + android:textColor="#777777" > + android:fontFamily="sans-serif-light" Just a note to update these with respect to ibarlow's comments. ::: mobile/android/base/resources/layout/reading_list_row.xml @@ +10,5 @@ > + android:layout_height="wrap_content" > + android:layout_marginRight="10dp" > + android:layout_marginLeft="10dp" > + android:layout_marginTop="14dp" > + android:layout_marginBottom="14dp"> Agreed - in general, we also avoid RelativeLayout unless absolutely necessary because they have a much higher performance hit for rendering (because there are more layout and positioning calculations). You could use LinearLayouts here with different android:orientation and weights. @@ +13,5 @@ > + android:layout_marginTop="14dp" > + android:layout_marginBottom="14dp"> > + > + <LinearLayout android:layout_width="wrap_content" > + android:layout_height="fill_parent" Our minimum SDK version we support is 8, and fill_parent has been renamed to match_parent. http://developer.android.com/reference/android/view/ViewGroup.LayoutParams.html @@ +31,5 @@ > + android:textColor="#222222" > + android:textSize="18sp" > + android:fontFamily="sans-serif-light" /> > + > + <TextView android:id="@+id/preview" If you rename "preview" to "excerpt," make sure to update that here too.

Comment on attachment 8360229 [details] [diff] [review] premilinary UI patch Review of attachment 8360229 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/ReadingListPage.java @@ -156,5 @@ > final ViewStub emptyViewStub = (ViewStub) mTopView.findViewById(R.id.home_empty_view_stub); > mEmptyView = emptyViewStub.inflate(); > - > - final TextView emptyHint = (TextView) mEmptyView.findViewById(R.id.home_empty_hint); > - String readingListHint = emptyHint.getText().toString(); the mock for an empty reading list didn't contain the hint. my understanding was that it should be removed.

Updates Lucas: -removed unused code from TwoLinePageRow -removed unnecessary RelativeLayout -Moved styles into style.xml Chenxia: -change "preview" to "excerpt" -change "fill_parent" to "match_parent" -Change relative layout to linear

-removed ellipsis on excerpt text -italicize readTime text -The readTime part isn't functional yet, its is being implemented in bug#959297

(In reply to Sola Ogunsakin [:sola] from comment #16) > Created attachment 8360619 [details] > Row-Screenshot.png > > -removed ellipsis on excerpt text > -italicize readTime text > -The readTime part isn't functional yet, its is being implemented in > bug#959297 Looks hot! Can't wait to try it. One last thing -- it looks like we lost some parts of the empty page that were implemented in bug 891953. There should be an icon there, as well as a little tip that goes at the bottom. Mockup from earlier: https://bug891953.bugzilla.mozilla.org/attachment.cgi?id=779766

-restore hint and icon to empty reading list page

empty reading list page

Comment on attachment 8360792 [details] [diff] [review] bug-889351-fix.patch Review of attachment 8360792 [details] [diff] [review]: ----------------------------------------------------------------- Looking pretty good. Just need some more cleanups and tweaks. ::: mobile/android/base/home/ReadingListPage.java @@ +154,5 @@ > // This avoids image flashing. > if ((c == null || c.getCount() == 0) && mEmptyView == null) { > final ViewStub emptyViewStub = (ViewStub) mTopView.findViewById(R.id.home_empty_view_stub); > mEmptyView = emptyViewStub.inflate(); > + mList.setEmptyView(mEmptyView); Don't we already call setEmptyView() down below in updateUiFromCursor? @@ +189,5 @@ > } > > @Override > public Cursor loadCursor() { > + return BrowserDB.getBookmarksInFolder(getContext().getContentResolver(), Bookmarks.FIXED_READING_LIST_ID); Why is this change necessary? Doesn't getReadingList() do exactly that (see LocalBrowserDB.getReadingList)? ::: mobile/android/base/home/ReadingListRow.java @@ +33,5 @@ > + LayoutInflater.from(context).inflate(R.layout.reading_list_row, this); > + > + mTitle = (TextView) findViewById(R.id.title); > + mExcerpt = (TextView) findViewById(R.id.excerpt); > + mReadTime = (TextView) findViewById(R.id.read_time); nit: we don't usually indent multiple assignments this way. It would be nice to keep the style as close as possible with the rest of our code. @@ +37,5 @@ > + mReadTime = (TextView) findViewById(R.id.read_time); > + } > + > + @Override > + protected void onAttachedToWindow() {} Unused, remove it. @@ +41,5 @@ > + protected void onAttachedToWindow() {} > + > + @Override > + protected void onDetachedFromWindow() { > + super.onDetachedFromWindow(); Same, remove it. ::: mobile/android/base/resources/layout/home_empty_reading_page.xml @@ +30,5 @@ > android:paddingRight="50dp" > + android:layout_weight="3" > + android:textColor="#777777" > + android:fontFamily="sans-serif-light" > + android:textSize="18sp" /> Why is this change needed in this patch? Empty views are unrelated to this bug, no? ::: mobile/android/base/resources/layout/reading_list_row.xml @@ +19,5 @@ > + style="@style/Widget.ReadingListRow.Excerpt" /> > + </LinearLayout> > + > + <TextView android:id="@+id/read_time" > + style="@style/Widget.ReadingListRow.ReadTime" /> I wonder if the excerpt view should match the width of the row? With this layout, we'd be wasting some space below the read_time view. Check with ibarlow. ::: mobile/android/base/resources/values/colors.xml @@ +89,5 @@ > <color name="url_bar_shadow">#12000000</color> > > <color name="home_last_tab_bar_bg">#FFF5F7F9</color> > > + <color name="readinglist_read_time_colour">#FF9400</color> I assuming this tone of orange is not already used somewhere else in the UI? Double-check and consolidate duplicate colors if necessary. ::: mobile/android/base/resources/values/dimens.xml @@ +85,5 @@ > <dimen name="toast_button_padding">8dp</dimen> > <dimen name="history_tab_indicator_height">50dp</dimen> > > + <!-- Reading List--> > + <dimen name="readinglist_row_title_text_size">18sp</dimen> No need to have separate dimens for text sizes. This should probably into TextAppearance style in styles.xml. See, as a reference, the styles around Widget.TwoLinePageRow.*. ReadingListRow should probably follow the same patterns. ::: mobile/android/base/resources/values/styles.xml @@ +147,5 @@ > + <item name="android:layout_width">match_parent</item> > + <item name="android:layout_height">0dp</item> > + <item name="android:layout_weight">1</item> > + <item name="android:textColor">@color/text_color_secondary</item> > + <item name="android:textSize">@dimen/readinglist_row_excerpt_text_size</item> All text-related style properties should go into separate TextAppearance styles. Take TwoLinePageRow as a reference. ReadingListRow should be analogous.

Comment on attachment 8360792 [details] [diff] [review] bug-889351-fix.patch Review of attachment 8360792 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/ReadingListPage.java @@ +154,5 @@ > // This avoids image flashing. > if ((c == null || c.getCount() == 0) && mEmptyView == null) { > final ViewStub emptyViewStub = (ViewStub) mTopView.findViewById(R.id.home_empty_view_stub); > mEmptyView = emptyViewStub.inflate(); > + mList.setEmptyView(mEmptyView); oops i'll get rid of it. @@ +189,5 @@ > } > > @Override > public Cursor loadCursor() { > + return BrowserDB.getBookmarksInFolder(getContext().getContentResolver(), Bookmarks.FIXED_READING_LIST_ID); LocalBrowserDB.getReadingList() is a change from another patch (for bug#959290) that made it into this by mistake. When that patch is landed i'll change it back. ::: mobile/android/base/resources/layout/home_empty_reading_page.xml @@ +30,5 @@ > android:paddingRight="50dp" > + android:layout_weight="3" > + android:textColor="#777777" > + android:fontFamily="sans-serif-light" > + android:textSize="18sp" /> spoke to ian. i misunderstood the mock. will revert ::: mobile/android/base/resources/layout/reading_list_row.xml @@ +19,5 @@ > + style="@style/Widget.ReadingListRow.Excerpt" /> > + </LinearLayout> > + > + <TextView android:id="@+id/read_time" > + style="@style/Widget.ReadingListRow.ReadTime" /> spoke to ian. he'd like to leave it like this for the time being :) ::: mobile/android/base/resources/values/colors.xml @@ +89,5 @@ > <color name="url_bar_shadow">#12000000</color> > > <color name="home_last_tab_bar_bg">#FFF5F7F9</color> > > + <color name="readinglist_read_time_colour">#FF9400</color> just searched. don't believe its being used any where else.

-Moved text related attributes to a TextAppearance a style -remove unused code from ReadingListRow -fixed indentation

I spoke with ibarlow on IRC and he mentioned he would like this to be a view available to HomePanel addon developers - to start, in the RSS addon I've been developing (bug 942283). Would it be possible to build this a bit more generically so we can attach this to HomePanels in a followup bug? Upon skimming, it seems like it would just need to be a few renames. ibarlow also mentioned this addon view shouldn't necessarily have the time counter, so it would need the ability to hide that view (perhaps have the "ReadingListRow" extend this other more generic row?). What do you think, Sola?

sounds good to me. i'll abstract out the view components (minus the time counter).

You may want to connect with Margaret or Lucas on how they're designing the generic layouts so that you have the appropriate hooks to the JS addon APIs (or at least have designed it to make it an easy followup).

Comment on attachment 8392603 [details] [diff] [review] bug-889351.patch Review of attachment 8392603 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, just needs some more cleanups. ::: mobile/android/base/home/ReadingListPanel.java @@ +220,5 @@ > } > > @Override > public View newView(Context context, Cursor cursor, ViewGroup parent) { > + return LayoutInflater.from(parent.getContext()).inflate(R.layout.reading_list_item_row, parent, false); This patch overlaps a bit with bug 920930. Please make sure you communicate that to the contributor working on bug 920930. ::: mobile/android/base/home/ReadingListRow.java @@ +22,5 @@ > + private final TextView mExcerpt; > + private final TextView mReadTime; > + > + private final int MINUTES_ID = R.string.reading_list_readtime_mintes; > + private final int OVER_AN_HOUR_ID = R.string.reading_list_readtime_over_an_hour; nit: a constant of a constant? :-) Just R.string.* directly. @@ +39,5 @@ > + super(context, attrs); > + > + mContext = context; > + > + LayoutInflater.from(context).inflate(R.layout.reading_list_row, this); Rename this layout to reading_list_row_view for clarity. @@ +65,5 @@ > + } > + > + final int titleIndex = cursor.getColumnIndexOrThrow(ReadingListItems.TITLE); > + final String title = cursor.getString(titleIndex); > + setTitle(title); Just do setTitle(cursor.getString(...)); @@ +77,5 @@ > + } > + > + final int excerptIndex = cursor.getColumnIndexOrThrow(ReadingListItems.EXCERPT); > + final String excerpt = cursor.getString(excerptIndex); > + setExcerpt(excerpt); Just do setExcerpt(cursor.getString(...)) @@ +92,5 @@ > + private int getEstimateReadTime(int length) { > + int readTime = (int) Math.ceil((length / AVERAGE_WORD_LENGTH) / AVERAGE_READING_SPEED); > + // Minimum of one minute. > + readTime = Math.max(readTime, 1); > + return readTime; nit: Make it a bit simpler: int readTime = (int) Math.ceil(... // Minimum of one minute return Math.max(readTime, 1); ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +294,5 @@ > <!ENTITY reading_list_added "Page added to your Reading List"> > <!ENTITY reading_list_removed "Page removed from your Reading List"> > <!ENTITY reading_list_failed "Failed to add page to your Reading List"> > <!ENTITY reading_list_duplicate "Page already in your Reading List"> > +<!ENTITY reading_list_readtime_mintes "&formatD;min"> What does this 'mintes' mean? Please add a l10n comment to give translators some more context about how this string is used in the UI. ::: mobile/android/base/resources/layout/reading_list_item_row.xml @@ +1,1 @@ > +<?xml version="1.0" encoding="utf-8"?> This one should be named reading_list_row.xml ::: mobile/android/base/resources/layout/reading_list_row.xml @@ +1,1 @@ > +<?xml version="1.0" encoding="utf-8"?> And this one should be named reading_list_row_view.xml ::: mobile/android/base/resources/values/colors.xml @@ +89,5 @@ > <color name="url_bar_shadow">#12000000</color> > > <color name="home_last_tab_bar_bg">#FFF5F7F9</color> > > + <color name="readinglist_read_time_colour">#FF9400</color> Make sure this color is not already defined somewhere else. ::: mobile/android/base/resources/values/styles.xml @@ +130,5 @@ > + <style name="Widget.DetailedListRow" /> > + > + <style name="Widget.DetailedListRow.Title"> > + <item name="android:layout_width">match_parent</item> > + <item name="android:layout_height">wrap_content</item> Define layout parameters (width and height) directly in the layout file. Defining them in the style adds a level of indirection that makes the layout file hard to follow. @@ +134,5 @@ > + <item name="android:layout_height">wrap_content</item> > + <item name="android:maxLines">2</item> > + <item name="android:ellipsize">end</item> > + <item name="android:singleLine">false</item> > + <item name="android:textAppearance">@style/TextAppearance.Widget.DetailedListRow.Title</item> You can probably reuse TextAppearance.Widget.Home.ItemTitle here. Remove redundant style parameters accordingly. @@ +139,5 @@ > + </style> > + > + <style name="Widget.DetailedListRow.Description"> > + <item name="android:layout_width">match_parent</item> > + <item name="android:layout_height">0dp</item> Ditto. @@ +142,5 @@ > + <item name="android:layout_width">match_parent</item> > + <item name="android:layout_height">0dp</item> > + <item name="android:maxLines">4</item> > + <item name="android:singleLine">false</item> > + <item name="android:drawablePadding">5dp</item> We we use any drawable in the description's TextView? @@ +144,5 @@ > + <item name="android:maxLines">4</item> > + <item name="android:singleLine">false</item> > + <item name="android:drawablePadding">5dp</item> > + <item name="android:layout_weight">1</item> > + <item name="android:textAppearance">@style/TextAppearance.Widget.DetailedListRow.Description</item> Ditto for TextAppearance.Widget.Home.ItemDescription. @@ +384,5 @@ > </style> > > + <!-- Reading List Row Item --> > + > + <style name="TextAppearance.Widget.DetailedListRow" /> No need to define those extra style, just use TextAppearance.Widget.Home.ItemTitle/ItemDescription

Comment on attachment 8396668 [details] [diff] [review] bug-889351.patch Review of attachment 8396668 [details] [diff] [review]: ----------------------------------------------------------------- Looking pretty good, just needs some cleanups in style definitions. ::: mobile/android/base/resources/values/styles.xml @@ +126,5 @@ > </style> > > + <!-- Detailed Row Item --> > + > + <style name="Widget.DetailedListRow" /> I don't follow. Why do you need this parent style that is only used by ReadingListRow? Get rid of these DetailedListRow styles and define ReadingListRow based on TextAppearance.Widget.Home.Item* text appearance. It should look more or less like: <style name="Widget.ReadingListRow" /> <style name="Widget.ReadingListRow.Title"> <item name="android:textAppearance">@style/TextAppearance.Widget.Home.ItemTitle</item> <item name="android:maxLines">2</item> <item name="android:ellipsize">end</item> </style> <style name="Widget.ReadingListRow.Description"> <item name="android:textAppearance">@style/TextAppearance.Widget.Home.ItemDescription</item> <item name="android:maxLines">4</item> <item name="android:ellipsize">end</item> </style> Note how I didn't define fontFamily and textColor as they are defined by the text appearance already. Also, move layout_weight to the layout file. @@ +153,5 @@ > + <style name="Widget.ReadingListRow.Title" parent="Widget.DetailedListRow.Title" /> > + <style name="Widget.ReadingListRow.Excerpt" parent="Widget.DetailedListRow.Description" /> > + > + <style name="Widget.ReadingListRow.ReadTime"> > + <item name="android:layout_weight">0</item> Move layout_weight to the layout file. @@ +155,5 @@ > + > + <style name="Widget.ReadingListRow.ReadTime"> > + <item name="android:layout_weight">0</item> > + <item name="android:paddingTop">3dp</item> > + <item name="android:drawablePadding">5dp</item> Still don't understand why you need to define drawablePadding here. @@ +383,5 @@ > + <!-- Reading List Row Item --> > + > + <style name="TextAppearance.Widget.ReadingListRow.ReadTime" parent="TextAppearance.Small"> > + <item name="android:textStyle">italic</item> > + <item name="android:fontFamily">sans-serif-condensed</item> fontFamily is only available in Android >= 16. Define a different version of ReadingListRow.Readtime in values-v16/styles.xml that uses fontFamily. ::: mobile/android/base/strings.xml.in @@ +244,5 @@ > <string name="reading_list_added">&reading_list_added;</string> > <string name="reading_list_removed">&reading_list_removed;</string> > <string name="reading_list_failed">&reading_list_failed;</string> > <string name="reading_list_duplicate">&reading_list_duplicate;</string> > + <string name="reading_list_readtime_mintes">&reading_list_readtime_mintes;</string> Still don't know what 'mintes' stands for here.

> > + <string name="reading_list_readtime_mintes">&reading_list_readtime_mintes;</string> > > Still don't know what 'mintes' stands for here. My guess is "minutes".

(In reply to Lucas Rocha (:lucasr) from comment #29) > ::: mobile/android/base/resources/values/styles.xml > @@ +126,5 @@ > > </style> > > > > + <!-- Detailed Row Item --> > > + > > + <style name="Widget.DetailedListRow" /> > > I don't follow. Why do you need this parent style that is only used by > ReadingListRow? Get rid of these DetailedListRow styles and define > ReadingListRow based on TextAppearance.Widget.Home.Item* text appearance. It > should look more or less like: > Remember back during work week, I was asked to make a generic version of ReadingListRow for use in home panels (https://bugzilla.mozilla.org/show_bug.cgi?id=889351#c23). You said to make an abstract style, DetailedListRow, that could be used in home panels. > <style name="Widget.ReadingListRow" /> > > <style name="Widget.ReadingListRow.Title"> > <item > name="android:textAppearance">@style/TextAppearance.Widget.Home.ItemTitle</ > item> > <item name="android:maxLines">2</item> > <item name="android:ellipsize">end</item> > </style> > > <style name="Widget.ReadingListRow.Description"> > <item > name="android:textAppearance">@style/TextAppearance.Widget.Home. > ItemDescription</item> > <item name="android:maxLines">4</item> > <item name="android:ellipsize">end</item> > </style> > > Note how I didn't define fontFamily and textColor as they are defined by the > text appearance already. Also, move layout_weight to the layout file. > This is because I got rid of TextAppearance.Widget.DetailedListRow.Description etc. Those styles weren't entirely redundant. They included custom fontFamily and textColor attributes that aren't in Home.ItemDescription. > @@ +153,5 @@ > > + <style name="Widget.ReadingListRow.Title" parent="Widget.DetailedListRow.Title" /> > > + <style name="Widget.ReadingListRow.Excerpt" parent="Widget.DetailedListRow.Description" /> > > + > > + <style name="Widget.ReadingListRow.ReadTime"> > > + <item name="android:layout_weight">0</item> > > Move layout_weight to the layout file. > Alright will do. > ::: mobile/android/base/strings.xml.in > @@ +244,5 @@ > > <string name="reading_list_added">&reading_list_added;</string> > > <string name="reading_list_removed">&reading_list_removed;</string> > > <string name="reading_list_failed">&reading_list_failed;</string> > > <string name="reading_list_duplicate">&reading_list_duplicate;</string> > > + <string name="reading_list_readtime_mintes">&reading_list_readtime_mintes;</string> > > Still don't know what 'mintes' stands for here. oops, will rename.

(In reply to Sola Ogunsakin [:sola] from comment #31) > (In reply to Lucas Rocha (:lucasr) from comment #29) > > ::: mobile/android/base/resources/values/styles.xml > > @@ +126,5 @@ > > > </style> > > > > > > + <!-- Detailed Row Item --> > > > + > > > + <style name="Widget.DetailedListRow" /> > > > > I don't follow. Why do you need this parent style that is only used by > > ReadingListRow? Get rid of these DetailedListRow styles and define > > ReadingListRow based on TextAppearance.Widget.Home.Item* text appearance. It > > should look more or less like: > > > Remember back during work week, I was asked to make a generic version of > ReadingListRow for use in home panels > (https://bugzilla.mozilla.org/show_bug.cgi?id=889351#c23). You said to make > an abstract style, DetailedListRow, that could be used in home panels. Things have changed quite a bit since then. The article item view has its own style and uses TextAppearance.Widget.Home.Item*. Let's only worry about the shared style once/if we need it. > > <style name="Widget.ReadingListRow" /> > > > > <style name="Widget.ReadingListRow.Title"> > > <item > > name="android:textAppearance">@style/TextAppearance.Widget.Home.ItemTitle</ > > item> > > <item name="android:maxLines">2</item> > > <item name="android:ellipsize">end</item> > > </style> > > > > <style name="Widget.ReadingListRow.Description"> > > <item > > name="android:textAppearance">@style/TextAppearance.Widget.Home. > > ItemDescription</item> > > <item name="android:maxLines">4</item> > > <item name="android:ellipsize">end</item> > > </style> > > > > Note how I didn't define fontFamily and textColor as they are defined by the > > text appearance already. Also, move layout_weight to the layout file. > > > > This is because I got rid of > TextAppearance.Widget.DetailedListRow.Description etc. Those styles weren't > entirely redundant. They included custom fontFamily and textColor attributes > that aren't in Home.ItemDescription. Ok, inherit from ItemDescription and only override what needs to be different then.

It looks like bug 959297 did the leg work to give us the data we need here. I'm going to see if I can revive this patch.

I believe this addresses your previous review comments. I also updated the layout to make it look a bit better (the description wasn't taking up the whole horizontal space before).

I just realized that this will likely have issues with items that are added to the reading list from the share overlay, or items where caching failed, since they won't have an excerpt/length. I can file a follow-up bug to address that, since in that case we may want to show the URL instead of a non-existent excerpt.

(In reply to :Margaret Leibovic from comment #35) > Created attachment 8502774 [details] > screenshot > > I just realized that this will likely have issues with items that are added > to the reading list from the share overlay, or items where caching failed, > since they won't have an excerpt/length. I can file a follow-up bug to > address that, since in that case we may want to show the URL instead of a > non-existent excerpt. Yeah, follow-up is fine. The duration labels in the screenshot look slightly misaligned to me. File a follow-up to get feedback from antlam?

Comment on attachment 8502772 [details] [diff] [review] Show more details in reading list row Review of attachment 8502772 [details] [diff] [review]: ----------------------------------------------------------------- Looking great. ::: mobile/android/base/home/ReadingListRow.java @@ +77,5 @@ > + * > + * @param length of the article (in characters) > + * @return estimated time to read the article (in minutes) > + */ > + private int getEstimatedReadTime(int length) { nit: can be static ::: mobile/android/base/resources/values/styles.xml @@ +146,5 @@ > + </style> > + > + <style name="Widget.ReadingListRow.ReadTime"> > + <item name="android:textStyle">italic</item> > + <item name="android:textColor">#FF9400</item> We probably already have this orange defined somewhere else, no? Reuse a color resource if possible here.

(In reply to Lucas Rocha (:lucasr) from comment #36) > (In reply to :Margaret Leibovic from comment #35) > > Created attachment 8502774 [details] > > screenshot > > > > I just realized that this will likely have issues with items that are added > > to the reading list from the share overlay, or items where caching failed, > > since they won't have an excerpt/length. I can file a follow-up bug to > > address that, since in that case we may want to show the URL instead of a > > non-existent excerpt. > > Yeah, follow-up is fine. The duration labels in the screenshot look slightly > misaligned to me. File a follow-up to get feedback from antlam? Good spot, I think that's probably because the text sizes are different for the title and the duration text. Maybe we should just add some padding to the top of the duration text to make it vertically centered with the top line of the title. I can file a follow-up to iterate on this with antlam when he's back from PTO. (In reply to Lucas Rocha (:lucasr) from comment #37) > ::: mobile/android/base/resources/values/styles.xml > @@ +146,5 @@ > > + </style> > > + > > + <style name="Widget.ReadingListRow.ReadTime"> > > + <item name="android:textStyle">italic</item> > > + <item name="android:textColor">#FF9400</item> > > We probably already have this orange defined somewhere else, no? Reuse a > color resource if possible here. I took this color from Sola's patch, and I see that we just use it as one of our swipe-to-refresh colors, and in about:reader: http://mxr.mozilla.org/mozilla-central/search?find=/mobile/android/&string=FF9400 I can create a new color value for it, though, so that we don't end up with an orphan orange.

(In reply to :Margaret Leibovic from comment #38) > (In reply to Lucas Rocha (:lucasr) from comment #37) > > > ::: mobile/android/base/resources/values/styles.xml > > @@ +146,5 @@ > > > + </style> > > > + > > > + <style name="Widget.ReadingListRow.ReadTime"> > > > + <item name="android:textStyle">italic</item> > > > + <item name="android:textColor">#FF9400</item> > > > > We probably already have this orange defined somewhere else, no? Reuse a > > color resource if possible here. > > I took this color from Sola's patch, and I see that we just use it as one of > our swipe-to-refresh colors, and in about:reader: > http://mxr.mozilla.org/mozilla-central/search?find=/mobile/android/ > &string=FF9400 > > I can create a new color value for it, though, so that we don't end up with > an orphan orange. Actually, text_color_highlight is #FF9500, which is almost exactly the same, so I'll just use that, because I don't think we need another extemely similar orange value in the tree.

https://hg.mozilla.org/integration/fx-team/rev/f2e655475068 I filed bug 1082110 for the polish refinements.

Reading list items now display: a title, a description and approx. reading time Verified fixed on: Devices: Alcatel One Touch(Android 4.1.2) and Nexus 7 (Android 5.0) Build: Firefox for Android 36.0a1 (2014-11-23)