Opened 2 years ago

Closed 15 months ago

#14553 closed defect (fixed)

The Delete key does not work when renaming an entry in a wxListCtrl/wxTreeCtrl.

Reported by: jdagresta Owned by:
Priority: normal Milestone: 2.9.5
Component: wxGTK Version: stable-latest
Keywords: wxListCtrl wxTreeCtrl wxTextCtrl Cc: jdagresta@…, jralls@…
Blocked By: Blocking:
Patch: no

Description

The Delete key does not work when renaming an entry in a wxListCtrl/wxTreeCtrl. It just beeps when the Delete key is pressed.

Platforms: Solaris, AIX, and Red Hat Linux.

I tried running in the debugger and setting a breakpoint at wxTextCtrl::OnChar() and it gets there for BACKSPACE key in rename mode but never for the DELETE key.

Apparently there is an automatic Delete accelerator set by wxWidgets due to having DEL set in a menu label (e.g. <label>_Delete\tDEL</label> in the xrc file) of the application and this seems to be causing the problem. Would like a solution that does not require removing the DEL accelerator from the menu.

See this thread started by Vadim Zeitlin on this subject:
http://groups.google.com/group/wx-dev/browse_thread/thread/2fddc94a256a14bf/d73fb58a7ac3ef64

Attachments (1)

header-bg.png download (303 bytes) - added by Slavon 5 weeks ago.
www.buzzfeed.com

Download all attachments as: .zip

Change History (14)

comment:1 Changed 2 years ago by jdagresta

For a workaround (when using wxGTK) I was dynamically able to remove the DEL accelerator from a Delete main menu item (wxID_DELETE) in an EVT_LIST_BEGIN_LABEL_EDIT function and then restore the DEL accelerator in an EVT_LIST_END_LABEL_EDIT function, and this allowed the Delete key to work during the Rename of a wxListCtrl item (and correspondingly for wxTreeCtrl).

For example, I setup for this up front by doing something like this:

sDeleteLabel = mb->GetLabel(wxID_DELETE);
nIdx = sDeleteLabel.Find("\t");
if (nIdx != -1)
    sNewDeleteLabel = sDeleteLabel.Left(nIdx);
else
    sNewDeleteLabel = sDeleteLabel;

and in the EVT_LIST_BEGIN_LABEL_EDIT function I do:

SetLabel(wxID_DELETE, sNewDeleteLabel);

and in the EVT_LIST_END_LABEL_EDIT function I do:

SetLabel(wxID_DELETE, sDeleteLabel);

My application also has Cut/Copy/Paste main menu items (wxID_CUT/wxID_COPY/wxID_PASTE) and so Ctrl+X/Ctrl+C/Ctrl+V also do not function during a Rename (they just beep).

I tried the same workaround, however it did not work because GTK is not allowing the accelerators to be removed from those standard menu items. The accelerators just (silently) do not get removed when the e.g. SetLabel(wxID_CUT, sNewCutLabel) is performed. I debugged and the gtk_widget_remove_accelerator(m_menuItem, m_parentMenu->m_accel, accel_key, accel_mods) call in wxMenuItem::SetItemLabel() just did not remove the acclerator.

So for my workaround (when using wxGTK) to get Ctrl+X/Ctrl+C/Ctrl+V to also work correctly during a Rename, I was forced to make my own "menu_cut"/"menu_copy"/"menu_paste" main menu items (and not use the standard wxID_CUT/wxID_COPY/wxID_PASTE) and mimic the labels/bitmaps/accelerators of the standard menu items, and then GTK allowed the accelerators to be removed using the method above.

Here's how I defined my own main menu items in an XRC file:

<object platform="unix|mac" class="wxMenuItem" name="menu_cut">
    <label>Cu_t\tCtrl+X</label>
    <bitmap stock_id="wxART_CUT"/>
    <help>Cut the selection and put it on the clipboard</help>
</object>
<object platform="unix|mac" class="wxMenuItem" name="menu_copy">
    <label>_Copy\tCtrl+C</label>
    <bitmap stock_id="wxART_COPY"/>
    <help>Copy the selection and put it on the clipboard</help>
</object>
<object platform="unix|mac" class="wxMenuItem" name="menu_paste">
    <label>_Paste\tCtrl+k</label>
    <bitmap stock_id="wxART_PASTE"/>
    <help>Insert clipboard contents</help>
</object>

GTK also did not allow the accelerator to be removed from a standard wxID_NEW menu item, but since Ctrl+N does not have any standard meaning during a Rename I did not bother doing anything to take off that accelerator.

I'm also overriding some other acclerators set with SetAcceleratorTable() by having a second accelerator table used during the Rename that does not have accelerators defined for Shift+Delete (to do an wxID_CUT), Shift+Insert (to do an wxID_PASTE), and Ctrl+Insert (to do an wxID_COPY).

Again, I would really like a solution that does not require all these workarounds.

comment:2 Changed 2 years ago by vadz

  • Cc vadim@… removed

Here is a patch which ought to fix the issue by ensuring that the currently focused widget always gets the key press first, before the accelerators are used (credits to John Rails):

  • src/gtk/toplevel.cpp

     
    639639                      G_CALLBACK (gtk_frame_focus_in_callback), this); 
    640640    g_signal_connect_after (m_widget, "focus_out_event", 
    641641                      G_CALLBACK (gtk_frame_focus_out_callback), this); 
     642    // Gtk processes key events at the toplevel first, which handles for 
     643    // menu accelerators and shortcuts before passing the event on to the 
     644    // focus child window to begin propagation. We want to propagate 
     645    // first, so we connect gtk_window_propagate_key_event to 
     646    // key_press_event. 
     647    g_signal_connect (m_widget, "key_press_event", 
     648                      G_CALLBACK (gtk_window_propagate_key_event), NULL); 
    642649 
    643650#ifdef GDK_WINDOWING_X11 
    644651#ifdef __WXGTK3__ 

Could you please test if it does fix the problem and if it doesn't introduce any new ones?

I didn't test it yet but from looking at the code it seems like it ought to work and I think the workaround for preventing keyboard events from propagating upwards added in r70945 should still work.

comment:3 Changed 2 years ago by jdagresta

Yes, that fix does work.

Although the last argument to g_signal_connect() is "NULL" and all the other occurrences of g_signal_connect() in toplevel.cpp have "this" as the last argument, it didn't seem to cause any problems in our application.

I did not see any new problems in our application introduced by the fix, but I did not do anything to test if r70945 does still work.

Thanks for the fix.

comment:4 Changed 23 months ago by vadz

  • Milestone set to 2.9.5
  • Status changed from new to confirmed

comment:5 Changed 23 months ago by jdagresta

  • Cc jdagresta@… added

comment:6 Changed 22 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [72640]) Ensure that key events are sent to focused window first in wxGTK.

Start processing key events from the currently focused window, this ensures
that its key event handlers are tried before the top level window
accelerators.

This is consistent with wxMSW and allows a window to locally override the
global accelerators which really makes sense.

Closes #14553.

comment:7 Changed 20 months ago by vadz

  • Cc jralls@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Unfortunately this change broke accelerators handling as can be seen in the keyboard sample: under wxGTK neither "A" nor "Ctrl-A" work as accelerators any more.

And my commit message above is misleading: wxMSW does allow to override accelerators at window level but this needs to be done explicitly, by overriding MSWShouldPreProcessMessage(), and not implicitly and by default as it happens in wxGTK now. We'll probably need to revert this change as the cure is worse than the original problem.

A real solution would be to handle just WXK_DELETE (and other keys) at wxTextCtrl level first but I don't know how to do it in GTK+.

comment:8 Changed 16 months ago by vadz

I think I finally have an almost correct solution: instead of either using the default GTK+ handler, which calls gtk_window_activate_key() first and then gtk_window_propagate_key_event() only if the event hadn't been processed by the TLW menu accelerators or just using gtk_window_propagate_key_event() which doesn't take accelerators into account at all, let's do call both of them but in the reverse order. This seems to fix the problem with DEL but is only almost correct because we still have 2 other problems:

  1. Unlike under wxMSW, the accelerator will be triggered only if the user code doesn't handle (i.e. skips) wxEVT_CHAR. This is actually kind of logical, but different.
  2. Simple keys, such as 'A' in the keyboard sample, are intercepted by IM and so always processed and can't be used for accelerators.

The latter problem is hopefully not very common in practice while the former is definitely bad because the same code can work under MSW but not GTK, but at least we now can write the code that works correctly under GTK too, so I'll commit my changes.

But we still need to define which behaviour is more correct here and if we can reproduce it on the other platforms (as usual, I have no idea what happens in wxOSX here...).

comment:9 Changed 16 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [73692]) Tweak wxGTK keyboard handling to allow accelerators to work again.

Accelerators were broken by the changes of r72640 which were done to ensure
that the focused window gets the keyboard event first, before its top level
parent. Fix them now by still passing the events to the focused window first
but also passing them to the parent top level if it hasn't been handled by the
focused child.

Unfortunately this is still not fully compatible with wxMSW because in wxGTK
wxEVT_CHAR handler must skip the event in order to allow the accelerator using
the same key to work, while in wxMSW the accelerator can only be suppressed by
overriding wxWindow::MSWShouldPreProcessMessage(). We will need to make the
two ports behave in the same way when the event is not skipped in the future.

But for now, at least make them both behave correctly when the handler does
skip the event.

Closes #14553.

comment:10 Changed 16 months ago by jdagresta

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm reopening this because it seems that the top level menu (or menu bar) accelerators (e.g. Alt+f for File menu) are broken now.

You can see this with the keyboard sample. The Alt+f and Alt+h menu accelerators (if that's the correct terminology) for File and Help do not work now. I'm now seeing this same problem in our application with Alt+ accelerators.

On the first round of changes with r72640, I think these Alt menu accelerators were still working (but as you found out the other Ctrl and char accelerators on the pop-up menu items were broken).

Now with the [73692] changes, the Ctrl and char accelerators on the pop-up menus are working but the Alt accelerators on the menu bar seem to be broken.

Actually, maybe it's specifically an Alt+? (Alt plus char) accelerator that's not working (whether on menu bar or pop-up menu item) because I just noticed with the keyboard sample that Alt+x on the File pop-up menu (to do Exit) is also not working now.

comment:11 Changed 15 months ago by vadz

Both Alt+F and Alt+H work fine for me in the keyboard sample with GTK+ 2.20, which version of GTK+ did you test with?

comment:12 Changed 15 months ago by jdagresta

Thank you for your feedback; I investigated further and there was some good news and some bad news.

The problem I had with Alt+? not working was actually only a problem on our Solaris 10 systems; Alt+? was working fine in our application on Red Hat Linux and AIX platforms.

We have GTK+ 2.10 on our Red Hat Linux 5 (RH5) systems, GTK+ 2.18 on our AIX 5.3 systems, but only GTK+ 2.4 (specifically 2.4.9) on our SPARC Solaris 10 systems.

I don't know why (all of) our Solaris systems have such an older version of GTK+.

I was able to determine that there is a GTK+ version 2.12.0 package available (on sunfreeware.com), but in trying to see what would be involved to "upgrade" to that version I found that I would need to install around 19 packages total (and possibly have to do some font setup for the fontconfig library dependency). That's poses a real problem for our customers using the Solaris platform (and even for us internally). Also the older GTK+ was installed to /usr and the newer packages all install to a different /usr/local path (unless that can be changes when you install, but I could not see how to to do that).

That may be why we still have only GTK+ 2.4 (it probably had zero or very few dependencies) even though the Solaris 10 GTK+ 2.12.0 package has been available (on sunfreeware.com) since 5/2008.

I'm not a system guru and I found it to be very difficult to deal with doing 19 packages - maybe there's an "easier" way to do this task: "I want to install the latest gtk+ package available (on a Solaris 10 system) and any package dependencies" (rather than having to individually do 'pkgadd' on each of the 19 downloaded and gunzip'd packages)?

I decided to try to determine why Alt+? was not working with the GTK+ 2.4 version on Solaris.

Debugging and comparing to behavior on the other platforms that work, what I found was that in the wxgtk_tlw_key_press_event() routine in src/gtk/toplevel.cpp the call to gtk_window_propagate_key_event() with the Alt+? event was incorrectly returning TRUE with GTK+ 2.4 and so it never calls gtk_window_activate_key() on that event, which would actually cause the Alt+? to happen.

On the other platforms with newer GTK+, the call to gtk_window_propagate_key_event() with the Alt+? event returns FALSE, and then gtk_window_activate_key() does the right thing for Alt+?.

Interestingly, the same "bug" does not occur with Ctrl+? sequences, but only with Alt+?.

I'm not expecting that there would be any changes made to wxWidgets to handle this problem with the very old version of GTK+, so you can again set the ticket status to "closed" with resolution "fixed".

But do you have any suggestions on how I might workaround this problem (in our own version of wxWidgets we will use on the Solaris platform)?

comment:13 Changed 15 months ago by vadz

  • Resolution set to fixed
  • Status changed from reopened to closed

Interesting, I wonder how exactly can this bug happen considering that gtk_window_propagate_key_event() doesn't seem to have changed since 2004 (see commit c454880a0 in GTK+ repository), i.e. before 2.4.

The best I can suggest you do is to avoid calling it for Alt+X key events (you can get the state of modifiers from GdkEventKey). This should work fine if you don't need to use these keys as shortcuts AFAICS...

But if this only happens with GTK+ 2.4, then I don't think we need to keep this ticket opened -- we don't even support 2.4 officially any more (I wonder how did you manage to even compile wxGTK with it...).

Changed 5 weeks ago by Slavon

Note: See TracTickets for help on using tickets.