Closed Bug 1092011 Opened 7 years ago Closed 7 years ago

Re-instate Page-Already-In-ReadingList message

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: capella, Assigned: capella)

References

Details

Attachments

(1 file)

Attached patch bugAlready.diffSplinter Review
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."
Attachment #8514795 - Flags: review?(margaret.leibovic)
Assignee: nobody → markcapella
Blocks: readerv2, 1085685
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.
Attachment #8514795 - Flags: feedback?(rnewman)
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
(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 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.
Attachment #8514795 - Flags: review?(margaret.leibovic)
Attachment #8514795 - Flags: review+
Attachment #8514795 - Flags: feedback?(rnewman)
(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.)
Was playing it safe, I can believe you're correct
https://tbpl.mozilla.org/?tree=Try&rev=584f9405bd92
https://hg.mozilla.org/mozilla-central/rev/a9902edc4870
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.