Re: Review Request 129170: Add KArchive::errorString() method to provide more details on KArchive errors
---
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
> 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
> 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
> 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
---
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
> On Oct. 30, 2016, 10:59 p.m., David Faure wrote:
> > src/k7zip.cpp, line 2499
> > <https://git.reviewboard.kde.org/r/129170/diff/4/?file=483000#file483000line2499>
> >
> > 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
---
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
> 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
> On Oct. 15, 2016, 4:05 p.m., David Faure wrote:
> > src/k7zip.cpp, line 2343
> > <https://git.reviewboard.kde.org/r/129170/diff/3/?file=482230#file482230line2343>
> >
> > 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
> > <https://git.reviewboard.kde.org/r/129170/diff/3/?file=482230#file482230line2874>
> >
> > 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
> 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
---
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
---
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
> 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
> > <https://git.reviewboard.kde.org/r/129170/diff/1/?file=482073#file482073line292>
> >
> > 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
>
>
Review Request 129170: Add KArchive::errorString() method to provide more details on KArchive errors
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129170/
---
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 125941: Add KCompressionDevice tests to KArchive
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125941/ --- (Updated Nov. 25, 2015, 9:19 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, Aleix Pol Gonzalez and David Faure. Changes --- Submitted with commit 31a735a3dfb75d9f345a326fe5dbfa3651647463 by Luiz Romário Santana Rios to branch master. Repository: karchive Description --- I recently noticed that using KTar with KCompressionDevice and QNetworkReply did not work, so I'm adding some tests to confirm that Diffs - autotests/CMakeLists.txt 1b2e819 autotests/kcompressiondevicetest.h PRE-CREATION autotests/kcompressiondevicetest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/125941/diff/ Testing --- The tests run and show that KCompressionDevice works properly with QBuffer when used with KTar Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125941: Add KCompressionDevice tests to KArchive
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125941/ --- (Updated Nov. 25, 2015, 9:16 p.m.) Review request for KDE Frameworks, Aleix Pol Gonzalez and David Faure. Changes --- Make Qt5Network optional Repository: karchive Description --- I recently noticed that using KTar with KCompressionDevice and QNetworkReply did not work, so I'm adding some tests to confirm that Diffs (updated) - autotests/CMakeLists.txt 1b2e819 autotests/kcompressiondevicetest.h PRE-CREATION autotests/kcompressiondevicetest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/125941/diff/ Testing --- The tests run and show that KCompressionDevice works properly with QBuffer when used with KTar Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125941: Add KCompressionDevice tests to KArchive
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125941/ --- (Updated Nov. 25, 2015, 3:12 p.m.) Review request for KDE Frameworks, Aleix Pol Gonzalez and David Faure. Changes --- Remove QNetworkReply tests without QBuffer for now Repository: karchive Description --- I recently noticed that using KTar with KCompressionDevice and QNetworkReply did not work, so I'm adding some tests to confirm that Diffs (updated) - autotests/CMakeLists.txt 1b2e819 autotests/kcompressiondevicetest.h PRE-CREATION autotests/kcompressiondevicetest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/125941/diff/ Testing (updated) --- The tests run and show that KCompressionDevice works properly with QBuffer when used with KTar Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125974: Make KTar KCompressionDevice-friendly
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125974/ --- (Updated Nov. 23, 2015, 2:57 p.m.) Status -- This change has been discarded. Review request for KDE Frameworks and Aleix Pol Gonzalez. Repository: karchive Description --- Up until now, since at least 5.12, decompressing some data coming directly from a QIODevice by using KCompressionDevice because this is a sequential device, and KTar and KArchive used to use QIODevice::seek and pos and some places, which made the decompression fail. This patch makes KTar sequential-friendly by replacing the calls to seek and pos with read and a simple counter, respectively. Diffs - src/karchive.cpp 0ece37c src/ktar.cpp 824395e Diff: https://git.reviewboard.kde.org/r/125974/diff/ Testing --- Makes the tests from review #125941 pass Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125974: Make KTar KCompressionDevice-friendly
> On Nov. 7, 2015, 9:25 a.m., David Faure wrote: > > BTW why create a KTar on top of a KCompressionDevice? KTar is able to > > handle compression automatically all by itself... and that's exactly the > > case where it will use a tempfile for the uncompressed data, making seeking > > work. This patch is unnecessary if you use KTar the intended way: if the > > filename doesn't end with .gz or .bz2, specify the mimetype explicitly in > > the KTar constructor. KCompressionDevice is supposed to be used when you want to extract data from a QIODevice directly instead of a file -- KTar only handles compression automatically if you pass it a filename. ATM, KCompressionDevice doesn't seem to work properly with many QIODevices -- not even with QBuffer. - Romário --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125974/#review88126 ------- On Nov. 6, 2015, 2:52 a.m., Romário Rios wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125974/ > --- > > (Updated Nov. 6, 2015, 2:52 a.m.) > > > Review request for KDE Frameworks and Aleix Pol Gonzalez. > > > Repository: karchive > > > Description > --- > > Up until now, since at least 5.12, decompressing some data coming directly > from a QIODevice by using KCompressionDevice because this is a sequential > device, and KTar and KArchive used to use QIODevice::seek and pos and some > places, which made the decompression fail. This patch makes KTar > sequential-friendly by replacing the calls to seek and pos with read and a > simple counter, respectively. > > > Diffs > - > > src/karchive.cpp 0ece37c > src/ktar.cpp 824395e > > Diff: https://git.reviewboard.kde.org/r/125974/diff/ > > > Testing > --- > > Makes the tests from review #125941 pass > > > Thanks, > > Romário Rios > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125974: Make KTar KCompressionDevice-friendly
> On Nov. 6, 2015, 7:52 a.m., David Faure wrote: > > src/karchive.cpp, line 652 > > <https://git.reviewboard.kde.org/r/125974/diff/1/?file=415544#file415544line652> > > > > This is not equivalent to seek(d->pos). > > seek(d->pos) goes to the absolute position d->pos, counting from the > > beginning of the device. > > > > read(d->pos) goes to the absolute position (current position + d->pos). > > > > If this is seeking forward, you could emulate it with a dummy read, but > > what if this is seeking backward? That's not possible on a sequential > > device, so my advice is, forget using a sequential device here, download to > > a temp file. > > Romário Rios wrote: > Actually, this is irrelevant to the issue. I reverted this code back to > the original and the fix still works. > > You're right about the situation in this case, but, inside > KTar::openArchive, it works because it originally only seeks the device > forward. > > David Faure wrote: > Maybe but if you call KArchiveFile::data() on the last file in the > archive, and then on the first file in the archive, seeking back is obviously > needed. When would that happen? KArchiveFile::data() isn't ever called from inside KTar::openArchive(). - Romário --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125974/#review88078 --- On Nov. 6, 2015, 2:52 a.m., Romário Rios wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125974/ > --- > > (Updated Nov. 6, 2015, 2:52 a.m.) > > > Review request for KDE Frameworks and Aleix Pol Gonzalez. > > > Repository: karchive > > > Description > --- > > Up until now, since at least 5.12, decompressing some data coming directly > from a QIODevice by using KCompressionDevice because this is a sequential > device, and KTar and KArchive used to use QIODevice::seek and pos and some > places, which made the decompression fail. This patch makes KTar > sequential-friendly by replacing the calls to seek and pos with read and a > simple counter, respectively. > > > Diffs > ----- > > src/karchive.cpp 0ece37c > src/ktar.cpp 824395e > > Diff: https://git.reviewboard.kde.org/r/125974/diff/ > > > Testing > --- > > Makes the tests from review #125941 pass > > > Thanks, > > Romário Rios > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125974: Make KTar KCompressionDevice-friendly
> On Nov. 6, 2015, 7:52 a.m., David Faure wrote: > > src/karchive.cpp, line 652 > > <https://git.reviewboard.kde.org/r/125974/diff/1/?file=415544#file415544line652> > > > > This is not equivalent to seek(d->pos). > > seek(d->pos) goes to the absolute position d->pos, counting from the > > beginning of the device. > > > > read(d->pos) goes to the absolute position (current position + d->pos). > > > > If this is seeking forward, you could emulate it with a dummy read, but > > what if this is seeking backward? That's not possible on a sequential > > device, so my advice is, forget using a sequential device here, download to > > a temp file. Actually, this is irrelevant to the issue. I reverted this code back to the original and the fix still works. You're right about the situation in this case, but, inside KTar::openArchive, it works because it originally only seeks the device forward. - Romário --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125974/#review88078 --- On Nov. 6, 2015, 2:52 a.m., Romário Rios wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125974/ > --- > > (Updated Nov. 6, 2015, 2:52 a.m.) > > > Review request for KDE Frameworks and Aleix Pol Gonzalez. > > > Repository: karchive > > > Description > --- > > Up until now, since at least 5.12, decompressing some data coming directly > from a QIODevice by using KCompressionDevice because this is a sequential > device, and KTar and KArchive used to use QIODevice::seek and pos and some > places, which made the decompression fail. This patch makes KTar > sequential-friendly by replacing the calls to seek and pos with read and a > simple counter, respectively. > > > Diffs > - > > src/karchive.cpp 0ece37c > src/ktar.cpp 824395e > > Diff: https://git.reviewboard.kde.org/r/125974/diff/ > > > Testing > --- > > Makes the tests from review #125941 pass > > > Thanks, > > Romário Rios > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 125974: Make KTar KCompressionDevice-friendly
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125974/ --- Review request for KDE Frameworks and Aleix Pol Gonzalez. Repository: karchive Description --- Up until now, since at least 5.12, decompressing some data coming directly from a QIODevice by using KCompressionDevice because this is a sequential device, and KTar and KArchive used to use QIODevice::seek and pos and some places, which made the decompression fail. This patch makes KTar sequential-friendly by replacing the calls to seek and pos with read and a simple counter, respectively. Diffs - src/karchive.cpp 0ece37c src/ktar.cpp 824395e Diff: https://git.reviewboard.kde.org/r/125974/diff/ Testing --- Makes the tests from review #125941 pass Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 125941: Add KCompressionDevice tests to KArchive
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125941/#review88018 --- autotests/kcompressiondevicetest.cpp (line 29) <https://git.reviewboard.kde.org/r/125941/#comment60401> I don't believe it can. It can do the opposite -- CompressionType by filename or mimetype --, but not this. - Romário Rios On Nov. 4, 2015, 2:08 a.m., Romário Rios wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125941/ > --- > > (Updated Nov. 4, 2015, 2:08 a.m.) > > > Review request for KDE Frameworks and Aleix Pol Gonzalez. > > > Repository: karchive > > > Description > --- > > I recently noticed that using KTar with KCompressionDevice and QNetworkReply > did not work, so I'm adding some tests to confirm that > > > Diffs > - > > autotests/CMakeLists.txt 1b2e819 > autotests/kcompressiondevicetest.h PRE-CREATION > autotests/kcompressiondevicetest.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/125941/diff/ > > > Testing > --- > > The tests run and show that KCompressionDevice isn't working as expected > > > Thanks, > > Romário Rios > > ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 125941: Add KCompressionDevice tests to KArchive
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125941/ --- Review request for KDE Frameworks and Aleix Pol Gonzalez. Repository: karchive Description --- I recently noticed that using KTar with KCompressionDevice and QNetworkReply did not work, so I'm adding some tests to confirm that Diffs - autotests/CMakeLists.txt 1b2e819 autotests/kcompressiondevicetest.h PRE-CREATION autotests/kcompressiondevicetest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/125941/diff/ Testing --- The tests run and show that KCompressionDevice isn't working as expected Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124165: Make KArchive preserve executable permissions from the files inside the archive
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124165/ --- (Updated June 29, 2015, 9:39 a.m.) Review request for KDE Frameworks. Changes --- Remove unnecessary casts Repository: karchive Description --- The KArchiveFile::copyTo() doesn't set the permissions of the files it copies -- it just copies the data. So if an archive contains an executable, it's extracted from the archive as a non-executable. This patch makes it apply the executable permissions found in the KArchiveFile to the QFile being copied. Diffs (updated) - autotests/karchivetest.cpp afc8387 src/karchive.cpp 344cba8 Diff: https://git.reviewboard.kde.org/r/124165/diff/ Testing --- Seems to work for all files (tested with xz and zip files containing executables, directories, text, etc.). Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124165: Make KArchive preserve executable permissions from the files inside the archive
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124165/ --- (Updated June 29, 2015, 9:54 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Changes --- Submitted with commit c0d40446bb2edd42ae6be061cdd17964bba147ca by Luiz Romário Santana Rios to branch master. Repository: karchive Description --- The KArchiveFile::copyTo() doesn't set the permissions of the files it copies -- it just copies the data. So if an archive contains an executable, it's extracted from the archive as a non-executable. This patch makes it apply the executable permissions found in the KArchiveFile to the QFile being copied. Diffs - autotests/karchivetest.cpp afc8387 src/karchive.cpp 344cba8 Diff: https://git.reviewboard.kde.org/r/124165/diff/ Testing --- Seems to work for all files (tested with xz and zip files containing executables, directories, text, etc.). Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124165: Make KArchive preserve executable permissions from the files inside the archive
On June 29, 2015, 8:40 a.m., David Faure wrote:
src/karchive.cpp, line 684
https://git.reviewboard.kde.org/r/124165/diff/3/?file=381630#file381630line684
The /8 /8 %2 bit looks too arithmetic to me, this should be 'bitwise
and' instead (faster, more approriate here, and IMHO more readable).
We can't use S_IXUSR(perms), S_IXGRP(perms) and S_IXOTH(perms) due to
windows portability, so I would suggest this instead:
if (perm 01)
ret |= QFileDevice::ExeOther;
if (perm 010)
ret |= QFileDevice::ExeGroup;
if (perm 0100)
ret |= QFileDevice::ExeOwner;
Also, QFileDevice::Permissions is a QFlags, so no casting needed.
Declare
QFileDevice::Permission ret = filePerms;
and return ret;
Also, QFileDevice::Permissions is a QFlags, so no casting needed. Declare
QFileDevice::Permission ret = filePerms;
and return ret;
About that...:
```
[ 31%] Building CXX object src/CMakeFiles/KF5Archive.dir/karchive.cpp.o
/home/luizromario/karchive/src/karchive.cpp: In function
'QFileDevice::Permission withExecutablePerms(QFileDevice::Permissions, mode_t)':
/home/luizromario/karchive/src/karchive.cpp:682:35: error: invalid conversion
from 'QFlagsQFileDevice::Permission::Int {aka int}' to
'QFileDevice::Permission' [-fpermissive]
/home/luizromario/karchive/src/karchive.cpp:685:27: error: invalid conversion
from 'int' to 'QFileDevice::Permission' [-fpermissive]
/home/luizromario/karchive/src/karchive.cpp:688:27: error: invalid conversion
from 'int' to 'QFileDevice::Permission' [-fpermissive]
/home/luizromario/karchive/src/karchive.cpp:691:27: error: invalid conversion
from 'int' to 'QFileDevice::Permission' [-fpermissive]
```
- Romário
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124165/#review81846
---
On June 28, 2015, 10:31 a.m., Romário Rios wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124165/
---
(Updated June 28, 2015, 10:31 a.m.)
Review request for KDE Frameworks.
Repository: karchive
Description
---
The KArchiveFile::copyTo() doesn't set the permissions of the files it copies
-- it just copies the data. So if an archive contains an executable, it's
extracted from the archive as a non-executable. This patch makes it apply the
executable permissions found in the KArchiveFile to the QFile being copied.
Diffs
-
autotests/karchivetest.cpp afc8387
src/karchive.cpp 344cba8
Diff: https://git.reviewboard.kde.org/r/124165/diff/
Testing
---
Seems to work for all files (tested with xz and zip files containing
executables, directories, text, etc.).
Thanks,
Romário Rios
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124165: Make KArchive preserve executable permissions from the files inside the archive
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124165/ --- (Updated June 28, 2015, 10:31 a.m.) Review request for KDE Frameworks. Changes --- Changed to the three ifs and also added tests Repository: karchive Description --- The KArchiveFile::copyTo() doesn't set the permissions of the files it copies -- it just copies the data. So if an archive contains an executable, it's extracted from the archive as a non-executable. This patch makes it apply the executable permissions found in the KArchiveFile to the QFile being copied. Diffs (updated) - autotests/karchivetest.cpp afc8387 src/karchive.cpp 344cba8 Diff: https://git.reviewboard.kde.org/r/124165/diff/ Testing --- Seems to work for all files (tested with xz and zip files containing executables, directories, text, etc.). Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124165: Make KArchive preserve executable permissions from the files inside the archive
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124165/ --- (Updated June 25, 2015, 12:44 p.m.) Review request for KDE Frameworks. Changes --- Changing patch as requested by dfaure Summary (updated) - Make KArchive preserve executable permissions from the files inside the archive Repository: karchive Description (updated) --- The KArchiveFile::copyTo() doesn't set the permissions of the files it copies -- it just copies the data. So if an archive contains an executable, it's extracted from the archive as a non-executable. This patch makes it apply the executable permissions found in the KArchiveFile to the QFile being copied. Diffs (updated) - src/karchive.cpp 344cba8 Diff: https://git.reviewboard.kde.org/r/124165/diff/ Testing --- Seems to work for all files (tested with xz and zip files containing executables, directories, text, etc.). Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 124165: Make KArchive preserve permissions from the files inside the archive
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124165/ --- (Updated June 24, 2015, 5 p.m.) Review request for KDE Frameworks. Repository: karchive Description --- The KArchiveFile::copyTo() wasn't setting the permissions of the files it copied -- it just copied the data. So, for instance, if an archive contained an executable, it was extracted from the archive as a non-executable. This patch makes it apply the permission information found in the KArchiveFile to the QFile being copied. Diffs - src/karchive.cpp 344cba8 Diff: https://git.reviewboard.kde.org/r/124165/diff/ Testing --- Seems to work for all files (tested with xz and zip files containing executables, directories, text, etc.). Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 111319: Move kmessagewidget to kwidgetsaddons
On July 1, 2013, 3:08 p.m., Kevin Ottens wrote: tier1/kwidgetsaddons/src/kmessagewidget.cpp, line 90 http://git.reviewboard.kde.org/r/111319/diff/1/?file=166799#file166799line90 Could be changed to Close this message I guess. Close document has always been weird in that context after all. :-) Yeah, I also found it a bit weird, but, since that's how it's always been, I didn't touch it. On July 1, 2013, 3:08 p.m., Kevin Ottens wrote: tier1/kwidgetsaddons/src/kmessagewidget.cpp, line 269 http://git.reviewboard.kde.org/r/111319/diff/1/?file=166799#file166799line269 I think it's more readable if you keep the variable declaration as it was: QColor bg0, bg1, bg2, border, fg; It's kind of odd to have bg1 declared separately and then the other ones. Personally, I don't like all declarations on top to then initialize the variables afterwards, and I only do this with bg1 because it's necessary in that case -- bg1 changes in the switch right below it. But, yeah, changing the coding style is also not very nice, so, reverting. - Romário --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111319/#review35367 --- On June 29, 2013, 8:24 p.m., Romário Rios wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111319/ --- (Updated June 29, 2013, 8:24 p.m.) Review request for KDE Frameworks. Description --- Moves kmessagewidget to kwidgetsaddons framework and removes all KDE dependencies from it. I don't know how to get the Close button shortcuts the same way it was gotten in kglobalactions, but I don't know if it's necessary in the case of this widget specifically. Diffs - kdeui/CMakeLists.txt ab43f17 kdeui/tests/CMakeLists.txt f489b6b kdeui/tests/kmessagewidgettest.cpp eb151b9 kdeui/widgets/kmessagewidget.h 9fbb176 kdeui/widgets/kmessagewidget.cpp 7fb91e8 tier1/kwidgetsaddons/src/CMakeLists.txt 5a37efa tier1/kwidgetsaddons/src/kmessagewidget.h PRE-CREATION tier1/kwidgetsaddons/src/kmessagewidget.cpp PRE-CREATION tier1/kwidgetsaddons/tests/CMakeLists.txt 9738c6c tier1/kwidgetsaddons/tests/kmessagewidgettest.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111319/diff/ Testing --- Compiles, test runs fine. Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 111319: Move kmessagewidget to kwidgetsaddons
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111319/ --- Review request for KDE Frameworks. Description --- Moves kmessagewidget to kwidgetsaddons framework and removes all KDE dependencies from it. I don't know how to get the Close button shortcuts the same way it was gotten in kglobalactions, but I don't know if it's necessary in the case of this widget specifically. Diffs - kdeui/CMakeLists.txt ab43f17 kdeui/tests/CMakeLists.txt f489b6b kdeui/tests/kmessagewidgettest.cpp eb151b9 kdeui/widgets/kmessagewidget.h 9fbb176 kdeui/widgets/kmessagewidget.cpp 7fb91e8 tier1/kwidgetsaddons/src/CMakeLists.txt 5a37efa tier1/kwidgetsaddons/src/kmessagewidget.h PRE-CREATION tier1/kwidgetsaddons/src/kmessagewidget.cpp PRE-CREATION tier1/kwidgetsaddons/tests/CMakeLists.txt 9738c6c tier1/kwidgetsaddons/tests/kmessagewidgettest.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111319/diff/ Testing --- Compiles, test runs fine. Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 111090: Deprecate kdeui/widgets/kstringvalidator.* and kio/kfile/kfilemetainfowidget.*
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111090/ --- Review request for KDE Frameworks and kdelibs. Description --- Since, according to lxr, both these classes aren't used by anyone, I just marked them as deprecated and moved them to kde4support. Diffs - kdeui/CMakeLists.txt cb30283 kdeui/widgets/kstringvalidator.h 01dbec6 kdeui/widgets/kstringvalidator.cpp c4fcc21 kio/CMakeLists.txt 9254283 kio/kfile/kfilemetainfowidget.h eaebcc8 kio/kfile/kfilemetainfowidget.cpp d0f74df staging/kde4support/src/CMakeLists.txt f89e845 staging/kde4support/src/kdeui/kstringvalidator.h PRE-CREATION staging/kde4support/src/kdeui/kstringvalidator.cpp PRE-CREATION staging/kde4support/src/kio/kfilemetainfowidget.h PRE-CREATION staging/kde4support/src/kio/kfilemetainfowidget.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/111090/diff/ Testing --- kdelibs compiles after the move. There weren't any tests for them anywhere and I didn't bother to write any, since they're being deprecated -- I assume that's OK. Thanks, Romário Rios ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
