Details

Reviewers

Commits

R36:aaed7270b611: CliInterface refactoring
R36:1074bf07a9b9: Refactor CliInterface
R36:fc25a20d533f: Implement Elvis' suggestions.
R36:826d0993f9c4: Merge branch 'cli-refactor' of git.kde.org:ark into cli-refactor
R36:4141c3833470: Merge branch 'master' into cli-refactor

Summary

CliInterface is refactored by creating a new class (CliParameters) which stores and handles all commands, switches and regexp patterns used for interacting with the cli executables.

Diff Detail

Repository

R36 Ark

Branch

cli-refactor

Lint

No Linters Available

Unit

No Unit Test Coverage

rthomsen updated this revision to Diff 7839.Nov 2 2016, 6:05 PM

rthomsen retitled this revision from to CliInterface refactoring.

rthomsen updated this object.

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

rthomsen added a reviewer: elvisangelaccio.

Restricted Application added a project: Ark. · View Herald TranscriptNov 2 2016, 6:05 PM

Restricted Application added a subscriber: kde-utils-devel. · View Herald Transcript

I just had a quick look, but overall it looks fine :)

kerfuffle/archiveinterface.cpp
54 ↗	(On Diff #7839)	This check should be the first thing in the ctor
kerfuffle/cliinterface.h
73 ↗	(On Diff #7839)	This method in not really necessary. Every plugin could just do the setup in their own constructor (since they have access to `m_cliParameters`). This way we can also drop the need for `cacheParameterList()`.
87 ↗	(On Diff #7839)	Why move this from `private` to `public`?
116 ↗	(On Diff #7839)	Make it protected and add a public getter
kerfuffle/cliparameters.h
38–76 ↗	(On Diff #7839)	Since this new class is not only about parameters/switches but also other CLI stuff (regexes, etc.) maybe we should find a more descriptive name, though I can't think any at the moment :p
81 ↗	(On Diff #7839)	Once you drop `CliInterface::cacheParameterList()` you can probably drop this as well.
106 ↗	(On Diff #7839)	unnecessary keyword

This revision now requires changes to proceed.Nov 3 2016, 1:49 PM

Implement Elvis' suggestions.
Merge branch 'master' into cli-refactor

Closed by commit R36:aaed7270b611: CliInterface refactoring (authored by rthomsen). · Explain WhyNov 3 2016, 6:45 PM

This revision was automatically updated to reflect the committed changes.

rthomsen reopened this revision.Nov 3 2016, 7:39 PM

rthomsen marked 3 inline comments as done.

Upload a new diff manually, since arc messed it up.

elvisangelaccio requested changes to this revision.Nov 4 2016, 4:28 PM

elvisangelaccio edited edge metadata.

elvisangelaccio added inline comments.

kerfuffle/cliinterface.h
54–58		Maybe we should keep the docs in this enum. You can move them above the Q_PROPERTY they end up in.
146 ↗	(On Diff #7839)	This method can be moved back to the private section.
kerfuffle/cliproperties.h
81–109 ↗	(On Diff #7874)	All the methods of the new class can be marked as `const` , it seems.

This revision now requires changes to proceed.Nov 4 2016, 4:28 PM

Make substitute* functions in CliProperties class const.
Make CliInterface::escapeFileName() private again.

rthomsen marked 2 inline comments as done.Nov 4 2016, 6:15 PM

rthomsen added inline comments.

kerfuffle/cliinterface.h
54–58	I would rather not keep the doc. It reduces the readability of the code and most of the properties are pretty self-explanatory.

elvisangelaccio requested changes to this revision.Nov 4 2016, 6:36 PM

elvisangelaccio edited edge metadata.

elvisangelaccio added inline comments.

plugins/cliunarchiverplugin/cliplugin.h
58 ↗	(On Diff #7839)	One more thing: since all cli plugins need this method, make it virtual and call in just once in the CliInterface ctor

This revision now requires changes to proceed.Nov 4 2016, 6:36 PM

elvisangelaccio accepted this revision.Nov 4 2016, 7:01 PM

elvisangelaccio edited edge metadata.

elvisangelaccio added inline comments.

plugins/cliunarchiverplugin/cliplugin.h
58 ↗	(On Diff #7839)	Forgot that virtual methods can't be called from the base class ctor. Ignore this.

This revision is now accepted and ready to land.Nov 4 2016, 7:01 PM

Closed by commit R36:1074bf07a9b9: Refactor CliInterface (authored by rthomsen). · Explain WhyNov 4 2016, 7:31 PM

This revision was automatically updated to reflect the committed changes.

rthomsen mentioned this in R36:1074bf07a9b9: Refactor CliInterface.

		Path
M		autotests/kerfuffle/jobstest.cpp (5 lines)
M		autotests/plugins/cli7zplugin/cli7ztest.cpp (81 lines)
M		autotests/plugins/clirarplugin/clirartest.cpp (92 lines)
M		autotests/plugins/cliunarchiverplugin/cliunarchivertest.cpp (52 lines)
M		autotests/plugins/clizipplugin/cliziptest.h (7 lines)
M		autotests/plugins/clizipplugin/cliziptest.cpp (76 lines)
M		kerfuffle/CMakeLists.txt (1 line)
M		kerfuffle/archive_kerfuffle.h (2 lines)
M		kerfuffle/archive_kerfuffle.cpp (3 lines)
M		kerfuffle/archiveformat.h (6 lines)
M		kerfuffle/archiveformat.cpp (11 lines)
M		kerfuffle/archiveinterface.h (7 lines)
M		kerfuffle/archiveinterface.cpp (12 lines)
M		kerfuffle/cliinterface.h (324 lines)
M		kerfuffle/cliinterface.cpp (542 lines)
A	M	kerfuffle/cliparameters.h (157 lines)
A	M	kerfuffle/cliparameters.cpp (348 lines)
A	M	kerfuffle/cliproperties.h (152 lines)
A	M	kerfuffle/cliproperties.cpp (342 lines)
M		kerfuffle/compressionoptionswidget.cpp (2 lines)
M		plugins/cli7zplugin/cliplugin.h (9 lines)
M		plugins/cli7zplugin/cliplugin.cpp (156 lines)
M		plugins/cli7zplugin/kerfuffle_cli7z.json.cmake (36 lines)
M		plugins/clirarplugin/cliplugin.h (5 lines)
M		plugins/clirarplugin/cliplugin.cpp (154 lines)
M		plugins/clirarplugin/kerfuffle_clirar.json.cmake (20 lines)
M		plugins/cliunarchiverplugin/cliplugin.h (4 lines)
M		plugins/cliunarchiverplugin/cliplugin.cpp (52 lines)
M		plugins/cliunarchiverplugin/kerfuffle_cliunarchiver.json.cmake (10 lines)
M		plugins/clizipplugin/cliplugin.h (4 lines)
M		plugins/clizipplugin/cliplugin.cpp (112 lines)
M		plugins/clizipplugin/kerfuffle_clizip.json.cmake (16 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	7839	04d8a98		Nov 2 2016, 6:05 PM	★	★
Diff 2	7872	596e7a6	- Implement Elvis' suggestions.	Nov 3 2016, 6:33 PM	★	★
Diff 3	7873	17625fc	rARKaaed7270b611d8cdad951a55ed33bf5fac0dea69	Nov 2 2016, 6:05 PM	★	★
Diff 4	7874			Nov 3 2016, 8:21 PM	★	★
Diff 5	7908			Nov 4 2016, 6:11 PM	★	★
Diff 6	7910	b01c0c2	rARK1074bf07a9b971a9cc6cb406b51436fbcd891b81	Nov 4 2016, 7:03 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
4141c3833470	3dffb54f734e	fc25a20d533f 596e7a665b1c	Ragnar Thomsen	Merge branch 'master' into cli-refactor	Nov 3 2016, 6:32 PM
fc25a20d533f	1414fb349f3b	aaed7270b611	Ragnar Thomsen	Implement Elvis' suggestions.	Nov 3 2016, 6:31 PM
aaed7270b611	4847b55a96c0	17625fc3bf3f	Ragnar Thomsen	CliInterface refactoring (Show More…)	Nov 2 2016, 5:59 PM
17625fc3bf3f	6c33561ac03c	830a6b3bd28b	Ragnar Thomsen	Cleanup	Nov 2 2016, 5:34 PM
830a6b3bd28b	9c0e2fb26bcc	bd9361f2b2b7 04d8a98133ff	Ragnar Thomsen	Merge branch 'master' into cli-refactor	Nov 2 2016, 4:49 PM
bd9361f2b2b7	851ca667e524	d44059d33b90	Ragnar Thomsen	Fix cliunarchivertest	Nov 1 2016, 8:12 PM
d44059d33b90	2bb34286290d	35e75f7bc27a	Ragnar Thomsen	Port cliunarchivertest	Oct 30 2016, 8:22 PM
35e75f7bc27a	059a31d69a3a	2b5aff67a9a8	Ragnar Thomsen	Port cliziptest	Oct 30 2016, 7:27 PM
2b5aff67a9a8	06042b06a447	3e3677d2c7e4	Ragnar Thomsen	Port clirartest	Oct 30 2016, 4:11 PM
3e3677d2c7e4	20e8b40bfcff	2a19173167b2	Ragnar Thomsen	Port cli7ztest	Oct 27 2016, 6:59 PM
2a19173167b2	fe42583ec5b5	7c1ac9a66fcd	Ragnar Thomsen	Re-enable cliunarchiverplugin	Oct 27 2016, 6:00 PM
7c1ac9a66fcd	fb4fb8941eb5	cce669b49242	Ragnar Thomsen	Port the patterns and kill nasty enum	Oct 27 2016, 5:31 PM
cce669b49242	c252be10e57d	f4559cd01b55	Ragnar Thomsen	Remove old commented code	Oct 24 2016, 8:07 PM
f4559cd01b55	5dc17bac354c	126220d36953	Ragnar Thomsen	Initial refactoring	Oct 24 2016, 8:03 PM

CliInterface refactoring
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 7872

autotests/kerfuffle/jobstest.cpp

autotests/plugins/cli7zplugin/cli7ztest.cpp

autotests/plugins/clirarplugin/clirartest.cpp

autotests/plugins/cliunarchiverplugin/cliunarchivertest.cpp

autotests/plugins/clizipplugin/cliziptest.h

autotests/plugins/clizipplugin/cliziptest.cpp

kerfuffle/CMakeLists.txt

kerfuffle/archive_kerfuffle.h

kerfuffle/archive_kerfuffle.cpp

kerfuffle/archiveformat.h

kerfuffle/archiveformat.cpp

kerfuffle/archiveinterface.h

kerfuffle/archiveinterface.cpp

kerfuffle/cliinterface.h

kerfuffle/cliinterface.cpp

kerfuffle/cliparameters.h

kerfuffle/cliparameters.cpp

kerfuffle/cliproperties.h

kerfuffle/cliproperties.cpp

kerfuffle/compressionoptionswidget.cpp

plugins/cli7zplugin/cliplugin.h

plugins/cli7zplugin/cliplugin.cpp

plugins/cli7zplugin/kerfuffle_cli7z.json.cmake

plugins/clirarplugin/cliplugin.h

plugins/clirarplugin/cliplugin.cpp

plugins/clirarplugin/kerfuffle_clirar.json.cmake

plugins/cliunarchiverplugin/cliplugin.h

plugins/cliunarchiverplugin/cliplugin.cpp

plugins/cliunarchiverplugin/kerfuffle_cliunarchiver.json.cmake

plugins/clizipplugin/cliplugin.h

plugins/clizipplugin/cliplugin.cpp

plugins/clizipplugin/kerfuffle_clizip.json.cmake

CliInterface refactoringClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 7872

autotests/kerfuffle/jobstest.cpp

autotests/plugins/cli7zplugin/cli7ztest.cpp

autotests/plugins/clirarplugin/clirartest.cpp

autotests/plugins/cliunarchiverplugin/cliunarchivertest.cpp

autotests/plugins/clizipplugin/cliziptest.h

autotests/plugins/clizipplugin/cliziptest.cpp

kerfuffle/CMakeLists.txt

kerfuffle/archive_kerfuffle.h

kerfuffle/archive_kerfuffle.cpp

kerfuffle/archiveformat.h

kerfuffle/archiveformat.cpp

kerfuffle/archiveinterface.h

kerfuffle/archiveinterface.cpp

kerfuffle/cliinterface.h

kerfuffle/cliinterface.cpp

kerfuffle/cliparameters.h

kerfuffle/cliparameters.cpp

kerfuffle/cliproperties.h

kerfuffle/cliproperties.cpp

kerfuffle/compressionoptionswidget.cpp

plugins/cli7zplugin/cliplugin.h

plugins/cli7zplugin/cliplugin.cpp

plugins/cli7zplugin/kerfuffle_cli7z.json.cmake

plugins/clirarplugin/cliplugin.h

plugins/clirarplugin/cliplugin.cpp

plugins/clirarplugin/kerfuffle_clirar.json.cmake

plugins/cliunarchiverplugin/cliplugin.h

plugins/cliunarchiverplugin/cliplugin.cpp

plugins/cliunarchiverplugin/kerfuffle_cliunarchiver.json.cmake

plugins/clizipplugin/cliplugin.h

plugins/clizipplugin/cliplugin.cpp

plugins/clizipplugin/kerfuffle_clizip.json.cmake

CliInterface refactoring
ClosedPublic
Actions

Revision Contents
Changeset List