Closed Bug 822979 Opened 11 years ago Closed 9 years ago

Duplicate bookmark should be announced by a pop-up

Categories

(Firefox for Android Graveyard :: General, enhancement)

ARM
Android
enhancement
Not set
normal

Tracking

(firefox27 affected, firefox28 affected, firefox29 affected, firefox30 affected, firefox31 affected, firefox32 affected, firefox33 affected, firefox34 affected, firefox37 affected, firefox38 affected, firefox39 affected, firefox40 affected, firefox41 verified, relnote-firefox 41+)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox27 --- affected
firefox28 --- affected
firefox29 --- affected
firefox30 --- affected
firefox31 --- affected
firefox32 --- affected
firefox33 --- affected
firefox34 --- affected
firefox37 --- affected
firefox38 --- affected
firefox39 --- affected
firefox40 --- affected
firefox41 --- verified
relnote-firefox --- 41+

People

(Reporter: teo.vermesan, Assigned: vivek, Mentored)

References

Details

(Whiteboard: [lang=java])

Attachments

(2 files, 4 obsolete files)

Firefox 19.0a2 (2012-12-18)
Device: Asus Transformer TF 101
OS: Android 4.0.3

Steps to reproduce:
1. Open Firefox for Android
2. Go to news.google.com
3. Long tap and hold on a news article link, then select "bookmark link".
4. Repeat step 3

Expected result:
- Bookmark pop-up should appear:"Bookmark already added"

Actual result:
- Bookmark pop-up appears:"Bookmark added"

Note: The bookmark doesn't appear duplicate in Awesomescreen/Bookmarks
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: Firefox 19 → Trunk
CC'ing Margaret here. I just tested it in Nightly and this is still a thing.

How hard would a change to the copy of the Toast be to solve this issue?
Flags: needinfo?(margaret.leibovic)
(In reply to Anthony Lam (:antlam) from comment #1)
> CC'ing Margaret here. I just tested it in Nightly and this is still a thing.
> 
> How hard would a change to the copy of the Toast be to solve this issue?

We would have to do a bit more than just change the copy of the toast, since we would need to add some logic to check to see if the bookmark was already present. That being said, we have similar exisitng logic for reading list, so I think this should be pretty straightforward.

However, looking through the code, it seems like we have logic for creating these toasts in multiple places, so we would probably want to consolidate that:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#546
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#576
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/service/sharemethods/AddBookmark.java#29

Consolidating this logic would also address bug 1150010.

I would probably go about this by modifying addBookmark to return a boolean that's only true if a new bookmark was added.

Looking into the implementation in LocalBrowserDB, we would also have to change the addBookmark implementation to not update the existing bookmark, which is what we currently do.

Vivek, I know you're looking for more bugs to work on. Does this sound like something that interests you?
Blocks: 1150010
Mentor: margaret.leibovic
Flags: needinfo?(margaret.leibovic) → needinfo?(vivekb.balakrishnan)
Whiteboard: [lang=java]
Thanks Margaret,
I'll take it.
Assignee: nobody → vivekb.balakrishnan
Flags: needinfo?(vivekb.balakrishnan)
Attached patch 822979.patch (obsolete) — Splinter Review
With this patch, bookmark are added to DB iff there is no bookmark in mobile bookmark folder for same url.
Attachment #8602218 - Flags: review?(margaret.leibovic)
Attached patch 822979.patch (obsolete) — Splinter Review
A new patch with the missing strings.
Attachment #8602218 - Attachment is obsolete: true
Attachment #8602218 - Flags: review?(margaret.leibovic)
Attachment #8602333 - Flags: review?(margaret.leibovic)
Comment on attachment 8602333 [details] [diff] [review]
822979.patch

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

This is looking good! I just have a few nits for you to address, and I'd like us to double check with antlam on the string to use.

::: mobile/android/base/GeckoApp.java
@@ +555,5 @@
>              final BrowserDB db = getProfile().getDB();
>              ThreadUtils.postToBackgroundThread(new Runnable() {
>                  @Override
>                  public void run() {
> +                    final boolean newBookMark = db.addBookmark(getContentResolver(), title, url);

Nit: I would call this variable `added` or `bookmarkAdded`.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +943,5 @@
>          long folderId = getFolderIdFromGuid(cr, Bookmarks.MOBILE_FOLDER_GUID);
> +        if (isBookmarkForUrlInFolder(cr, uri, folderId)) {
> +            // Bookmark added already.
> +            return false;
> +        } else {

Nit: No need for the else statement, since you're returning above.

@@ +952,5 @@
> +    }
> +
> +    private boolean isBookmarkForUrlInFolder(ContentResolver cr, String uri, long folderId) {
> +        final Cursor c = cr.query(bookmarksUriWithLimit(1),
> +                new String[] { Bookmarks._ID },

Nit: Align these lines with the bookmarksUiWithLimit parameter above (to be consistent with other methods in this file).

@@ +955,5 @@
> +        final Cursor c = cr.query(bookmarksUriWithLimit(1),
> +                new String[] { Bookmarks._ID },
> +                Bookmarks.URL + " = ? AND " + Bookmarks.PARENT + " = ?",
> +                new String[] { uri, String.valueOf(folderId) },
> +                Bookmarks.URL);

I think you'll also need a check to make sure the bookmark isn't deleted, since we don't immediately remove deleted bookmarks from the DB for sync.

However, I don't currently see that check in `isBookmark`... I think that might be a bug. Can you file a follow-up to look into this and cc rnewman?

@@ +958,5 @@
> +                new String[] { uri, String.valueOf(folderId) },
> +                Bookmarks.URL);
> +
> +        if (c == null) {
> +            Log.e(LOGTAG, "Null cursor in isBookmark");

I don't think we need to log an error here.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +43,5 @@
>  
>  <!ENTITY bookmark "Bookmark">
>  <!ENTITY bookmark_remove "Remove bookmark">
>  <!ENTITY bookmark_added "Bookmark added">
> +<!ENTITY bookmark_already_added "Bookmark already added">

We already have a similar string in the tree for the share overlay that says "Already bookmarked":
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#107

We should try to be consistent, so let's double check with antlam to make sure this is the string we should use for the toast.
Attachment #8602333 - Flags: review?(margaret.leibovic) → review+
Just chatted with Vivek on IRC, we agree we should use the same string "Already bookmarked".
Flags: needinfo?(vivekb.balakrishnan)
Attached patch 822979.patch (obsolete) — Splinter Review
Addressed Review comments. Added check for deleted bookmark.
Attachment #8602333 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8602864 - Flags: review?(margaret.leibovic)
> @@ +955,5 @@
> > +        final Cursor c = cr.query(bookmarksUriWithLimit(1),
> > +                new String[] { Bookmarks._ID },
> > +                Bookmarks.URL + " = ? AND " + Bookmarks.PARENT + " = ?",
> > +                new String[] { uri, String.valueOf(folderId) },
> > +                Bookmarks.URL);
> 
> I think you'll also need a check to make sure the bookmark isn't deleted,
> since we don't immediately remove deleted bookmarks from the DB for sync.
> 
> However, I don't currently see that check in `isBookmark`... I think that
> might be a bug. Can you file a follow-up to look into this and cc rnewman?
> 

Filed followup bug https://bugzilla.mozilla.org/show_bug.cgi?id=1162650
Comment on attachment 8602864 [details] [diff] [review]
822979.patch

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

::: mobile/android/base/GeckoApp.java
@@ +560,5 @@
>                      ThreadUtils.postToUiThread(new Runnable() {
>                          @Override
>                          public void run() {
> +                            Toast.makeText(context, bookmarkAdded ? R.string.bookmark_added : R.string.overlay_share_bookmark_btn_label_already,
> +                                    Toast.LENGTH_SHORT).show();

Nit: To make this easier to read, you could declare a local variable for the string.

I'm a bit concerned about us using this string in a new context without informing localizers. Instead of just re-purposing the string, I think we should either add a new separate string with the same text, or rename this entity and update the l10n comments to explain the 2 places where it's used.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +953,5 @@
> +
> +    private boolean isBookmarkForUrlInFolder(ContentResolver cr, String uri, long folderId) {
> +        final Cursor c = cr.query(bookmarksUriWithLimit(1),
> +                                  new String[] { Bookmarks._ID },
> +                                  Bookmarks.URL + " = ? AND " + Bookmarks.PARENT + " = ? AND " + Bookmarks.IS_DELETED + " == ?",

You can just put the 0 directly in here, since it's not a variable we need to concatenate. E.g:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#615
Attachment #8602864 - Flags: review?(margaret.leibovic) → review+
Attached patch 822979.patch (obsolete) — Splinter Review
Review comments addressed.
@Margaret: I took the easy option. However, let me know if additional comments are required with respect to Localizer
Attachment #8602864 - Attachment is obsolete: true
Attachment #8602945 - Flags: review?(margaret.leibovic)
Comment on attachment 8602945 [details] [diff] [review]
822979.patch

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

::: mobile/android/base/strings.xml.in
@@ +79,5 @@
>    <string name="quit">&quit;</string>
>    <string name="bookmark">&bookmark;</string>
>    <string name="bookmark_remove">&bookmark_remove;</string>
>    <string name="bookmark_added">&bookmark_added;</string>
> +  <string name="bookmark_already_added">&overlay_share_bookmark_btn_label_already;</string>

This still doesn't address the issue... you need to make a new entity in android_strings.dtd

The core problem is that yes, in English, using the same words in these two places makes sense, but they are displayed in a different context (one is a button label, and one is a toast message), so we need localizers to understand the context in which these two string will be displayed.

So, if you go the route of creating two separate strings, you need two separate entities. And in either case, you should update the localization notes to explain where these strings are used:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#102
Attached patch 822979.patchSplinter Review
A new patch with localization notes updated.
Attachment #8602945 - Attachment is obsolete: true
Attachment #8602945 - Flags: review?(margaret.leibovic)
Attachment #8603031 - Flags: review?(margaret.leibovic)
Comment on attachment 8603031 [details] [diff] [review]
822979.patch

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

Nice. Thanks for being patient with my reviews!
Attachment #8603031 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
Please provide a Try link for this patch.
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/c58e69b54333
Whiteboard: [lang=java] → [lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c58e69b54333
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][fixed-in-fx-team] → [lang=java]
Target Milestone: --- → Firefox 41
Release Note Request (optional, but appreciated)
[Why is this notable]: interesting feature
[Suggested wording]: Duplicate bookmark detection
[Links (documentation, blog post, etc)]:

Softvison this should be tested.
relnote-firefox: --- → ?
Flags: needinfo?(fennec)
Tested with:
Device: Samsung S5 (Android 4.4.4)
Build: Firefox for Android 41.0a1 (2015-05-27)

Trying to add twice the same link as a bookmark, the "Already bookmarked" message appears
Flags: needinfo?(fennec)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.