1065969 - Make sending articles to Reading List easier from home panel add-ons

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Awesomescreen, defect, P2)

Description

[Karen Rudnitski [:kar]]

Because we'd like to sync the reading list over to other Firefox instances attached to FxA (including over to desktop), I'd like to make it even easier to use the Reading List as a repository of articles you'd like to read later (or on a different screen), offline or online.

There as a start, can we add 'send to reading list' [or something along those lines] to the long-press menu when I tap on an item in another home panel feed I have installed? Alternate ways also welcome.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Any UX feedback?

Comment 2

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

I think that's a good idea. Our quick win is to add "Add to Reading List" in the long press menu for articles. It's the same wording as the action for the Share overlay.

Comment 3

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

Attachment: prev_rmode_menu1.png

That sounds like a good starting point.

If we have this goal in mind of working with our desktop counterparts, maybe we could even put that in the right click menu as well? Not unlike our attempts at trying to do something like this (which was more technically challenging).

Comment 4

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

Attachment: Screen Shot 2014-09-11 at 2.43.01 PM.png

Attaching desktops right click menu.

Comment 5

[Karen Rudnitski [:kar]]

If it would be in the sharing element, we would need to refer to this as Reading List and not reader mode - because the idea is we're adding it to the list (plus I *hate* having 'mode' in-product)

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #3)
> Created attachment 8488259 [details]
> prev_rmode_menu1.png
> 
> That sounds like a good starting point.
> 
> If we have this goal in mind of working with our desktop counterparts, maybe
> we could even put that in the right click menu as well? Not unlike our
> attempts at trying to do something like this (which was more technically
> challenging).

We already have this in the Sharing system. If you share using "Firefox", "Add to Reading List" is already there.

This bug is 100% about the Home Panels inside Firefox.

Comment 7

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

Attachment: Screenshot_2014-09-26-10-38-30.png

Fair enough, an item in the long press menu makes sense when the user is in the Panels view.

Is that what the "Add to Home Screen" does? I pressed it but not sure where it "added" the link to... 

If not, "Add to Reading list" makes sense :) Maybe a toast too?

Comment 8

[Karen Rudnitski [:kar]]

'Add to Home Screen' looks like it ads the link directly to my Android home screen as a short cut. This is awful :(. Can we *not* do that and replace that item to 'Add to Reading List'?

Comment 9

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

(In reply to Karen Rudnitski [:kar] from comment #8)
> 'Add to Home Screen' looks like it ads the link directly to my Android home
> screen as a short cut. This is awful :(. Can we *not* do that and replace
> that item to 'Add to Reading List'?

I'm not exactly sure what it does. I found that in Nightly when I long pressed on a link in my History Panel.

Comment 10

[Karen Rudnitski [:kar]]

I'm not sure what you're saying here. Right now, 'add to home screen' means the link gets pasted on my wallpaper as a link. 

Now I do know that my husband uses that feature *all* the time as a way to bookmark his top sites directly from his wallpaper. Can we check telemetry to see how many people are like my husband and simply add to the list in order to 'add to reading list'?

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 12

[Karen Rudnitski [:kar]]

this is core to future work, so we need to ensure that we make it as simple and easy for folks to add articles to their reading list.

Comment 13

[Robin Ricard [:rricard]]

This one should be nice to work on. I think I'll take it after fixing bug 1130872.

Comment 14

[Robin Ricard [:rricard]]

Maybe add some tests too ...

Comment 15

[Robin Ricard [:rricard]]

I'll need some help to work on it though ...

Comment 16

[Robin Ricard [:rricard]]

Attachment: Make sending articles to Reading List easier from home panel add-ons

Add a new menu item
Bind the menu item to a Reading List Addition

Comment 17

[Robin Ricard [:rricard]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c48d17536bf4

Comment 18

[Robin Ricard [:rricard]]

Attachment: Feature added, screenshot

Comment 19

[Robin Ricard [:rricard]]

Uh ! bad try flags !

Comment 20

[Robin Ricard [:rricard]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d7346d8b992

Comment 21

[:Margaret Leibovic]

Thanks for picking this up! I think we need to get some input here from antlam about what we want to be doing here.

antlam, do we want an "Add to Reading List" context menu item on all home panel items?

Comment 22

[:Margaret Leibovic]

Comment on attachment 8564572 [details] [diff] [review]
Make sending articles to Reading List easier from home panel add-ons

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

This is going in the right direction, I just want to get some feedback from antlam before proceeding further.

::: mobile/android/base/home/HomeFragment.java
@@ +260,5 @@
> +
> +            AddToReadingList addToReadingList = new AddToReadingList(context);
> +            ShareData shareData = new ShareData(info.url, info.title, null, ShareMethod.Type.ADD_TO_READING_LIST);
> +            addToReadingList.handle(shareData);
> +            return true;

This is an interesting way to re-purpose the share overlay code. However, I think it would be more straightforward to just call into BrowserDB directly, like we do else where in this class. You'll also want to do this in a background task, since it's doing disk I/O.

I also think you should wait until the patches bug 1130461 land, since those will change the APIs available to you (and I think it should make this easier).

Comment 23

[:Margaret Leibovic]

Comment on attachment 8564572 [details] [diff] [review]
Make sending articles to Reading List easier from home panel add-ons

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

rnewman, what do you think of this share handler approach as opposed to just directly querying BrowserDB?

Comment 24

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

(In reply to :Margaret Leibovic from comment #21)
> Thanks for picking this up! I think we need to get some input here from
> antlam about what we want to be doing here.
> 
> antlam, do we want an "Add to Reading List" context menu item on all home
> panel items?

Yeah I think that's still a pretty valid idea. We might have to unify on copy here like "Add to Reading List" instead of "Send to Reading List" but I like where this is headed :) thanks Rricard!

Comment 25

[Richard Newman [:rnewman]]

Comment on attachment 8564572 [details] [diff] [review]
Make sending articles to Reading List easier from home panel add-ons

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

::: mobile/android/base/home/HomeFragment.java
@@ +260,5 @@
> +
> +            AddToReadingList addToReadingList = new AddToReadingList(context);
> +            ShareData shareData = new ShareData(info.url, info.title, null, ShareMethod.Type.ADD_TO_READING_LIST);
> +            addToReadingList.handle(shareData);
> +            return true;

Yeah, ReadingListAccessor will do pretty much exactly this without having to make a ShareData instance.

Comment 26

[Robin Ricard [:rricard]]

Ok, going to work on ReadingListAccessor instead. Thanks for the feedback !

Comment 27

[Robin Ricard [:rricard]]

I'll wait for bug 1130461 first, ping me if you need help on it. I'm following it anyway.

Comment 28

[:Margaret Leibovic]

(In reply to Robin Ricard [rricard] from comment #27)
> I'll wait for bug 1130461 first, ping me if you need help on it. I'm
> following it anyway.

Okay, thanks.

My one concern is that this change requires a string, so it would be good to land that before the merge. We have a bit of leeway on reader-related string for Fx38 aurora, but we could also just pre-land the strings for this. If you're interested in doing that, could you split up your patch to create an initial patch that just adds the string for the context menu? I think it should be "Add to Reading List" for consistency with the other places we have this in our UI.

Comment 29

[:Margaret Leibovic]

We're removing the reading list in bug 1234314.