GNOME Bugzilla – Bug 606007
Context menu opens at 0,0 when using keyboard's context menu key
Last modified: 2011-04-17 23:32:59 UTC
In search all notes window: 1. Select notebook 2. Select note 3. Click on selected notebook (press escape to avoid editing) 4. Press context menu key => Two context menus open. Corresponding Gnote bug: https://bugzilla.gnome.org/602493
Looks like the Gnote bug was fixed, so the same fix can probably be applied to Tomboy. Patches welcome; I'd need to dig out my old keyboard to be able to test right now.
In Gnome 2.2, I'm not getting two context menus. I'm curious, but which two do you see? I just get the note context menu, but the context menu key makes it open at 0,0 on the screen instead of over the note.
Ahh, and I figured out why. TreeSelection.GetSelected doesn't work when using multiple selections--so we're always returning immediately in RecentChanges.cs:872. Pretty much no matter what.
Created attachment 150934 [details] [review] Patch to address comment 2 This doesn't address the original problem as I'm unable to reproduce it, but at least it opens closer to the selection now. Any closer, and I'd have to dig further into Gtk than I would like to atm.
Comment on attachment 150934 [details] [review] Patch to address comment 2 New patch coming that addresses this in a different way and fixes the bug.
Created attachment 150943 [details] [review] Fixes multiple context menus Thanks for the tip on the Gnote commit. That was pretty helpful, and does exactly what we need it to. I also retooled my previous patch to address the context-menu appearing at 0,0 on the screen.
Created attachment 151104 [details] [review] Replacement for previous patch Got rid of a couple of dangling variable declarations that I forgot to remove.
I still don't have a keyboard to confirm this fix with.
Created attachment 176097 [details] [review] Patch to only show one context menu I wanted to confirm the fix, I ended up digging deeper and showing the popup menu nearer to the selected row than Greg's patch. I also separated the two patches: the one backporting the fix from Gnote concerning the two context menus shown (this one), and the one showing the popup menu at a better position (see next comment).
Created attachment 176098 [details] [review] Patch making the context menu opening near the selected row
Review of attachment 176097 [details] [review]: The reason I ended up rolling these into a single patch was because 1) the gnote patch had the problem of opening at 0,0 and 2) get_selected() can't be used: (Tomboy:27182): Gtk-CRITICAL **: IA__gtk_tree_selection_get_selected: assertion `selection->type != GTK_SELECTION_MULTIPLE' failed You have to use get_selected_rows instead (or GetSelectedRows()). I'm going to re-verify my original patch and push that.
Review of attachment 176098 [details] [review]: I prefer what you did here to what I did in my patch. There's one problem, however: Addins are able to use GetWidgetScreenPos(), so we can't remove it. Addins/Tasks/TaskListWindow.cs: GetWidgetScreenPos (tree, ref pos_x, ref pos_y); Addins/Tasks/TaskListWindow.cs: void GetWidgetScreenPos (Gtk.Widget widget, ref int x, ref int y) Addins/Tasks/TaskListWindow.cs: GetWidgetScreenPos (widget.Parent, ref x, ref y); I'd like to see a combination of your two patches that doesn't remove GetWidgetScreenPos(), then I can re-visit GetWidgetScreenPos() at some point later. The positioning of the pop-up menu is good. Thanks for the GdkWindow catch, that's slick. I hadn't read the API docs for Gtk.Widget yet.
Also, ignore what I said about pushing my patch in comment 11. I'll push a combination of both. If you want to put it together, feel free, but I can knock it out this week.
Review of attachment 151104 [details] [review]: Use a combination of other submitted patches instead, since this one is crufty.
Created attachment 185235 [details] [review] Proposed fixed This is what I proposed in my previous comment. Leave GetWidgetScreenPos() in place, but otherwise accept all of Nicolas's work.
Created attachment 185780 [details] [review] Proposed patch Fixed a couple of simple typos I noticed.
Review of attachment 185780 [details] [review]: Looks good, minor comments. ::: Tomboy/RecentChanges.cs @@ +909,3 @@ + selected_rows = currentTree.Selection.GetSelectedRows (); + // Get TreeView's coordinates + currentTree.GdkWindow.GetOrigin(out pos_x, out pos_y); Code style: space between method name and "(" @@ -1395,3 @@ - // Allow Escape to close the window - OnCloseWindow (this, EventArgs.Empty); - break; Why remove this?
Thanks for catching those. I'm not sure how the Escape key change got in there. I'll look at that and see if it was intentional, but I don't think it was.
Created attachment 186160 [details] [review] proposed, reviewed fix with corrections. Okay. Last time around. I'm attaching the last patch and pushing with the changes you mentioned.
Review of attachment 186160 [details] [review]: Reviewed and committed.
commit 94d5294f8fca60495db88da15cf9fd305d3955b1