Closed
Bug 822979
Opened 12 years ago
Closed 9 years ago
Duplicate bookmark should be announced by a pop-up
Categories
(Firefox for Android Graveyard :: General, enhancement)
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)
17.27 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
456.25 KB,
image/png
|
Details |
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•12 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: Firefox 19 → Trunk
Updated•10 years ago
|
status-firefox27:
--- → affected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
Updated•10 years ago
|
status-firefox31:
--- → affected
Updated•10 years ago
|
status-firefox32:
--- → affected
Updated•10 years ago
|
status-firefox33:
--- → affected
status-firefox34:
--- → affected
Updated•9 years ago
|
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → affected
Comment 1•9 years ago
|
||
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•9 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
Flags: needinfo?(margaret.leibovic) → needinfo?(vivekb.balakrishnan)
Whiteboard: [lang=java]
Assignee | ||
Comment 3•9 years ago
|
||
Thanks Margaret, I'll take it.
Assignee: nobody → vivekb.balakrishnan
Flags: needinfo?(vivekb.balakrishnan)
Assignee | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
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•9 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+
Comment 7•9 years ago
|
||
Just chatted with Vivek on IRC, we agree we should use the same string "Already bookmarked".
Flags: needinfo?(vivekb.balakrishnan)
Assignee | ||
Comment 8•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
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•9 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•9 years ago
|
||
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•9 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•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 16•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=925b0a7bd45c
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c58e69b54333
Keywords: checkin-needed
Comment 18•9 years ago
|
||
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
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=java][fixed-in-fx-team] → [lang=java]
Target Milestone: --- → Firefox 41
Comment 20•9 years ago
|
||
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)
Comment 21•9 years ago
|
||
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)
Updated•9 years ago
|
Release note added to Firefox 41.0a2
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•