Differential D408

Improve error handling when loading plugins
ClosedPublic
Actions

Authored by rthomsen on Oct 13 2015, 5:44 PM.

Details

Reviewers
elvisangelaccio
Commits
R36:3fcd8425b398: Improve error handling when loading plugins
Summary

Previously, if Archive::create() failed to load a plugin, a QInputDialog was shown where the user could choose the mime-type of the archive. This was not suitable anymore, since mime-type detection is much better now and because we try all registered plugins for a mimetype.

New behavior: When a plugin fails to load, two different error messages can be shown:

  1. No suitable plugin was found.
  2. All suitable plugins failed to load.

Because we (potentially) try to load different plugins, which can fail for different reasons (missing executables, broken plugin, etc.), error message 2 can not be made more specific.

This was implemented by adding an alternative constructor for Archive which takes an error code. Part::openFile() then checks if an error message is found before continuing to load the interface, and displays the error messages.

Test Plan
  1. Try to open an unsupported mimetype with Ark, e.g. a text file. Error message 1 is displayed.
  2. Rename e.g. unrar binary so rar plugin fails to load, then use Ark to open a rar archive or create a new rar archive. Error message 2 is displayed.

Diff Detail

Repository
R36 Ark
Lint
Lint Skipped
Unit
Unit Tests Skipped
rthomsen updated this revision to Diff 972.Oct 13 2015, 5:44 PM
rthomsen retitled this revision from to Improve error handling when loading plugins.
rthomsen updated this object.
rthomsen edited the test plan for this revision. (Show Details)
rthomsen added a reviewer: elvisangelaccio.
rthomsen set the repository for this revision to R36 Ark.
rthomsen added a project: Ark.
Restricted Application added a subscriber: kde-utils-devel. · View Herald TranscriptOct 13 2015, 5:44 PM
rthomsen added a comment.Oct 13 2015, 5:49 PM

Screenshot, Error type 1:

Screenshot, Error type 2:

rthomsen updated this revision to Diff 973.Oct 13 2015, 6:07 PM

Created diff with git diff -U9999.

elvisangelaccio edited edge metadata.Oct 19 2015, 4:24 PM

Before I start looking at the code, is this patch related to task T921?

rthomsen added a comment.Oct 19 2015, 4:44 PM
In D408#8276, @elvisangelaccio wrote:

Before I start looking at the code, is this patch related to task T921?

Sort of. The new behavior of Ark is to check for the executables when loading the archive, and then present error box 2 in this RR if they are missing.

So the old errorbox in CliInterface::runProcess() only gets displayed if an executable is missing when the actual ListJob/ExtractJob etc. are run. This shouldn't happen unless in extraordinary circumstances, such as if the user uninstalls/removes e.g. unrar after opening an archive, but before e.g. extracting.

elvisangelaccio requested changes to this revision.Oct 20 2015, 3:18 PM
elvisangelaccio edited edge metadata.
elvisangelaccio added inline comments.
kerfuffle/archive_kerfuffle.cpp
174

You should also initialize m_error to NoError in the other constructor.

kerfuffle/archive_kerfuffle.h
81

I miss something like NoError as default entry. See comments below.

142

This is fine, but I would like also a isValid() method, which returns true if m_error == NoError.

part/part.cpp
458

Are you sure that archive here is always not null?

474

Even better: Q_ASSERT(archive && archive->isValid()

This revision now requires changes to proceed.Oct 20 2015, 3:18 PM
rthomsen updated this revision to Diff 1021.Oct 20 2015, 5:02 PM
rthomsen edited edge metadata.

Implement Elvis' suggestions.

rthomsen marked 5 inline comments as done.Oct 20 2015, 5:06 PM

Reply to in-line comments.

part/part.cpp
451–465 ↗(On Diff #972)

Yes, Archive::create should now always return a pointer to Archive. However, I moved Q_ASSERT(archive) check above these if's.

elvisangelaccio accepted this revision.Oct 21 2015, 2:29 PM
elvisangelaccio edited edge metadata.
This revision is now accepted and ready to land.Oct 21 2015, 2:29 PM
Closed by commit R36:3fcd8425b398: Improve error handling when loading plugins (authored by rthomsen). · Explain WhyOct 25 2015, 4:09 PM
This revision was automatically updated to reflect the committed changes.
rthomsen marked 2 inline comments as done.
rthomsen mentioned this in R36:3fcd8425b398: Improve error handling when loading plugins.

Revision Contents
Changeset List

Diff 973

View Options

kerfuffle/archive_kerfuffle.h

Loading...
View Options

kerfuffle/archive_kerfuffle.cpp

Loading...
View Options

part/part.cpp

Loading...