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

[Romário Rios]

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

(Updated Nov. 8, 2016, 1:08 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 34f00f0d1402351206a19e6967c29d4d4cc18ba2 by Luiz Romário 
Santana Rios to branch master.


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.h 5b95f49 
  src/k7zip.cpp 692b1db 
  src/kar.h 85bd650 
  src/kar.cpp 7204fb1 
  src/karchive.h b528a4a 
  src/karchive.cpp a1a160a 
  src/karchive_p.h 256620d 
  src/krcc.h 18c7d00 
  src/krcc.cpp 1947dd6 
  src/ktar.h 4bca898 
  src/ktar.cpp f70b155 
  src/kzip.h 84156c7 
  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

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

[David Faure]

> On Nov. 6, 2016, 9:49 a.m., David Faure wrote:
> > Looks ok now, except for the ATime/MTime/CTime business, and @since 5.28 
> > should now be @since 5.29 (which is good because the translators have one 
> > month to translate all these new strings).
> > 
> > Neither ATime nor CTime is actually used in this code, feel free to remove 
> > it, maybe in a commit that you would rebase this one upon, or I can do it 
> > after you push. So at a minimum, replace MTime with "modification time", 
> > fix @since, then push.
> > 
> > Thanks!
> 
> Romário Rios wrote:
> No problem.
> 
> By the way, how should I apply these changes to git? Should I make one 
> big commit or merge my local branch into master?
> 
> David Faure wrote:
> If I was able to review it as a single commit, I'd say it should be 
> pushed as a single commit :-)
> 
> The removal of ATime and CTime should be a separate commit (ideally 
> before the big one).
> 
> Romário Rios wrote:
> Makes sense :)
> 
> Do I have to wait for 5.28 final before pushing?
> 
> David Faure wrote:
> No, you can push now. The release process uses temporary branches so that 
> development never has to stop ;)
> 
> Romário Rios wrote:
> About Friederich's request to add the scripts in order to enable 
> translations, I can do that in another commit, right?

Yes, this way it can be reviewed by those who know more about that...


- David


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


On Nov. 1, 2016, 8:10 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Nov. 1, 2016, 8:10 p.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.h 5b95f49 
>   src/k7zip.cpp 692b1db 
>   src/kar.h 85bd650 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.h 18c7d00 
>   src/krcc.cpp 1947dd6 
>   src/ktar.h 4bca898 
>   src/ktar.cpp f70b155 
>   src/kzip.h 84156c7 
>   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
> 
>

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

[Romário Rios]

> On Nov. 6, 2016, 9:49 a.m., David Faure wrote:
> > Looks ok now, except for the ATime/MTime/CTime business, and @since 5.28 
> > should now be @since 5.29 (which is good because the translators have one 
> > month to translate all these new strings).
> > 
> > Neither ATime nor CTime is actually used in this code, feel free to remove 
> > it, maybe in a commit that you would rebase this one upon, or I can do it 
> > after you push. So at a minimum, replace MTime with "modification time", 
> > fix @since, then push.
> > 
> > Thanks!
> 
> Romário Rios wrote:
> No problem.
> 
> By the way, how should I apply these changes to git? Should I make one 
> big commit or merge my local branch into master?
> 
> David Faure wrote:
> If I was able to review it as a single commit, I'd say it should be 
> pushed as a single commit :-)
> 
> The removal of ATime and CTime should be a separate commit (ideally 
> before the big one).
> 
> Romário Rios wrote:
> Makes sense :)
> 
> Do I have to wait for 5.28 final before pushing?
> 
> David Faure wrote:
> No, you can push now. The release process uses temporary branches so that 
> development never has to stop ;)

About Friederich's request to add the scripts in order to enable translations, 
I can do that in another commit, right?


- Romário


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


On Nov. 1, 2016, 8:10 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Nov. 1, 2016, 8:10 p.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.h 5b95f49 
>   src/k7zip.cpp 692b1db 
>   src/kar.h 85bd650 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.h 18c7d00 
>   src/krcc.cpp 1947dd6 
>   src/ktar.h 4bca898 
>   src/ktar.cpp f70b155 
>   src/kzip.h 84156c7 
>   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
> 
>

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

[David Faure]

> On Nov. 6, 2016, 9:49 a.m., David Faure wrote:
> > Looks ok now, except for the ATime/MTime/CTime business, and @since 5.28 
> > should now be @since 5.29 (which is good because the translators have one 
> > month to translate all these new strings).
> > 
> > Neither ATime nor CTime is actually used in this code, feel free to remove 
> > it, maybe in a commit that you would rebase this one upon, or I can do it 
> > after you push. So at a minimum, replace MTime with "modification time", 
> > fix @since, then push.
> > 
> > Thanks!
> 
> Romário Rios wrote:
> No problem.
> 
> By the way, how should I apply these changes to git? Should I make one 
> big commit or merge my local branch into master?
> 
> David Faure wrote:
> If I was able to review it as a single commit, I'd say it should be 
> pushed as a single commit :-)
> 
> The removal of ATime and CTime should be a separate commit (ideally 
> before the big one).
> 
> Romário Rios wrote:
> Makes sense :)
> 
> Do I have to wait for 5.28 final before pushing?

No, you can push now. The release process uses temporary branches so that 
development never has to stop ;)


- David


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


On Nov. 1, 2016, 8:10 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Nov. 1, 2016, 8:10 p.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.h 5b95f49 
>   src/k7zip.cpp 692b1db 
>   src/kar.h 85bd650 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.h 18c7d00 
>   src/krcc.cpp 1947dd6 
>   src/ktar.h 4bca898 
>   src/ktar.cpp f70b155 
>   src/kzip.h 84156c7 
>   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
> 
>

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

[Romário Rios]

> On Nov. 6, 2016, 9:49 a.m., David Faure wrote:
> > Looks ok now, except for the ATime/MTime/CTime business, and @since 5.28 
> > should now be @since 5.29 (which is good because the translators have one 
> > month to translate all these new strings).
> > 
> > Neither ATime nor CTime is actually used in this code, feel free to remove 
> > it, maybe in a commit that you would rebase this one upon, or I can do it 
> > after you push. So at a minimum, replace MTime with "modification time", 
> > fix @since, then push.
> > 
> > Thanks!
> 
> Romário Rios wrote:
> No problem.
> 
> By the way, how should I apply these changes to git? Should I make one 
> big commit or merge my local branch into master?
> 
> David Faure wrote:
> If I was able to review it as a single commit, I'd say it should be 
> pushed as a single commit :-)
> 
> The removal of ATime and CTime should be a separate commit (ideally 
> before the big one).

Makes sense :)

Do I have to wait for 5.28 final before pushing?


- Romário


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


On Nov. 1, 2016, 8:10 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Nov. 1, 2016, 8:10 p.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.h 5b95f49 
>   src/k7zip.cpp 692b1db 
>   src/kar.h 85bd650 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.h 18c7d00 
>   src/krcc.cpp 1947dd6 
>   src/ktar.h 4bca898 
>   src/ktar.cpp f70b155 
>   src/kzip.h 84156c7 
>   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
> 
>

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

[David Faure]

> On Nov. 6, 2016, 9:49 a.m., David Faure wrote:
> > Looks ok now, except for the ATime/MTime/CTime business, and @since 5.28 
> > should now be @since 5.29 (which is good because the translators have one 
> > month to translate all these new strings).
> > 
> > Neither ATime nor CTime is actually used in this code, feel free to remove 
> > it, maybe in a commit that you would rebase this one upon, or I can do it 
> > after you push. So at a minimum, replace MTime with "modification time", 
> > fix @since, then push.
> > 
> > Thanks!
> 
> Romário Rios wrote:
> No problem.
> 
> By the way, how should I apply these changes to git? Should I make one 
> big commit or merge my local branch into master?

If I was able to review it as a single commit, I'd say it should be pushed as a 
single commit :-)

The removal of ATime and CTime should be a separate commit (ideally before the 
big one).


- David


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


On Nov. 1, 2016, 8:10 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Nov. 1, 2016, 8:10 p.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.h 5b95f49 
>   src/k7zip.cpp 692b1db 
>   src/kar.h 85bd650 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.h 18c7d00 
>   src/krcc.cpp 1947dd6 
>   src/ktar.h 4bca898 
>   src/ktar.cpp f70b155 
>   src/kzip.h 84156c7 
>   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
> 
>

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

[Romário Rios]

> On Nov. 6, 2016, 9:49 a.m., David Faure wrote:
> > Looks ok now, except for the ATime/MTime/CTime business, and @since 5.28 
> > should now be @since 5.29 (which is good because the translators have one 
> > month to translate all these new strings).
> > 
> > Neither ATime nor CTime is actually used in this code, feel free to remove 
> > it, maybe in a commit that you would rebase this one upon, or I can do it 
> > after you push. So at a minimum, replace MTime with "modification time", 
> > fix @since, then push.
> > 
> > Thanks!

No problem.

By the way, how should I apply these changes to git? Should I make one big 
commit or merge my local branch into master?


- Romário


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


On Nov. 1, 2016, 8:10 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Nov. 1, 2016, 8:10 p.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.h 5b95f49 
>   src/k7zip.cpp 692b1db 
>   src/kar.h 85bd650 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.h 18c7d00 
>   src/krcc.cpp 1947dd6 
>   src/ktar.h 4bca898 
>   src/ktar.cpp f70b155 
>   src/kzip.h 84156c7 
>   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
> 
>

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

[David Faure]

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


Ship it!




Looks ok now, except for the ATime/MTime/CTime business, and @since 5.28 should 
now be @since 5.29 (which is good because the translators have one month to 
translate all these new strings).

Neither ATime nor CTime is actually used in this code, feel free to remove it, 
maybe in a commit that you would rebase this one upon, or I can do it after you 
push. So at a minimum, replace MTime with "modification time", fix @since, then 
push.

Thanks!

- David Faure


On Nov. 1, 2016, 8:10 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Nov. 1, 2016, 8:10 p.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.h 5b95f49 
>   src/k7zip.cpp 692b1db 
>   src/kar.h 85bd650 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.h 18c7d00 
>   src/krcc.cpp 1947dd6 
>   src/ktar.h 4bca898 
>   src/ktar.cpp f70b155 
>   src/kzip.h 84156c7 
>   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
> 
>

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

[David Faure]

> On Oct. 30, 2016, 10:59 p.m., David Faure wrote:
> > src/k7zip.cpp, line 2499
> > 
> >
> > Translators (and users) won't know what CTime is, but oh well, not many 
> > people know anyway ;)
> 
> Romário Rios wrote:
> I'm not sure what it is either, which is why I kept the message intact -- 
> same applies to the ATime and MTime error messages.
> 
> Any idea of a better message?

ATime => access time
MTime => modification time
CTime => technically it's the "inode change time", in practice I would remove 
the corresponding code because it's completely useless anyway.


- David


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


On Nov. 1, 2016, 8:10 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Nov. 1, 2016, 8:10 p.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.h 5b95f49 
>   src/k7zip.cpp 692b1db 
>   src/kar.h 85bd650 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.h 18c7d00 
>   src/krcc.cpp 1947dd6 
>   src/ktar.h 4bca898 
>   src/ktar.cpp f70b155 
>   src/kzip.h 84156c7 
>   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
> 
>

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

[Romário Rios]

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

(Updated Nov. 1, 2016, 8:10 p.m.)


Review request for KDE Frameworks.


Changes
---

Fix issues pointed out by David


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 (updated)
-

  autotests/karchivetest.cpp d0fbf41 
  src/k7zip.h 5b95f49 
  src/k7zip.cpp 692b1db 
  src/kar.h 85bd650 
  src/kar.cpp 7204fb1 
  src/karchive.h b528a4a 
  src/karchive.cpp a1a160a 
  src/karchive_p.h 256620d 
  src/krcc.h 18c7d00 
  src/krcc.cpp 1947dd6 
  src/ktar.h 4bca898 
  src/ktar.cpp f70b155 
  src/kzip.h 84156c7 
  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

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

[Romário Rios]

> On Oct. 30, 2016, 10:59 p.m., David Faure wrote:
> > src/k7zip.cpp, line 2499
> > 
> >
> > Translators (and users) won't know what CTime is, but oh well, not many 
> > people know anyway ;)

I'm not sure what it is either, which is why I kept the message intact -- same 
applies to the ATime and MTime error messages.

Any idea of a better message?


- Romário


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


On Nov. 1, 2016, 8:10 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Nov. 1, 2016, 8:10 p.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.h 5b95f49 
>   src/k7zip.cpp 692b1db 
>   src/kar.h 85bd650 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.h 18c7d00 
>   src/krcc.cpp 1947dd6 
>   src/ktar.h 4bca898 
>   src/ktar.cpp f70b155 
>   src/kzip.h 84156c7 
>   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
> 
>

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

[David Faure]

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




autotests/karchivetest.cpp (line 369)


the additional parenthesis around tr are not needed



src/k7zip.cpp (line 2306)


typo: undelying => underlying



src/k7zip.cpp (line 2369)


tr() missing



src/k7zip.cpp (line 2499)


Translators (and users) won't know what CTime is, but oh well, not many 
people know anyway ;)



src/k7zip.cpp (line 2588)


tr() missing



src/karchive.cpp (line 403)


indentation should be 4 spaces, this looks like 2



src/ktar.cpp (line 587)


%1? missing arg()?


- David Faure


On Oct. 27, 2016, 2:23 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. 27, 2016, 2:23 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.h 5b95f49 
>   src/k7zip.cpp 692b1db 
>   src/kar.h 85bd650 
>   src/kar.cpp 7204fb1 
>   src/karchive.h b528a4a 
>   src/karchive.cpp a1a160a 
>   src/karchive_p.h 256620d 
>   src/krcc.h 18c7d00 
>   src/krcc.cpp 1947dd6 
>   src/ktar.h 4bca898 
>   src/ktar.cpp f70b155 
>   src/kzip.h 84156c7 
>   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
> 
>

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

[Romário Rios]

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

(Updated Oct. 27, 2016, 2:23 a.m.)


Review request for KDE Frameworks.


Changes
---

Applies fixes proposed by David. Still does not apply measures to fully 
internationalize framework as proposed by Friederich, but I wanted to get a 
review of my newest changes sooner and make sure they are good to go before 
internationalizing.


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 (updated)
-

  autotests/karchivetest.cpp d0fbf41 
  src/k7zip.h 5b95f49 
  src/k7zip.cpp 692b1db 
  src/kar.h 85bd650 
  src/kar.cpp 7204fb1 
  src/karchive.h b528a4a 
  src/karchive.cpp a1a160a 
  src/karchive_p.h 256620d 
  src/krcc.h 18c7d00 
  src/krcc.cpp 1947dd6 
  src/ktar.h 4bca898 
  src/ktar.cpp f70b155 
  src/kzip.h 84156c7 
  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

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

[Luigi Toscano]

> On Ott. 15, 2016, 7:40 p.m., Friedrich W. H. Kossebau wrote:
> > Seems you are the first to add translation features to karchive. So some 
> > more things are needed to get translations working:
> > a) adding a script which enables e.g. the bot "scripty" on the KDE i18n 
> > server to extract the strings to translate from the sources, to feed them 
> > into the database used by the translators for their work
> > b) adding code which loads the matching translations at runtime into the Qt 
> > translation system, so any calls to tr() will be properly resolved.
> > c) adding logic to the buildsystem to install any translation files, if 
> > existing (added during packaging builds)
> > 
> > For background information see 
> > https://techbase.kde.org/Development/Tutorials/Localization/i18n_Build_System
> >  and especially the section 
> > https://techbase.kde.org/Development/Tutorials/Localization/i18n_Build_Systems#Qt5-only:_Code_using_Qt_translation_system
> > 
> > To get you started, for a) you might want to simply copy from the kcodecs 
> > repo the file src/Messages.sh into the same position in karchive. And for 
> > b) see the link above and https://api.kde.org/ecm/module/ECMPoQmTools.html 
> > which should give you an idea what to do.
> > For c) you add code like this to the toplevel CMakeLists.txt, compare again 
> > to how kcodecs does it:
> > ```
> > if (IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/po")
> > ecm_install_po_files_as_qm(po)
> > endif()
> > ```
> 
> Romário Rios wrote:
> Thanks for the info. I'll work on it when I have the time.

Sorry for hijacking the discussion, but we really need to have 

if (IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/po")
ecm_install_po_files_as_qm(po)
endif()


as policy? It seems that ecm_install_po_files_as_qm should work even if the po 
directory is not defined (the GLOB returns nothing and exits). ki18n_install 
and kdoctools_install do not fail either when po/ does not exist.


- Luigi


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


On Ott. 15, 2016, 5: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 Ott. 15, 2016, 5: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
> 
>

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

[Romário Rios]

> On Oct. 15, 2016, 5:40 p.m., Friedrich W. H. Kossebau wrote:
> > Seems you are the first to add translation features to karchive. So some 
> > more things are needed to get translations working:
> > a) adding a script which enables e.g. the bot "scripty" on the KDE i18n 
> > server to extract the strings to translate from the sources, to feed them 
> > into the database used by the translators for their work
> > b) adding code which loads the matching translations at runtime into the Qt 
> > translation system, so any calls to tr() will be properly resolved.
> > c) adding logic to the buildsystem to install any translation files, if 
> > existing (added during packaging builds)
> > 
> > For background information see 
> > https://techbase.kde.org/Development/Tutorials/Localization/i18n_Build_System
> >  and especially the section 
> > https://techbase.kde.org/Development/Tutorials/Localization/i18n_Build_Systems#Qt5-only:_Code_using_Qt_translation_system
> > 
> > To get you started, for a) you might want to simply copy from the kcodecs 
> > repo the file src/Messages.sh into the same position in karchive. And for 
> > b) see the link above and https://api.kde.org/ecm/module/ECMPoQmTools.html 
> > which should give you an idea what to do.
> > For c) you add code like this to the toplevel CMakeLists.txt, compare again 
> > to how kcodecs does it:
> > ```
> > if (IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/po")
> > ecm_install_po_files_as_qm(po)
> > endif()
> > ```

Thanks for the info. I'll work on it when I have the time.


- Romário


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


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

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

[Friedrich W. H. Kossebau]

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



Seems you are the first to add translation features to karchive. So some more 
things are needed to get translations working:
a) adding a script which enables e.g. the bot "scripty" on the KDE i18n server 
to extract the strings to translate from the sources, to feed them into the 
database used by the translators for their work
b) adding code which loads the matching translations at runtime into the Qt 
translation system, so any calls to tr() will be properly resolved.
c) adding logic to the buildsystem to install any translation files, if 
existing (added during packaging builds)

For background information see 
https://techbase.kde.org/Development/Tutorials/Localization/i18n_Build_System 
and especially the section 
https://techbase.kde.org/Development/Tutorials/Localization/i18n_Build_Systems#Qt5-only:_Code_using_Qt_translation_system

To get you started, for a) you might want to simply copy from the kcodecs repo 
the file src/Messages.sh into the same position in karchive. And for b) see the 
link above and https://api.kde.org/ecm/module/ECMPoQmTools.html which should 
give you an idea what to do.
For c) you add code like this to the toplevel CMakeLists.txt, compare again to 
how kcodecs does it:
```
if (IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/po")
ecm_install_po_files_as_qm(po)
endif()
```

- Friedrich W. H. Kossebau


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

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

[Romário Rios]

> On Oct. 15, 2016, 4:05 p.m., David Faure wrote:
> > src/k7zip.cpp, line 2343
> > 
> >
> > 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)
> 
> Friedrich W. H. Kossebau wrote:
> The ts -> po tools make extra efforts to indeed care about the context :) 
> Cmp. 
> https://websvn.kde.org/trunk/l10n-kf5/templates/messages/frameworks/kcoreaddons5_qt.pot?view=markup
>  and see the extra tags `#, qt-format` and `msgctxt "KAboutLicense|"`, which 
> lconvert then uses for creating qm files with the context as wanted.
> 
> So best no QObject::tr(), but using Q_DECLARE_TR_FUNCTIONS when needed, 
> indeed. Especially for apps linking to lots of libs that use 
> QTranslator-based translations, having different context is important to 
> avoid clashes for common terms.

Thanks for the info, I wasn't aware of `Q_DECLARE_TR_FUNCTIONS`.


> On Oct. 15, 2016, 4:05 p.m., David Faure wrote:
> > src/k7zip.cpp, line 2874
> > 
> >
> > 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;))

Why was the `qWarning` commented out in the first place, though? Should we 
bring it back?
> And the tr() string might need adjustment, this is shown the user, but the 
> "you" in the string isn't the user.

Makes sense.
> file a bug report at https://bugs.kde.org

I think this implies the application using the library is a KDE App, which 
isn't necessarily the case.


- Romário


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


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

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

[Friedrich W. H. Kossebau]

> On Oct. 15, 2016, 4:05 p.m., David Faure wrote:
> > src/k7zip.cpp, line 2343
> > 
> >
> > 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)

The ts -> po tools make extra efforts to indeed care about the context :) Cmp. 
https://websvn.kde.org/trunk/l10n-kf5/templates/messages/frameworks/kcoreaddons5_qt.pot?view=markup
 and see the extra tags `#, qt-format` and `msgctxt "KAboutLicense|"`, which 
lconvert then uses for creating qm files with the context as wanted.

So best no QObject::tr(), but using Q_DECLARE_TR_FUNCTIONS when needed, indeed. 
Especially for apps linking to lots of libs that use QTranslator-based 
translations, having different context is important to avoid clashes for common 
terms.


- Friedrich W. H.


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


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

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

[David Faure]

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




src/k7zip.cpp (line 2343)


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)


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)


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)


qWarning



src/k7zip.cpp (line 2951)


qWarning



src/k7zip.cpp (line 2956)


qWarning



src/kar.cpp (line 63)


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)


4-spaces indentation



src/karchive.cpp (line 220)


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)


tr("Writing failed: %1")



src/karchive_p.h (line 54)


missing tr()



src/krcc.cpp (line 90)


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)


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



src/ktar.cpp (line 376)


typo: underlying

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



src/ktar.cpp (line 585)


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



src/ktar.cpp (line 730)


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



src/ktar.cpp (line 735)


same



src/kzip.cpp (line 468)


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



src/kzip.cpp (line 786)


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



src/kzip.cpp (line 1043)


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



src/kzip.cpp (line 1207)


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)


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

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

[Romário Rios]

> On Oct. 14, 2016, 4:44 a.m., Anthony Fieroni wrote:
> >

Whoops, you're right. I forgot about that.


- Romário


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


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

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

[Romário Rios]

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


Changes
---

Make tests use translated strings


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 (updated)
-

  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

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

[Anthony Fieroni]

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




autotests/karchivetest.cpp (line 353)


If you run tests in other language rather than english they will fail, all 
of them.



src/k7zip.cpp (line 2369)


Not translated



src/k7zip.cpp (line 2588)


same



src/karchive_p.h (line 54)


same


- Anthony Fieroni


On Окт. 14, 2016, 7:29 преди обяд, Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Окт. 14, 2016, 7:29 преди обяд)
> 
> 
> 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
> 
>

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

[Romário Rios]

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

(Updated Oct. 14, 2016, 4:29 a.m.)


Review request for KDE Frameworks.


Changes
---

Make everything translatable and fix issues raised by dfaure


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 (updated)
-

  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

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

[Romário Rios]

> On Oct. 13, 2016, 6:58 a.m., Anthony Fieroni wrote:
> >

> Second, i don't think library should provide strings to translate, it's an 
> application job.

`QIODevice` provides an `errorString` method (although I don't know if it is 
translated) and I think other frameworks do so too, as well. I think I could 
provide an `error` method returning an enum as well, but the `errorString` 
method is very important to me because it provides more detail on the error 
than an error code.


> On Oct. 13, 2016, 6:58 a.m., Anthony Fieroni wrote:
> > src/karchive.h, line 292
> > 
> >
> > First of all you must not provide const char interface and you must use 
> > i18n in all strings.
> > Second, i don't think library should provide strings to translate, it's 
> > an application job.
> > So about me, like a programmer not a maintainer, interface should be 
> > setErrorCode and client application can show user help string.
> 
> Luigi Toscano wrote:
> Apart from the discussion of whether embedding the strings in the library 
> makes sense or not (maybe no), *if* this is accepted, the string should use 
> the Qt system and not (unfortunately) ki18n, because KArchive is a Tier 1 
> framework:
> 
> https://community.kde.org/Frameworks/Frameworks_Localization_Policy#Qt_installation_code

Thanks, Luigi. I'll work on that and update the patch.


- Romário


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


On Oct. 13, 2016, 4:45 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. 13, 2016, 4:45 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
> 
>

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

[Luigi Toscano]

> On Ott. 13, 2016, 8:58 a.m., Anthony Fieroni wrote:
> > src/karchive.h, line 292
> > 
> >
> > First of all you must not provide const char interface and you must use 
> > i18n in all strings.
> > Second, i don't think library should provide strings to translate, it's 
> > an application job.
> > So about me, like a programmer not a maintainer, interface should be 
> > setErrorCode and client application can show user help string.

Apart from the discussion of whether embedding the strings in the library makes 
sense or not (maybe no), *if* this is accepted, the string should use the Qt 
system and not (unfortunately) ki18n, because KArchive is a Tier 1 framework:
https://community.kde.org/Frameworks/Frameworks_Localization_Policy#Qt_installation_code


- Luigi


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


On Ott. 13, 2016, 6:45 a.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129170/
> ---
> 
> (Updated Ott. 13, 2016, 6:45 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
> 
>

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

[Anthony Fieroni]

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




src/karchive.h (line 292)


First of all you must not provide const char interface and you must use 
i18n in all strings.
Second, i don't think library should provide strings to translate, it's an 
application job.
So about me, like a programmer not a maintainer, interface should be 
setErrorCode and client application can show user help string.


- Anthony Fieroni


On Oct. 13, 2016, 7:45 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. 13, 2016, 7:45 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
> 
>