Closed Bug 1391166 Opened 7 years ago Closed 7 years ago

Reordering of Bookmarks via Drag&Drop is incorrect

Categories

(Firefox :: Bookmarks & History, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- fixed
firefox58 --- verified

People

(Reporter: alice0775, Assigned: standard8)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(1 file)

@dsmith reported a bug, see http://forums.mozillazine.org/viewtopic.php?p=14761577#p14761577 I'm seeing something strange in bookmarks ordering. Reproducible: always Steps to Reproduce: Given bookmarks (in folder on the bookmarks toolbar): A B C If I drag A *down* to be above C, (Expected Results) I expect to get: B A C (Actual Results) But instead I get: B C A However I can drag A back *up* the list and it will work. For example, moving A above C properly gives me: B A C 並び替えは正しくない結果になる Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0da00124af43d44fed96133300ba5e32b0d821a8&tochange=0a4690dfd7b383e2f732210cf8906ce51a5b2433 Regressed by:0a4690dfd7b3 Mark Banner — Bug 1071513 - Enable async PlacesTransactions for nightly builds. r=mak
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee: nobody → standard8
The patch adds in a bit of code that is handled in the old transactions but isn't in the new ones: http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/toolkit/components/places/nsNavBookmarks.cpp#1386
IIRC the reason the new code doesn't act like the old one, is that I didn't want to call update with index: 1 and have the bookmark end up at index: 2. Basically, I wanted the final situation to respect what was passed to update(). That means it's the consumer that must do its own calculations. What do you think?
well, actually it was the opposite, update with index: 2 and the bookmark ends up at index 1.
Status: NEW → ASSIGNED
Comment on attachment 8900222 [details] Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list. I'm happy with the other approach of making the caller change. I was kinda split and went with one, but I'm happy to swap this over, the logic makes sense.
Attachment #8900222 - Flags: review?(mak77)
Comment on attachment 8900222 [details] Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list. https://reviewboard.mozilla.org/r/171590/#review177062 There's still a problem, try draggin up a bookmark just above itself (so the drop indicator appears where it is already, we seem to still subtract 1 and move the bookmark up by 1 position, while it should not move. Though, it seems to happen also when disabling Async Transactions, so probably it's just a miscalculation of the insertion point. Worth filing a follow-up, but won't block this since it's not an async transactions regression.
Attachment #8900222 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #7) > There's still a problem, try draggin up a bookmark just above itself (so the > drop indicator appears where it is already, we seem to still subtract 1 and > move the bookmark up by 1 position, while it should not move. > Though, it seems to happen also when disabling Async Transactions, so > probably it's just a miscalculation of the insertion point. Worth filing a > follow-up, but won't block this since it's not an async transactions > regression. It turned out this was async transactions only. I've added an additional check for when the indexes are the same, we won't add the transaction onto the queue, and hence we won't do a batch operation if we don't need to.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 801900a6e2f5 -d 8cb237283ed0: rebasing 416840:801900a6e2f5 "Bug 1391166 - Fix the order of bookmarks via drag and drop when moving a bookmark down in the list. r=mak" (tip) merging browser/components/places/tests/browser/browser.ini warning: conflicts while merging browser/components/places/tests/browser/browser.ini! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7d7e8ffee135 Fix the order of bookmarks via drag and drop when moving a bookmark down in the list. r=mak
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1395784
I have reproduced this bug with Nightly 57.0a1 (2017-08-16) in Windows 10 (64-bit). This bug's fix is verified with latest Nightly 57.0a1 (64-bit). Build ID : 20170906100107 User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 [bugday 20170906]
I have reproduced this bug with Nightly 56.0a1 (2017-07-27) (64-bit) on Ubuntu 16.04 LTS! This bug's fix is Verified with latest Nightly! Build ID : 20170913100125 User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170913]
This bug is marked verified Fixed. However, it is not fixed in the latest 32 bit Nightly. Build ID: faa897d7948b7e2439573f39c34366c138913663
(In reply to Bruce from comment #16) > This bug is marked verified Fixed. However, it is not fixed in the latest > 32 bit Nightly. What are your steps to reproduce the bug? I just tried and it WFM.
Sorry I failed to indicate the str. Folder structure: Level1 ...Level2 ......Level3 Moving from any same folder level it works fine. If moving a folder or BM from Level3 to level1 or level2 it may not land where you want it.
(In reply to Bruce from comment #18) > If moving a folder or BM from Level3 to level1 or level2 it may not land > where you want it. it looks like a different bug from comment 0, please file it as a separate bug so we can handle it properly.
Depends on: 1400846
Build ID: 20171017220415 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: