bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Disable the "New Separator" command when the BM view is sorted

VERIFIED FIXED in Firefox 3 alpha6

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Christine Yen, Assigned: Christine Yen)

Tracking

Trunk
Firefox 3 alpha6
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

11 years ago
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.)
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 alpha6
(Assignee)

Comment 3

11 years ago
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.")
(Assignee)

Comment 4

11 years ago
See Bug #333758
(Assignee)

Updated

11 years ago
Whiteboard: [swag 1d]
(Assignee)

Comment 5

11 years ago
Created attachment 268447 [details] [diff] [review]
patch

Disables the "New Separator" command when the BM view is sorted.
Attachment #268447 - Flags: review?(sspitzer)
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.
Attachment #268447 - Flags: review?(sspitzer)
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.
(Assignee)

Comment 8

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

Comment 9

11 years ago
Created attachment 268818 [details] [diff] [review]
patch
Attachment #268447 - Attachment is obsolete: true
Attachment #268818 - Flags: review?(sspitzer)
(Assignee)

Comment 10

11 years ago
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
Attachment #268818 - Attachment is obsolete: true
Attachment #268859 - Flags: review?(sspitzer)
Attachment #268818 - Flags: review?(sspitzer)
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.
Attachment #268859 - Flags: review?(sspitzer)
(Assignee)

Comment 12

11 years ago
Created attachment 268870 [details] [diff] [review]
v4

Seth's right... fixing sidebar problem in this patch - "new separator" always enabled in sidebar.
Attachment #268859 - Attachment is obsolete: true
Attachment #268870 - Flags: review?(sspitzer)
Index: controller.js
===================================================================

+      var content = document.getElementById('placeContent');

controller.js cannot depend on any dom node but <command> elements.
(Assignee)

Comment 14

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

Comment 16

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

Comment 18

11 years ago
Created attachment 268897 [details] [diff] [review]
patch

see Comment #17 c/o mano
Attachment #268870 - Attachment is obsolete: true
Attachment #268897 - Flags: review?(sspitzer)
Attachment #268870 - Flags: review?(sspitzer)
Why is the sort commandupdate-event addition? Isn't a "select" event triggered either way as a result of treeview's invalidateAll?
(Assignee)

Comment 20

11 years ago
(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
Comment on attachment 268897 [details] [diff] [review]
patch

>Index: controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.159
>diff -u -8 -p -r1.159 controller.js
>--- controller.js	12 Jun 2007 15:24:44 -0000	1.159
>+++ controller.js	19 Jun 2007 02:50:48 -0000
>@@ -165,17 +165,20 @@ PlacesController.prototype = {  
>              this._view.peerDropTypes
>                  .indexOf(PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) != -1;
>     case "placesCmd_new:bookmark":
>       return this._canInsert() &&
>              this._view.peerDropTypes.indexOf(PlacesUtils.TYPE_X_MOZ_URL) != -1;
>     case "placesCmd_new:separator":
>       return this._canInsert() &&
>              this._view.peerDropTypes
>-                 .indexOf(PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) != -1;
>+                 .indexOf(PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) != -1 &&
>+             !asQuery(this._view.getResult().root).queryOptions.excludeItems &&
>+             this._view.getResult().sortingMode ==
>+                 Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
>     case "placesCmd_show:info":
>       if (this._view.hasSingleSelection) {
>         var selectedNode = this._view.selectedNode;
>         if (PlacesUtils.nodeIsFolder(selectedNode) ||
>             (PlacesUtils.nodeIsBookmark(selectedNode) &&
>             !PlacesUtils.nodeIsLivemarkItem(selectedNode)))
>           return true;
>       }
>Index: placesOverlay.xul
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/placesOverlay.xul,v
>retrieving revision 1.12
>diff -u -8 -p -r1.12 placesOverlay.xul
>--- placesOverlay.xul	1 Jun 2007 00:39:09 -0000	1.12
>+++ placesOverlay.xul	19 Jun 2007 02:50:48 -0000
>@@ -58,17 +58,17 @@
>           src="chrome://browser/content/places/treeView.js"/>
>   <script type="application/x-javascript"
>           src="chrome://browser/content/places/treeHelpers.js"/>
>   <script type="application/x-javascript"
>           src="chrome://global/content/nsDragAndDrop.js"/>
> 
>   <commandset id="placesCommands"
>               commandupdater="true"
>-              events="focus"
>+              events="focus,sort"
>               oncommandupdate="goUpdatePlacesCommands();">
>     <command id="placesCmd_open"
>              oncommand="goDoCommand('placesCmd_open');"/>
>     <command id="placesCmd_open:window"
>              oncommand="goDoCommand('placesCmd_open:window');"/>
>     <command id="placesCmd_open:tab"
>              oncommand="goDoCommand('placesCmd_open:tab');"/>
>     <command id="placesCmd_open:tabs"
>Index: treeHelpers.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/treeHelpers.js,v
>retrieving revision 1.6
>diff -u -8 -p -r1.6 treeHelpers.js
>--- treeHelpers.js	20 May 2007 15:18:42 -0000	1.6
>+++ treeHelpers.js	19 Jun 2007 02:50:48 -0000
>@@ -287,10 +287,14 @@ var OptionsFilter = {
>    */
>   update: function OF_update(result) {
>     var queryNode = asQuery(result.root);
>     var queries = queryNode.getQueries({});
>     
>     var options = queryNode.queryOptions.clone();
>     options.sortingMode = result.sortingMode;
>     this.getHandler(queries).value = options;
>+
>+    // Makes sure that changes to the view triggers the updating of
>+    // the commandset placesCommands
>+    window.document.commandDispatcher.updateCommands("sort");

* the correct place to do this is sotringChanged of treeView.js
* you could use window.updateCommands.
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.
Blocks: 384989
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

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+

Comment 30

11 years ago
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.
Flags: in-litmus?
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?
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+

Comment 34

11 years ago
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.