GNOME Bugzilla – Bug 647462
Selecting all text in search box and hitting Delete key triggers the deletion of note
Last modified: 2012-12-17 21:15:48 UTC
Created attachment 185708 [details] screenshot I came across this problem today: I searched for a particular note by typing in the search box, some notes displayed in the search results. I opened one of the notes and closed it after reading. Now I wanted to search something else, so I had to clear the text box. Unknowingly I used Ctrl+A and Delete key to clear the text (rather than using the clear text button available in Tomboy UI) . What should have happened: The search box should have cleared. What happened instead: A message box popped up, asking me if I am sure I want to delete the selected note ? p.s.: Using Ctrl+A and backspace clears the text box as expected.
Well, this is easily confirmed in my Tomboy install. I'm surprised nobody's noticed before.
I suppose most users use the clear button. I was doing it manually to test something else :-)
We currently bind the delete key to delete note for the menu bar: RecentChanges.cs: 220 Gtk.MenuBar CreateMenuBar () 221 { 222 ActionManager am = Tomboy.ActionManager; 223 Gtk.MenuBar menubar = 224 am.GetWidget ("/MainWindowMenubar") as Gtk.MenuBar; 225 226 am ["OpenNoteAction"].Activated += OnOpenNote; 227 am ["DeleteNoteAction"].Activated += OnDeleteNote; ActionManager.cs: 120 private void PopulateActionGroups () 121 { 122 /// 123 /// Global Actions 124 /// 125 main_window_actions.Add (new Gtk.ActionEntry [] { 126 new Gtk.ActionEntry ("FileMenuAction", null, 127 Catalog.GetString ("_File"), null, null, null), 128 129 new Gtk.ActionEntry ("NewNoteAction", Gtk.Stock.New, 130 Catalog.GetString ("_New"), "<Control>N", 131 Catalog.GetString ("Create a new note"), null), 132 133 new Gtk.ActionEntry ("OpenNoteAction", Gtk.Stock.Open, 134 Catalog.GetString ("_Open..."), "<Control>O", 135 Catalog.GetString ("Open the selected note"), null), 136 137 new Gtk.ActionEntry ("DeleteNoteAction", Gtk.Stock.Delete, 138 Catalog.GetString ("_Delete"), "Delete", 139 Catalog.GetString ("Delete the selected note"), null), We should remove the handler from the menubar, and have an OnTreeViewKeyPressed event handler instead. I can knock this out probably today or tomorrow, unless someone else gets to it first.
I'm not sure that is the right way to go about doing it, actually. Instead, I'm going to try stacking a KeyPress event handler on top of the combo box and seeing if that works.
Hi, I have to possible solutions (which might or might not work): 1. When the user moves the focus the the search box, unselect the note in the tree widget. 2. Use a global Boolean variable which will be set to True when the user moves focus to the Search box. The code which is triggered on hitting the delete key should check the value of this variable, if its true then its value is true then it shouldn't execute.
Created attachment 185774 [details] [review] Proposed patch for Bug 647462 Attached is a proposed fix for this bug. Here is a brief outline of what I have done: I have added an event handler to the Search box for the FocusInEvent. So now when the user moves the focus to the Search combo box, the Delete option in the menu bar is disabled. This required another case to be handled: When the user clicks back on one of the notes in the tree widget, the Delete option in the menubar (and in the right click context menu) should be re-enabled, so I added the relevant code in the ButtonPress event handler of the TreeView widget. Comments are there in the patch to explain this :-). Thanks
Review of attachment 185774 [details] [review]: ::: Tomboy/RecentChanges.cs @@ +771,3 @@ + // Enable the Delete Note option in the menu bar, which might have been + // disabled in case the focus was in the search combo box perviously. + Tomboy.ActionManager ["DeleteNoteAction"].Sensitive = true; This should be done when we focus on the note list view. Otherwise there are a few rare cases where we can get the focus onto the note list view, but the delete action is still disabled.
The case I found where this doesn't work: 1. Open Search All Notes and click a note in the list 2. Type some text in the search box such that the note selected is a match 3. Press Shift+tab to bring the focus to the note list 4. At this point, the note you first clicked on should be highlighted. You would think pushing delete would delete this note, but the action hasn't been enabled yet. Pushing up or down activates the action and then we're all good. I think this is good enough to make it into 3.0.1, anyone else have an opinion?
Created attachment 186335 [details] [review] Revised patch I have made necessary changes as per the suggestions of Aaron. Thanks to him for reviewing my patch. The changes are much more simple now. I added a FocusInEvent handler to the search combo box to disable the Delete option when focus is in the search box, and now I have also added a FocusOutEvent handler to the search box, which handles the case for enabling the Delete option back when the user moves focus out of the search box. The test case as described by Aaron in the above case now works pretty well. Thanks
I take back what I said in IRC about this. Good work, Abhinav. Appreciate the fix!
abhinav, its better, but the enabling/disabling of the action should really be on the note list. 1. Open the Search All Notes window 2. click on a notebook 3. Click on a note in the note list 4. Click on the text of the notebook you selected in 2. to rename the notebook 5. Use 'Delete' to edit the text Its the same problem we just solved with the search entry. Best thing to do is put those events on the note list instead. Again, really appreciate the patches!
Created attachment 186366 [details] [review] new patch adding focus event handlers to the Notes TreeView Hi, Thanks Greg and Aaron for reviewing my patches and liking my work :-) I have moved the event handlers to the Notes TreeView from the Search combo entry box as suggested by Aaron, as it solves all the related problems (hopefully). Thanks.
Review of attachment 186366 [details] [review]: This patch addresses the issue for me. (I am able to reproduce ever _once_ in a while). The patch does have some whitespace errors. Since I am new to the group I don't know if these are a deal or not.
oh I guess, the tabs are 8 spaces wide ? Monodevelop is the culprit, I did the editing in it and I don't use it that regularly to fine tune the bits :(
Jared, you say you can still repro once in a while? Can you give specific repro steps or is it transient? It looks good to me.
Comment on attachment 186366 [details] [review] new patch adding focus event handlers to the Notes TreeView commited to master 7274ebd68216e7956b84804f61de22e0809e59d0 we'll cherry pick it for 3.0.1 unless there are any objections.
(In reply to comment #15) > Jared, you say you can still repro once in a while? Can you give specific repro > steps or is it transient? It looks good to me. It's so minor, but here's what I did and it would dup it *most* of the time. 1 - Search 2 - Open Note 3 - Close note 4 - Hit delete Key 5 - Click Cancel 6 - Click in Search Box 7 - Hit Delete Key 8 - Be prompted to delete note I don't think I could ever make it happen if I skipped step 4.
reopening because the patch caused a regression. (delete was disabled in list menu #653420) I am going to attach a patch that I created, which I think is cleaner (unless it breaks something)
*** Bug 653420 has been marked as a duplicate of this bug. ***
Created attachment 190680 [details] [review] Addresses regression, but still fixes the delete key issue Please review.
Review of attachment 190680 [details] [review]: It looks like similar to what I had tried to do in one of my previous patches for this bug (http://bugzilla-attachments.gnome.org/attachment.cgi?id=185774). The problem with this approach is that: Although when you hit Delete key in the combo box, note won't be deleted. But if you select a note, then go to the Notebooks pane and double click on a Notebook to rename it, hit delete key and Notes are again prompted to be deleted.
Created attachment 190683 [details] [review] Fixes the issue as well as regression Ok. I am sure, that when initially I had submitted the patch, this regression was not there. This patch fixes the issue, still a bit weird behaviour though. When you first try to take the focus to the tree, by selecting a note, and then right click, the delete option in the context menu is still greyed out (this was there in my previous patch as well, before regression). Although, after this if you select another note and right click, the context menu has the Delete option Enabled. Ideally, ActionManager ["DeleteNoteAction"].Sensitive = true should enable it in the menus as well. Just can't understand why this was not working.
Created attachment 190828 [details] [review] Proposed patch After a combined hack session with Jared, we came up with this patch. It seems to fix the most of the issues except one: Select a note, right click on it, then select another note, now take the focus to the combo, hit delete and we are again prompted for note deletion. Except the above, I don't think there are any weird behaviours.
Comment on attachment 190828 [details] [review] Proposed patch Another issue is that if you open the Search All Notes window so that no notes are selected and then right-click on the note list, Open.. is not available but Delete is. Selecting Delete does nothing. Have you guys looked at teh Open... option and tried to model delete after that? Another thing to look into is Gtk Accelerators. Since we have the Delete option in the Edit menu.
I checked this out in Tomboy 1.6 (that shipped with Ubuntu 11.04). The same issue exists there. I think this is an independent bug. Although looking at the ActionManager.cs code (line no 183, 184) main_window_actions.GetAction ("OpenNoteAction").Sensitive = false; main_window_actions.GetAction ("DeleteNoteAction").Sensitive = false; So I think this is what is responsible for disabling the Open and Delete menu entries initially, but Delete is not getting disabled.
Ok. I take that back. The Tomboy 1.6 in Ubuntu also has my patch as part of debian/patches. I checked this out in an even older version (1.2) and the issue was not there. If you open the search window, without selecting any note, right click then the open and delete options in the context menu are disabled.
I think this goes back to the row selection isn't changed on right click bug. It seems that we are missing something here in this bug and a couple of others. As if we are not thinking outside of the box enough. I feel like we are trying to swat a flight with the incorrect tool.
*** Bug 671359 has been marked as a duplicate of this bug. ***
Jared, would you please assign this one to me, I'd like to tackle it instead of bug 671359.
Created attachment 230833 [details] [review] Proposed patch v1 Here's my take at it. Generally it does two things: 1) reverts the solution introduced earlier that made it into the code (and caused the regression with context menu always having "delete" grayed out) 2) adds two handlers for FocusIn event: for notebooks tree and search box, both simply unselect currently selected notes in the notes tree. That automatically switches off the DeleteNoteAction and properly deactivates controls (which are activated back in OnSelectionChanged handler for tree). This approach does the job for me, works both for keyboard and mouse navigation, introduces minimal code changes and doesn't cause any side effects to user experience. I couldn't come up with any use case when user would need to keep the note selected when going to search box or to notebooks pane, but feel free to share your thoughts if you think otherwise. Tested to work as intended on OpenSUSE 12.2. Your comments/reviews are welcome.
Folks, anyone tried this one, any feedback? Let me know if you're not ready to mess with compiling and would want to get a binary package for testing.
(In reply to comment #31) Alex, your patch appears good. I will attempt to test this week and then commit to master if all is good. Remind me if I don't respond by end of week.
Review of attachment 230833 [details] [review]: Committed.
Thank you Jared.