Review Request 125974: Make KTar KCompressionDevice-friendly

[Luiz Romário Santana Rios]

2015-11-25 5:08 GMT-03:00 David Faure <faure at kde.org>:
> On Tuesday 24 November 2015 23:38:28 David Faure wrote:
>> On Monday 23 November 2015 12:11:12 Luiz Romário Santana Rios wrote:
>> > 2015-11-21 21:02 GMT-03:00 David Faure <faure at kde.org>:
>> > >
>> > > This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125974/
>> > >
>> > > On November 7th, 2015, 9:25 a.m. UTC, 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.
>> > >
>> > > On November 21st, 2015, 7:11 p.m. UTC, Romário Rios wrote:
>> > >
>> > > 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.
>> > >
>> > > KCompressionDevice should certainly work on top of a QBuffer. I just committed a unittest that shows it working (but of course there might be a bug in some other way to use it, feel free to show me in which case it doesn't work).
>> >
>> > Please take a look at my patch from the #125941 review request:
>> > https://git.reviewboard.kde.org/r/125941/diff/2#2
>> > It seems none of the test*BufferedNetworkReplyDevice tests work.
>>
>> Ah, I see. I debugged the issue and came up with this patch:
>>
>> diff --git a/src/kcompressiondevice.cpp b/src/kcompressiondevice.cpp
>> index 77d7deb..aeefe6a 100644
>> --- a/src/kcompressiondevice.cpp
>> +++ b/src/kcompressiondevice.cpp
>> @@ -196,7 +196,7 @@ bool KCompressionDevice::seek(qint64 pos)
>>          return d->filter->device()->reset();
>>      }
>>
>> -    if (ioIndex > pos) { // we can start from here
>> +    if (ioIndex < pos) { // we can start from here
>>          pos = pos - ioIndex;
>>      } else {
>>          // we have to start from 0 ! Ugly and slow, but better than the previous
>>
>>
>> This fixes the runtime warnings but somehow openArchive returns false, I'm confused as to why,
>> but it's time for bed, more next time.
>
> Found another bug in this same method, clearly KCompressionDevice::seek had never
> been called before your unittest ;)

Thanks for the effort put into looking for that bug.

>
> Here's the fix:
> http://commits.kde.org/karchive/b31b66f4ac0f0941fbf784cc6aeba7190dbd78bf
> Now the *Buffered methods of your unittests pass. Can you commit that unittest,
> at least the *Buffered methods? Don't forget to add a license+copyright header.
> Ah and rename these methods to remove "NetworkReply" from their name,
> they really test KCompressionDevice with QBuffer, there's no networkreply in the mix.

I uploaded a new diff to reviewboard.

Btw, the commit 0f0230f7d2feeca7ed00072e7b17b24c14f53698 ("Fix clang
warnings") makes the compilation fail in my machine. In the
KGzipFilter::setInBuffer() method, you change a C-cast to (Bytef *) to
a reinterpret_cast<const Bytef *>, but you're attributing it to
d->zStream.next_in, which is a char *. To make it work, I had to
change it to const_cast<Bytef *>(reinterpret_cast<const Bytef
*>(data)). Is there a better way to do this.

>
> Now that things work with a QBuffer, we can talk about QNetworkReply again :)

Good :). I already started talking about it in my original thread to
the mailing list, check it out.

>
> --
> David Faure, faure at kde.org, http://www.davidfaure.fr
> Working on KDE Frameworks 5
>

-- 
Luiz Romário Santana Rios