<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/129275/">https://git.reviewboard.kde.org/r/129275/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 29th, 2016, 7:49 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/129275/diff/2/?file=483171#file483171line283" style="color: black; font-weight: bold; text-decoration: underline;">autotests/jobtest.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">283</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QVERIFY</span><span class="p">(</span><span class="n">tempFile</span><span class="p">.</span><span class="n">flush</span><span class="p">());</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's stream.flush() that you need, to tell QTextStream to do the actual writing to the underlying device.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QTextStream only flushes automatically when being deleted or when the underlying device is a file and is closed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Once doing stream.flush() here, this test fails like the other one, so this isn't related to QXmlStreamWriter, but to passing an open QTemporaryFile as the device to storedPut. We need to at least rewind it to pos 0...</p></pre>
</blockquote>
<p>On October 29th, 2016, 7:51 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Confirmed, tempFile.seek(0) before KIO::storedPut() fixes it.</p></pre>
</blockquote>
<p>On October 30th, 2016, 10:21 a.m. UTC, <b>Elvis Angelaccio</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, it this something that the job should do or the clients should do before calling KIO::storedPut()?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">IMHO it's something the clients should do. When you pass a QIODevice to a method, you expect the method to read from the current position. If you're being clever and you're reusing the same QIODevice for writing and for reading, then you have to rewind your iodevice. Let's see if there's a precedent in Qt... Well, yeah, QDataStream(QIODevice *) for instance, it does not seek(0) automatically, there are many cases where you wouldn't want that. In fact I can find more examples of that, and none where an implicit seek(0) happens.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Maybe you would find it more intuitive to close and reopen the device, that would work too, AFAICS.
seek(0) only works here because QTemporaryFile uses ReadWrite mode. If you were using QFile, and you had a bit of code to write and then a bit of code to re-read what you wrote, then you'd have to either:
- use ReadWrite and seek(0)
- use WriteOnly, close, reopen ReadOnly.
So it's the same here with QTemporaryFile, you need one of those two options.</p></pre>
<br />
<p>- David</p>
<br />
<p>On October 30th, 2016, 10:20 a.m. UTC, Elvis Angelaccio wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and David Faure.</div>
<div>By Elvis Angelaccio.</div>
<p style="color: grey;"><i>Updated Oct. 30, 2016, 10:20 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">https://bugs.kde.org/show_bug.cgi?id=368314 seems to be an actual bug in KIO, rather than a regression in kwalletmanager. I reproduced the bug in a test case: basically <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">StoredTransferJob</code> fails to copy a QTemporaryFile when the file has been written by a QXmlStreamWriter (destination file is created, but empty).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If we write to the temp file using a QTextStreamer instead, everything works as expected...</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">jobtest still passes (with one expected failure).</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>autotests/jobtest.h <span style="color: grey">(5687b4a448d26d703d33603e5a0939bc4f8c77b4)</span></li>
<li>autotests/jobtest.cpp <span style="color: grey">(11bac4d51e97840181aff99fdddb92cdf4db1ba2)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129275/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>