First item in History search results has all disabled items in context menu

RESOLVED FIXED

Status

Camino Graveyard
History
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1.1})

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

2.57 KB, patch
Stuart Morgan
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
STR:

1. Open History
2. Search for something you know will have at least 1 result
3. Select the first (top-most) result and try to ctrl-click or Action-menu it

Expected: Active CM items
Actual: All CM items disabled

This also happens if you select the first item and any additional item.

What I think is happening is that somehow the cm-control is still thinking that the "Today" folder is the first item in the search results (we don't allow CMs on folders in history) but also knows it's a history item and getting very confused...  Observe the grey-highlight in the search results if you have 1) the Today folder selected before searching 2) if you have the first/second history *item* selected before searching....
*** Bug 362642 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 2

11 years ago
Created attachment 247342 [details] [diff] [review]
Patch

|selectedRowEnumerator| is deprecated.  This uses the preferred |selectedRowIndexes| instead (which fixes the bug).
Assignee: nobody → stridey
Status: NEW → ASSIGNED
Attachment #247342 - Flags: review?(stuart.morgan)

Comment 3

11 years ago
Comment on attachment 247342 [details] [diff] [review]
Patch

Ew; thanks Apple.  What an ugly recommended enumeration scheme. Anyway, the bug doesn't actually have anything to do with selectedRowEnumerator being deprecated, it's because 
  while ((currentRow = [[rowEnum nextObject] intValue]))
stops if the first selected row has an index of 0. I think I fixed this bug for Bookmarks a while back, and apparently didn't realize history had parallel code.

This version does a lot more work than it needs to in most cases. Given that we are usually going to be able to stop very quickly, I'd rather see a loop based on firstIndex and indexGreaterThanIndex:, keeping the early return.
Attachment #247342 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 4

11 years ago
Created attachment 247359 [details] [diff] [review]
Patch

Yeah, it's an ugly scheme.  firstIndex and indexGreaterThanIndex is better though, thanks.
Attachment #247342 - Attachment is obsolete: true
Attachment #247359 - Flags: review?(stuart.morgan)

Comment 5

11 years ago
Comment on attachment 247359 [details] [diff] [review]
Patch

r=me
Attachment #247359 - Flags: superreview?(mikepinkerton)
Attachment #247359 - Flags: review?(stuart.morgan)
Attachment #247359 - Flags: review+
Comment on attachment 247359 [details] [diff] [review]
Patch

sr=pink
Attachment #247359 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 7

11 years ago
Checked in on 1.8branch and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.