262329 - Sort by Name should sort between separators

Status: RESOLVED FIXED

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

Description

[Ger Teunis]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040930 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040930 Firefox/0.10

Sort by Name now deletes all seperators and then sorts the remaining items.
It should sort items between two seperators.
I.e.

Bookmarks:

Folder2
Folder1
---------
Item3
Item1
---------
Item4
Item2


Should sort into:

Folder1
Folder2
---------
Item1
Item3
---------
Item2
Item4

Reproducible: Always
Steps to Reproduce:
1. Open Bookmarks Menu
2. Rightclick an bookmark item
3. Select Sort By Name

Actual Results:  
All seperators are deleted and remaining items sorted.

Expected Results:  
Bookmarks with the seperators and sorted items between two seperators. Look at
details desription for example.

Comment 1

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20040930
Firefox/0.10

Confirming

->NEW


related bugs:
bug 258692
and a dupe of bug 211023#c53 (a comment that was simply ignored)

Comment 2

[Phil Ringnalda (:philor)]

*** Bug 270586 has been marked as a duplicate of this bug. ***

Comment 3

[Uri Bernstein]

*** Bug 270986 has been marked as a duplicate of this bug. ***

Comment 4

[:Gavin Sharp [email: [e-mail]]]

*** Bug 281215 has been marked as a duplicate of this bug. ***

Comment 5

[:Gavin Sharp [email: [e-mail]]]

The previous dupe mentions possible dataloss when undoing sort.

Comment 6

[supersheepincognito]

If you ctrl-z to undo after the separator is deleted, the separator returns 
and overwrites the bookmark/folder in its previous space.

Comment 7

[Mike Connor [:mconnor]]

nice to have, probably better to wait for the bookmarks rewrite so we can do
this right.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

*** Bug 310809 has been marked as a duplicate of this bug. ***

Comment 9

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change

Comment 10

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

Blocking
--> Places, which might do this already, at which point they can close it. :)

Comment 12

[Christine Yen]

In the current Firefox 3 trunk:
Sorting bookmarks by any criteria in the Bookmarks Manager sets all separators to invisible, then sorts all contents by bookmark/folder name.

The problem preventing this from being a quick fix to achieve the behavior listed in Comment #0 is that in the _buildVisibleList ( http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/treeView.js#166 ) function, you can see that it makes a query (line 183, with the asContainer(rootNode) method) to a nsNavHistoryContainerResultNode. The node then figures out what the sort comparator should be from the UI, and eventually calls nsNavHistoryContainerResultNode::RecursiveSort ( http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryResult.cpp#615 ).

Separators are not currently included in the sorted list for the following reasons:

1) With little data associated with each separator, the SortComparator used to sort a list of the node's children would be ineffective. Firefox 2 doesn't hide separators while sorting; as a result, separators are often bunched together or scattered randomly through the sorted bookmark list.

2) The way RecursiveSort is written, groups of bookmarks separated by separators would have to be recognized as separate containers to be sorted independently. I can think of two ways to do that off the top of my head -- one, a separator would have to somehow act as a container for all of the bookmarks below it to fool RecursiveSort. A second way would be... splitting the root node's children into individual nsNavHistoryContainerResultNodes by separators, then somehow merging each of the new ns..Nodes into the root node again.

Another problem Seth saw when we were discussing this is if, while viewing a list of bookmarks correctly separated by separators as in Comment #0, what happens when a user tries to insert a separator in the middle of the bookmarks.

Firefox 2 shows bookmarks while sorted, but the separators (IIRC from testing 20 minutes ago) just get bunched together and that behavior doesn't make much more sense than separators disappearing.

One other alternative I'd like to suggest -- keeping the trunk behavior of hiding separators while sorted, but also instead preventing users from adding a new separator unless they're in the unsorted mode. Plus, then there's an additional visual cue (the "New Separator" button greying out) upon sorting.

Comment 13

[Christine Yen]

> One other alternative I'd like to suggest -- keeping the trunk behavior of
> hiding separators while sorted, but also instead preventing users from adding a
> new separator unless they're in the unsorted mode. Plus, then there's an
> additional visual cue (the "New Separator" button greying out) upon sorting.
> 

See Bug #333758

Comment 14

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

another angle:

I am wondering if sorting in the bookmark organizer dialog should not show containers (like we exclude separators) and only show bookmark items (and livemark items) when sorted.

I'm trying to think of another UI where sorting sorts within containers, yet keeps the containers themselves in the hierarchical order.

one analogy is our history sidebar.  you can have containers (view by date), but when you sort (last visited) the containers go away.

another UI that hides container on sorting is thread panes in mail readers.  there are containers when in threaded mode, but when you sort, the containers go away.

you could argue:  we should show containers when sorting, and just sort them as well.  one problem with that is if you delete a container when sorted, you may not realize that you are deleting all of its children as well.

Perhaps this is just food for thought for the places organizer UI redesign that is going on in d.a.firefox.

Comment 15

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

another way is I'm proposing sorting to be a search that returns all bookmarks for the selected container, and then we sort it based on the sorting mode.

and the commands would be enabled / disabled like the should be when searching over a container.

Comment 16

[Alex Faaborg [:faaborg] (Firefox UX)]

I think the current behavior is fine: choosing unsorted shows the separators, and sorting hides them.

Comment 17

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

based on alex's comment #16, I think the next step will be to mark this WONTFIX.

I still think there might be value in not showing containers at all when sorted (see comment #14 - #15), but I'll spin off a new bug about that.

Comment 18

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

from christine's comment #12:

"Firefox 2 shows separators while sorted, but the separators (IIRC from testing
20 minutes ago) just get bunched together and that behavior doesn't make much
more sense than separators disappearing."

Comment 19

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

OK, I'm a little confused.

Comment 0 is talking about the "Sort by Name" function which isn't a view mode for bookmarks, but rather a permanent data manipulation action that changes the order of the bookmarks as they exist in the bookmarks menu.

Yet, comment 12 and onward seems to imply that "Sort by Name" is a mode which can be turned on and off when the user wishes to see their bookmarks in alphabetical order. (This, fwiw, is not the behaviour I see when using my Minefield build, but maybe certain changes have yet to land on trunk?)

Separators are only explicitly added by users as an organizational mechanism, and I think it's fairly safe to assume that they are added to indicate some form of semantic split. Similarly, "Sort by Name" wasn't originally meant to be a view option (like "Keep arranged by" or "Sort by") , but rather a way for someone to have the computer do what computers are good at reorder the hodge-podge that was inadvertantly created by adding bookmarks over time.

Now, this bug is asking that the "Sort by Name" function observe the semantic dividers implied by separators, which I think is a laudable goal and not at all a WONTFIX. For types like my lovely wife, who spend hours dreaming up new and exciting organization schemes, it's a fantastic tool.

Creating a view-mode where we view would be a seperate bug, IMO.

Comment 20

[Mike Connor [:mconnor]]

To resummarize:

Given a folder as follows:

--------------------
Why Programs Crash
Things That Go Boom
--------------------
Nutty Things on Google Video
Crazy Things On YouTube
--------------------
My Buglist
Seth's Buglist
--------------------

If you right click on the folder, or on a non-folder child, we currently get: 

--------------------
Crazy Things On YouTube
My Buglist
Nutty Things on Google Video
Seth's Buglist
Things That Go Boom
Why Programs Crash
--------------------

Separators are removed, all contents sorted in a single list.

But what we really want is:

--------------------
Things That Go Boom
Why Programs Crash
--------------------
Crazy Things On YouTube
Nutty Things on Google Video
--------------------
My Buglist
Seth's Buglist
--------------------

Separators are maintained, and the items between the separators are sorted as distinct groups.

Comment 21

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

mconnor / beltzer:  thanks for clarifying.  

I went off into the weeds there on Comment #12 - #18, sorry about that.

This bug is about what "Sort by Name" does to the contents.

follow up comments / quetsions:

1)  it seems like that command, like insert separator, should only be enabled when you are not sorted.  (menus are not sorted, so the command is enabled and in the organized bookmark dialog, the "right pane" can be sorted.   when sorted, I think the "sort by name command" should be disabled)

2)  Should we have "Edit | Sort By Name" menu item?

3)  Is it confusing to have "Sort by Name" but also "View | Sort by Title"?

Comment 22

[Gonzalo HIGUERA DÍAZ]

(In reply to comment #21)
> 1)  it seems like that command, like insert separator, should only be enabled
> when you are not sorted.  (menus are not sorted, so the command is enabled and
> in the organized bookmark dialog, the "right pane" can be sorted.   when
> sorted, I think the "sort by name command" should be disabled)

Since the sorting doesn't require user intervention and would be well defined, it would not be necessary to disable it (contrary to the separator insertion, which is impossible since the user can't make a decision about it's placement when using a sorted view). However, the idea might be good as to avoid the risk of accidentally sorting  by name and not getting visual feedback of the mistake in order to promptly act upon the mistake.


> 2)  Should we have "Edit | Sort By Name" menu item?

Unless there are ambiguities, I would find it harmless (though being used to taking advantage of context menus, I cannot speak for its usefulness, except for being direct left-click accessible). How would it work? Would it act as if right clicking on the active node of the left pane, or would the action be restricted to when there is a folder selected on the right pane (I don't think it should do both unless a pop-up is used). (I would go for the former.)


> 3)  Is it confusing to have "Sort by Name" but also "View | Sort by Title"?

If you mean "View | Sorted by Title", in Firefox 2.0.0.x it is called "View | Sorted by Name", which is more coherent: either "title" or "name" should be used, but not both.

Comment 23

[Asaf Romano (gone)]

Attachment: patch

Comment 24

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

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

this doesn't work, and at first glance, I don't see how it could.

I think you need to take items into sub arrays (split by seps) and then sort using the existing sort function.

note, extra ";" on this line, too:

+             this._view.getResult().sortingMode ==
+                 Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;;

Comment 25

[Asaf Romano (gone)]

Attachment: patch

Comment 26

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

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

I think you re-attached the first patch again.

Comment 27

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

>> 3)  Is it confusing to have "Sort by Name" but also "View | Sort by Title"?
>
> If you mean "View | Sorted by Title", in Firefox 2.0.0.x it is called "View |
> Sorted by Name", which is more coherent: either "title" or "name" should be
> used, but not both.

spun out to bug #386287

Comment 28

[Asaf Romano (gone)]

Attachment: patch

Comment 29

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

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

r=sspitzer

Comment 30

[Asaf Romano (gone)]

mozilla/browser/components/places/content/controller.js 1.164

Comment 32

[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