1232439 - Full-page edit bookmark dialog

Anthony's mockup has a "remove" icon in the actionbar, which is handy. If we are going to use the actionbar, maybe we could add "add to desktop" there too? In the future, we could add support for saving offline.

Richard Newman [:rnewman]

Comment 4

•

9 years ago

Desktop lets you add tags (which is a structured list of tags), not keywords. There is a 'keyword', which is used for search engine dispatch, but it can only be accessed in the Show All Bookmarks UI.

Anthony Lam (:antlam) Find me, @antlam on Slack!

Comment 5

•

9 years ago

•

9 years ago

(In reply to Anthony Lam (:antlam) from comment #12) > (In reply to Andrzej Hunt :ahunt from comment #11) > > One remaining issue: what do we do if the dialog content is unusable? In the > > current dialog we disable the save button if either (1) the bookmark title > > is empty or (2) the keyword is mangled (i.e. has leading or trailing spaces). > > > > Would it be useful to show a dialog containing something like this in those > > cases? > > > > "Please insert a bookmark title" (or "The keyword cannot contain leading or > > trailing spaces") > > |Return to bookmark| |Discard Changes| > > > > We can also show an Error icon/text as soon as the user removes the bookmark > > title / enters an invalid keyword (examples visible at > > https://www.google.com/search?q=TextView+setError&source=lnms&tbm=isch ), it > > might be nicer to use that and just show the following instead? > > > > "Bookmark changes can't be saved" > > |Return to bookmark| |Discard Changes| > > If there's an error with any of the fields, we should show that validation > and feedback in the form UI rather than a dialog. > > I think just highlighting the underline red and adding feedback underneath > as in the guidelines [1] would be best. > > [1] > https://www.google.com/design/spec/components/text-fields.html#text-fields- > single-line-text-field To get these error messages we'll need to replace FloatingHintEditText (our homegrown implementation) with TextInputLayout from the Design Support Library, tracked in Bug 1227325. (The relevant error setting method is in http://developer.android.com/reference/android/support/design/widget/TextInputLayout.html#setError%28java.lang.CharSequence%29 )

Depends on: 1227325

Flags: needinfo?(ahunt)

Richard Newman [:rnewman]

Comment 14

•

9 years ago

(In reply to Anthony Lam (:antlam) from comment #12) > If there's an error with any of the fields, we should show that validation > and feedback in the form UI rather than a dialog. Even better, trim leading and trailing whitespace from the keyword automatically, and disable saving with a whitespace-only title.

Richard Newman [:rnewman]

Updated

•

9 years ago

Status: NEW → ASSIGNED

OS: Unspecified → Android

Hardware: Unspecified → All

Version: 35 Branch → Trunk

Andrzej Hunt :ahunt

Comment 15

•

9 years ago

Attached file MozReview Request: Bug 1232439 - Part 1: Implement removeBookmarkWithID in BrowerDB r=mcomella (obsolete) — Details

Review commit: https://reviewboard.mozilla.org/r/30829/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30829/

Andrzej Hunt :ahunt

Comment 16

•

9 years ago

(In reply to Richard Newman [:rnewman] from comment #14) > (In reply to Anthony Lam (:antlam) from comment #12) > > > If there's an error with any of the fields, we should show that validation > > and feedback in the form UI rather than a dialog. > > Even better, trim leading and trailing whitespace from the keyword > automatically, and disable saving with a whitespace-only title. Looking at the the existing implementation: we already trim whitespace from the keyword (however sincew we disable the save button when that happens, so the trimming isn't actually of any use there) - so all that's needed is to remove the validation. I think it might be better to trim whitespace from the title when saving too - in this case, if the user enters a purely whitespace title, we can just act as if there's no title at all (which the UI in the rest of the app seems to handle without issues), rather than having to specially handle this case?

Andrzej Hunt :ahunt

•

9 years ago

Comment on attachment 8717560 [details] MozReview Request: Bug 1232439 - Part 2: convert edit bookmark dialog into fullscreen fragment r=mcomella Cancelling review for now - I've just realised there were some more changes / simplifications I'd forgotten to do.

Attachment #8717560 - Flags: review?(michael.l.comella)

Andrzej Hunt :ahunt

•

9 years ago

(In reply to Michael Comella (:mcomella) from comment #24) > Also, if we don't plan to recompose this, we could make this an Activity, > rather than a Fragment, which simplifies things (e.g. there's an overdraw > issue here). > I've investigated with turning this into an activity: one issue is that we're creating snackbars when shutting down the "dialog", they attach to the current activity, and get killed when activity dies - which means that we can't actually show these snackbars. We'd have to pass in the parent activity to attach the Snackbar there, or move our snackbar code into the calling activity (or provide some callback mechanism for this - which seems cleanest). However we _might_ want to reuse this dialog for creating/editing new bookmark folders (we have many of the same fields, i.e. name and bookmark folder), in this case keeping it as a fragment seems to simplify the flow. I.e. we'll need to support the following: 1. Edit bookmark -> opens edit bookmark "dialog" 2. Modify folder -> opens folder selector "dialog" (also being implemented as a fragment, that could change) 3. Create a new folder from the folder selector -> opens new folder dialog (i.e. the edit bookmark dialog with only 2 fields being shown) (4. Modify parent folder of this new folder -> open another folder selector.) That however doesn't stop us from creating a wrapper Activity, which then loads the EditBookmark fragment by default, and we can add the callbacks so that the parent activity can show snackbars when the bookmark is edited (but not show snackbars when we create the folder)?

Andrzej Hunt :ahunt

Comment 26

•

8 years ago

I'm not actively looking into this anymore. I do however have some WIP patches that will be uploaded to Bug 972193. If anyone is interested in taking this up again I'd reccomend looking there, especially since whatever architecture we choose for a full-screen dialog impacts how bookmark folder selection can be implemented. (I.e. it seems to be useful to have one activity deal with all bookmark related tasks, but we have multiple UIs that need to be swapped in/out for actual bookmark editing, followed by folder selection, and if needed folder creation/editing.)

Assignee: ahunt → nobody

Status: ASSIGNED → NEW

Comment hidden (mozreview-request)

Tom Klein

Comment 36

•

8 years ago

This isn't ready for review yet, but I think it's ready for feedback. Could you take a look when you can Andrzej? The version here implements this bug and bug 972193. The commits are oriented toward fixing both bugs at once, but if the approach looks good then I can pull them apart and can land them separately if that's better. I used DialogFragments for each of the edit screens. I think the code is in pretty good shape, in spite of the following long list of known issues and questions: 1) The main functional issue with this version is that for the text fields that jump you to a new folder selection dialog, you have to tap twice: once to focus the field, and then again to open the new dialog. (I expect I'll have to intercept or forward a click event to fix that, unless somebody has a better way.) 2) In the "Choose folder" dialog, the "New folder" item should scroll along with the folder tree items, but doesn't yet. (I hoped that was fixable by throwing everything in a ScrollView, setting layout_height="wrap_content" on the RecyclerView list, and then setting setNestedScrollingEnabled(false), but I haven't gotten that to work yet. Again I'd be happy for any thoughts on that :-) 3) The icons will need work - I just grabbed what I could find. I could use some input on that, and the rest of the UI as it stands, if/when it's ready for that. One note: I removed the "bin" option from the top right of the "Edit bookmark" dialog and replaced it, on all panels that use that action, with a checkmark used to save the input. Previously the back arrow (on the left of the toolbar) was used to save/confirm the "Edit bookmark" dialog, but on all the other panels the back arrow is used to cancel a particular panel, so for consistency's sake I think they should all work the same. There's a "remove" option on the menu you get by long-pressing a bookmark, so a bookmark can be deleted there instead. (The option doesn't currently exist in the menu you get if you star a page and then click on the "Options" action in the resulting snackbar, but it seems like it should be added there as well.) Or maybe there's some other good way to add it to the current "Edit bookmark" dialog panel? 5) As Andrzej points out in the code and Richard has mentioned elsewhere, if there are multiple bookmarks with the same url, there's no telling which one you'll wind up editing, and the user is never notified of that fact. 6) It seems the "Desktop" folders exist (currently unexposed I guess) even if you've never synced, and with the new dialog they show up as folders you can use for bookmarks. Is that worth worrying over? 7) It's not allowed to save a bookmark, or create a new folder, in the "Desktop" folder - is that correct? That's the way it's implemented here. 8) I should add some tests for the new folder db capabilities. Others? (I haven't looked at what kind of robocop tests we currently have yet.) And then some cleanup issues remaining: 9) All of the new strings, dimensions, etc. still need to be moved into resources. 10) I've added a number of new classes associated with the bookmarks editing dialog - should we create a new bookmarks package? Eventually we'll want to be able to change folder names and move them etc. as well, so there will probably be more to add then. 11) Should all ids for database entries be longs in java? 12) The "Create folder" dialog doesn't have visible input validation yet (though it already checks behind the scenes). 13) As Michael pointed out, maybe we should adjust the layout on tablets? 14) The padding on the folder trees is missing in an api 16 emulator; I haven't looked into that yet. 15) There are various other more or less minor TODOs in the code.

Flags: needinfo?(ahunt)

Comment hidden (mozreview-request)

•

8 years ago

Amazing! Thanks for the feedback Tom. This is coming along nicely! I'm going to NI Jack and Tori here too since they've been working on some of the Bookmarks Management UX

•

8 years ago

Attachment #8817952 - Attachment is obsolete: true

Jing-wei Wu [:jwu]

Assignee

Updated

•

8 years ago

Assignee: nobody → topwu.tw

Wesley Huang [:wesley_huang] (EPM) (NI me)

Updated

•

8 years ago

Priority: -- → P1

Comment hidden (mozreview-request)

Jing-wei Wu [:jwu]

Assignee

Comment 64

•

8 years ago

To enable full-page dialog, "ui.bookmark.mobilefolder.enabled" in about:config needs to be turn on.

Jing-wei Wu [:jwu]

Assignee

Updated

•

8 years ago

Blocks: 1329128

Jing-wei Wu [:jwu]

Assignee

•

8 years ago

Attachment #8847887 - Flags: review?(ahunt)

:Grisha Kruglov

Comment 70

•

8 years ago

Sebastian, could you please help provide some guidance on setting up a switchboard experiment on the kinto side of things? Management url in https://wiki.mozilla.org/Mobile/Fennec/Android/Downloadable_Content doesn't seem to work for me (perhaps Bug 1313038 is helpful).

Flags: needinfo?(s.kaspari)

:Grisha Kruglov

Comment 71

•

8 years ago

mozreview-review

Comment on attachment 8847889 [details] Bug 1232439 - Part 3: Implement full-page bookmark edit dialog. https://reviewboard.mozilla.org/r/120812/#review125174 Quick first pass, I'll do another one separately. Please `histedit` your patches before resubmitting. ::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkCacheHelper.java:43 (Diff revision 2) > - * }) > - * .duration(Snackbar.LENGTH_LONG) > - * .buildAndShow(); > - * </pre> > */ > public class BookmarkCacheHelper { Please roll these changes into your "Part 2" patch. Otherwise I have to review two different versions of this code :/ ::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkCacheHelper.java:43 (Diff revision 2) > - * }) > - * .duration(Snackbar.LENGTH_LONG) > - * .buildAndShow(); > - * </pre> > */ > public class BookmarkCacheHelper { This doesn't feel right to me. There are two major things going on here. 2) 1) You have conflated a simple thing, a cache, with a complicated thing - collection of functions to modify bookmarks, folders, etc in live storage. Keep your cache very simple, and move db operations to the data layer. For these data types, we have `BrowserProvider` and `LocalBrowserProvider`, which handle operations on the database - deletions, removals, etc. `BrowserProvider` handles various compound operations, and provides you with ways to perform queries transactionally. 2) You have implemented update/remove operations in a very unsafe manner. You should be wrapping these in transactions, and performing them atomically. For a very trivial case of why this is bad, consider what will happen if you kill fennec while you're trying to delete a deeply nested folder structure. ::: mobile/android/base/java/org/mozilla/gecko/bookmark/EditBookmarkTask.java:1 (Diff revision 2) > +package org.mozilla.gecko.bookmark; license ::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:61 (Diff revision 2) > * HomeFragment is an empty fragment that can be added to the HomePager. > * Subclasses can add their own views. > * <p> > * The containing activity <b>must</b> implement {@link OnUrlOpenListener}. > */ > -public abstract class HomeFragment extends Fragment { > +public abstract class HomeFragment extends Fragment implements BookmarkEditFragment.Callbacks { Is there a good reason why an abstract base class used by unrelated UI (classic top sites, activity stream, etc) is implementing bookmark-related callbacks? This doesn't seem right to me. ::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:310 (Diff revision 2) > > return true; > } > > if (itemId == R.id.home_edit_bookmark) { > + if (mBookmarkPrefs.isEnabled()) { See my comments on this from Part 1. ::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:410 (Diff revision 2) > > load(); > mIsLoaded = true; > } > > - protected static class RemoveItemByUrlTask extends UIAsyncTask.WithoutParams<Void> { > + class RemoveItemByUrlTask extends UIAsyncTask.WithoutParams<Void> { Avoid this unless there's a very good reason. If you need to access something in the wrapping class, just pass it in. Otherwise there's a good chance you're going to leak HomeFragment instance here. ::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:467 (Diff revision 2) > > @Override > public void onPostExecute(Void result) { > + // Currently only support restore bookmark. > + if (mType == RemoveItemType.BOOKMARKS) { > + SnackbarBuilder.builder((Activity) mContext) This is duplicated almost verbatim in EditBookmarkTask. What's going on here? ::: mobile/android/base/locales/en-US/android_strings.dtd:91 (Diff revision 2) >  > <!ENTITY bookmark_folder_items "&formatD; items"> > <!ENTITY bookmark_folder_one_item "1 item"> > +<!ENTITY bookmark_parent_folder "Parent Folder"> > +<!ENTITY bookmark_undo "UNDO"> Why all caps? No other string is in all caps.

Attachment #8847889 - Flags: review?(gkruglov) → review-

:Grisha Kruglov

Comment 72

•

8 years ago

mozreview-review

Comment on attachment 8847888 [details] Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB. https://reviewboard.mozilla.org/r/120810/#review125178 Please see my comments on your other patch, which changes a lot of this code. As you find yourself refactoring your work, it's a very good idea to go through your patch series and move things around. Changes in Part 3 _should_ be part of this patch. This kind of re-shuffling should be pretty easy via `hg histedit`. ::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkCacheHelper.java:52 (Diff revision 2) > + * }) > + * .duration(Snackbar.LENGTH_LONG) > + * .buildAndShow(); > + * </pre> > + */ > +public class BookmarkCacheHelper { I don't think this class should exist in its current form. Please see my comment regarding refactoring this, and moving actual editing logic into `BrowserProvider` and friends. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1205 (Diff revision 2) > addBookmarkItem(cr, title, uri, folderId); > return true; > } > > + @Override > + public boolean restoreBookmark(ContentResolver cr, String title, String url, int type, This isn't going to work well. These operations need to happen atomically. That is, they need to be wrapped in transactions. You'll likely need to move this logic to BrowserProvider.

Attachment #8847888 - Flags: review?(gkruglov) → review-

Jing-wei Wu [:jwu]

Assignee

•

8 years ago

Richard, NI as a sanity check. Does Comment 82 sound reasonable to you? I can't think of a sane, incremental way to do this sort of a thing (rollback changes to bookmarks) correctly.

Flags: needinfo?(rnewman)

Richard Newman [:rnewman]

Comment 84

•

8 years ago

(In reply to :Grisha Kruglov from comment #83) > Richard, NI as a sanity check. Does Comment 82 sound reasonable to you? I > can't think of a sane, incremental way to do this sort of a thing (rollback > changes to bookmarks) correctly. I agree that implementing restore by re-adding to storage from a cache will be complex and buggy, and that's not the direction to go in; Part 2 as written should be removed. To expand on your comment, I think there are actually _two_ things to worry about. The first, as you describe, is racing data. Indeed, the backing data can change _while the editing UI is open_. Whenever possible, avoid having to correctly manage caches and UI state! (As they say, there are two hard problems in computer science: naming things, cache invalidation, and off-by-one errors.) The second thing to worry about is that a read of, or change to, the database is quite likely to trigger a sync -- Android will do this automatically. Not only does that make race conditions more likely, it also means that a deletion is likely to immediately propagate to other devices. That's a huge can of worms: the restore will trigger a sync, but the rate limiter will kick in, so the restore probably won't be uploaded for another hour. The restored data needs to be marked in order to have the correct effect on remote clients, including in the presence of conflicts -- one cannot simply restore the original database row, but marking it as changed will have an effect on conflict resolution elsewhere. Even getting this half right requires relatively deep knowledge of both BrowserProvider and Sync on all three platforms. Getting it 100% right would involve quite a bit of time on a whiteboard, if it's even possible, and I would expect to see a thousand lines of tests to make sure it behaves as expected. There are two ways in which undo could be meaningfully implemented. One is to do it by flipping a flag in the DB, and only exposing the UI to users without Sync configured. Don't bother with any of the fancy cache stuff. The other, more complete way is the way email clients offer an "oops!" button to undo a send: hitting Send doesn't actually send the email, it just looks like it does. The send happens later, when there's no chance that you've changed your mind. If deletion is rephrased as "queue for deletion and pretend it already happened", and this simple queue processed at various points (when the undo UI disappears, or when a sync begins), then it's possible to support something that _looks like_ undo without the data model actually needing to support it. If this is a feature that we think needs to ship, then that's the way I'd build it.

Flags: needinfo?(rnewman)

:Grisha Kruglov

Comment 85

•

8 years ago

Hi Wesley, Do you think we can ship this feature without "undo" functionality? See Comment 82 and Comment 84. That will significantly simplify things and lower risk around shipping this.

Flags: needinfo?(whuang)

:Grisha Kruglov

Comment 86

•

8 years ago

Same question for product, Joe - please see the few comments above.

Flags: needinfo?(jcheng)

Wesley Huang [:wesley_huang] (EPM) (NI me)

Comment 87

•

8 years ago

(In reply to :Grisha Kruglov from comment #85) > Hi Wesley, > > Do you think we can ship this feature without "undo" functionality? See > Comment 82 and Comment 84. That will significantly simplify things and lower > risk around shipping this. I checked with Tori on the UX perspective and Joe as well. As long as we have the follow-up bug to add that "undo" function, we're fine to proceed with the plan. I think that follow-up bug depends on Bug 1343985 so it makes sense to have it land a bit later. That said, bookmark management enhancement in 56 could have no "undo". Let's say adding "undo" in 57.

Flags: needinfo?(whuang)

Flags: needinfo?(jcheng)

Wesley Huang [:wesley_huang] (EPM) (NI me)

•

8 years ago

Assignee

Updated

•

8 years ago

Attachment #8847888 - Attachment is obsolete: true

Attachment #8847888 - Flags: review?(gkruglov)

Jing-wei Wu [:jwu]

•

8 years ago

mozreview-review

Comment on attachment 8858999 [details] Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly. https://reviewboard.mozilla.org/r/131010/#review133978 ::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkUtils.java:6 (Diff revision 1) > +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +package org.mozilla.gecko.bookmark; Name this `.bookmarks`.

Attachment #8858999 - Flags: review?(gkruglov) → review+

Comment hidden (mozreview-request)

Jing-wei Wu [:jwu]

Assignee

Comment 121

•

8 years ago

mozreview-review-reply

Comment on attachment 8859000 [details] Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB. https://reviewboard.mozilla.org/r/131012/#review133928 > Generally I think I'm fine with moving this logic here. > > It's important you don't perform these updates if insert is happening from sync. Sync will handle timestamps on its own, and so this is only necessary for updates from within fennec itself. > > However, beware that LocalBrowserDB's `addBookmarkItem` already does this! And so, you will need to unite the two. This could be as simple as removing LocalBrowserDB's bits, adding comments that the underlying content provider will handle timestamp trickery. Sync will handle timestamps on its own for all db operations(insert/update/delete), right? I'll use `isCallerSync()` to check if it's triggered from sync. https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/SQLiteBridgeContentProvider.java#113

Jing-wei Wu [:jwu]

Assignee

Comment 122

•

8 years ago

mozreview-review-reply

Comment on attachment 8847889 [details] Bug 1232439 - Part 3: Implement full-page bookmark edit dialog. https://reviewboard.mozilla.org/r/120812/#review132270 > Prefer immutability whenever possible. Ensure you have good reasons for modifying objects like this. These variables are mutable. When onSaveInstanceState() is triggered, we fetch the UI changes and save into them. When fragment is re-created, we can use these values to restore UI. > make sure there is no chance that the new parent id is the same as the old parent id. It will cause _a lot_ of unnecessary work - db writes, more to do during a sync, more to do on other clients you're syncing with, etc - if you get this wrong. > > Generally any screw ups that you might get away with locally are amplified ten fold when you start syncing their consequences and other clients start to act upon them. So beware. I leave the check in BookmarkEditFragment.java ``` if (bookmark.parentId != bookmark.originalParentId) { bundle.putLong(Bookmarks.PARENT, bookmark.parentId); } ``` In EditBookamrk.java, it doesn't update parent field in database if the bundle don't have the key 'PARENT'.

:Grisha Kruglov

Comment 123

•

8 years ago

mozreview-review

Comment on attachment 8859000 [details] Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB. https://reviewboard.mozilla.org/r/131012/#review134446 One important thing to change, and various nits around code itself. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1570 (Diff revision 2) > + > + // Bump parent's lastModified timestamp. > + final long lastModified = values.getAsLong(Bookmarks.DATE_MODIFIED); > + final ContentValues parentValues = new ContentValues(); > + parentValues.put(Bookmarks.DATE_MODIFIED, lastModified); > + final long parentId = values.getAsLong(Bookmarks.PARENT); We can also be inserting a root folder without a parent, right? In which case there wouldn't be a parentId, and so there wouldn't be anything to do here. Make sure your unit tests handle these variations (root vs non-root). ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1623 (Diff revision 2) > cursor.close(); > } > > beginWrite(db); > - return db.update(TABLE_BOOKMARKS, values, inClause, null); > + > + final boolean isCallerSync = isCallerSync(uri); Add a comment describing why what you're doing below is necessary. Explain that this is needed so that during a sync we know to upload folders whose lists of children changed. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1625 (Diff revision 2) > > beginWrite(db); > - return db.update(TABLE_BOOKMARKS, values, inClause, null); > + > + final boolean isCallerSync = isCallerSync(uri); > + final boolean isParentChanged = isBookmarkParentChanged(uri); > + final long now = values.getAsLong(Bookmarks.DATE_MODIFIED); `now` is a bad name for this. Perhaps `newLastModified`? ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1627 (Diff revision 2) > - return db.update(TABLE_BOOKMARKS, values, inClause, null); > + > + final boolean isCallerSync = isCallerSync(uri); > + final boolean isParentChanged = isBookmarkParentChanged(uri); > + final long now = values.getAsLong(Bookmarks.DATE_MODIFIED); > + > + // Bump old parent's lastModified timestamp before record updated. I don't think the order here matters. The end result is that lastModified timestamps for both the new parent, old parent and the child will be the same after your updates. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1629 (Diff revision 2) > + final boolean isParentChanged = isBookmarkParentChanged(uri); > + final long now = values.getAsLong(Bookmarks.DATE_MODIFIED); > + > + // Bump old parent's lastModified timestamp before record updated. > + if (!isCallerSync && isParentChanged) { > + final ContentValues parentValues = new ContentValues(); `oldParentValues` ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1636 (Diff revision 2) > + updateBookmarkParents(db, parentValues, inClause, null); > + } > + > + final int updated = db.update(TABLE_BOOKMARKS, values, inClause, null); > + if (updated == 0) { > + trace("No update on URI: " + uri); If nothing changed, for some reason, you don't want to bump parent. Re-order this. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1641 (Diff revision 2) > + trace("No update on URI: " + uri); > + return updated; > + } > + > + // Bump new parent's lastModified timestamp. > + if (!isCallerSync && isParentChanged) { After you re-order the above code, you should be able to do an early return on `isCallerSync`, and unite the two if blocks. This will make the function much more readable at a glance. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1642 (Diff revision 2) > + return updated; > + } > + > + // Bump new parent's lastModified timestamp. > + if (!isCallerSync && isParentChanged) { > + final ContentValues parentValues = new ContentValues(); `newParentValues` ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1645 (Diff revision 2) > + // Bump new parent's lastModified timestamp. > + if (!isCallerSync && isParentChanged) { > + final ContentValues parentValues = new ContentValues(); > + parentValues.put(Bookmarks.DATE_MODIFIED, now); > + > + final long parentId = values.getAsLong(Bookmarks.PARENT); call it `newParentId` to be super obvious. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java (Diff revision 2) > @RobocopTarget > public void removeBookmarksWithURL(ContentResolver cr, String uri) { > - Uri contentUri = mBookmarksUriWithProfile; > - > - // Do this now so that the items still exist! > - bumpParents(cr, Bookmarks.URL, uri); I think it should be possible to remove `bumpParents` as well now, and remove the parentsUri interface on the BrowserProvider, if no one else is using it anymore. If we're moving this bumping logic to internals of BrowserProvider, might as well go all the way if possible. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1230 (Diff revision 2) > - bumpParents(cr, Bookmarks.URL, uri); > - > final String[] urlArgs = new String[] { uri, String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) }; > final String urlEquals = Bookmarks.URL + " = ? AND " + Bookmarks.PARENT + " != ? "; > > - cr.delete(contentUri, urlEquals, urlArgs); > + cr.delete(mBookmarksUriWithProfile, urlEquals, urlArgs); Be consistent in your two delete remove methods. They look different at a glance but largely do the same thing. I'd just drop these variables.

Attachment #8859000 - Flags: review?(gkruglov) → review-

Jing-wei Wu [:jwu]

Assignee

Comment 124

•

8 years ago

mozreview-review-reply

Comment on attachment 8859000 [details] Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB. https://reviewboard.mozilla.org/r/131012/#review134446 > We can also be inserting a root folder without a parent, right? In which case there wouldn't be a parentId, and so there wouldn't be anything to do here. > > Make sure your unit tests handle these variations (root vs non-root). I don't think we can insert a folder without a parent. 1. To create a folder, the API `addBookmarkFolder` in BrowserDB.java needs caller to provide parent id. 2. To create a bookmark, it's always insert to root parent whose guid is `Bookmarks.MOBILE_FOLDER_GUID`. > I don't think the order here matters. The end result is that lastModified timestamps for both the new parent, old parent and the child will be the same after your updates. Since the ContentValues only has new parent id, I'll do a little refactor that put old parent id into the uri's query parameter. Then we can update lastModified timestamps for both new parent and old parent at once. > I think it should be possible to remove `bumpParents` as well now, and remove the parentsUri interface on the BrowserProvider, if no one else is using it anymore. > > If we're moving this bumping logic to internals of BrowserProvider, might as well go all the way if possible. I'll remove `bumpParents` since Android Studio said no one calls this method. But for removing parentsUri interface, maybe we should file another bug for refactoring(with some checks and test cases) since this is a litte out of scope?

Comment hidden (mozreview-request)

:Grisha Kruglov

Comment 129

•

8 years ago

mozreview-review

Comment on attachment 8859000 [details] Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB. https://reviewboard.mozilla.org/r/131012/#review135042 Looks good. Make sure that our non-unit tests all pass, and please test this extensively. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1567 (Diff revision 3) > + > + // Bump parent's lastModified timestamp. > + final long lastModified = values.getAsLong(Bookmarks.DATE_MODIFIED); > + final ContentValues parentValues = new ContentValues(); > + parentValues.put(Bookmarks.DATE_MODIFIED, lastModified); > + final long parentId = values.getAsLong(Bookmarks.PARENT); Add a comment here explaining why values will always have a parent, and why you don't need an explicit check. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1568 (Diff revision 3) > + // Bump parent's lastModified timestamp. > + final long lastModified = values.getAsLong(Bookmarks.DATE_MODIFIED); > + final ContentValues parentValues = new ContentValues(); > + parentValues.put(Bookmarks.DATE_MODIFIED, lastModified); > + final long parentId = values.getAsLong(Bookmarks.PARENT); > + db.update(TABLE_BOOKMARKS, parentValues, Bookmarks._ID + " = ?", new String[] { String.valueOf(parentId) }); nit: break this into multiple lines ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1163 (Diff revision 3) > values, > Bookmarks.URL + " = ? AND " + > Bookmarks.PARENT + " = " + folderId, > new String[] { uri }); > > - // Bump parent modified time using its ID. > + // BrowserProvider will handle updating parent's lastModified timestamp, nothing need to do here. "nothing else to do." ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1207 (Diff revision 3) > + values.put(Bookmarks.DATE_MODIFIED, now); > + values.put(Bookmarks.GUID, Utils.generateGuid()); > + values.put(Bookmarks.PARENT, parentId); > + values.put(Bookmarks.TITLE, title); > + values.put(Bookmarks.TYPE, Bookmarks.TYPE_FOLDER); > + return cr.insert(mBookmarksUriWithProfile, values); Add a comment mentioning that BrowserProvider will bump parent here as well. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1213 (Diff revision 3) > + } > + > + @Override > @RobocopTarget > public void removeBookmarksWithURL(ContentResolver cr, String uri) { > - Uri contentUri = mBookmarksUriWithProfile; > + cr.delete(mBookmarksUriWithProfile, Add a comment mentioning that BrowserProvider will bump parents. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1213 (Diff revision 3) > + } > + > + @Override > @RobocopTarget > public void removeBookmarksWithURL(ContentResolver cr, String uri) { > - Uri contentUri = mBookmarksUriWithProfile; > + cr.delete(mBookmarksUriWithProfile, Note that you did subtly change the behaviour here. Specifically the selection criteria for bumping parents are different. Which means we won't bump l-m timestamp of the special FIXED_PINNED_LIST folder. Which is probably more correct than the previous behaviour - when we did bump it, but wouldn't delete the record itself if it turned out to be a pin. I don't think we sync that folder anyway, so I'm not sure this matters anyway, but make sure this won't regress behaviour of pinned sites on some weird way. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1220 (Diff revision 3) > - final String[] urlArgs = new String[] { uri, String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) }; > - final String urlEquals = Bookmarks.URL + " = ? AND " + Bookmarks.PARENT + " != ? "; > > - cr.delete(contentUri, urlEquals, urlArgs); > + @Override > + public void removeBookmarkWithId(ContentResolver cr, long id) { > + cr.delete(mBookmarksUriWithProfile, Same comment about bumping parents ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1246 (Diff revision 3) > Bookmarks._ID + " = ?", > new String[] { String.valueOf(id) }); > } > > @Override > + public void updateBookmark(ContentResolver cr, long id, String uri, String title, String keyword, Just make sure this API isn't awkward to use from the other end. It's a good thing you're writing a consumer for this as well. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1255 (Diff revision 3) > + values.put(Bookmarks.URL, uri); > + values.put(Bookmarks.KEYWORD, keyword); > + values.put(Bookmarks.PARENT, newParentId); > + values.put(Bookmarks.DATE_MODIFIED, System.currentTimeMillis()); > + > + final Uri contentUri = mBookmarksUriWithProfile.buildUpon() You could also pass it via contentvalues above, but this works as well.

Attachment #8859000 - Flags: review?(gkruglov) → review+

Comment hidden (mozreview-request)

Jing-wei Wu [:jwu]

Assignee

Comment 134

•

8 years ago

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe3262ac57de5584bae5d08edd20945812c1687c Btw the job name 'android-api-15-frontend' should rename to 'android-test'. https://bugzilla.mozilla.org/show_bug.cgi?id=1260874

Jing-wei Wu [:jwu]

Assignee

Updated

•

•

7 years ago

Clear this needinfo since ux spec is delivered.

Flags: needinfo?(tchen)

Jan Henning [:JanH]

Updated

•

7 years ago

Depends on: 1390140

Jan Henning [:JanH]

Updated

•

7 years ago

Depends on: 1405215

Jan Henning [:JanH]

Updated

•

7 years ago

Depends on: 1406410

Nobody; OK to take it and work on it

Updated

•

4 years ago

Product: Firefox for Android → Firefox for Android Graveyard

prev_bookmarks_bookmarkmgmt_L2_4.png 9 years ago Anthony Lam (:antlam) Find me, @antlam on Slack! 208.58 KB, image/png		Details
fennec-edit-bookmark-native.png 9 years ago Mark Finkle (:mfinkle) (use needinfo?) 65.15 KB, image/png		Details
MozReview Request: Bug 1232439 - Part 1: Implement removeBookmarkWithID in BrowerDB r=mcomella 9 years ago Andrzej Hunt :ahunt 58 bytes, text/x-review-board-request	mcomella : review+	Details
WIP draft patch screenshot 9 years ago Andrzej Hunt :ahunt 74.08 KB, image/png		Details
MozReview Request: Bug 1232439 - Part 2: convert edit bookmark dialog into fullscreen fragment r=mcomella 9 years ago Andrzej Hunt :ahunt 58 bytes, text/x-review-board-request		Details
Bug 1232439 - Part 1: Implement removeBookmarkWithID in BrowserDB 8 years ago Tom Klein 58 bytes, text/x-review-board-request		Details
Bug 1232439 - Part 2: convert edit bookmark dialog into fullscreen fragment 8 years ago Tom Klein 58 bytes, text/x-review-board-request		Details
[WIP] Bug 1232439 (972193) - Part 1: introduce BrowserDB.getBookmarkFolders 8 years ago Tom Klein 58 bytes, text/x-review-board-request		Details
[WIP] Bug 1232439 (972193) - Add more database folder methods for bookmark edit dialogs. 8 years ago Tom Klein 58 bytes, text/x-review-board-request		Details
[WIP] Bug 1232439 (972193) - Add FolderTreeView for choosing a folder from a list. 8 years ago Tom Klein 58 bytes, text/x-review-board-request		Details
[WIP] Bug 1232439 (972193) - Create base classes for use in bookmark edit dialogs. 8 years ago Tom Klein 58 bytes, text/x-review-board-request		Details
[WIP] Bug 1232439 (972193) - Add "Choose Folder" bookmark edit dialog option. 8 years ago Tom Klein 58 bytes, text/x-review-board-request		Details
[WIP] Bug 1232439 (972193) - Add "Create folder" bookmark edit dialog option. 8 years ago Tom Klein 58 bytes, text/x-review-board-request		Details
[WIP] Bug 1232439 (972193) - Add dialog for selecting folder in which to place new bookmarks folder. 8 years ago Tom Klein 58 bytes, text/x-review-board-request		Details
[WIP] Bug 1232439 (972193) - Only include desktop folder if a sync has occurred. 8 years ago Tom Klein 59 bytes, text/x-review-board-request		Details
Edit bookmark dialogs screenshots 8 years ago Tom Klein 230.29 KB, image/png		Details
Whitespace folder name handling 8 years ago Tom Klein 229.69 KB, image/png		Details
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly. 8 years ago Jing-wei Wu [:jwu] 59 bytes, text/x-review-board-request	Grisha : review+	Details
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB. 8 years ago Jing-wei Wu [:jwu] 59 bytes, text/x-review-board-request		Details
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog. 8 years ago Jing-wei Wu [:jwu] 59 bytes, text/x-review-board-request	Grisha : review+ ahunt : review+	Details
Bug 1232439 - Part 4: Adjust bookmark list UI layout. 8 years ago Jing-wei Wu [:jwu] 59 bytes, text/x-review-board-request	ahunt : review+	Details
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly. 8 years ago Jing-wei Wu [:jwu] 59 bytes, text/x-review-board-request	Grisha : review+	Details
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB. 8 years ago Jing-wei Wu [:jwu] 59 bytes, text/x-review-board-request	Grisha : review+	Details