About:home "Saved for later" panel (reading list)

RESOLVED FIXED in Firefox 26

Status

()

P1
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: ibarlow, Assigned: Margaret)

Tracking

unspecified
Firefox 26
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-fig)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
This is the bug to track work on the new "saved for later" panel on about:home, which will first be used to display the user's Reading List.

Note that we will be 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

* Title
* Intro
* Picture, if available
* Length of article, in minutes

Mockups to follow shortly.
(Reporter)

Comment 1

6 years ago
Created attachment 739732 [details]
Mockups of "Reading list" panel
(Reporter)

Comment 2

6 years ago
This bug should include work for:

* Displaying a more editorial list of articles (using twitter cards, for example)
* Calculating the length of the article, in minutes
Whiteboard: good-first-bug-fig
Summary: About:home "Saved for later" panel → About:home "Saved for later" panel (reading list)
(In reply to Ian Barlow (:ibarlow) from comment #2)
> This bug should include work for:
> 
> * Displaying a more editorial list of articles (using twitter cards, for
> example)
> * Calculating the length of the article, in minutes

Open a new bug for any new features. This bug is about feature parity with current "reading list"
Priority: -- → P1
(Reporter)

Updated

6 years ago
Depends on: 889351
(Assignee)

Comment 4

6 years ago
I can take this (implementing feature parity with the current reading list).
Assignee: nobody → margaret.leibovic
OS: Mac OS X → Android
Hardware: x86 → ARM
(Assignee)

Comment 5

6 years ago
Created attachment 770495 [details] [diff] [review]
patch

I feel like I did a bit too much copy/pasting to get to this patch, but it gets the job done. It's probably not worth factoring some of the copy/pasted stuff out into some shared superclass, since we'll be customizing the views of this page later.

I still don't fully understand how CursorLoaders work, so I very well may have done something wrong :)

One known issue here is that I need to add that "Articles you save for later go here" message when the list is empty, but I figured that could be done in a follow-up.
Attachment #770495 - Flags: review?(sriram)
Attachment #770495 - Flags: review?(bnicholson)
(Assignee)

Comment 6

6 years ago
Created attachment 770496 [details]
screenshot
Comment on attachment 770495 [details] [diff] [review]
patch

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

Looks really good. Only thing is the use of SimpleCursorAdapter. I would like to switch that to CursorAdapter, and see a version.
Other than that, everything else is fine. r- just to see a version with CursorAdapter.

::: mobile/android/base/home/ReadingListPage.java
@@ +42,5 @@
> +    // Callbacks used for the reading list and favicon cursor loaders
> +    private CursorLoaderCallbacks mCursorLoaderCallbacks;
> +
> +    // Inflater used by the adapter
> +    private LayoutInflater mInflater;

This is not needed.

@@ +60,5 @@
> +            throw new ClassCastException(activity.toString()
> +                    + " must implement HomePager.OnUrlOpenListener");
> +        }
> +
> +        mInflater = (LayoutInflater) activity.getSystemService(Context.LAYOUT_INFLATER_SERVICE);

Not needed.

@@ +135,5 @@
> +            return BrowserDB.getBookmarksInFolder(getContext().getContentResolver(), Bookmarks.FIXED_READING_LIST_ID);
> +        }
> +    }
> +
> +    private class ReadingListAdapter extends SimpleCursorAdapter {

Please use a CursorAdapter.

@@ +144,5 @@
> +        @Override
> +        public View getView(int position, View convertView, ViewGroup parent) {
> +            final TwoLinePageRow row;
> +            if (convertView == null) {
> +                row = (TwoLinePageRow) mInflater.inflate(R.layout.home_item_row, mList, false);

This could be a direct call like,

row = new TwoLinePageRow(parent.getContext());

Hence you won't need an mInflater. And that part will be in newView(). bindView() will take care of coversion.

@@ +150,5 @@
> +                row = (TwoLinePageRow) convertView;
> +            }
> +
> +            final Cursor c = getCursor();
> +            if (!c.moveToPosition(position)) {

This won't be needed when CursorAdapter is used. Always the cursor is moved to required position.

@@ +160,5 @@
> +            return row;
> +        }
> +    }
> +
> +    private class CursorLoaderCallbacks implements LoaderCallbacks<Cursor> {

A small documentation may be?
Attachment #770495 - Flags: review?(sriram) → review-
Comment on attachment 770495 [details] [diff] [review]
patch

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

r+ with Sriram's comments addressed.

::: mobile/android/base/home/ReadingListPage.java
@@ +47,5 @@
> +
> +    // On URL open listener
> +    private OnUrlOpenListener mUrlOpenListener;
> +
> +    public ReadingListPage() { }

This is unnecessary since Java automatically creates a default constructor if none are specified.
Attachment #770495 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 770541 [details] [diff] [review]
patch v2

Updated to address comments.
Attachment #770495 - Attachment is obsolete: true
Attachment #770541 - Flags: review?(sriram)
(Assignee)

Updated

6 years ago
Depends on: 889677
Comment on attachment 770541 [details] [diff] [review]
patch v2

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

Looks good.

::: mobile/android/base/home/ReadingListPage.java
@@ +44,5 @@
> +
> +    // On URL open listener
> +    private OnUrlOpenListener mUrlOpenListener;
> +
> +    public ReadingListPage() { }

Please remove this.
Attachment #770541 - Flags: review?(sriram) → review+
(Assignee)

Comment 11

6 years ago
https://hg.mozilla.org/projects/fig/rev/7f1cfc8c84e8
Whiteboard: good-first-bug-fig → fixed-fig
(Assignee)

Comment 12

6 years ago
Backed out for build failures:
https://hg.mozilla.org/projects/fig/rev/35f62a290d4e

I'll look into fixing and relanding.
Whiteboard: fixed-fig
(Assignee)

Comment 13

6 years ago
Turned out I botched resolving a conflict in the Makefile. Fixed:
https://hg.mozilla.org/projects/fig/rev/b689792b6fd2
Whiteboard: fixed-fig
https://hg.mozilla.org/mozilla-central/rev/b689792b6fd2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
(Assignee)

Updated

4 years ago
No longer depends on: 889351
You need to log in before you can comment on or make changes to this bug.