1092011 - Re-instate Page-Already-In-ReadingList message

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Mark Capella [:capella]]

Attachment: bugAlready.diff

I think in somewhere around bug 1085685 comment #6 we meant to do this:

"We shouldn't ever fail to add an item, so I don't think we need that toast anymore, but maybe we should add some logic to the Java side to show a toast if you're adding something that's already in your reading list."

Comment 1

[:Margaret Leibovic]

Comment on attachment 8514795 [details] [diff] [review]
bugAlready.diff

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

::: mobile/android/base/ReadingListHelper.java
@@ +95,5 @@
> +                    values.put(ReadingListItems.URL, url);
> +                    values.put(ReadingListItems.TITLE, message.optString("title"));
> +                    values.put(ReadingListItems.LENGTH, message.optInt("length"));
> +                    values.put(ReadingListItems.EXCERPT, message.optString("excerpt"));
> +                    BrowserDB.addReadingListItem(cr, values);

I feel like addReadingListItem should return 0 if it doesn't add a new item, and we should use the return value to check for the toast.

We do get an updated value here, but we never return it:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#953

However, this will probably return 1, regardless of whether or not the item is new, so maybe this doesn't actually help.

But thinking about this more, what should we do if the user wants to add an item to the reading list that has the same URL, but has different metadata? Should we still update the metadata? I want to know what rnewman thinks about this.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +358,5 @@
>  <!ENTITY site_settings_clear        "Clear">
>  <!ENTITY site_settings_no_settings  "There are no settings to clear.">
>  
>  <!ENTITY reading_list_added "Page added to your Reading List">
> +<!ENTITY reading_list_duplicate2 "Page already in your Reading List">

I don't think we need to rev an entity that we removed and then put back without changing.

Comment 2

[Mark Capella [:capella]]

Right, I considered to simply adding a return value to addReadingListItem(), but it was pointed out previously that wesj's work re: Bug 1077590 has us avoiding mods too close to BrowserDB/LocalBrowserDB for a while.

And yah, I see we can fail-but-still-add a (stub) item:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Reader.js?mark=109-109#104

So maybe change from reporting "Already in List" to more like "Listitem Updated" wfm

Comment 3

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #1)

> But thinking about this more, what should we do if the user wants to add an
> item to the reading list that has the same URL, but has different metadata?
> Should we still update the metadata? I want to know what rnewman thinks
> about this.

I think we should update the metadata if it's "better" -- e.g., filling in a title that's missing.

We shouldn't bump the date added, alter read state, etc.

This can all be done with a single smart UPDATE query using COALESCE.

And this should probably still result in an "already in reading list" message.

We can assume that if they have the same URL they're the same thing. That will sometimes be wrong, but whatcha gonna do?

(Sidenote: we currently do something suboptimal if you, say, add a page from m.nytimes.com, then add the t.co link to the same article via the share overlay, then use desktop mode to load the www.nytimes.com version. Some of this can be mitigated by using a destination URL, as I noted in recent protocol sketching. We can't solve this here, but keep the concept of dupes in mind for the future.)

Comment 4

[:Margaret Leibovic]

Comment on attachment 8514795 [details] [diff] [review]
bugAlready.diff

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

Well, since previously we weren't even trying to add the item if we saw it already exists, this patch is consistent. We can address the updating metadata situation in another bug, especially since we will likely be augmenting the reading list provider schema in the near future.

Comment 5

[:Margaret Leibovic]

(But see my previous comment about rev-ing the entity id. Unless you know a reason why we should do that, I don't think we need to.)

Comment 6

[Mark Capella [:capella]]

Was playing it safe, I can believe you're correct
https://tbpl.mozilla.org/?tree=Try&rev=584f9405bd92

Comment 7

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/a9902edc4870

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a9902edc4870