<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/125778/">https://git.reviewboard.kde.org/r/125778/</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 25th, 2015, 5:09 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;">So every app (packaged this way) will need a copy of the .protocol files and the kioslave executable...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I much preferred the solution we talked about previously, where .protocol files would be embedded (as JSON) into the kio_<protocol> slave plugin. This at least simplified the deployment question to: how to ship and locate these plugins. And it's the Qt5 way :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Wouldn't that make it simpler to package other apps like what you're doing?</p></pre>
</blockquote>
<p>On October 25th, 2015, 5:30 p.m. UTC, <b>Christoph Cullmann</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;">Yeah, you need the kioslave exe in each bundle/installer, but that is unavoidable, as you can't rely on having a "compatible" version around.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For the .protocol files: I would like to have the metadata solution, too, but after looking more into how it works ATM, that is more work than thought, given you can have multiple .protocol files per slave (like for kio_http) and then one needs an extra tool to convert that stuff (as e.g. the current desktoptojson will just take one file and output one toplevel json map).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I am still thinking about doing that long term, but at least the local lookup of kioslave exe will still be needed and I think that the above proposed solution is neither intrusive nor will it hurt the normal non-bundled use case on linux or some ports system and it gives people a 'working' kio on plain qt.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And yes, without .protocol files one deployment step would be removed.</p></pre>
</blockquote>
<p>On October 25th, 2015, 5: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;">Will your deployment solution be documented somewhere for everyone to see and possibly replicate?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't like adding "weird" code that makes one undocumented use case work.
Looking up .protocol files in AppLocalDataLocation (which is btw the non-deprecated name for DataLocation in Qt >= 5.4) is very unintuitive and unexpected to me, I assume you're doing that because you're adding a path in the .qrc file to AppLocalDataLocation somehow, or because that's where you're copying files after installation. But then, wouldn't it be possible to copy these files to somewhere where GenericDataLocation will find them?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To say this another way... if we modified QSP to look into an app-specific dir also for GenericDataLocation, then you wouldn't need to modify every single use of GenericDataLocation in KF5 and KDE apps and replace it with [AppLocal]DataLocation.
GenericDataLocation means shared, but it's already documented to not really be shared on sandboxed OSes like iOS and Android .... and what you're doing is basically sandboxing on OSX ;)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As long as the app-specific dir is the fallback, I see no reason against doing this.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Completely untested: http://www.davidfaure.fr/2015/qstandardpaths_mac_fallback.diff</p></pre>
</blockquote>
<p>On October 25th, 2015, 6:18 p.m. UTC, <b>Christoph Cullmann</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;">For the deployment and documentation: using DataLocation (or its replacement from Qt 5.4 which I can't use given we rely only on 5.3 ;=) as default lookup place seems easy to document and handle by bundling. If you create application bundles you need anyway to put your stuff somewhere in the bundle, neither with this or any other patch this can be changed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For the patch to standarddirs, I think that doesn't cut it: First must be the app local lookup, then the generic one. Otherwise, you can break any bundle by just installing globally some different frameworks version. E.g. you can break the kate.app bundle by installing some mac ports that uses a more up-to-date Qt version.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We really need to ensure that application local stuff wins (which is btw. a problem with my other patches with "use resources" as fallback, too. You just need to install some old/broken ui_standards.rc in etc => bamm, all applications that use kxmlgui are dead, there in the long run I would even remove the lookup in the filesystem).</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;">Btw., what would be nice would be if one could tell QStandardPaths: Search first in that location, than in that. That way one could decide which order is wanted without my ugly "search here and then there" hack.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Or simpler some fallback from app => generic for all operating systems, just the reverse of your patch and for all OSes, but perhaps I misread the diff.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Still even then, as least until such a thing is in and we require such a version of Qt, the manual app => generic search fallback would be needed?</p></pre>
<br />
<p>- Christoph</p>
<br />
<p>On October 24th, 2015, 7:06 p.m. UTC, Christoph Cullmann 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 Christoph Cullmann.</div>
<p style="color: grey;"><i>Updated Oct. 24, 2015, 7:06 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;">Allow .protocol files to be bundled with applications.
Search in app local paths first.
Side effect: protocols() result is now sorted by name and allProtocols won't create duplicated cache entries if .protocol files are duplicated</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kioslave helper is search locally, too.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">forkSlaves check was missing in holdSlave to avoid segfault without dbus.</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;">.protocol files are now found if bundled with the application
kwrite http://www.kde.org works on mac now.</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/kprotocolinfofactory.cpp <span style="color: grey">(29ba8f4)</span></li>
<li>src/core/slave.cpp <span style="color: grey">(9ce0d78)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125778/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>