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

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

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
56	This check should be the first thing in the ctor
kerfuffle/cliinterface.h
72–73	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()`.
86	Why move this from `private` to `public`?
105	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	This method can be moved back to the private section.
kerfuffle/cliproperties.h
82–110	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	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	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/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	★	★

CliInterface refactoring
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 7910

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/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 7910

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/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