Accelerator debug log messages
Robin Dunn
I'm seeing a lot of messages like this in a recent trunk snapshot:
13:12:54: Debug: Unrecognized accel key 'about python', accel string
ignored.
13:12:54: Debug: Unrecognized accel key 'about python', accel string
ignored.
13:12:54: Debug: No accel key found, accel string ignored.
13:12:54: Debug: No accel key found, accel string ignored.
13:12:54: Debug: Unrecognized accel key '&file', accel string ignored.
13:12:54: Debug: Unrecognized accel key '&file', accel string ignored.
Shouldn't wxAcceleratorEntry::ParseAccel just return false if there is
no TAB character in the string?
--
Robin Dunn
Software Craftsman
http://wxPython.org
Vadim Zeitlin
RD> I'm seeing a lot of messages like this in a recent trunk snapshot:
RD>
RD> 13:12:54: Debug: Unrecognized accel key 'about python', accel string
RD> ignored.
RD> 13:12:54: Debug: Unrecognized accel key 'about python', accel string
RD> ignored.
RD> 13:12:54: Debug: No accel key found, accel string ignored.
RD> 13:12:54: Debug: No accel key found, accel string ignored.
RD> 13:12:54: Debug: Unrecognized accel key '&file', accel string ignored.
RD> 13:12:54: Debug: Unrecognized accel key '&file', accel string ignored.
RD>
RD>
RD> Shouldn't wxAcceleratorEntry::ParseAccel just return false if there is
RD> no TAB character in the string?
As I wrote in the commit message of r66308, at least wxAcceleratorEntry::
FromString() should really parse the strings without TABs to be compatible
with ToString(). It also makes sense to me for wxAcceleratorEntry::Create()
to accept strings without TABs, i.e. just the accelerator and not the full
menu item label, although I'm less sure about this.
So ideally I'd rather avoid calling ParseAccel() if we don't really have
an accelerator. This could be achieved by checking for '\t' in Create() and
not calling ParseAccel() from there at all if there is no TAB, would this
be better? Where exactly is it called from in your case?
Thanks,
VZ
Robin Dunn
Yes, that makes sense. I'll try it that way.
> Where exactly is it called from in your case?
It looks like they are all being called from wxAcceleratorEntry::Create.
Is that a good place to check for '\t' or should it go one step
further up the stack?
This patch takes care of it for me.
Vadim Zeitlin
RD> > So ideally I'd rather avoid calling ParseAccel() if we don't really have
RD> > an accelerator. This could be achieved by checking for '\t' in Create() and
RD> > not calling ParseAccel() from there at all if there is no TAB, would this
RD> > be better?
RD>
RD> Yes, that makes sense. I'll try it that way.
RD>
RD> > Where exactly is it called from in your case?
RD>
RD> It looks like they are all being called from wxAcceleratorEntry::Create.
RD> Is that a good place to check for '\t' or should it go one step
RD> further up the stack?
I'm still not sure about this but I think we should allow using Create()
with strings not containing accelerators anyhow for compatibility so this
looks like the right level to me.
The problem that annoys me personally is that FromString() accepts (and
even required, until recently) a string with a TAB in it. It would really
make more sense if FromString() parsed exactly the same strings as
ToString() returns. I.e. I'd prefer to not only pass str to ParseAccel()
only if it contains TAB but use str.AfterFirst('\t') and only call
ParseAccel() if this string is non-empty and remove Find('\t') from
ParseAccel() itself entirely. Of course, this wouldn't be backwards
compatible neither and there is probably no good enough justification to
break existing code using it like this especially because, once again, it
just had to do it like this and add a dummy TAB, see e.g. line 48 in
https://github.com/etexteditor/e/blob/master/src/Accelerators.cpp (nothing
special about this project, it's just what Google code search found me).
So finally I'd like to propose a very similar but slightly different
patch:
--- a/interface/wx/accel.h
+++ b/interface/wx/accel.h
@@ -119,7 +119,10 @@ class wxAcceleratorEntry
ToString(), i.e. contain the accelerator itself only, or have the
format of a full menu item text with i.e. <code>Label TAB
Accelerator</code>. In the latter case, the part of the string
- before the TAB is ignored.
+ before the TAB is ignored. Notice that the latter format is only
+ supported for the compatibility with the previous wxWidgets
+ versions and the new code should pass only the accelerator string
+ itself to this function.
@return @true if the given string correctly initialized this object
(i.e. if IsOk() returns true after this call)
diff --git a/src/common/accelcmn.cpp b/src/common/accelcmn.cpp
index 718275e..5df35dc 100644
--- a/src/common/accelcmn.cpp
+++ b/src/common/accelcmn.cpp
@@ -158,11 +158,13 @@ static int IsNumberedAccelKey(const wxString& str,
{
// the parser won't like trailing spaces
wxString label = text;
- label.Trim(true); // the initial \t must be preserved so don't strip leading whitespaces
+ label.Trim(true);
- // If we're passed the entire menu item label instead of just the
- // accelerator, skip the label part and only look after the TAB.
- // check for accelerators: they are given after '\t'
+ // For compatibility with the old wx versions which accepted (and actually
+ // even required) a TAB character in the string passed to this function we
+ // ignore anything up to the first TAB. Notice however that the correct
+ // input consists of just the accelerator itself and nothing else, this is
+ // done for compatibility and compatibility only.
int posTab = label.Find(wxT('\t'));
if ( posTab == wxNOT_FOUND )
posTab = 0;
@@ -274,9 +276,18 @@ static int IsNumberedAccelKey(const wxString& str,
/* static */
wxAcceleratorEntry *wxAcceleratorEntry::Create(const wxString& str)
{
+ const wxString accelStr = str.AfterFirst('\t');
+ if ( accelStr.empty() )
+ {
+ // It's ok to pass strings not containing any accelerators at all to
+ // this function, wxMenuItem code does it and we should just return
+ // NULL in this case.
+ return NULL;
+ }
+
int flags,
keyCode;
- if ( !ParseAccel(str, &flags, &keyCode) )
+ if ( !ParseAccel(accelStr, &flags, &keyCode) )
return NULL;
return new wxAcceleratorEntry(flags, keyCode);
What do you think about it?
Thanks,
VZ
Robin Dunn
> So finally I'd like to propose a very similar but slightly different
> patch:
...
>
> What do you think about it?
Looks good to me. I'll try to test and commit later today.
Vadim Zeitlin
RD> On 12/14/10 7:08 AM, Vadim Zeitlin wrote:
RD>
RD> > So finally I'd like to propose a very similar but slightly different
RD> > patch:
RD> ...
RD> >
RD> > What do you think about it?
RD>
RD> Looks good to me. I'll try to test and commit later today.
Thanks! I already have this in a local branch so I can also commit it
very easily if you confirm that it works for you. If you prefer to commit
it yourself here is the commit message I wrote for it:
Don't pass strings not containing accelerators to ParseAccel().
Check for the presence of accelerator part in the string passed to
wxAcceleratorEntry::Create() and don't call ParseAccel() at all if it's not
there. This avoids the spurious warnings about unrecognized accelerators when
creating menu items that don't have any accelerators at all.
Also update wxAcceleratorEntry::FromString() documentation to mention that
the new code should pass just the accelerator to this function and that it
only accepts full menu item labels for compatibility.
Closes #12770.
Thanks,
VZ
Robin Dunn
> On Tue, 14 Dec 2010 11:48:07 -0800 Robin Dunn<ro...@alldunn.com> wrote:
>
> RD> On 12/14/10 7:08 AM, Vadim Zeitlin wrote:
> RD>
> RD> > So finally I'd like to propose a very similar but slightly different
> RD> > patch:
> RD> ...
> RD> >
> RD> > What do you think about it?
> RD>
> RD> Looks good to me. I'll try to test and commit later today.
>
> Thanks! I already have this in a local branch so I can also commit it
> very easily if you confirm that it works for you.
Ok. Tested. It works as expected. Go ahead and commit.
Thanks,