Implement KMessageWidget to report import errors.
ClosedPublic

Authored by bdevries on Nov 1 2019, 10:50 PM.

Details

Summary

Add a KMessageWidget to report any errors that have occured during
the import:

  • Files that have failed to copy into the temporary directory.
  • Files that failed to rename/move to their final location.
  • Subfolders (as defined by FileRenameFormatter) that failed to create.

Depending on the kind of failure(s), the KMessageWidget will contain
one or two actions to show a detailed list of files and/or subfolder
failures.

Test Plan

Tested extensively with all kinds of errors:

  • set file permissions such that files cannot be copied into their final location;
  • set directory permissions such that subfolders cannot be created.

For each case observed that errors were reported correctly.
Tested that KMessageWidget stays hidden after import without errors.
Tested that previous errors have been correctly cleared when doing
a second import right after the first (without closing the program).

Diff Detail

Repository
R260 Gwenview
Branch
error-handling
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19058
Build 19076: arc lint + arc unit
bdevries created this revision.Nov 1 2019, 10:50 PM
Restricted Application added a project: Gwenview. · View Herald TranscriptNov 1 2019, 10:50 PM
bdevries requested review of this revision.Nov 1 2019, 10:50 PM
ngraham added a subscriber: ngraham.Nov 8 2019, 9:52 PM

Nice, will review soon.

ngraham requested changes to this revision.Nov 15 2019, 5:53 PM
ngraham added a reviewer: KDE Applications.

Thanks again for the patch!

Unfortunately I don't have a device capable of importing photos using Gwenview so I'm afraid I can't test this out, so I'll have to leave that part to someone else. I've done some code review; please see the following comments:

importer/dialogpage.cpp
55

Localize this string

61

Localize this string

76

Use $1 instead of one (some languages use the singular form for numbers beyond the first)

84

Use $1 instead of one (some languages use the singular form for numbers beyond the first)

90

We don't use raw HTML like this. Instead, do it like so:

mErrorMessageWidget->setText(xi18nc("@info", message.join("<nl/>")));

See also https://api.kde.org/frameworks/ki18n/html/prg_guide.html#kuit_markup

96

Localize this string

105

Localize this string

This revision now requires changes to proceed.Nov 15 2019, 5:53 PM
bdevries updated this revision to Diff 70164.Nov 22 2019, 2:48 PM

Internationalize strings.

bdevries marked 7 inline comments as done.Nov 22 2019, 2:58 PM
bdevries added inline comments.
importer/dialogpage.cpp
90

I tried the syntax above and variations thereof as described in the kuit_markup guide, but I couldn't get the correct behavior. None of the variants seemed to accept <br/> as line break. They all showed it as a literal string.

At that point I realized that running it through an internationalization function is overkill. The only reason to have the join() function is to show two already internationalized strings on separate lines (only in case there are two strings, i.e. failed files AND failed folders).
Are there languages were this wouldn't apply? If so, I would need to rethink the way that the strings are constructed.

BTW, I took the original implementation with raw html from the already existing code to report the successful imports. So maybe that also needs to be updated then.

bdevries marked an inline comment as done.Nov 22 2019, 2:59 PM
ngraham added inline comments.Nov 22 2019, 6:14 PM
importer/dialogpage.cpp
90

It's <nl/> in this context, not <br/> But if the other code already does something else, let's copy that, and refactor them both in the next patch? Does that sound like a plan?

bdevries added a comment.EditedNov 22 2019, 9:25 PM

Yes, sounds good.
The current patch is quite aligned with what the existing code is doing, so no modification should be required. This patch could then be used as-is.

I will look into detail into the kuit documentation to figure out what I was doing wrong, because I tried both <nl/> and <br/> and neither worked. To be honest, it's been a while so I don't remember all the details of the compiler errors and warnings. I will look into it and submit another patch. I still need to figure out the "double internationalization" problem, though.

NB: I have also submitted another oneliner patch which I promised to split off from the main "support remote URLs" patch a while ago.

ngraham accepted this revision.Dec 3 2019, 6:22 PM

Yep, thanks. Sorry this patch has dragged on for so long. I'm landing it today!

This revision is now accepted and ready to land.Dec 3 2019, 6:22 PM
This revision was automatically updated to reflect the committed changes.

No problem.
I will follow up, as promised, with a patch to refactor the internationalization of the info/warning/error messages.