<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/121095/">https://git.reviewboard.kde.org/r/121095/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 11th, 2014, 2:39 p.m. UTC, <b>Albert Astals Cid</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 sincerely think this is a <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">VERY BAD</em> idea. I don't want my app behaving differently depending if a third party misterious compononent that is not documented anywhere is installed or not. If you have a dependency, well put it in a tier with direct access to that dependency, or put a huge warning in the docu saying this will lose the functionality if something else is not there.</p></pre>
</blockquote>
<p>On November 11th, 2014, 2:48 p.m. UTC, <b>Daniel Vrátil</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 requirement is documented in ktexttohtml.h in /r/121094.</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%"><span style="color: #408080; font-style: italic">/**</span>
<span style="color: #408080; font-style: italic"> * Replace text emoticons smileys by emoticons images.</span>
<span style="color: #408080; font-style: italic"> *</span>
<span style="color: #408080; font-style: italic"> * @note</span>
<span style="color: #408080; font-style: italic"> * This option works only when FrameworkIntegration plugin is installed,</span>
<span style="color: #408080; font-style: italic"> * and requires QGuiApplication. This will not work with QCoreApplication.</span>
<span style="color: #408080; font-style: italic"> * If the FrameworkIntegration plugin is not available, or this is called</span>
<span style="color: #408080; font-style: italic"> * from a QCoreApplication, this option will not do anything.</span>
<span style="color: #408080; font-style: italic"> */</span>
ReplaceSmileys <span style="color: #666666">=</span> <span style="color: #666666">1</span> <span style="color: #666666"><<</span> <span style="color: #666666">2</span>,
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The behaviour change in case the conditions above are not fulfiled are not drastical - the flag will simply be ignored. I don't see much difference over MessageBox not remembering "Don't show this again" between sessions, because it would be using only memory storage when FrameworkIntegrationPlugin is not installed (which btw is not documented at all)</p></pre>
</blockquote>
<p>On November 11th, 2014, 7:07 p.m. UTC, <b>Albert Astals Cid</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;">Why is the plugin in FrameworkIntegration and not in KEmoticons?</p></pre>
</blockquote>
<p>On November 11th, 2014, 10:25 p.m. UTC, <b>Daniel Vrátil</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;">Theoretically it could be in KEmoticons as some sort of plugin, but the original idea was to simply follow the same path as KMessageBox to create a runtime frameworks integration (for which "FrameworkIntegrationPlugin" seems to be exactly the right place).</p></pre>
</blockquote>
<p>On November 11th, 2014, 10:30 p.m. UTC, <b>Albert Astals Cid</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 what's the point? now if i want to provide that feature i need to pull a bazillion of dependencies instead of just KEmoticons. KMessageBox don't show again is actually an integration feature, is this feature one?</p></pre>
</blockquote>
<p>On November 12th, 2014, 2:17 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;">I guess what Albert means is why you aren't moving the class to kemoticons altogether...</p></pre>
</blockquote>
<p>On November 12th, 2014, 2:36 p.m. UTC, <b>Albert Astals Cid</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;">No, i understand this class has value without ";) to image" support, i just don't understand why the emoticons support for this class is hidden in frameworksintegration. As another option, instead of a runtime plugin could it be a class that inherits from it? So if i want emoticon support use KTextToHTMLWithEmoticons and otherwie just use KTextToHTML?</p></pre>
</blockquote>
<p>On November 12th, 2014, 10:13 p.m. 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;">I agree that the plugin should be in KEmoticons, since that depends on kcoreaddons.
The KMessageBox case is different because kconfig and kwidgetaddons don't depend on each other.</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;">Yes, in fact, it would probably make sense to start working towards renaming frameworksintegration to plasmaintegration. These problems get more clear then.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also +1 to move the integration plugin into kemoticons, it reduces a complexity level.</p></pre>
<br />
<p>- Aleix</p>
<br />
<p>On November 11th, 2014, 2:51 p.m. UTC, Daniel Vrátil 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, David Faure and Michael Pyne.</div>
<div>By Daniel Vrátil.</div>
<p style="color: grey;"><i>Updated Nov. 11, 2014, 2:51 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
frameworkintegration
</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 patch is related to /r/121094, which moves KTextToHTML conversion utility from KPimUtils to KCoreAddons. Since KCoreAddons can't depend on KEmoticons needed for smileys conversion, I added the actual KEmoticons code here, to create a run-time dependency, similar to the KWidgetsAddons-KConfig dependency for KMessageBox.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch refactors the FrameworkIntegrationPlugin a bit - I split the KMessageBox-specific code into a separate file, and added a new file with the KTextToHTMLEmoticonsInterface implementation, as we can't just keep stacking more and more classes into a single file :-)</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;">Tested with KTextToHTML code from /r/121094 in a QGuiApplication and it seems to work.</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">(3721bfa)</span></li>
<li>src/integrationplugin/CMakeLists.txt <span style="color: grey">(3395368)</span></li>
<li>src/integrationplugin/frameworkintegrationplugin.h <span style="color: grey">(6dc6825)</span></li>
<li>src/integrationplugin/frameworkintegrationplugin.cpp <span style="color: grey">(a45ba9d)</span></li>
<li>src/integrationplugin/kmessagebox.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/integrationplugin/kmessagebox.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/integrationplugin/ktexttohtml.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/integrationplugin/ktexttohtml.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121095/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>