DevHeads.net

Review Request: Fix keypress stealing issue

Review request for kdelibs and David Faure.

Summary
Shortcut context was not set correctly for the new "Find Text as You Type" action, which can result in the khtmlpart "stealing" '/'-keypresses from other widgets in the same main window. This patch would fix that in the most straight-forward fashion. However:

a) I do wonder, why this is not simply set for *all* applicable actions (including those which do not have a shortcut set by default, but might have a user-configured shortcut!). I.e. something like

foreach (QAction *action, actionCollection()->actions())
{
action->setShortcutContext(Qt::WidgetWithChildrenShortcut);
}

But perhaps I am overlooking something?

b) This is sort of OT, but I could not help wondering:
I was pretty confused about the difference between Find... and Find Text as You Type. Since Find... really is find-as-you-type, too. Before I finally gathered it from the sources. May I suggest condensing the three actions
- Find...
- Find Text as You Type
- Find Links as You Type
to two actions
- Find Text (with default shortcuts Ctrl+F *and* '/')
- Find Links

Regards
Thomas

Diffs
khtml/khtml_part.cpp ec89b0c8083989afb52ebde714e1fe757ab2e387

Diff: <a href="http://git.reviewboard.kde.org/r/101491/diff" title="http://git.reviewboard.kde.org/r/101491/diff">http://git.reviewboard.kde.org/r/101491/diff</a>

Testing

Thanks,

Thomas

Comments

Re: Review Request: Fix keypress stealing issue

By Commit Hook at 06/08/2011 - 12:12

This review has been submitted with commit ce405591dde09c3462c214d9f31350740ecea8c8 by Thomas Friedrichsmeier.

- Commit

On June 2, 2011, 8:38 a.m., Thomas Friedrichsmeier wrote:

Re: Review Request: Fix keypress stealing issue

By Commit Hook at 06/08/2011 - 12:12

This review has been submitted with commit 18a232984b8414795aabfdbff4349053432b2a8b by Thomas Friedrichsmeier.

- Commit

On June 2, 2011, 8:38 a.m., Thomas Friedrichsmeier wrote:

Re: Review Request: Fix keypress stealing issue

By David Faure at 06/05/2011 - 22:16

Ship it!

Patch is okay, I didn't realize the issue was that simple to fix ;)

I agree that, especially in a component like khtml, all shortcuts should probably be limited to "when the part widget has focus", like you suggest.

About the second question, here's my commit log for 60ae8626e917cb1f3:

Re-route the old actions "Find Text as You Type" and "Find Links as You Type" to the findbar.
Remove them from the menu, since they are redundant with "Find text", but kept two actions
just for the shortcuts:

'/' will still activate "find text (not just links)", but the shortcut for "find links" is removed by default,
advanced users will have to set one again. Otherwise newbies would get confused after pressing the shortcut
(was single-quote) by mistake: Esc and Ctrl+F would still find in links only.

=========

No strong opinion though, on whether it's really needed to keep the '/' shortcut for compatibility. Let's email all KDE users and ask them... :-)

- David

On June 2, 2011, 8:38 a.m., Thomas Friedrichsmeier wrote:

Re: Review Request: Fix keypress stealing issue

By Thomas Friedric... at 06/08/2011 - 12:44

On Monday 06 June 2011, David Faure wrote:
Yes, I had seen the commit message.

Well, yes, that sounds like a good idea...

Actually, my suggestion would have been to keep both shortcuts, and to change
the behavior of "Find", rather than that of "Find Text as You Type". I.e.
using the "Find"-shortcut would always reset the search mode to "Find in all
text". But of course that definitely has potential for confusion, too, and I
certainly don't care enough to steer any further into those troubled waters.

Regards
Thomas