Workaround fact that Qt 5.5 does not allow shortcuts to be triggered by playing QKeyEvents
ClosedPublic
Actions

Authored by sstjames on Jun 15 2016, 3:06 PM.

Details

Reviewers

michalhumpula
dhaumann

Group Reviewers

KTextEditor

Summary

(This patch is based against my other pending patch, https://phabricator.kde.org/D1892)

Annoyingly, in Qt 5.5 we can no longer trigger shortcuts by replaying QKeyEvents (something that the Vi Mode - which stores and swallows all key events by default in case they are part of a Mapping and replays them if it turns out they weren't - depends on), so if we are stealing shortcuts, they will no longer be triggered e.g. pressing Ctrl+S will not save! This patch reworks the whole mechanism, moving from "swallow all QKeyEvents and maybe replay later" to "decide whether to swallow shortcuts, but if not, let Qt's shortcut handling system despatch the QShortcutOverride itself". This mostly fixes things, though we can't e.g. create a mapping that, when triggered, saves the document or triggers any of Kate's shortcuts, plus some other small edge cases :(

BUG:353332

Test Plan

Would be good to get more people manually testing this - in the future, hopefully we'll be able to test via QTestLib.

Diff Detail

Repository

R39 KTextEditor

Lint

Lint Skipped

Unit

Unit Tests Skipped

sstjames updated this revision to Diff 4512.Jun 15 2016, 3:06 PM

sstjames retitled this revision from to Workaround fact that Qt 5.5 does not allow shortcuts to be triggered by playing QKeyEvents.

sstjames updated this object.

sstjames edited the test plan for this revision. (Show Details)

sstjames set the repository for this revision to R39 KTextEditor.

Restricted Application added a subscriber: kwrite-devel. · View Herald TranscriptJun 15 2016, 3:06 PM

sstjames added a dependency: D1892: Re-instate Emulated Command Bar tests, and refactor Emulated Command Bar.Jun 15 2016, 3:07 PM

sstjames added a reviewer: KTextEditor.Jun 15 2016, 3:10 PM

Looks ok to me.
(You can consider using the style if () { ... } else {... where {} are on the same line as if, and else. But no showstopper ;)

This revision is now accepted and ready to land.Jun 15 2016, 4:21 PM

Seems to be working correctly. Compiled, tested (tests and manual) on my computer with Qt5.6.1. One of the vim_keys test is failing, but at least the vi mode is back functioning, so ship it.

In D1894#35395, @michalhumpula wrote:

Seems to be working correctly. Compiled, tested (tests and manual) on my computer with Qt5.6.1. One of the vim_keys test is failing, but at least the vi mode is back functioning, so ship it.

Thanks for testing :) Which vim_keys test is failing? They seem to all be working here, though I'm using Qt 5.5.1.

In D1894#35400, @sstjames wrote:

In D1894#35395, @michalhumpula wrote:

Seems to be working correctly. Compiled, tested (tests and manual) on my computer with Qt5.6.1. One of the vim_keys test is failing, but at least the vi mode is back functioning, so ship it.

Thanks for testing :) Which vim_keys test is failing? They seem to all be working here, though I'm using Qt 5.5.1.

It might be just a new bug in Qt5.6 :)

>>> running command  "qa"  on text  ""
FAIL!  : KeysTest::MacroTests() 'kate_view->viewModeHuman().contains(macroIsRecordingStatus)' returned FALSE. ()
   Loc: [/amonsul/kde/ktexteditor/autotests/src/vimode/keys.cpp(707)]
QDEBUG : KeysTest::MarkTests()

In D1894#35403, @michalhumpula wrote:
In D1894#35400, @sstjames wrote:

In D1894#35395, @michalhumpula wrote:

Seems to be working correctly. Compiled, tested (tests and manual) on my computer with Qt5.6.1. One of the vim_keys test is failing, but at least the vi mode is back functioning, so ship it.

Thanks for testing :) Which vim_keys test is failing? They seem to all be working here, though I'm using Qt 5.5.1.

It might be just a new bug in Qt5.6 :)
>>> running command  "qa"  on text  ""
 FAIL!  : KeysTest::MacroTests() 'kate_view->viewModeHuman().contains(macroIsRecordingStatus)' returned FALSE. ()
    Loc: [/amonsul/kde/ktexteditor/autotests/src/vimode/keys.cpp(707)]
 QDEBUG : KeysTest::MarkTests()

Thanks Michal - fingers crossed :) Can you manually check whether simple macros appear to work OK (and whether the (recording) appears in the status bar)? I'm a bit concerned that something fundamental has gone wrong here (this is the very first macro test, and the others won't be run if that one fails).

In D1894#35410, @sstjames wrote:
In D1894#35403, @michalhumpula wrote:
In D1894#35400, @sstjames wrote:

In D1894#35395, @michalhumpula wrote:

Seems to be working correctly. Compiled, tested (tests and manual) on my computer with Qt5.6.1. One of the vim_keys test is failing, but at least the vi mode is back functioning, so ship it.

Thanks for testing :) Which vim_keys test is failing? They seem to all be working here, though I'm using Qt 5.5.1.

It might be just a new bug in Qt5.6 :)
>>> running command  "qa"  on text  ""
 FAIL!  : KeysTest::MacroTests() 'kate_view->viewModeHuman().contains(macroIsRecordingStatus)' returned FALSE. ()
    Loc: [/amonsul/kde/ktexteditor/autotests/src/vimode/keys.cpp(707)]
 QDEBUG : KeysTest::MarkTests()
Thanks Michal - fingers crossed :) Can you manually check whether simple macros appear to work OK (and whether the (recording) appears in the status bar)? I'm a bit concerned that something fundamental has gone wrong here (this is the very first macro test, and the others won't be run if that one fails).

As far as I can tell, the macro replay doubles every keystroke. For example the sequence qaitest<esc>q@a will yield test\ntteesstt.

UPDATE: Hopefully fix the "replay macro/ last change duplicates characters" bug - no automated test for this yet (for some reason, the tests fail for Michal, but not for me!). Michal - would you mind acting as Guinea Pig again? :)

In D1894#35680, @sstjames wrote:

UPDATE: Hopefully fix the "replay macro/ last change duplicates characters" bug - no automated test for this yet (for some reason, the tests fail for Michal, but not for me!). Michal - would you mind acting as Guinea Pig again? :)

All the tests are passing now. Manual macro test works as expected.

The ui tests in vimode_keys were failing, because the whole thing depends on C locale to pass correctly, which is silly.

sstjames mentioned this in R39:025edb11ca9b: Annoyingly, in Qt 5.5 we can no longer trigger shortcuts by replaying….Jun 18 2016, 8:35 AM

Committed: https://phabricator.kde.org/rKTEXTEDITOR025edb11ca9b0fa3685802e6c9cb608a4daa7911

Revision Contents
Changeset List

			Path	Packages
M			src/inputmode/kateabstractinputmode.h (2 lines)
M			src/inputmode/katenormalinputmode.h (2 lines)
M			src/inputmode/katenormalinputmode.cpp (2 lines)
M			src/inputmode/kateviinputmode.h (4 lines)
M			src/inputmode/kateviinputmode.cpp (47 lines)
M			src/vimode/emulatedcommandbar/emulatedcommandbar.h (3 lines)
M			src/vimode/emulatedcommandbar/emulatedcommandbar.cpp (6 lines)
M			src/vimode/inputmodemanager.cpp (2 lines)
M			src/vimode/keymapper.cpp (30 lines)
M			src/vimode/lastchangerecorder.h (7 lines)
M			src/vimode/lastchangerecorder.cpp (15 lines)
M			src/vimode/macrorecorder.cpp (5 lines)
M			src/vimode/modes/normalvimode.cpp (9 lines)

	Status	Author	Revision
	Closed	sstjames	D1894 Workaround fact that Qt 5.5 does not allow shortcuts to be triggered by playing QKeyEvents
	Closed	sstjames	D1892 Re-instate Emulated Command Bar tests, and refactor Emulated Command Bar