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

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Christine Yen]

Attachment: 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.

Comment 1

[Christine Yen]

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...

Comment 2

[(not reading, please use [e-mail] instead)]

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.)

Comment 3

[Christine Yen]

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.")

Comment 4

[Christine Yen]

See Bug #333758

Comment 5

[Christine Yen]

Attachment: patch

Disables the "New Separator" command when the BM view is sorted.

Comment 6

[(not reading, please use [e-mail] instead)]

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.

Comment 7

[(not reading, please use [e-mail] instead)]

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.

Comment 8

[Christine Yen]

(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

Comment 9

[Christine Yen]

Attachment: patch

Comment 10

[Christine Yen]

Attachment: 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 11

[(not reading, please use [e-mail] instead)]

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.

Comment 12

[Christine Yen]

Attachment: v4

Seth's right... fixing sidebar problem in this patch - "new separator" always enabled in sidebar.

Comment 13

[Asaf Romano (gone)]

Index: controller.js
===================================================================

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

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

Comment 14

[Christine Yen]

(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?

Comment 15

[Asaf Romano (gone)]

Isn't view.getResult().sortingMode == Ci.nsINavHistoryQueryOptions.SORT_BY_NONE sufficient? Other views disallow sorting...

Comment 16

[Christine Yen]

(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

Comment 17

[Asaf Romano (gone)]

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

Comment 18

[Christine Yen]

Attachment: patch

see Comment #17 c/o mano

Comment 19

[Asaf Romano (gone)]

Why is the sort commandupdate-event addition? Isn't a "select" event triggered either way as a result of treeview's invalidateAll?

Comment 20

[Christine Yen]

(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 21

[(not reading, please use [e-mail] instead)]

Comment on attachment 268897 [details] [diff] [review]
patch

r=sspitzer

Comment 22

[(not reading, please use [e-mail] instead)]

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

Comment 23

[Asaf Romano (gone)]

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.

Comment 24

[Asaf Romano (gone)]

Also, we should probably disable New Bookmark as well when exculdeItems is set.

Comment 25

[(not reading, please use [e-mail] instead)]

Attachment: follow up patch, per mano's comment

Comment 26

[(not reading, please use [e-mail] instead)]

> Also, we should probably disable New Bookmark as well when exculdeItems is set.

I'll spin that off to a new bug.

Comment 27

[(not reading, please use [e-mail] instead)]

> 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 28

[Asaf Romano (gone)]

Comment on attachment 268905 [details] [diff] [review]
follow up patch, per mano's comment

r=mano

Comment 29

[(not reading, please use [e-mail] instead)]

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

Comment 30

[Al Billings [:abillings - ex-MoCo]]

I cannot verify this until Bug 389759 is fixed. I can't add separators.

Comment 31

[(not reading, please use [e-mail] instead)]

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.

Comment 32

[(not reading, please use [e-mail] instead)]

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.

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 34

[Al Billings [:abillings - ex-MoCo]]

I've verified this with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/200707270404 Minefield/3.0a7pre.

Comment 35

[Aakash Desai [:aakashd]]

Test case https://litmus.mozilla.org/show_test.cgi?id=7474 was created in litmus for regression testing.

Comment 36

[Gervase Markham [:gerv]]

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