<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/124281/">https://git.reviewboard.kde.org/r/124281/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 7th, 2015, 3:56 p.m. CEST, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124281/diff/1/?file=383552#file383552line89" style="color: black; font-weight: bold; text-decoration: underline;">src/knotificationmanager.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">KNotificationManager::KNotificationManager()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">86</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KService</span><span class="o">::</span><span class="n">List</span> <span class="n">offers</span> <span class="o">=</span> <span class="n">KServiceTypeTrader</span><span class="o">::</span><span class="n">self</span><span class="p">()</span><span class="o">-></span><span class="n">query</span><span class="p">(</span><span class="s">"KNotification/NotifyPlugin"</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">87</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">pluginPath</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<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;">Would it make any sense to remove this altogether?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If nobody is using it, it sounds useless.
Was it used for those Ubuntu-specific notifications, maybe?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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, there were requests for custom plugins, but nobody was using it because the plugin class was not public until very recently. So yeah, this is wanted feature and I'd like to keep it in.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 7th, 2015, 3:56 p.m. CEST, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/124281/diff/1/?file=383553#file383553line565" style="color: black; font-weight: bold; text-decoration: underline;">src/notifybypopup.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void NotifyByPopupPrivate::fillPopup(KPassivePopup *popup, KNotification *notification, const KNotifyConfig ¬ifyConfig)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">560</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KIconLoader</span> <span class="nf">iconLoader</span><span class="p">(</span><span class="n">iconName</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">561</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">int</span> <span class="n">iconDimension</span> <span class="o">=</span> <span class="n">QFontMetrics</span><span class="p">(</span><span class="n">QApplication</span><span class="o">::</span><span class="n">font</span><span class="p">()).</span><span class="n">height</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<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;">Will that pass the DPI test? Have you tested with a different QT_DEVICE_PIXEL_RATIO?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">Yeah it works fine if the app has the Qt::AA_UseHighDpiPixmaps attribute set (which they are supposed to be setting for hidpi support), then it scales correctly with any QT_DEVICE_PIXEL_RATIO.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On July 7th, 2015, 2:52 p.m. CEST, Martin Klapetek 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 and Martin Gräßlin.</div>
<div>By Martin Klapetek.</div>
<p style="color: grey;"><i>Updated July 7, 2015, 2:52 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
knotifications
</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 reduces the dependencies of KNotifications framework and effectively makes it a tier 2 framework.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KService is used only for locating additional notification plugins and to my knowledge,
there are none such plugins currently existing, at least not in all around KDE plus
the class for the plugins wasn't exported until about two months ago, so this should
be safe without a legacy support.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KIconThemes is used only to get "KIconLoader::Small" icon pixmap for notifications
using KPassivePopup. After some playing around it turns out that it puts the icon into
the KPassivePopup title and makes it as big as the text. So I've made the icon size to
be the same as the text height. So this keeps things visually the same + still DPI aware,
though I believe the KPassivePopup should receive a complete visual overhaul anyway.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Additionally, KCodecs dependency has again one single usage for decoding html entities
to QChars within QXmlStreamReader parser, so eventually could also be removed/replaced
with QTextDocument::toPlainText() which seems to do the same job as QXmlStreamReader+KCodecs.</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;">Everything still compiles + I added a test for KPassivePopup with an icon.</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">(2d5437b)</span></li>
<li>metainfo.yaml <span style="color: grey">(7fc15f7)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(1cebece)</span></li>
<li>src/knotificationmanager.cpp <span style="color: grey">(8d4f953)</span></li>
<li>src/notifybypopup.cpp <span style="color: grey">(e377051)</span></li>
<li>tests/kpassivepopuptest.h <span style="color: grey">(bc0dedc)</span></li>
<li>tests/kpassivepopuptest.cpp <span style="color: grey">(2486fd8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124281/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>