GNOME Bugzilla – Bug 720444
Add support for saving links to Pocket
Last modified: 2017-08-07 11:39:24 UTC
In bug 704564, you can find a simple application that uses librest to add new URLs to Pocket. In Epiphany, I would: - Monitor GoaClient for accounts added/removed etc. - Show a "Read Later" menu item in right-click menus, and in the toolbar - When called, make the simple network call, and show a GdNotification-like popup that the URL was added (which would go away after a couple of seconds) Optionally, I would add a hack to find the tweet ID that the link came from (walk up the DOM, and find the tweet div, get the data-item-id) to add tweet attributions. But that's probably a bit too hackish for something that's not a plugin.
Created attachment 264204 [details] [review] Add "Read Later" support for Pocket TODO: - UI
Created attachment 264210 [details] [review] Add "Read Later" support for Pocket TODO: - Monitor GoaClient for accounts - Add a GdNotification on success/failure - Make sure that the button is hidden if not "available"
Created attachment 264234 [details] [review] widgets: Add GdNotification widget Copied from libgd 41820ae96142312ffc0109b65d2adad6f007e1fd
Created attachment 264235 [details] [review] embed: Add "read later" notification And a helper to show feedback after clicking the "read later" button.
Created attachment 264236 [details] [review] Add "Read Later" support for Pocket
Created attachment 264237 [details] [review] embed: Add "read later" notification And a helper to show feedback after clicking the "read later" button.
Created attachment 264238 [details] error screenshot
Created attachment 264239 [details] page saved screenshot
I just noticed that I didn't add "Read Later" for links through the right-click menu. What's the way to do this? Adding a new EphyLinkAction type?
Created attachment 264243 [details] [review] embed: Add "read later" notification And a helper to show feedback after clicking the "read later" button.
Created attachment 264244 [details] [review] Add "Read Later" support for Pocket
Review of attachment 264234 [details] [review]: Not reviewing this :)
Review of attachment 264243 [details] [review]: I think that setting up in this commit a GDNotification in the embed is a good idea, but it needs a more generic approach. Stuff related to "read later" support maybe need to go in the next commit.. (need to review that one yet). ::: embed/ephy-embed.h @@ +74,3 @@ +void ephy_embed_show_read_later_notification (EphyEmbed *embed, + gboolean in_progress, + gboolean success); I'm wondering whether we should add in this commit a more generic API for GDNotifications that we could use for other cases.
Created attachment 265921 [details] [review] Add "Read Later" support for Pocket
Ctrl+Alt+L without having configured anything yet: ** (epiphany:26371): CRITICAL **: ephy_read_later_add_url: assertion 'self->priv->consumer_key && self->priv->access_token' failed Also the "Saving Page" notification stays there and nothing happens.
Some comments on things as I understand them today. The current patch: If the user has already set up a Pocket account in GOA, then a button (using the document-open-recent-symbolic icon) appears in the header bar. Pressing this button (or using Ctrl+Alt+L) will attempt to save the currently loaded page to the web service. First, an in-app notification will appear, with the text "Saving Page" accompanied by a spinner. If the operation fails for any reason an in-app notification is used to present the error (https://bug720444.bugzilla-attachments.gnome.org/attachment.cgi?id=264238). Which offers to go to the GOA. If the operation succeeds then "Page Saved" is displayed in an in-app notification. The operation may succeed if the page has been saved before. There is currently no indication that something occurred after the in-app notification goes away. In particular, there is no way to undo the action, there is no indication that the page is already saved in the reading list on this view of the page or any subsequent view, and there is no indication of where the thing was saved to or how to get to it. Comments: In general, I think we need to be extremely cautious with how we use our symbolic icons. Once a metaphor gets used for something it is difficult for us to change it. Also in general, I think we need to be cautious with how we make the distinction between built-in functionality and add-on functionality - which often mirrors the distiction between symbolic and branded icons. Again generally, we usually want to offer symmetry in actions such as: the ability to undo something I have just done, the ability to see something I've just saved. Now specifically: This functionality is displayed to the user as if it is built-in functionality but isn't available before I have Pocket set up, doesn't help me set up Pocket, doesn't have a symmetric remove or undo actions, doesn't support different backends, doesn't have a way for me to actually read it later built-in. So it isn't a good fit for built-in functionality. That doesn't mean this isn't cool. What I would prefer to see is this be presented as if it was part of the prospective sharing framework. So conceptually it would be "Send to Pocket" or "Save to Pocket". This is not that different from "Save to Documents" or "Save to Imgur". I am open to having a built-in read it later (perhaps even using Pocket as a backend) but until the above things are satisfied I think we are better seeing this as a one way push (ie. share). Some details: * This should not use a symbolic icon but use a pocket branded icon. * This should be presented inside a standard "share" dialog/popover and not directly from its own icon on the headerbar * C+A+L is not a good choice for keybinding as mentioned on IRC. Because it was (and in my opinion still should be :) ) the lock screen binding. * I think we should be "network aware" and follow the state of connectivity so we don't get sent to GOA to debug a network problem. * The Page Saved notification should probably link to where it was saved to. * I would still like to see us detect if a page is already in the reading list. We could use a little indicator in the title box or something. - This should be possible to do, theoretically, using things like Open Graph Protocol. Dunno.
(In reply to comment #16) <snip> > Comments: > > In general, I think we need to be extremely cautious with how we use our > symbolic icons. Once a metaphor gets used for something it is difficult for us > to change it. Also in general, I think we need to be cautious with how we make > the distinction between built-in functionality and add-on functionality - which > often mirrors the distiction between symbolic and branded icons. Again > generally, we usually want to offer symmetry in actions such as: the ability to > undo something I have just done, the ability to see something I've just saved. > > Now specifically: This functionality is displayed to the user as if it is > built-in functionality but isn't available before I have Pocket set up, doesn't > help me set up Pocket, doesn't have a symmetric remove or undo actions, doesn't > support different backends, doesn't have a way for me to actually read it later > built-in. So it isn't a good fit for built-in functionality. Epiphany used to support plugins. It would have been written as a plugin before. It doesn't support multiple backends because nobody did the work to support multiple backends either. > That doesn't mean this isn't cool. What I would prefer to see is this be > presented as if it was part of the prospective sharing framework. So > conceptually it would be "Send to Pocket" or "Save to Pocket". This is not that > different from "Save to Documents" or "Save to Imgur". > > I am open to having a built-in read it later (perhaps even using Pocket as a > backend) but until the above things are satisfied I think we are better seeing > this as a one way push (ie. share). > > Some details: > * This should not use a symbolic icon but use a pocket branded icon. We can't use a branded icon here, because it would need to have 2 states. Pocket's stock icons only have one such state. > * This should be presented inside a standard "share" dialog/popover and not > directly from its own icon on the headerbar Fine. > * C+A+L is not a good choice for keybinding as mentioned on IRC. Because it > was (and in my opinion still should be :) ) the lock screen binding. Ideas on a new keyboard shortcut? > * I think we should be "network aware" and follow the state of connectivity so > we don't get sent to GOA to debug a network problem. That's fixable within the "pocket object", which could queue the additions. > * The Page Saved notification should probably link to where it was saved to. The Pocket web UI? That's possible (the URL is http://getpocket.com/a/read/<id>) > * I would still like to see us detect if a page is already in the reading > list. We could use a little indicator in the title box or something. We can't do that easily. We'd need to make sure our list of items is up to date for each new page loaded (!) and it wouldn't be 100% accurate (we can't just check if URLs are equal). I wouldn't do this unless I can be confident it's fully accurate (and I'm not even talking about being slightly updated, but just accurate). That's just not possible. > - This should be possible to do, theoretically, using things like Open Graph > Protocol. Dunno. I don't know what that is, or means.
Comment on attachment 265921 [details] [review] Add "Read Later" support for Pocket Jon reviewed this already.
Hm... We have: * Bug #685785 to implement our own native reading list feature. It's very important to have this built-in IMO. * Bug #777630 to sync it with Firefox Sync. I'm not sure that we really want integration with a third-party service like Pocket, so I'm inclined to WONTFIX this. But I also don't want to shoot down good work on a reading list feature. What do you think, Bastien?
(In reply to Michael Catanzaro from comment #19) > Hm... > > We have: > > * Bug #685785 to implement our own native reading list feature. It's very > important to have this built-in IMO. I personally wouldn't want this builtin. I prefer having a cross-platform solution that doesn't depend on local storage. > * Bug #777630 to sync it with Firefox Sync. And again, there'd be no way to get it out of the list other than using Epiphany itself. If anything, that would call for a service like Pocket, instapaper, or Wallabag. > I'm not sure that we really want integration with a third-party service like > Pocket, so I'm inclined to WONTFIX this. But I also don't want to shoot down > good work on a reading list feature. What do you think, Bastien? If the patches still applied, and it wasn't visible by default (unless a Pocket account was set up), I'd say merge it until we have that elusive sharing service. I'd be happy making the changes necessary to this patchset if there was a clear, even if short-lived in its current form, path to inclusion. (the sharing target would be provided by Bolso, which might become a Wallabag client if I don't get off my arse. https://github.com/felipeborges/bolso)
(In reply to Bastien Nocera from comment #20) > I'd be happy making the changes necessary to this patchset if there was a > clear, even if short-lived in its current form, path to inclusion. To be honest I think I need more time to consider the UI for this, so don't spend more time on it right now. I definitely want something built-in; that's a top requirement as there's no way some third-party service can compete with a good built-in reading list, assuming we can get the UI right. It would very be nice to sync with Firefox if possible, but I'll be content with syncing with just other Epiphany instances if not. I saw from the Firefox Sync documentation that it can sync your "reading list", but it's missing from another portion of the documentation so I'm not sure if that is accurate or outdated.
(In reply to Michael Catanzaro from comment #19) > * Bug #685785 to implement our own native reading list feature. It's very > important to have this built-in IMO. That's a different bug. Typo? (Trying to understand what 'built-in' means in this context.)
(In reply to Debarshi Ray from comment #22) > (In reply to Michael Catanzaro from comment #19) > > * Bug #685785 to implement our own native reading list feature. It's very > > important to have this built-in IMO. > > That's a different bug. Typo? Yeah, I meant bug #685685 > (Trying to understand what 'built-in' means in this context.) Like http://home.bt.com/tech-gadgets/computing/what-is-reading-list-microsoft-edge-11364066709705 Except ours would be either use either some popover or maybe a full webpage rather than a sidebar.
So I think the plan here should be: (a) First land built-in reading list support. I don't want this feature to be gated behind a login to a third-party service. (Hopefully it can happen later this year.) (b) Then Bastien can update his Pocket patches based on whatever reading list lands.
(In reply to Michael Catanzaro from comment #24) > So I think the plan here should be: > > (a) First land built-in reading list support. I don't want this feature to > be gated behind a login to a third-party service. (Hopefully it can happen > later this year.) > (b) Then Bastien can update his Pocket patches based on whatever reading > list lands. By which time we'll have a Sharing portal and I'll close this bug as WONTFIX.
OK then. Sharing portal sounds better indeed. It'll need to be hooked up, but that's yet another bug.