<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/129741/">https://git.reviewboard.kde.org/r/129741/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 2nd, 2017, 9:38 a.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;">Renaming is really a special case of moving. Saying that "kio_trash doesn't support renaming" is correct but only a partial truth. It also doesn't support moving from trash:/ to trash:/subdir/. So it would be more correct to say that kio_trash supports moving trash-to-file and file-to-trash but not trash-to-trash.
So it seems to me that this patch models the wrong thing, and is likely to give us further trouble down the line.
I wonder if we could invent something more dynamic than the .protocol keys. An additional way to talk to a slave and ask if a specific operation (for specific URLs) is implemented. Something like KIO::CapabilityJob *job = KIO::capability(Move, url1, url2); connect; and in the slot, enable/disable the action accordingly. By default this would just use the information from the .protocol files, but in addition a new SlaveBase method (fake-virtual until KF6, using virtual_hook) would allow slaves to answer the query with more precision, depending on the actual URLs. What do you think?
Of course the alternative (which is actually simpler short term) is to implement renaming in kio_trash ;)
Actual renaming should be easy, moving between subdirs is a bit more tricky but doable too. I can take a look at that next weekend.</pre>
</blockquote>
<p>On January 6th, 2017, 9:06 p.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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Renaming is really a special case of moving. Saying that "kio_trash doesn't support renaming" is correct but only a partial truth. It also doesn't support moving from trash:/ to trash:/subdir/. So it would be more correct to say that kio_trash supports moving trash-to-file and file-to-trash but not trash-to-trash.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Good point...</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Something like KIO::CapabilityJob *job = KIO::capability(Move, url1, url2); connect; and in the slot, enable/disable the action accordingly.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This would be more flexible indeed. On the other hand, it's more verbose and low-level. Also, for the "renaming" case one needs a way to make up url2 (url1 would just be the selected url in the dolphin view).</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Of course the alternative (which is actually simpler short term) is to implement renaming in kio_trash ;)</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Definitely :)
All right, let's discuss this again later, waiting for news from kio_trash.</p></pre>
</blockquote>
<p>On February 22nd, 2017, 8:53 a.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;">Renaming implemented in kio_trash (for toplevel entries) https://commits.kde.org/kio/20f0b84f51ff7f0767da118de79eda28af091ec9</p></pre>
</blockquote>
<p>On February 24th, 2017, 3:45 p.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;">Thanks! Works fine from kioclient, hower it doesn't from Dolphin, which doesn't prepend the 0- prefix to the destination url (so it uses <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">trash:/destination</code> instead of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">trash:/0-destination</code>). I wonder how this could be fixed...</p></pre>
</blockquote>
<p>On February 26th, 2017, 2:22 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;">Good point. Does it work better now after https://commits.kde.org/kio/9afce8395c6604697379046581bce24786a1bcb7 ?</p></pre>
</blockquote>
<p>On February 26th, 2017, 5:24 p.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;">Yes, now I can rename from dolphin. There is a (malformed url) error if I try to restore a file that I just renamed, but it does work after if I reload the view. Anyway I think I can discard this patch now!</p></pre>
</blockquote>
<p>On February 26th, 2017, 10:20 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;">Oh, I see. Dolphin (via KIO) asks to rename trash:/0-foo to trash:/bar, and the resulting file has a URL of trash:/0-bar.
So the KFileItem has the wrong URL, anything you do with it will fail (e.g. renaming it again). Argh.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ah, the KDirNotify signal emits the wrong URL, that's why.
https://commits.kde.org/kio/a1c040a43a19bc896c7a2f19703ce43c4c9b9423 fixes this.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Of course there's always one bug left: now the view shows 0-newname instead of newname. It seems the item text is simply extracted from the URL rather than UDS_DISPLAY_NAME being used. But now that looks like a KIO (or Dolphin) bug, not a kio_trash issue.</p></pre>
</blockquote>
</blockquote>
<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;">I can confirm. Smells like a bug in the dolphin's KFileItemModel...
Thanks again, discarding this patch.</p></pre>
<br />
<p>- Elvis</p>
<br />
<p>On January 1st, 2017, 11:01 p.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, David Faure and Emmanuel Pescosta.</div>
<div>By Elvis Angelaccio.</div>
<p style="color: grey;"><i>Updated Jan. 1, 2017, 11:01 p.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;">An ioslave protocol might support moving files but not "renaming" them (rename here means "the F2 shortcut in filemanagers"). trash:/ is a good example.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The new <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">renaming</code> field defaults to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">moving</code>, so that we don't have to add renaming=true to every protocol file out there.</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;">Using the new <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">supportsRenaming()</code> api from Dolphin, F2 in Trash is now disabled. More context in https://git.reviewboard.kde.org/r/129714</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>src/core/kfileitemlistproperties.h <span style="color: grey">(2b4a5b33166513493e6499e6479a04965a895b57)</span></li>
<li>src/core/kfileitemlistproperties.cpp <span style="color: grey">(5c6e6bba693f8b9bfc942ff39bee5d24f159fd7f)</span></li>
<li>src/core/kprotocolinfo.cpp <span style="color: grey">(0290c63b37a45a22995238f9cfcc11b8334d339c)</span></li>
<li>src/core/kprotocolinfo_p.h <span style="color: grey">(8d05bd194fdaa7b7e7552e0d1d22bf16b28ffbc1)</span></li>
<li>src/core/kprotocolmanager.h <span style="color: grey">(13b8c0756f8e355b1ec84cdf1c44086f41fa05c5)</span></li>
<li>src/core/kprotocolmanager.cpp <span style="color: grey">(9a0a96fe749a11c66a22bb3eff62d0601e3b7b36)</span></li>
<li>src/ioslaves/trash/tests/testtrash.cpp <span style="color: grey">(67a6130e7c86af00e596dc439125c29eb74fc99f)</span></li>
<li>src/ioslaves/trash/trash.json <span style="color: grey">(d7dc03eb073c7bfdde3c7eebfbac144ad62964fe)</span></li>
<li>src/protocoltojson/main.cpp <span style="color: grey">(0bf3c7062d076412c779f6cae25a98e1b2c61be4)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129741/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>