<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/125425/">https://git.reviewboard.kde.org/r/125425/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 28th, 2015, 7:30 vorm. 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;">The filename not matching the servicetype name in it, is very confusing.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How about deprecating KonqPopupMenu/Plugin, introducing a KIOServiceMenuPlugin servicetype, installing desktop files for both, querying for both.... and skipping the installation of konqpopupmenuplugin.desktop if the KIO version is > 5.15?</p></pre>
</blockquote>
<p>On September 29th, 2015, 7:44 nachm. UTC, <b>Frank Reininghaus</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;">If we change the ServiceType entry in the file, then every service menu will have to be changed, right? Not only those that are installed by applications and libraries that are hosted on git.kde.org (which we could at least find and fix), but also service menus that are released on kde-apps.org, or unreleased menus that users have written for themselves. I thought that it's unrealistic that this happens for every single service menu, that's why I thought that the KonqPopupMenu/Plugin type should be kept. Or am I overlooking something? If not, then I'm happy to change the file name - probably having a konqpopupmenuplugin.desktop in KIO is really less confusing than the ServiceType/file name mismatch.</p></pre>
</blockquote>
<p>On September 30th, 2015, 7:08 vorm. 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;">Yes this is why I was suggesting to install desktop files for both names.
Deprecating != removing.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Make both servicetype names work, but issue a warning if a plugin only implements KonqPopupMenu/Plugin and not KIOServiceMenuPlugin (both is ok, it's a way to make it work for older versions of KF5). Only KIOServiceMenuPlugin is ok of course.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyway, that's more work, if you want to just rename the file and move on I'm ok with that.
In any case, make sure to avoid the conflict between kio and libkonq installing the same file, with a version check.</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;">Thanks David for the clarification! Yes, that makes a lot of sense. Sorry for the misunderstanding - I did not realize that you meant "skipping the installation of konqpopupmenuplugin.desktop <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">in kde-baseapps</em> if the KIO version is > 5.15". I'll update the patch soon.</p></pre>
<br />
<p>- Frank</p>
<br />
<p>On September 27th, 2015, 6:18 nachm. UTC, Frank Reininghaus 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 Frank Reininghaus.</div>
<p style="color: grey;"><i>Updated Sept. 27, 2015, 6:18 nachm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=350769">350769</a>
</div>
<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;">This is a modified version of the file konqpopupmenuplugin.desktop in kde-baseapps (see https://quickgit.kde.org/?p=kde-baseapps.git&a=blob&h=94a680ac215b4638a0c7cdd2b20bc7830b9619f2&hb=35e8bc2992f48ffaff9007cfbf8faf3c856b18a3&f=lib%2Fkonq%2Fkonqpopupmenuplugin.desktop for the latest version).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I modified the name to kioservicemenuplugin.desktop because the file has not been Konqueror-specific for quite some time. I also updated the 'Comment' accordingly and removed the outdated translations.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I hope I did that right - any comments are welcome!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Note: Just like https://git.reviewboard.kde.org/r/124983/ this should probably be pushed to master after the tagging for the next version because of the translations. On the one hand, the translation of this 'Comment' might not be that important because the it is not shown anywhere in the UI as far as I know (it is shown in the 'Type' column in Dolphin though when viewing the directory where this file is installed). But on the other hand, it might be better to resolve both context menu issues in the same KIO release. What do others think?</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;">Konsole service actions are shown in the context menu again.</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/widgets/CMakeLists.txt <span style="color: grey">(820cd34)</span></li>
<li>src/widgets/kioservicemenuplugin.desktop <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125425/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>