412988 - Wrong viewIndex after lazy changes of title

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ondrej Brablc]

Attachment: Fix order when title gets updated

When a page is added to a history query node sorted by title, it is inserted often on wrong place, because it uses part of the query string as sort key, instead of using the title. Title is lazily updated later and item is resorted.

I have observed during implementation of bug 385245 in "By Date and Site" view, that a page belonging to one site gets added to a wrong one.

Initial insert is:

[-] Today
    [-] SiteA
        - http://sitea.com/blog  "blog"
        - http://sitea.com/about "SiteA::About"
    [-] SiteB
        - "SiteB::About"

Temporary title "blog" is less than "SiteA::About" so it is initially inserted on position 0 inside of SiteA (viewIndex=2). "SiteA::About" has viewIndex=3.

Now title gets updated to "SiteA::Blog" and the code calculates new position inside of "SiteA" which is 1 (2nd item). Now the old algorithm would calculate the new viewIndex for "SiteA::Blog" as viewIndex of "SiteA::About" + 1. That is 4. This would place it to SiteB. So the fix is that if we detect, that we are swapping the elements inside of one parent, we need to subtract one, because "SiteA::About" will be moving by one up.

Comment 1

[Dietrich Ayala (:dietrich)]

Comment on attachment 297814 [details] [diff] [review]
Fix order when title gets updated

this change looks good, r=me.

Can you please add a comment here on the bug w/ clear STR for QA verification?

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 297814 [details] [diff] [review]
Fix order when title gets updated

drivers: low impact fix for a live-update node ordering scenario.

Comment 3

[Ondrej Brablc]

Steps To Reproduce
------------------
Instruction - wait after each visit for changes in the sidebar. Expand all nodes that will appear.

1) Clear Browsing History.
2) Open History sidebar.
3) View by Date and Site.
4) Visit http://www.mozilla.org/
5) Visit http://winscp.net/
6) Visit http://winscp.net/eng/docs/protocols

Now you should see both WinSCP pages to be inside of "winscp.net" node. Before this patch, the page http://winscp.net/eng/docs/protocols with title "WinSCP :: Supported Transfer Protocols" would land in "www.mozilla.org" node. You should try at least 3 or 5 times. This was not always visible.

Comment 4

[Reed Loden [:reed]]

Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.29; previous revision: 1.28
done

Comment 5

[Ondrej Brablc]

I was looking at bug 409676 and found problems with sorting on unexpected place, analysis showed, it has been caused by this bug, so I'm reopening it.

Comment 6

[Ondrej Brablc]

Attachment: Do not fix the order, if viewIndex is not defined

At the moment sorting a bookmark folder with items: "1", "2" and "3" leads to "1",  "3", "2". This patch fixes this for situations when viewIndex is not defined (item  has been removed and is being added again).

Comment 7

[Dietrich Ayala (:dietrich)]

we need a comprehensive series of litmus (or preferably automated) tests to catch regressions for liveupdate in the tree view.

Comment 8

[Reed Loden [:reed]]

Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.32; previous revision: 1.31
done

Comment 9

[Marco Bonardo [:mak]]

this regressed drag & drop in the Library

now if you drag an item in the empty space under a tree it is inserted at the wrong index (one place before!)

STR: 
- open in the library a bookmark folder
- take a bookmark and drag to bottom into the empty space
- the bookmark is inserted in the wrong place, should be appended at the end

Comment 10

[Marco Bonardo [:mak]]

reopening due to library D&D regression

Comment 11

[Marco Bonardo [:mak]]

resetting as fixed (sorry) and opening a new one on the regression

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I can verify this bug with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008021704 Minefield/3.0b4pre ID:2008021704 and the given steps in comment 3.

But there is one thing which looks curios. After step 6 I clicked again on http://winscp.net/ and now I get an extra entry under winscp.net with the same name. The title isn't used now. It looks like following:

winscp.net
     WinSCP :: Free...
     WinSCP :: Supported...
     winscp.net

I think it should be filed as a new bug?

Comment 13

[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