<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/119977/">https://git.reviewboard.kde.org/r/119977/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On agost 28th, 2014, 5:02 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;">Errr, what do you mean it doesn't do anything?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's used http://lxr.kde.org/source/frameworks/kxmlgui/src/kaboutapplicationdialog.cpp?v=kf5-qt5<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
http://lxr.kde.org/source/frameworks/kjobwidgets/src/kuiserverjobtracker.cpp?v=kf5-qt5<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
http://lxr.kde.org/source/frameworks/kconfigwidgets/src/kstandardaction.cpp?v=kf5-qt5</p></pre>
</blockquote>
<p>On agost 28th, 2014, 5:10 p.m. UTC, <b>Jonathan Riddell</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;">Ug, so all these classes expect a property to be set in QApplication which may be set by KAboutData if you remember to call setProgramIconName(), but not used by any part of Qt? Should these should be ported to QApplication::windowIcon?</p></pre>
</blockquote>
<p>On agost 28th, 2014, 5:14 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;">Which QApplication property are you talking about?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QString KAboutData::programIconName() const<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
{<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
return d->programIconName.isEmpty() ? componentName() : d->programIconName;<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
}</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KAboutData &KAboutData::setProgramIconName(const QString &iconName)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
{<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
d->programIconName = iconName;<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
return *this;<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
}</p></pre>
</blockquote>
<p>On agost 28th, 2014, 5:32 p.m. UTC, <b>Jonathan Riddell</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's the one, it sets a variable in KAboutData and sets a property in QCoreApplication. But not the same property as is actually used by Qt to set the icon. most app authors won't expect to have to call a second method to set the icon especially one that doesn't actually set the icon.</p></pre>
</blockquote>
<p>On agost 28th, 2014, 7:23 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 think it's a much better idea to have a Q_COREAPP_STARTUP_FUNCTION in kguiaddons to set the icon based on that property and document that setProgramIconName will set the icon if linking to kguiaddons it than deprecating something that we use.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Or even better implement support for applicationIconName in Qt itself.</p></pre>
</blockquote>
<p>On agost 28th, 2014, 7:55 p.m. UTC, <b>Jonathan Riddell</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;">Those both seems excessive. This property adds no value to Qt, it should just be scrapped.</p></pre>
</blockquote>
<p>On agost 28th, 2014, 9:34 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;">Adds value to ourselves, there's lots of places where we pass the application icon by name, not by QIcon as QApplication has, how are you going to change the use of <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
http://lxr.kde.org/source/frameworks/kjobwidgets/src/kuiserverjobtracker.cpp?v=kf5-qt5</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To use a QIcon there?</p></pre>
</blockquote>
<p>On setembre 2nd, 2014, 10:17 a.m. UTC, <b>Alexander Volkov</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;">-QString programIconName = QCoreApplication::instance()->property("applicationIconName").toString();<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
+QString programIconName = QGuiApplication::windowIcon().name();</p></pre>
</blockquote>
<p>On setembre 2nd, 2014, 10:39 a.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;">Problem is that nothing guarantees QIcon will have a name().</p></pre>
</blockquote>
<p>On setembre 2nd, 2014, 10:44 a.m. UTC, <b>Jonathan Riddell</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;">Nothing guarantees there is a applicationIconName property, especially since setting it doesn't actually set the icon so most app developers will not use 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;">Define "it doesn't actually set the icon", because it sets it for me in all the places i need, sure it doesn't set QGuiApplication::windowIcon(), but what's that useful for?</p></pre>
<br />
<p>- Albert</p>
<br />
<p>On agost 28th, 2014, 4:39 p.m. UTC, Jonathan Riddell 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 Jonathan Riddell.</div>
<p style="color: grey;"><i>Updated ago. 28, 2014, 4:39 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kcoreaddons
</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;">Mark setProgramIconName() as deprecated, it did not do anything since<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
kcoreaddons no longer uses QtGui so it can not set the icon.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">BUG: 337938</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 a program made with kapptemplate 4.14.1 which uses this function, successfully gives a warning during compile</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>src/lib/kaboutdata.h <span style="color: grey">(2d8bd5645150a57739e94f9f71b112b20ec0e01f)</span></li>
<li>src/lib/kaboutdata.cpp <span style="color: grey">(5ad81de814c123f50b17bb542331459e15649f4b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119977/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>