Review Request 129170: Add KArchive::errorString() method to provide more details on KArchive errors

Sat Oct 15 16:05:54 UTC 2016

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129170/#review100017
-----------------------------------------------------------


src/k7zip.cpp (line 2343)
<https://git.reviewboard.kde.org/r/129170/#comment67175>

    QObject::tr() is bad practice in Qt code (the context class name is then "QObject" instead of e.g. K7Zip).
    
    However I assume that our ts -> po tools don't really care about the context classname (since po doesn't have that), so maybe it doesn't matter. It still shows bad practice for people actually using lupdate.
    
    (BTW the solution for the fact that this isn't a QObject-derived class is to add Q_DECLARE_TR_FUNCTIONS to the class definition)


src/k7zip.cpp (line 2874)
<https://git.reviewboard.kde.org/r/129170/#comment67176>

    As previously discussed, I think this should (also) be a qWarning.
    
    API misuse => qWarning.
    
    Reasoning: if a bad programmer misuses the API *and* doesn't check errorString, he/she won't find out.
    
    And the tr() string might need adjustment, this is shown the user, but the "you" in the string isn't the user. I'd say
    
    setErrorString(tr("Application error: 7-Zip file not open before writing to it, please file a bug report at https://bugs.kde.org"))


src/k7zip.cpp (line 2879)
<https://git.reviewboard.kde.org/r/129170/#comment67177>

    qWarning
    
       .. BTW all this talk about "tar file" is wrong in this file (copy/paste errors). Should be "7-zip file"


src/k7zip.cpp (line 2915)
<https://git.reviewboard.kde.org/r/129170/#comment67178>

    qWarning


src/k7zip.cpp (line 2951)
<https://git.reviewboard.kde.org/r/129170/#comment67179>

    qWarning


src/k7zip.cpp (line 2956)
<https://git.reviewboard.kde.org/r/129170/#comment67180>

    qWarning


src/kar.cpp (line 63)
<https://git.reviewboard.kde.org/r/129170/#comment67181>

    In the tr() message: "for ar files"  (KAr is an internal implementation name, untranslatable and opaque to users)
    
    The qWarning() can mention KAr though.


src/karchive.cpp (line 193)
<https://git.reviewboard.kde.org/r/129170/#comment67182>

    4-spaces indentation


src/karchive.cpp (line 220)
<https://git.reviewboard.kde.org/r/129170/#comment67183>

    stat'ing is too technical. Maybe like this:
    
    tr("Failed accessing the file %1 for adding to the archive. The error was: %2")


src/karchive.cpp (line 356)
<https://git.reviewboard.kde.org/r/129170/#comment67184>

    tr("Writing failed: %1")


src/karchive_p.h (line 54)
<https://git.reviewboard.kde.org/r/129170/#comment67185>

    missing tr()


src/krcc.cpp (line 90)
<https://git.reviewboard.kde.org/r/129170/#comment67186>

    see KAr.
    
    And this should be a qWarning, it's definitely a programmer error to try and write to an rcc file.
    I'm not even sure I would set the error string, but ok, let's be consistent.
    
    Maybe just tr("Cannot write to a RCC file") in all methods, no point in having different error messages [to translate].


src/ktar.cpp (line 337)
<https://git.reviewboard.kde.org/r/129170/#comment67187>

    Adding %1 and the filename might be useful.
      ("which archive?")


src/ktar.cpp (line 376)
<https://git.reviewboard.kde.org/r/129170/#comment67188>

    typo: underlying
    
    But that's very probably a programmer error => qWarning.


src/ktar.cpp (line 585)
<https://git.reviewboard.kde.org/r/129170/#comment67189>

    coding style: missing { ... } even for single-line if statements.


src/ktar.cpp (line 730)
<https://git.reviewboard.kde.org/r/129170/#comment67190>

    qWarning + tr() that doesn't use "You", as above


src/ktar.cpp (line 735)
<https://git.reviewboard.kde.org/r/129170/#comment67191>

    same


src/kzip.cpp (line 468)
<https://git.reviewboard.kde.org/r/129170/#comment67192>

    Maybe s/#1/Error code: 1/, otherwise user and translators might wonder what this #1 is.


src/kzip.cpp (line 786)
<https://git.reviewboard.kde.org/r/129170/#comment67193>

    Remove space after dot, so that this is the same string as the one before.


src/kzip.cpp (line 1043)
<https://git.reviewboard.kde.org/r/129170/#comment67194>

    qWarning + tr() without You
      same for all similar ones below


src/kzip.cpp (line 1207)
<https://git.reviewboard.kde.org/r/129170/#comment67195>

    Let's not have the "ouch" in the user-visible string ;)
    
    In fact, in general Qt/KF5 code doesn't test for out-of-memory errors, so this if() should just be removed (as well as the Q_ASSERT).


src/kzip.cpp (line 1273)
<https://git.reviewboard.kde.org/r/129170/#comment67196>

    This will overwrite the error string that was set by doPrepareWriting itself. I would suggest to remove this line completely. Same for the other two below.


- David Faure


On Oct. 15, 2016, 3:08 a.m., Romário Rios wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> -----------------------------------------------------------
> 
> (Updated Oct. 15, 2016, 3:08 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: karchive
> 
> 
> Description
> -------
> 
> This method is similar to `QIODevice::errorString()`. I added a public `errorString()` method and a protected `setErrorString()` method, to allow `KArchive`'s subclasses to implement their own error messages. I also implemented most error messages from most subclasses.
> 
> 
> Diffs
> -----
> 
>   autotests/karchivetest.cpp d0fbf41 
>   src/k7zip.cpp 692b1db 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.cpp 1947dd6 
>   src/ktar.cpp f70b155 
>   src/kzip.cpp 94d4276 
> 
> Diff: https://git.reviewboard.kde.org/r/129170/diff/
> 
> 
> Testing
> -------
> 
> I added `QVERIFY` calls after all errors in `karchivetests.cpp`. Perhaps we'll need more tests, but I'm not sure how to make an archive to fail in some specific way aside from the very basics ("file not found", etc.).
> 
> 
> Thanks,
> 
> Romário Rios
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20161015/76d23fd5/attachment-0001.html>