<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/122680/">https://git.reviewboard.kde.org/r/122680/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 23rd, 2015, 11:23 a.m. UTC, <b>Martin Klapetek</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;">From the docs "Currently, the values set here are shown by the "About" box (see KAboutDialog), used by the bug report dialog (see KBugReport), and by the help shown on command line (see KAboutData::setupCommandLine())."</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So daemon has no About box but might need it for the KBugReport. And it doesn't implement --help anyway.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think this might be fine for a daemon however, but for sure should be replaced by the QApplication bits:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">app.setApplicationVersion(aboutData.version());
app.setApplicationName(aboutData.componentName());
app.setApplicationDisplayName(aboutData.displayName());
app.setOrganizationDomain(aboutData.organizationDomain());</p></pre>
</blockquote>
<p>On February 23rd, 2015, 11:38 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;">I agree that the application data needs to be set, so that bug reports in case of e.g. crashes work.</p></pre>
</blockquote>
<p>On February 23rd, 2015, 12:53 p.m. UTC, <b>Jerome Leclanche</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 would something like this be acceptable?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">app.setApplicationVersion(<span style="color: #BA2121">"0.2"</span>);
app.setApplicationName(<span style="color: #BA2121">"kglobalaccel"</span>);
app.setApplicationDisplayName(tr(<span style="color: #BA2121">"KDE Global Shortcuts Service"</span>));
app.setOrganizationDomain(<span style="color: #BA2121">"kde.org"</span>);
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I guess the tr() would not be accepted, but bringing in k18n because of the <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">one</em> string which will almost never appear is a huge shame.</p></pre>
</blockquote>
<p>On February 23rd, 2015, 1:23 p.m. UTC, <b>Martin Klapetek</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'm not sure actually if the display name would be used anywhere in the case of daemon. Very likely in the crash dialog but I can't think of anything else.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I can't say if tr() would get rejected though. So I'd suggest to try and update the diff, I'll see if we can get some i18n people to comment.</p></pre>
</blockquote>
<p>On February 23rd, 2015, 1:28 p.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;">yes, that would be fine. But without creating KAboutData the feature will not work for KCrash:
// only for KCrash (no memory allocation allowed)
const KAboutData *KAboutData::applicationDataPointer()
{
if (s_registry.exists()) {
return s_registry->m_appData;
}
return 0;
}</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Without creating a KAboutData instance s_registry will not exist. This means the change would break bug reporting for KDE :-(</p></pre>
</blockquote>
<p>On February 23rd, 2015, 1:31 p.m. UTC, <b>Jerome Leclanche</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;">Is there a way to fix this at a higher level and make KCrash not implicitly depend on KAboutData?</p></pre>
</blockquote>
<p>On February 23rd, 2015, 2:22 p.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;">Hmm, from KCrash point of view it's not really implicitly, but rather explicitly. KCrash uses KAboutData and KAboutData has rather many hooks specificly for KCrash as KCrash may not allocate new memory.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyway: the usage of Ki18n can be removed as far as I see. KCoreAddons and KCrash is more difficult. I think we agree that KCrash provides required functionality :-)</p></pre>
</blockquote>
<p>On February 23rd, 2015, 2:26 p.m. UTC, <b>Jerome Leclanche</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 will rescope this to only drop dependency on KI18n. We can discuss the rest off-list.</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;">Updated the diff. Apologies for the delay.</p></pre>
<br />
<p>- Jerome</p>
<br />
<p>On March 2nd, 2015, 3:52 a.m. UTC, Jerome Leclanche 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, Martin Gräßlin and Martin Klapetek.</div>
<div>By Jerome Leclanche.</div>
<p style="color: grey;"><i>Updated March 2, 2015, 3:52 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kglobalaccel
</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;">Remove the runtime's KAboutData
The about data was unexposed, but created a dependency on KCoreAddons (for
KAboutData) and in turn on KI18n for the translations of the aboutData.
This removes both dependencies as well as the string extraction scripts.
--
Author notes: This is a RFC. We don't use kglobalaccel in LXQt but we would
like to, however it currently has too many dependencies. See
https://github.com/lxde/lxqt/issues/507 for related discussion.
I'm unsure myself if the about data is actually exposed somewhere I completely
missed, but it doesn't look that way.</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;">Compiles and runs. No further testing done.</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">(68ad795)</span></li>
<li>src/Messages.sh <span style="color: grey">(8eae937)</span></li>
<li>src/runtime/CMakeLists.txt <span style="color: grey">(e639fa5)</span></li>
<li>src/runtime/Messages.sh <span style="color: grey">(8a5e4a9)</span></li>
<li>src/runtime/globalshortcutsregistry.cpp <span style="color: grey">(3e4d720)</span></li>
<li>src/runtime/kglobalacceld.cpp <span style="color: grey">(4e7cb9d)</span></li>
<li>src/runtime/main.cpp <span style="color: grey">(fdf4d62)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122680/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>