Differential D1892

Re-instate Emulated Command Bar tests, and refactor Emulated Command Bar
ClosedPublic
Actions

Authored by sstjames on Jun 15 2016, 2:20 PM.

Details

Reviewers

michalhumpula

Group Reviewers

KTextEditor

Summary

A bit of a monster, I'm afraid ;)

This patch mainly does two things -

Re-instates the globs of Emulated Command Bar tests that managed to get lost when the Vi Mode tests were split out, way back in 2014 (whoops! :))
Refactors the emulatedcommandbar class to be less huge - it is now split into several files, and moved into its own emulatedcommandbar/ subdirectory.

It uses a small amount of C++11 features which I hope are OK (I'm not sure what the policy is, nowadays) - mainly in-class initialisers and lambdas + std::function.

Test Plan

Requires no special testing, but since the newly-reinstated Emulated Command Bar tests have not been run on any machine but mine since 2014, they might need to be temporarily disabled once Jenkins gets its hands on them :)

Diff Detail

Repository

R39 KTextEditor

Lint

Lint Skipped

Unit

Unit Tests Skipped

sstjames updated this revision to Diff 4510.Jun 15 2016, 2:20 PM

sstjames retitled this revision from to Re-instate Emulated Command Bar tests, and refactor Emulated Command Bar.

sstjames updated this object.

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

sstjames added a reviewer: KTextEditor.

sstjames set the repository for this revision to R39 KTextEditor.

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

This is a big one - essentially I think it is ok to commit. While browsing through the code, my only real comment is that instead of QRegExp, we should use QRegularExpression. However, as the patch is already big enough, I wouldn't change it now.

What is more important to note is that KDE Frameworks is released every month, see https://community.kde.org/Schedules/Frameworks.
So next tagging date is 2nd of July (bit more than two weeks). This is good, since it means that we have two weeks to fix regressions :-)

So I would be fine with committing this, and if you target the KDE Frameworks 5.24, please rather commit early than late.

More feedback and / or better reviews are of course welcome :)

In D1892#34993, @dhaumann wrote:

This is a big one - essentially I think it is ok to commit. While browsing through the code, my only real comment is that instead of QRegExp, we should use QRegularExpression. However, as the patch is already big enough, I wouldn't change it now.

What is more important to note is that KDE Frameworks is released every month, see https://community.kde.org/Schedules/Frameworks.
So next tagging date is 2nd of July (bit more than two weeks). This is good, since it means that we have two weeks to fix regressions :-)

So I would be fine with committing this, and if you target the KDE Frameworks 5.24, please rather commit early than late.

More feedback and / or better reviews are of course welcome :)

Thanks Dominik - I'll commit tomorrow evening (about 2000 GMT) if there are no objections before then, and sort out the QRegExp stuff later :)

sstjames added a dependent revision: D1894: Workaround fact that Qt 5.5 does not allow shortcuts to be triggered by playing QKeyEvents.Jun 15 2016, 3:07 PM

Since you have moved the code to separate folder, don't you want to move the code to separate C++ namespace too?

Few code style issues (old code I guess). Otherwise ok.

src/vimode/history.h
29	I suppose this is needed for tests only. Was there a way to do it only for tests?

This revision is now accepted and ready to land.Jun 16 2016, 3:19 PM

In D1892#35365, @michalhumpula wrote:

Since you have moved the code to separate folder, don't you want to move the code to separate C++ namespace too?

Probably - I think I'll leave that for the time being, though - this huge patch is burning a hole in my harddrive :)

Crap - didn't realise that *every single commit* would be broadcast to the mailing list. Sorry about that!

Committed, with last commit 7acf83ca944bbec6f970d00d2f0de2f64c5a83d7

Revision Contents
Changeset List

		Path
M		autotests/src/vimode/CMakeLists.txt (3 lines)
M		autotests/src/vimode/base.cpp (6 lines)
M		autotests/src/vimode/completion.cpp (4 lines)
A	M	autotests/src/vimode/emulatedcommandbar.h (87 lines)
A	M	autotests/src/vimode/emulatedcommandbar.cpp (3377 lines)
A	M	autotests/src/vimode/emulatedcommandbarsetupandteardown.h (48 lines)
A	M	autotests/src/vimode/emulatedcommandbarsetupandteardown.cpp (68 lines)
M		autotests/src/vimode/keys.h (40 lines)
M		autotests/src/vimode/keys.cpp (72 lines)
M		src/CMakeLists.txt (8 lines)
M		src/include/ktexteditor/command.h (8 lines)
M		src/inputmode/kateviinputmode.cpp (2 lines)
M		src/utils/katesedcmd.cpp (10 lines)
M		src/vimode/cmds.cpp (2 lines)
D	M	src/vimode/emulatedcommandbar.h (178 lines)
D	M	src/vimode/emulatedcommandbar.cpp (1321 lines)
A	M	src/vimode/emulatedcommandbar/activemode.h (64 lines)
A	M	src/vimode/emulatedcommandbar/activemode.cpp (78 lines)
A	M	src/vimode/emulatedcommandbar/commandmode.h (77 lines)
A	M	src/vimode/emulatedcommandbar/commandmode.cpp (387 lines)
A	M	src/vimode/emulatedcommandbar/completer.h (82 lines)
A	M	src/vimode/emulatedcommandbar/completer.cpp (236 lines)
A	M	src/vimode/emulatedcommandbar/emulatedcommandbar.h (130 lines)
A	M	src/vimode/emulatedcommandbar/emulatedcommandbar.cpp (438 lines)
A	M	src/vimode/emulatedcommandbar/interactivesedreplacemode.h (42 lines)
A	M	src/vimode/emulatedcommandbar/interactivesedreplacemode.cpp (89 lines)
A	M	src/vimode/emulatedcommandbar/matchhighlighter.h (32 lines)
A	M	src/vimode/emulatedcommandbar/matchhighlighter.cpp (44 lines)
A	M	src/vimode/emulatedcommandbar/searchmode.h (52 lines)
A	M	src/vimode/emulatedcommandbar/searchmode.cpp (367 lines)
M		src/vimode/history.h (3 lines)
M		src/vimode/inputmodemanager.cpp (2 lines)
M		src/vimode/mappings.cpp (2 lines)
M		src/vimode/modes/normalvimode.cpp (2 lines)
M		src/vimode/searcher.h (21 lines)
M		src/vimode/searcher.cpp (63 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

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	4510			Jun 15 2016, 2:14 PM	★	★

Re-instate Emulated Command Bar tests, and refactor Emulated Command BarClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 4510

autotests/src/vimode/CMakeLists.txt

autotests/src/vimode/base.cpp

autotests/src/vimode/completion.cpp

autotests/src/vimode/emulatedcommandbar.h

autotests/src/vimode/emulatedcommandbar.cpp

autotests/src/vimode/emulatedcommandbarsetupandteardown.h

autotests/src/vimode/emulatedcommandbarsetupandteardown.cpp

autotests/src/vimode/keys.h

autotests/src/vimode/keys.cpp

src/CMakeLists.txt

src/include/ktexteditor/command.h

src/inputmode/kateviinputmode.cpp

src/utils/katesedcmd.cpp

src/vimode/cmds.cpp

src/vimode/emulatedcommandbar.h

src/vimode/emulatedcommandbar.cpp

src/vimode/emulatedcommandbar/activemode.h

src/vimode/emulatedcommandbar/activemode.cpp

src/vimode/emulatedcommandbar/commandmode.h

src/vimode/emulatedcommandbar/commandmode.cpp

src/vimode/emulatedcommandbar/completer.h

src/vimode/emulatedcommandbar/completer.cpp

src/vimode/emulatedcommandbar/emulatedcommandbar.h

src/vimode/emulatedcommandbar/emulatedcommandbar.cpp

src/vimode/emulatedcommandbar/interactivesedreplacemode.h

src/vimode/emulatedcommandbar/interactivesedreplacemode.cpp

src/vimode/emulatedcommandbar/matchhighlighter.h

src/vimode/emulatedcommandbar/matchhighlighter.cpp

src/vimode/emulatedcommandbar/searchmode.h

src/vimode/emulatedcommandbar/searchmode.cpp

src/vimode/history.h

src/vimode/inputmodemanager.cpp

src/vimode/mappings.cpp

src/vimode/modes/normalvimode.cpp

src/vimode/searcher.h

src/vimode/searcher.cpp

Re-instate Emulated Command Bar tests, and refactor Emulated Command Bar
ClosedPublic
Actions

Revision Contents
Changeset List