338558 - Selection in Bookmarks Manager should persist when search is canceled

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Bookmarks, defect)

Description

[Martin Girschick]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; chrome://navigator/locale/navigator.properties; rv:1.9a1) Gecko/20060501 Camino/1.2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; chrome://navigator/locale/navigator.properties; rv:1.9a1) Gecko/20060501 Camino/1.2+

If an entry has been selected while search was active within bookmarks and the search is canceled the select entry is not visible anymore as all folders have been collapsed.

Reproducible: Always

Steps to Reproduce:
1. Open bookmarks
2. Search for something using the lower right search field
3. Click once on any entry (which is supposed to be within a folder)
4. Cancel the search by clicking the X next to the search field


Actual Results:  
All bookmarks are shown again with all folder collapsed

Expected Results:  
At least the folder containing the selected entry should be open and the entry should still be selected.

The main rational behind that is that I search for something I browsed a few days ago and using the search within the history I only find an entry which I saw around the same time. By canceling the search and revealing the item I can easily browse through the entries next to it.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Given that we store a persistent selection in each collection "sometimes", I think this seems reasonable (we also shouldn't be collapsing all the folders when we cancel a search--we don't collapse them permanently, but you have to switch collections to get your state back).

Comment 2

[Martin Girschick]

I think the rephrasing of the summary didn't catch my initial idea. I don't want the "pre-search" state to be restored but the item which has been selected during search to stay selected after canceling search.

Comment 3

[Chris Lawson (gone)]

Yeah, this is eminently doable. Pseudocode:

when canceling search
if outline view has selection
  store selected item
cancel search
if we have a stored item
  ensure that it's visible and selected

I'll try to knock this out by the end of the weekend to go with the other bookmarks improvements I've been working on.

cl

Comment 4

[Chris Lawson (gone)]

Re-summarising to make it a bit clearer what's supposed to happen.

Comment 5

[Chris Lawson (gone)]

I have working code for the single-selection case, but I'm wondering what to do for the multiple-selection case. It's quite possible to select multiple bookmarks in a search and then click on the cancel button. I'm not entirely sure what user expectation would be here.

cl

Comment 6

[Stuart Morgan]

I don't see any reason not to leave them all selected.

Comment 7

[Chris Lawson (gone)]

Attachment: Fix for both single- and multiple-selection cases

Comment 8

[Sean Murphy]

I'll also vote for preserving a multiple bookmark selection.  That's the kind of behavior I'd expect.

Comment 9

[Stuart Morgan]

Comment on attachment 248049 [details] [diff] [review]
Fix for both single- and multiple-selection cases

Definitely a big behavioral improvement. My only issue is that this:

>+      for (unsigned int i = 0; i < [selectedResults count]; ++i) {
>+        unsigned int newRowIndex = [mSearchResultArray indexOfObject:[selectedResults objectAtIndex:i]];

is n*m complexity because it will rescan mSearchResultsArray from the beginning over and over. Since you know that selectedResults is in order, this could be max(n,m) instead by using indexOfObject:inRange: with a range that is always updated to contain the part of mSearchResultArray after the last match.

Comment 10

[Chris Lawson (gone)]

Attachment: no longer n*m-complex

Comment 11

[Stuart Morgan]

Comment on attachment 248876 [details] [diff] [review]
no longer n*m-complex

>+                                                             inRange:NSMakeRange(start,[mSearchResultArray count] - start)];

Space after |,|

>+        if (newRowIndex != NSNotFound) {
>+          // update the range
>+          start = newRowIndex;
>+          // scroll to the first item (arbitrary, but at least one should show)
>+          if (i == 0)
>+            [[self activeOutlineView] scrollRowToVisible:newRowIndex];

Rather than storing |start| and remaking the range each time, store the range directly and update it here.  You'll also want to do something like:

  if (remainingRange.location == 0)
    // do scrolling
  //update remaining range

Right now the scrolling would only work if the first thing in the selection were still in the results after the search changed

r=me with those changes.

Comment 12

[Chris Lawson (gone)]

--> 1.6. I'll get a fresh patch up here ASAP, hopefully in the next few days.

Comment 13

[Chris Lawson (gone)]

(In reply to comment #9)
> >+      for (unsigned int i = 0; i < [selectedResults count]; ++i) {
> >+        unsigned int newRowIndex = [mSearchResultArray indexOfObject:[selectedResults objectAtIndex:i]];
> 
> is n*m complexity because it will rescan mSearchResultsArray from the beginning
> over and over. Since you know that selectedResults is in order, this could be
> max(n,m) instead by using indexOfObject:inRange: with a range that is always
> updated to contain the part of mSearchResultArray after the last match.

Yes, selectedResults is in order, but mSearchResultArray isn't (and isn't guaranteed to stay in the same order across changes of the search string).

Let's say I have the following three bookmarks:

Bar <http://bar.example.com/>
Baz <http://baz.example.com/>
Foo <http://foo.example.com/>

in selectedResults. They'll maintain that order in selectedResults, sure, but they won't necessarily retain that order in mSearchResultArray. In this example, searching on "example" vs. searching on "http" can yield completely different results order. The net effect of this is that if Baz and Foo show up before Bar in the new search results, they're dropped from the selection because using the range assumes that they're in the search results *after* Bar.

Unless I'm totally missing something, that is. Which is entirely possible.

cl

Comment 14

[Stuart Morgan]

(In reply to comment #13)
> Yes, selectedResults is in order, but mSearchResultArray isn't (and isn't
> guaranteed to stay in the same order across changes of the search string).

Bug 383827

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Chris, can you update this patch now that bug 383827 has landed?

Comment 16

[Chris Lawson (gone)]

Yeah, I'll get to this soon.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Can you get to this before you leave us again?

Comment 18

[Samuel Sidler (old account; do not CC)]

Chris, do you have time to get this patch up-to-date?

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Pushing out, but if this is ready, we can still consider taking it.

Comment 20

[Chris Lawson (gone)]

Attachment: fix v1.3

Un-rotted and review comments addressed. Sorry this took 18 months to get around to.

Comment 21

[Stuart Morgan]

Comment on attachment 345427 [details] [diff] [review]
fix v1.3

>     [[self activeOutlineView] reloadData];

You use [self activeOutlineView] several times now; get it once and store it.

>+      for (unsigned int i = 0; i < [selectedResults count]; ++i) 

Use an enumerator in cases like this where you don't need the index.

>+          remainingRange = NSMakeRange(newRowIndex, [mSearchResultArray count] - newRowIndex);

The range should start and newRowIndex + 1; no need to recheck the one you just added next time.

>+  NSMutableArray* selectedItems = [[NSMutableArray alloc] initWithCapacity:[self numberOfSelectedRows]];

This is leaked.

Comment 22

[Chris Lawson (gone)]

Attachment: fix v1.31

Addresses Stuart's review comments.

Comment 23

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 352354 [details] [diff] [review]
fix v1.31

sr=pink

Comment 24

[Stuart Morgan]

Comment on attachment 352354 [details] [diff] [review]
fix v1.31

r=me

Comment 25

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 352354 [details] [diff] [review]
fix v1.31

This breaks search-cancelling in History the Top 10 List.

In History, cancelling a search now switches back to one of the bookmarks collection and selects (but does not focus, so grey color) the last selection(s) in that particular bookmarks collection.  Even worse, if you don't make a selection in History, you're still switched to that bookmarks collection (and something random is selected but not focused).

In the Top 10 List, the new, broken behavior is odder: we choose one of the selected bookmarks, switch to the bookmarks collection that houses said bookmark, and select (but not focus) that bookmark and other random bookmarks, as well as expanding the folder for the other bookmark in the other bookmark collection.

Previously, cancelling a search in each of those collections just re-displayed the collection (c.f. bug 423578).

Comment 26

[Chris Lawson (gone)]

This is sort of a tricky problem.

The history part wasn't too difficult, but I'm not sure how to handle the three smart folders. The |selectItems| method calls |selectAndRevealItem|, which reveals the item in its original parent folder (leading to the odd results Smokey describes for the Top Ten collection in comment 25). The easy way around that is to disable searching for the Top Ten, which makes sense for a host of other reasons, primarily because there are only 10 items in the list and there's not much point in searching. If we want to keep searching enabled in the Top Ten, that's where things start to get really tricky.

The Bonjour and Address Book collections could potentially be a lot bigger than 10 items, so we probably don't want to disable searching there. I'm not sure whether it's possible for a Bonjour bookmark to be present elsewhere, but we might run into the same problem that plagues the Top Ten there if it is. (The Address Book smart group should be safe from these sorts of issues.)

Comment 27

[Chris Lawson (gone)]

Oh, I forgot to mention...I can get history back to where it was, but preserving selection in history is a similarly complicated nightmare. Almost every method we currently use in this assumes BookmarkItem input and doesn't necessarily work with HistoryItem input. Some guidance as to how to handle this problem would be appreciated (as preserving selection in history would also be nice).