[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request 126895: Make KGlobalAccel dependency in KXmlGui optional
From: Aleix Pol Gonzalez <aleixpol () kde ! org>
Date: 2016-01-27 0:42:59
Message-ID: 20160127004259.17778.28538 () mimi ! kde ! org
[Download RAW message or body]
--===============1435876153096195005==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
> On Jan. 26, 2016, 7:40 p.m., Aleix Pol Gonzalez wrote:
> > Hi,
> > I'm a bit afraid of all these ifndef. Do you think it would make sense to \
> > abstract out the KGlobalAccel usage?
> > Otherwise, would it be possible to make KGlobalAccel useful (or just dumb) on \
> > Windows?
>
> Boudewijn Rempt wrote:
> 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.
> Andre Heinecke wrote:
> Hi,
>
> 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.
> 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.
> 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".
@boud, yes, I also thought about your RR, in fact I looked it up but couldn't find \
it.
Ok, maybe it's actually the way to make xmlgui viable to deploy.
Take this as a +1 by me.
- Aleix
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126895/#review91623
-----------------------------------------------------------
On Jan. 26, 2016, 7:25 p.m., Andre Heinecke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126895/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 7:25 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kxmlgui
>
>
> Description
> -------
>
> 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.
> 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.
> 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:
> 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
>
> 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)
> 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.
>
> Diffs
> -----
>
> CMakeLists.txt 9d79619
> src/CMakeLists.txt 58f0c7a
> src/config-xmlgui.h.cmake 07c882f
> src/kactioncollection.cpp 9c45725
> src/kkeysequencewidget.cpp b2e2b6a
> src/kshortcuteditwidget.cpp 670d031
> src/kshortcutseditor.cpp 99dfb3d
> src/kshortcutseditoritem.cpp 461a90c
> src/kxmlguifactory.cpp 2767e69
>
> Diff: https://git.reviewboard.kde.org/r/126895/diff/
>
>
> Testing
> -------
>
> Compiled with and without dependency. Tested Kleopatra against it.
> Not yet tested on Windows, will do so in the next days.
>
>
> Thanks,
>
> Andre Heinecke
>
>
--===============1435876153096195005==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<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, 7:40 p.m. CET, <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, 7:53 p.m. CET, <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, 7:55 p.m. CET, <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>
</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;">@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> <br \
/>
<p>- Aleix</p>
<br />
<p>On January 26th, 2016, 7:25 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. 26, 2016, 7: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>
--===============1435876153096195005==--
[Attachment #3 (text/plain)]
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic