<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/126895/">https://git.reviewboard.kde.org/r/126895/</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 29th, 2016, 9:43 a.m. CET, <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;">This looks good to me. It doesn't affect Linux, it doesn't affect API / binary compatibility, it uses #if so no risk of missing include, and it hides the globalaccel gui so it's not like users have the risk on clicking on something that doesn't work.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Default global shortcuts set by apps won't work of course, but AFAICS this is done using KGlobalAccel API anyway, not kxmlgui API. So that's perfect IMHO, neither users or apps will end up with broken functionality.</p></pre>
</blockquote>
<p>On January 29th, 2016, 10:05 a.m. CET, <b>Martin Gräßlin</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;">but AFAICS this is done using KGlobalAccel API anyway, not kxmlgui API.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KActionCollection sets the component name without the global shortcut won't work. So no, with the ifdef the shortcuts break.</p></pre>
</blockquote>
<p>On January 29th, 2016, 10:42 a.m. CET, <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;">Can you explain what you mean? I have trouble parsing the first sentence.</p></pre>
</blockquote>
<p>On January 29th, 2016, 10:52 a.m. CET, <b>Martin Gräßlin</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;">when registering a QAction in KActionCollection it gets properties installed. Those are needed by KGlobalAccel to trigger the action. The current patch disables that code as well. Thus an application using KGlobalAccel and KActionCollection will break if KXmlGui is compiled without KGlobalaccel support.</p></pre>
</blockquote>
<p>On January 29th, 2016, 12:11 p.m. CET, <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 of course, compiling lib2 without support for lib1, and THEN installing lib1, is always bound to create trouble. This is true for any optional dependency. "But I have lib1 installed!!!" ... "well, yeah ok, but you did that too late".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If we have a way to set the property without using kglobalaccel that would be even better, but I didn't look into it to know if that's possible, I guess not, if it uses KGlobalAccel API?</p></pre>
</blockquote>
<p>On January 29th, 2016, 12:45 p.m. CET, <b>Martin Gräßlin</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;">it is possible, I ported KWin away from using KActionCollection and set the property/objectName manually. The problem is IMHO rather that it needs porting.</p></pre>
</blockquote>
<p>On January 29th, 2016, 2:13 p.m. CET, <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;">But then why don't we do that in the kxmlgui code itself? instead of #if HAVE_KGLOBALACCEL /<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> call API that sets a property </em>/ #endif
we can do setProperty directly, no? Then the apps don't need any porting.</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;">Ah no the usage of KGlobalAccel seems to be more than that. Looking at what got ifdefed there seems to be code to load the shortcuts from a config file, etc.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not really familiar with Xmlgui and it's intereaction with KGlobalAccel. All I know is that back in KDELibs4 times one used a KActionCollection to get a KAction and each KAction could have a global shortcut (KShortcut). Now with QAction and QKeySequence it should be possible to do all without a KActionCollection. Whether it's possible to get the convenience of KActionCollection for registering global shortcuts without a KGlobalAccel dependency I do not know.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On January 28th, 2016, 2:29 p.m. CET, Andre Heinecke 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.</div>
<div>By Andre Heinecke.</div>
<p style="color: grey;"><i>Updated Jan. 28, 2016, 2:29 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kxmlgui
</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 part of a three patch series that aims to allow a "leightweight" build of KXmlGui without DBus and KService dependencies. I've added the patches to: https://phabricator.kde.org/T1390 I'm not sure if I can create reviews that depend on changes from another review, I'll try and if it does not work I'll open one after another.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Global shortcuts are a nice optional feature to have. But as they are not strictly neccessary for the core functionality of KXmlGui, as I see it, and pull in an extra dependency to DBus and need runtime support on the target platform they should be optional.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This (and the other changes) add lots of unloved ifdefs, I could understand if thats disliked. But let me explain the background of this change:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm currently updating Kleopatra in Gpg4win to a KDE Frameworks based build. This is nice. Frameworks are awesome, I can just pick what I need and don't have dependencies to lots of things that are actually not needed.
Then comes KXmlGui, adds 20 Framework dependencies, and I don't know what to do.
I want:
- configureable "KDE Style" GUI
- configurable Shortcuts
- KDE Standardactions (e.g. Help / WhatsThis)
- kbugreport
- KDE Integration in an KDE Environment</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But I don't want:
- Global Shortcuts (we don't have kded so this won't work for us anyway)
- DBus (our dbus is directory scoped and there are no other applications using dbus installed by us)
- KService dependency (System configuration has been troublesome in the past on Windows and is not neccessary if we provide just a single installation)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So these Patches are my way out of this Problem. Without the optional packages KXmlGui provides what I want and does not depend on what I don't want.</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;">Compiled with and without dependency. Tested Kleopatra against it.
Not yet tested on Windows, will do so in the next days.</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>CMakeLists.txt <span style="color: grey">(9d79619)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(58f0c7a)</span></li>
<li>src/config-xmlgui.h.cmake <span style="color: grey">(07c882f)</span></li>
<li>src/kactioncollection.cpp <span style="color: grey">(9c45725)</span></li>
<li>src/kkeysequencewidget.cpp <span style="color: grey">(b2e2b6a)</span></li>
<li>src/kshortcuteditwidget.cpp <span style="color: grey">(670d031)</span></li>
<li>src/kshortcutseditor.cpp <span style="color: grey">(99dfb3d)</span></li>
<li>src/kshortcutseditoritem.cpp <span style="color: grey">(461a90c)</span></li>
<li>src/kxmlguifactory.cpp <span style="color: grey">(2767e69)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126895/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>