Created attachment 471 [details]
patch

Just to amplify the misbehaviour of text box for searching in this case:
- you can enter multiple single letters
- if you include the ctrl modifier nothing is entered
- if you include the shift modifier, a capital letter is entered, rather than "shift + letter", and so the results are incorrect.

Thanks, David. It tests fine for me on Win, Mac and Linux, so patch_ready. Sorry it wasn't noticed before.  

I did test but it doesn't fix bug 682 (P2).

(In reply to comment #3)

I applied part of this patch (revision: 13291): 

* KeyConfigPrefs::Populate(): I don't see good reason to test for ViewByKey a second time, after the 

   mView->SetView(mViewType); 

call, so I moved that into the clause that sets mViewType for ViewByKey. More concise, more readable. 


* Not obvious to me why it's okay to remove the 

   if (mViewType == ViewByKey) {
      mFilterLabel->SetLabel(_("&Hotkey:"));
   }

near the bottom of KeyConfigPrefs::PopulateOrExchange(ShuttleGui & S). Didn't see it from a code scan, but if you let me know why it's okay, I'll do that, too.

I built the committed patch OK on all three OS'es but on more testing, don't we want the modified letter typed to only return that letter when searching in Hotkey?

For example SHIFT + U includes SHIFT + UP in the results and SHIFT + L (which doesn't exist by default) returns results including SHIFT + LEFT. That seems a little counter-intuitive to me.

(In reply to comment #4)
> (In reply to comment #3)

> * Not obvious to me why it's okay to remove the 
> 
>    if (mViewType == ViewByKey) {
>       mFilterLabel->SetLabel(_("&Hotkey:"));
>    }
> 
> near the bottom of KeyConfigPrefs::PopulateOrExchange(ShuttleGui & S). Didn't
> see it from a code scan, but if you let me know why it's okay, I'll do that,
> too.

The label only needs to be set after mViewType is initialized, or changed, which it is in KeyConfigPrefs::Populate and KeyConfigPrefs::OnViewBy.
In addition, when KeyConfigPrefs::PopulateOrExchange is called from KeyConfigPrefs::Populate, mViewType has not been initialized. So the above code should be removed from PopulateOrExchange.

(In reply to comment #5)
> I built the committed patch OK on all three OS'es but on more testing, don't we
> want the modified letter typed to only return that letter when searching in
> Hotkey?
> 
> For example SHIFT + U includes SHIFT + UP in the results and SHIFT + L (which
> doesn't exist by default) returns results including SHIFT + LEFT. That seems a
> little counter-intuitive to me.

I agree, it's still not quite right (but not directly related to this bug). I think the fix for bug 710 could be generalized to fix the above example.

(In reply to comment #7)
>> For example SHIFT + U includes SHIFT + UP in the results and SHIFT + L 
>> which doesn't exist by default) returns results including SHIFT + LEFT. 
>> That seems a little counter-intuitive to me.
> I agree, it's still not quite right (but not directly related to this bug).
> I think the fix for bug 710 could be generalized to fix the above example.
OK David, so if you think it best, please RESOLVE-FIX this bug (I've no other issues with it), then start a new bug for the above problem.


Gale

(In reply to comment #6)

Thanks, David. Sounds right. Exactly the kind of explanation I was looking for, without requiring my time to review the full code. 

Committed at revision 13292.

(In reply to comment #8)
> (In reply to comment #7)
> >> For example SHIFT + U includes SHIFT + UP in the results and SHIFT + L 
> >> which doesn't exist by default) returns results including SHIFT + LEFT. 
> >> That seems a little counter-intuitive to me.
> > I agree, it's still not quite right (but not directly related to this bug).
> > I think the fix for bug 710 could be generalized to fix the above example.
> OK David, so if you think it best, please RESOLVE-FIX this bug (I've no other
> issues with it), then start a new bug for the above problem.

Opened bug 744.