<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 26th, 2016, 6:40 p.m. UTC, <b>Aleix Pol Gonzalez</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;">Hi,
I'm a bit afraid of all these ifndef. Do you think it would make sense to abstract out the KGlobalAccel usage?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Otherwise, would it be possible to make KGlobalAccel useful (or just dumb) on Windows?</p></pre>
</blockquote>
<p>On January 26th, 2016, 6:53 p.m. UTC, <b>Boudewijn Rempt</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;">I would say: most applications do not need global accelerators, so making kglobalaccel functional on windows is not really relevant, you wouldn't want that dependency anyway because it doesn't add functionality. And building a library and including it is always a burden, so I would say it's much better to make it optional.</p></pre>
</blockquote>
<p>On January 26th, 2016, 6:55 p.m. UTC, <b>Andre Heinecke</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;">Hi,</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Abstracting it out would also be quite invasive imho. And from my experience abstraced (optional) features are even harder to maintain then ifdefs where you can easily see the codepath taken when something is not available. While side effects through abstraction are harder to see when hacking on code.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As for the other point:
- GlobalAccel means that you basically need to have a keylogger on your platform. I don't want that. There are system ways on Windows to create global shortcuts already. Not as fine grained as KDE Actions but you could use them to do command line calls.
- I don't really see this as a Windows only thing. KXmlGui provides very useful features even without Global Shortcuts. And as for making KGlobalAccel just dumb on Windows. While I think this would generally be a good idea I expect that others (from Kde-Windows) who provide several KDE applications and stuff like Systemsettings on Windows would disagree.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We could add a dummy class in KXmlGui thats used if KGlobalAccel is not available? This could avoid lots of ifdefs. But this would add a bit maintenance cost when new API is used. While the IFDEFS make it quite transparent to hackers that if you do thomething with GlobalAccel "please guard it".</p></pre>
</blockquote>
<p>On January 27th, 2016, 12:42 a.m. UTC, <b>Aleix Pol Gonzalez</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;">@boud, yes, I also thought about your RR, in fact I looked it up but couldn't find it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ok, maybe it's actually the way to make xmlgui viable to deploy.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Take this as a +1 by me.</p></pre>
</blockquote>
<p>On January 27th, 2016, 7:06 a.m. UTC, <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;">Could we maybe continue the route I went with making sure that kglobalaccel doesn't start? I'm quite concerned about the ifdefs. If there are still situations where kglobalacceld is started without being needed, let's fix that. Let's make it a proper runtime thing instead of an ifdef messery nobody will check.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm quite concerned about making the change optional as that's a route to breakage with distros and as the maintainer of kglobalaccel I'm not looking forward to those bug reports.</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;">Well, part of the point, for me at least, is not having the dependency at all. Any extra library, especially one that adds no functionality but is just present, is a burden just like #idefs are a burden.</p></pre>
<br />
<p>- Boudewijn</p>
<br />
<p>On January 26th, 2016, 6:25 p.m. UTC, 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. 26, 2016, 6:25 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>