Re: Review Request 120075: KArchive: add support for rcc files
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120075/#review68263
---
Ship it!
some small nitpicks from my side :)
src/krcc.h
https://git.reviewboard.kde.org/r/120075/#comment47565
ar file? maybe archive instead?
src/krcc.h
https://git.reviewboard.kde.org/r/120075/#comment47566
here and below: Writing *is* not supported by this class, *it* will always
fail.
src/krcc.h
https://git.reviewboard.kde.org/r/120075/#comment47568
scoped pointer?
src/krcc.cpp
https://git.reviewboard.kde.org/r/120075/#comment47567
here and below: the { should go to its own line, no?
src/krcc.cpp
https://git.reviewboard.kde.org/r/120075/#comment47569
maybe I'm missing something, but this looks like you don't need to override
it at all when you just delegate to the base class anyways?
tests/krcctest.cpp
https://git.reviewboard.kde.org/r/120075/#comment47570
I knew it, you are a time traveler! :P
- Milian Wolff
On Oct. 10, 2014, 8:46 p.m., David Faure wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120075/
---
(Updated Oct. 10, 2014, 8:46 p.m.)
Review request for KDE Frameworks, Mario Bensi and Kevin Krammer.
Repository: karchive
Description
---
These the dynamic binary resources created by Qt's rcc tool from a .qrc file
and the files it points to.
[ChangeLog] KArchive: add support for rcc files
The idea came up while talking to the GCompris author, who mentionned he's
using .rcc files quite a lot in his application and was missing a way to look
into such a file to find out what it contains. Now with very little work, ark
could support these too. It just needs to be ported to KF5 first :-)
Diffs
-
autotests/karchivetest.h 567be075758f6afd4bdff5a2f6f8ef5e392a53eb
autotests/karchivetest.cpp 6b50f32a6d99756ba246262067dd71573f129256
autotests/runtime_resource.rcc PRE-CREATION
src/CMakeLists.txt 948a25b5e017a11c1577c7c324f2fa3545b13f52
src/krcc.h PRE-CREATION
src/krcc.cpp PRE-CREATION
tests/CMakeLists.txt ebcd01f5bfdec1298bbc936b9429b187f50c9b45
tests/krcctest.cpp PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/120075/diff/
Testing
---
Manual test prog, autotest.
Thanks,
David Faure
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120075: KArchive: add support for rcc files
On Oct. 11, 2014, 2:38 p.m., Milian Wolff wrote: src/krcc.h, line 44 https://git.reviewboard.kde.org/r/120075/diff/1/?file=310011#file310011line44 ar file? maybe archive instead? Copy/paste error from kar.h :-) On Oct. 11, 2014, 2:38 p.m., Milian Wolff wrote: src/krcc.h, line 52 https://git.reviewboard.kde.org/r/120075/diff/1/?file=310011#file310011line52 here and below: Writing *is* not supported by this class, *it* will always fail. also pasted from kar.h - fixed there too. On Oct. 11, 2014, 2:38 p.m., Milian Wolff wrote: src/krcc.h, line 93 https://git.reviewboard.kde.org/r/120075/diff/1/?file=310011#file310011line93 scoped pointer? Hmm. Not done anywhere else in karchive or frameworks in general. Not much point, now that I checked the delete is there :-) On Oct. 11, 2014, 2:38 p.m., Milian Wolff wrote: src/krcc.cpp, line 154 https://git.reviewboard.kde.org/r/120075/diff/1/?file=310012#file310012line154 maybe I'm missing something, but this looks like you don't need to override it at all when you just delegate to the base class anyways? The whole point is to be able to extend this later without requiring an app recompilation. On Oct. 11, 2014, 2:38 p.m., Milian Wolff wrote: tests/krcctest.cpp, line 2 https://git.reviewboard.kde.org/r/120075/diff/1/?file=310014#file310014line2 I knew it, you are a time traveler! :P LOL :-) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120075/#review68263 --- On Oct. 10, 2014, 8:46 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120075/ --- (Updated Oct. 10, 2014, 8:46 p.m.) Review request for KDE Frameworks, Mario Bensi and Kevin Krammer. Repository: karchive Description --- These the dynamic binary resources created by Qt's rcc tool from a .qrc file and the files it points to. [ChangeLog] KArchive: add support for rcc files The idea came up while talking to the GCompris author, who mentionned he's using .rcc files quite a lot in his application and was missing a way to look into such a file to find out what it contains. Now with very little work, ark could support these too. It just needs to be ported to KF5 first :-) Diffs - autotests/karchivetest.h 567be075758f6afd4bdff5a2f6f8ef5e392a53eb autotests/karchivetest.cpp 6b50f32a6d99756ba246262067dd71573f129256 autotests/runtime_resource.rcc PRE-CREATION src/CMakeLists.txt 948a25b5e017a11c1577c7c324f2fa3545b13f52 src/krcc.h PRE-CREATION src/krcc.cpp PRE-CREATION tests/CMakeLists.txt ebcd01f5bfdec1298bbc936b9429b187f50c9b45 tests/krcctest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120075/diff/ Testing --- Manual test prog, autotest. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120075: KArchive: add support for rcc files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120075/ --- (Updated Oct. 11, 2014, 3:28 p.m.) Review request for KDE Frameworks, Mario Bensi and Kevin Krammer. Repository: karchive Description (updated) --- These the dynamic binary resources created by Qt's rcc tool from a .qrc file and the files it points to. [ChangeLog] KArchive: add support for rcc files REVIEW: 120075 Diffs (updated) - src/CMakeLists.txt 948a25b5e017a11c1577c7c324f2fa3545b13f52 src/krcc.h PRE-CREATION src/krcc.cpp PRE-CREATION tests/CMakeLists.txt ebcd01f5bfdec1298bbc936b9429b187f50c9b45 tests/krcctest.cpp PRE-CREATION autotests/karchivetest.h 567be075758f6afd4bdff5a2f6f8ef5e392a53eb autotests/karchivetest.cpp 6b50f32a6d99756ba246262067dd71573f129256 autotests/runtime_resource.rcc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120075/diff/ Testing --- Manual test prog, autotest. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120075: KArchive: add support for rcc files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120075/#review68270 --- Ship it! Ship It! - Milian Wolff On Oct. 11, 2014, 3:28 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120075/ --- (Updated Oct. 11, 2014, 3:28 p.m.) Review request for KDE Frameworks, Mario Bensi and Kevin Krammer. Repository: karchive Description --- These the dynamic binary resources created by Qt's rcc tool from a .qrc file and the files it points to. [ChangeLog] KArchive: add support for rcc files REVIEW: 120075 Diffs - src/CMakeLists.txt 948a25b5e017a11c1577c7c324f2fa3545b13f52 src/krcc.h PRE-CREATION src/krcc.cpp PRE-CREATION tests/CMakeLists.txt ebcd01f5bfdec1298bbc936b9429b187f50c9b45 tests/krcctest.cpp PRE-CREATION autotests/karchivetest.h 567be075758f6afd4bdff5a2f6f8ef5e392a53eb autotests/karchivetest.cpp 6b50f32a6d99756ba246262067dd71573f129256 autotests/runtime_resource.rcc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120075/diff/ Testing --- Manual test prog, autotest. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120075: KArchive: add support for rcc files
On Oct. 11, 2014, 2:38 p.m., Milian Wolff wrote: src/krcc.h, line 93 https://git.reviewboard.kde.org/r/120075/diff/1/?file=310011#file310011line93 scoped pointer? David Faure wrote: Hmm. Not done anywhere else in karchive or frameworks in general. Not much point, now that I checked the delete is there :-) You should start using smart pointers, and aim for the goal to never write a manual delete and new eventually. On Oct. 11, 2014, 2:38 p.m., Milian Wolff wrote: src/krcc.cpp, line 154 https://git.reviewboard.kde.org/r/120075/diff/1/?file=310012#file310012line154 maybe I'm missing something, but this looks like you don't need to override it at all when you just delegate to the base class anyways? David Faure wrote: The whole point is to be able to extend this later without requiring an app recompilation. as I said, maybe I'm missing something, but here I come again: the base function is virtual, so it will always go through the vtable, no matter whether you added an overload or not. you'll only ever need to recompile karchive when you add functionality to it, and that you'll need to do no matter what since you'll need to add code to this function in such a case. - Milian --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120075/#review68263 --- On Oct. 11, 2014, 3:37 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120075/ --- (Updated Oct. 11, 2014, 3:37 p.m.) Review request for KDE Frameworks, Mario Bensi and Kevin Krammer. Repository: karchive Description --- These the dynamic binary resources created by Qt's rcc tool from a .qrc file and the files it points to. [ChangeLog] KArchive: add support for rcc files REVIEW: 120075 Diffs - src/CMakeLists.txt 948a25b5e017a11c1577c7c324f2fa3545b13f52 src/krcc.h PRE-CREATION src/krcc.cpp PRE-CREATION tests/CMakeLists.txt ebcd01f5bfdec1298bbc936b9429b187f50c9b45 tests/krcctest.cpp PRE-CREATION autotests/karchivetest.h 567be075758f6afd4bdff5a2f6f8ef5e392a53eb autotests/karchivetest.cpp 6b50f32a6d99756ba246262067dd71573f129256 autotests/runtime_resource.rcc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120075/diff/ Testing --- Manual test prog, autotest. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120075: KArchive: add support for rcc files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120075/ --- (Updated Oct. 11, 2014, 3:37 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks, Mario Bensi and Kevin Krammer. Repository: karchive Description --- These the dynamic binary resources created by Qt's rcc tool from a .qrc file and the files it points to. [ChangeLog] KArchive: add support for rcc files REVIEW: 120075 Diffs - src/CMakeLists.txt 948a25b5e017a11c1577c7c324f2fa3545b13f52 src/krcc.h PRE-CREATION src/krcc.cpp PRE-CREATION tests/CMakeLists.txt ebcd01f5bfdec1298bbc936b9429b187f50c9b45 tests/krcctest.cpp PRE-CREATION autotests/karchivetest.h 567be075758f6afd4bdff5a2f6f8ef5e392a53eb autotests/karchivetest.cpp 6b50f32a6d99756ba246262067dd71573f129256 autotests/runtime_resource.rcc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120075/diff/ Testing --- Manual test prog, autotest. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 120075: KArchive: add support for rcc files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120075/ --- (Updated Oct. 10, 2014, 8:46 p.m.) Review request for KDE Frameworks, Mario Bensi and Kevin Krammer. Repository: karchive Description --- These the dynamic binary resources created by Qt's rcc tool from a .qrc file and the files it points to. [ChangeLog] KArchive: add support for rcc files The idea came up while talking to the GCompris author, who mentionned he's using .rcc files quite a lot in his application and was missing a way to look into such a file to find out what it contains. Now with very little work, ark could support these too. It just needs to be ported to KF5 first :-) Diffs - autotests/karchivetest.h 567be075758f6afd4bdff5a2f6f8ef5e392a53eb autotests/karchivetest.cpp 6b50f32a6d99756ba246262067dd71573f129256 autotests/runtime_resource.rcc PRE-CREATION src/CMakeLists.txt 948a25b5e017a11c1577c7c324f2fa3545b13f52 src/krcc.h PRE-CREATION src/krcc.cpp PRE-CREATION tests/CMakeLists.txt ebcd01f5bfdec1298bbc936b9429b187f50c9b45 tests/krcctest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120075/diff/ Testing --- Manual test prog, autotest. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 120075: KArchive: add support for rcc files
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120075/ --- Review request for KDE Frameworks. Repository: karchive Description --- These the dynamic binary resources created by Qt's rcc tool from a .qrc file and the files it points to. [ChangeLog] KArchive: add support for rcc files The idea came up while talking to the GCompris author, who mentionned he's using .rcc files quite a lot in his application and was missing a way to look into such a file to find out what it contains. Now with very little work, ark could support these too. It just needs to be ported to KF5 first :-) Diffs - autotests/karchivetest.h 567be075758f6afd4bdff5a2f6f8ef5e392a53eb autotests/karchivetest.cpp 6b50f32a6d99756ba246262067dd71573f129256 autotests/runtime_resource.rcc PRE-CREATION src/CMakeLists.txt 948a25b5e017a11c1577c7c324f2fa3545b13f52 src/krcc.h PRE-CREATION src/krcc.cpp PRE-CREATION tests/CMakeLists.txt ebcd01f5bfdec1298bbc936b9429b187f50c9b45 tests/krcctest.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/120075/diff/ Testing --- Manual test prog, autotest. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
