150186 - Bookmark contextual menus are relying on user selection

Status: VERIFIED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Braxton Sherouse]

Hey-
The contextual menus show all disabled options when right clicking on a bookmark
(in the sidebar and the toolbar) when there is no bookmark selected in the
sidebar. When you do select a bookmark, however, all the CM options are enabled
for any bookmark you might click. In addition, when there is a bookmark selected
in the sidebar, and you right click on another bookmark, the functions in the
contextual menu will be applied to the selected bookmark and not the one you
right clicked on.

Steps to reproduce:
1) have some bookmarks
2) open the bookmark drawer
3) verify that all the bookmarks are UNselected
4) right click one of the bookmarks (and see that all options will be disabled!)
5) select another bookmark by single clicking
6) right click the first bookmark you clicked, and see that all the options are
enabled.
7) choose one of those options, and you will see that the function took place on
the selected bookmark, not the one that was right clicked.

take care
-braxton

Comment 1

[saari (gone)]

->pinkerton

Comment 2

[Mike Pinkerton (not reading bugmail)]

this seems to be a cocoa bug, since cocoa is the one managing the context menu.
*shrug*

Comment 3

[Greg K.]

Related to 148932?

Comment 4

[Greg K.]

(That is, bug 148932.)

Comment 5

[Steve Smart]

Yes, looks like a dup of bug 148932, so I'll comment on that one.

Comment 6

[Max Horn]

Attachment: Patch that fixes the issue and adds the ability to specify a context menu & tooltips on a per-item level in outline views

This extends CHExtendedOutlineView to allow context menus and tooltips on a
per-item basis. I already modified BookmarksDataSource.mm to show a tooltip
(the URL), just to demonstrate how this works (I have no clue what group
bookmarks are or how to create them, thus I don't handle them right now, but
doing so should be trivial).

One remark on the context menu: if you open it by clickin on no item (e.g.
"below" all entries), it will use the default context menu for that view. Right
now this is the same as used for the items, which is obviously not what we
want. As a solution, Chimera should provide a different menu in this case
(which only has "Add Folder"/"Add Bookmark" items).

Comment 7

[Mike Pinkerton (not reading bugmail)]

pulling in because we have a patch. i'll look at it shortly.

Comment 8

[Max Horn]

Attachment: Update for my previous patch, to work with current CVS again.

Comment 9

[Max Horn]

Attachment: Update for my previous patch, to work with current CVS again.

I am the sorry, the previous attachment was the wrong patch :-(

Comment 10

[Max Horn]

Attachment: Oncre more updated for CVS (as of July 29th)

Works against current CVS.

Comment 11

[Mike Pinkerton (not reading bugmail)]

+    // There seems to be a bug in AppKit; selectRow is supposed to
+    // abort editing, but it doesn't, thus we do it manually.
+    [self abortEditing];

has a bug been filed with apple?

+/* FIXME - ugh, checking keycode directly is not good. It might be better to 
+check the character values, see NSText.h and NSEvent.h for some useful constants:
+
+    NSNewlineCharacter = 0x000a,
+    NSCarriageReturnCharacter = 0x000d,
+    NSEnterCharacter = 0x0003,
+    NSBackspaceCharacter = 0x0008,
+    NSBackTabCharacter = 0x0019,
+    NSDeleteCharacter = 0x007f
+
+	NSLeftArrowFunctionKey		= 0xF702,
+	NSRightArrowFunctionKey		= 0xF703,
+*/
+

where did this come from?

+  if (rows != oldRows || !NSEqualRects(oldFrameRect, frameRect))

it seems odd that oldFrameRect and oldRows are static. doesn't seem to make much
sense, though i guess it still should work since this code is only called when
the values would change anyway. If that's the case, though, why bother checking
for a change at all? If i change the size of one window, then go to another
window and set the size so that it doesn't change (where our no-change logic
would kick in), it will still do all the work because the oldFrameRect is from
the first window. It all seems rather pointless to me.

Comment 12

[Max Horn]

Hm, any idea why Bugzilla doesn't send me a mail when this bug is changed, even so I 
set it to always send me mails when anything changes?

Anyways: No, I didn't file a bug, but then I also didn't test this with 10.1.5 or Jaguar again - 
this is from code that was written under 10.0.0 till 10.1.3, roughly. I do use the code in 
projects that run under 10.1.5, but that particular part is only important if you can edit the 
rows of the table view, which you can't right now in our case.


As to where the key constants came from: the comments says it, NSEvent.h and  
NSText.h


"it seems odd that oldFrameRect and oldRows are static."
- you are right! This was an accident, I used an wrong version to make the patch. The 
code you see was a for a small project where only one table view was ever used, and no 
subclass of NSTableView was to be made, so instead we used a category. The correct 
way to do this of course is to make these two members of the NSTableView subclass. 
Shall I make a new patch changed accordingly (please email me, too, since I don't seem 
to get bugzilla changes.)

Comment 13

[Max Horn]

Attachment: Got rid of the 'static' use in favor of proper member vars

I replaced the 'static' with proper member vars. Also optimized a bit for
delegates that don't implement tooltips support (i.e. which don't implement the
delegate method used to query tooltips on a per-item base). In this case, we
don't have to compute and set the tooltip rects.

Comment 14

[Mike Pinkerton (not reading bugmail)]

checked in

Comment 15

[sairuh (rarely reading bugmail)]

vrfy'd fixed using 2002.08.26.05.