Duplicate bookmark should be announced by a pop-up

RESOLVED FIXED in Firefox 41

Status

()

Firefox for Android
General
--
enhancement
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Teodora Vermesan, Assigned: vivek, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 41
ARM
Android
Points:
---

Firefox Tracking Flags

(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+)

Details

(Whiteboard: [lang=java])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: Firefox 19 → Trunk

Updated

4 years ago
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
status-firefox30: --- → affected

Updated

4 years ago
status-firefox31: --- → affected

Updated

4 years ago
status-firefox32: --- → affected

Updated

4 years ago
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox39: --- → affected
status-firefox40: --- → affected
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)

Comment 2

3 years ago
(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@gmail.com
Flags: needinfo?(margaret.leibovic) → needinfo?(vivekb.balakrishnan)
Whiteboard: [lang=java]
(Assignee)

Comment 3

3 years ago
Thanks Margaret,
I'll take it.
Assignee: nobody → vivekb.balakrishnan
Flags: needinfo?(vivekb.balakrishnan)
(Assignee)

Comment 4

3 years ago
Created attachment 8602218 [details] [diff] [review]
822979.patch

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)
(Assignee)

Comment 5

3 years ago
Created attachment 8602333 [details] [diff] [review]
822979.patch

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 6

3 years ago
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)
(Assignee)

Comment 8

3 years ago
Created attachment 8602864 [details] [diff] [review]
822979.patch

Addressed Review comments. Added check for deleted bookmark.
Attachment #8602333 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8602864 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 9

3 years ago
> @@ +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 10

3 years ago
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+
(Assignee)

Comment 11

3 years ago
Created attachment 8602945 [details] [diff] [review]
822979.patch

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 12

3 years ago
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
(Assignee)

Comment 13

3 years ago
Created attachment 8603031 [details] [diff] [review]
822979.patch

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 14

3 years ago
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+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Please provide a Try link for this patch.
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
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
Last Resolved: 3 years ago
status-firefox41: --- → fixed
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)
Created attachment 8611203 [details]
Screenshot_2015-05-27-17-07-33.png

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)
status-firefox41: fixed → verified
Release note added to Firefox 41.0a2
relnote-firefox: ? → 41+
You need to log in before you can comment on or make changes to this bug.