Please report any other irregularities here.
94.94 KB, image/png
3.36 KB, patch
(not reading, please use firstname.lastname@example.org instead): review+
|Details | Diff | Splinter Review|
1.78 KB, patch
|Details | Diff | Splinter Review|
Created attachment 267668 [details] screenshot When the bookmarks manager is in a sorted view, and the separators are invisible, adding a new separator adds a separator below the bookmark at that position in reality, rather than the bookmark I clicked on. Steps to reproduce (looking at the screenshot): 1. Open up Bookmarks Manager and sort by title. Make sure the Bookmarks Sidebar is visible. 2. Click on a bookmark (in this case, CNN.com) under which you would like to create a separator. Expected results: A separator is created underneath the CNN.com bookmark in the sidebar. Actual results: A separator is created underneath the Google bookmark, which is the 5th bookmark from the top -- the same position CNN.com is at in the sorted view.
New bookmarks/folders are also affected by this -- It's got to be something wrong with view.insertionPoint ( http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/controller.js#979 ) and the fact that that's not set when the tree is resorted...
Christine, I agree with you about insertionPoint. See also bug #381751. Question: what does firefox 2 do in this case? Does it depend on if we are sorted ascending / descending? (I vaguely remember some old fx 2 bugs on inserting separators when sorted.)
Target Milestone: --- → Firefox 3 alpha6
In Firefox 2: Sorting bookmarks via the Bookmarks Manager also visually sorts the Bookmarks Sidebar, though the Menu does not change its ordering. Separators are not made invisible in Firefox 2, though they also don't seem to have consistent behavior -- sorting by name causes all the separators to bunch together, which makes sense, but sorting by location does not cause the bunching. Adding a separator in the BM while bookmarks are sorted causes the bookmarks to fall out of order temporarily but ultimately has the correct behavior. (e.g. adding a separator on an item starting with a "V" causes a separator to be added two indices above item "V," and for item "V" to jump above an item beginning with a "U." In the unsorted view, however, a separator is added above item "V.")
See Bug #333758
Created attachment 268447 [details] [diff] [review] patch Disables the "New Separator" command when the BM view is sorted.
Comment on attachment 268447 [details] [diff] [review] patch 1) + this._view.getResult().sortingMode == 0; instead of 0, use Ci.nsINavHistoryQueryOptions.SORT_BY_NONE 2) + result.viewer.selection.clearSelection(); for what scenario was that necessary? from a few quick tests (without your patch), it appears that we already clearing the selection when we change the sort mode.
also, I checked firefox 2 and we don't clear the selection when changing the sort mode, but firefox 2 appears to be buggy with how it restores the selection after changing the sort mode (it looks like we just reselect the previously selected rows?) some follow up items: 1) check if there is a bug logged on the buggy "selection-after-sorting" fx 2 behavior. (If not, we can log it.) 2) our current fx 3 behaviour is to clear selection (and not restore it) upon sorting. I'm not sure if that was intentional or if we just haven't gotten to it yet.
(In reply to comment #7) > 1) check if there is a bug logged on the buggy "selection-after-sorting" fx 2 > behavior. (If not, we can log it.) Bug logged -- See #384910
Created attachment 268818 [details] [diff] [review] patch
Created attachment 268859 [details] [diff] [review] patch, v3 Selects the content tree's sortingMode specifically instead of just the view - fixes case where a folder in the left panel is selected
Comment on attachment 268859 [details] [diff] [review] patch, v3 what about when we don't have placeContent, like the bookmarks sidebar? I imagine that content will be null, and we'll throw a js exception, and the separator command will not be enabled.
Created attachment 268870 [details] [diff] [review] v4 Seth's right... fixing sidebar problem in this patch - "new separator" always enabled in sidebar.
Index: controller.js =================================================================== + var content = document.getElementById('placeContent'); controller.js cannot depend on any dom node but <command> elements.
(In reply to comment #13) > Index: controller.js > =================================================================== > > + var content = document.getElementById('placeContent'); > > controller.js cannot depend on any dom node but <command> elements. > Possible solution: var view = PlacesUtils.getViewForNode(this._view.selectedNode); return this._canInsert() && this._view.peerDropTypes .indexOf(PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) != -1 && (view.id == "placesView" || view.getResult().sortingMode == Ci.nsINavHistoryQueryOptions.SORT_BY_NONE); Though it still references the direct ID... is that still not all right?
Isn't view.getResult().sortingMode == Ci.nsINavHistoryQueryOptions.SORT_BY_NONE sufficient? Other views disallow sorting...
(In reply to comment #15) > Isn't view.getResult().sortingMode == Ci.nsINavHistoryQueryOptions.SORT_BY_NONE > sufficient? Other views disallow sorting... > The left-hand pane is always unsorted, and the commandset is updated if the user clicks in the left-hand pane while the main content view is sorted - see Comment #10
Well, the reason you want "New Separator" disabled in the folders-tree is different - it hides them via excludeItem the following untested check should catch that: asQuery(this._view.getResult().root).queryOptions.excludeItems
Created attachment 268897 [details] [diff] [review] patch see Comment #17 c/o mano
Why is the sort commandupdate-event addition? Isn't a "select" event triggered either way as a result of treeview's invalidateAll?
(In reply to comment #19) > Why is the sort commandupdate-event addition? Isn't a "select" event triggered > either way as a result of treeview's invalidateAll? > No, it's not. A "select" event is triggered only when the selection is cleared as a result of a sorting change. Otherwise, if nothing in the tree is selected and the sorting mode is changed from sorted -> unsorted or vice versa, the updateCommands event isn't triggered.
Comment on attachment 268897 [details] [diff] [review] patch r=sspitzer
Attachment #268897 - Flags: review?(sspitzer) → review+
fixed. Checking in controller.js; /cvsroot/mozilla/browser/components/places/content/controller.js,v <-- control ler.js new revision: 1.161; previous revision: 1.160 done Checking in treeHelpers.js; /cvsroot/mozilla/browser/components/places/content/treeHelpers.js,v <-- treeHe lpers.js new revision: 1.7; previous revision: 1.6 done Checking in placesOverlay.xul; /cvsroot/mozilla/browser/components/places/content/placesOverlay.xul,v <-- pla cesOverlay.xul new revision: 1.13; previous revision: 1.12 done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
11 years ago
Also, we should probably disable New Bookmark as well when exculdeItems is set.
Created attachment 268905 [details] [diff] [review] follow up patch, per mano's comment
Attachment #268905 - Flags: review?(mano)
> Also, we should probably disable New Bookmark as well when exculdeItems is set. I'll spin that off to a new bug.
> I'll spin that off to a new bug. see bug #384989 for disabling the "New Folder" and "New Live Bookmark..." commands when excludeItems is set.
Comment on attachment 268905 [details] [diff] [review] follow up patch, per mano's comment r=mano
Attachment #268905 - Flags: review?(mano) → review+
supplimental fix landed. cvs ci browser/components/places/content/treeView.js browser/components/places/c ontent/treeHelpers.js Checking in browser/components/places/content/treeView.js; /cvsroot/mozilla/browser/components/places/content/treeView.js,v <-- treeView. js new revision: 1.11; previous revision: 1.10 done Checking in browser/components/places/content/treeHelpers.js; /cvsroot/mozilla/browser/components/places/content/treeHelpers.js,v <-- treeHe lpers.js new revision: 1.8; previous revision: 1.7 done
I cannot verify this until Bug 389759 is fixed. I can't add separators.
the summary of this bug is misleading. the resolution here was to disable inserting separtors if sorted. (see comment #5). updating summary to reflect that.
Summary: While BM is in a sorted view, adding a new separator is added below the position index, not the bookmark. → Disable the "New Separator" command when the BM view is sorted
Whiteboard: [swag 1d]
Comment on attachment 268897 [details] [diff] [review] patch looking for an after-the-fact ui-review on this change: inserting separators is now disabled when sorted.
Attachment #268897 - Flags: ui-review?
11 years ago
Attachment #268897 - Flags: ui-review? → ui-review?(beltzner)
Comment on attachment 268897 [details] [diff] [review] patch Yeah, this is right. When viewing as sorted by some attribute, it doesn't make sense to be able to insert separators.
Attachment #268897 - Flags: ui-review?(beltzner) → ui-review+
I've verified this with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/200707270404 Minefield/3.0a7pre.
Status: RESOLVED → VERIFIED
Test case https://litmus.mozilla.org/show_test.cgi?id=7474 was created in litmus for regression testing.
Flags: in-litmus? → in-litmus+
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.