<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/6933/">http://svn.reviewboard.kde.org/r/6933/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 1st, 2012, 9:10 p.m., <b>Albert Astals Cid</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="http://svn.reviewboard.kde.org/r/6933/diff/1/?file=47787#file47787line72" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdegames/libkdegames/kgtheme.h</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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class KDEGAMES_EXPORT KgTheme : public QObject</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">72</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="c1">//no NOTIFY signals here - it is not intended to allow these properties</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;">You should still have the NOTIFY signals otherwise when you do something like
Mylabeltext: theme.name
in QML it'll complain you are depending on a property that doesn't have NOTIFY support</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;">I've added a dummy signal which is never emitted and published this as the NOTIFY signal of all readonly properties.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 1st, 2012, 9:10 p.m., <b>Albert Astals Cid</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="http://svn.reviewboard.kde.org/r/6933/diff/1/?file=47787#file47787line126" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdegames/libkdegames/kgtheme.h</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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class KDEGAMES_EXPORT KgTheme : public QObject</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">126</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">QString</span> <span class="n">svgPath</span><span class="p">()</span> <span class="k">const</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;">This class seems abstract enough that it could work for non svg themes, why not call it themePath? or filepath?</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;">"Theme" is ambiguous: Is the theme file the ".desktop" or the ".svgz" file? I've called it graphicsPath now.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 1st, 2012, 9:10 p.m., <b>Albert Astals Cid</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="http://svn.reviewboard.kde.org/r/6933/diff/1/?file=47792#file47792line51" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdegames/libkdegames/kgthemeprovider.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">KgThemeProvider::KgThemeProvider(const QByteArray& configKey, QObject* parent)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">51</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">qRegisterMetaType</span><span class="o"><</span><span class="k">const</span> <span class="n">KgTheme</span><span class="o">*></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;">Do you need this given we already have Q_DECLARE_METATYPE(const KgTheme*) ?</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;">Yes, this is necessary. Q_DECLARE_METATYPE and qRegisterMetaType complement each other. The documentation for qRegisterMetaType has the details.</pre>
<br />
<p>- Stefan</p>
<br />
<p>On April 1st, 2012, 5:55 p.m., Stefan Majewsky wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Games.</div>
<div>By Stefan Majewsky.</div>
<p style="color: grey;"><i>Updated April 1, 2012, 5:55 p.m.</i></p>
<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;">KgTheme is a simple generalization (with QML-ready API) of KGameTheme. The main generalization lies in that it can be used without an installed desktop file.
KgThemeProvider implements a selection logic on top of KgTheme. You give it a bunch of KgTheme instances, and it determines which one is selected at the moment using the application config (if you want). KgThemeProvider includes discovery logic for themes installed somewhere in KStandardDirs, but (unlike e.g. the old KGameThemeSelector) is not restricted to ${DATA_INSTALL_DIR}/$APP/themes.
KGameRenderer, which previously used KGameTheme internally, now relies on a KgThemeProvider for its theme selection.
KgThemeSelector is an update to KGameThemeSelector, which borrows its appearance from Palapeli's puzzle list.
The current diff is just a snapshot of my work. It does not compile because I could not find a way to fully emulate old-style KGameRenderer usage with the new API. The full diff will thus need to port KGameRenderer usages to KgTheme(Provider) immediately.
Compared to the minimalistic KGameTheme, the KgTheme(Provider) API is quite complex, for the following reasons:
1. I plan to replace app-local theming logic and KGameThemeSelector copies by KgTheme usage. Most prominent example: libkmahjongg.
2. The KgThemeProvider::generatePreview method *can* be used to compose preview images dynamically, so the theme author does not need to care about the preview anymore.
3. KgTheme can be subclassed for app-specific behavior. I want to use this feature in KDiamond: Two of three KDiamond themes include bitmap canvas backgrounds, which amount for >50% of the SVG size and greatly increase SVG loading time and cache burden. I want to optionally pull these bitmaps out into separate files, using logic in a custom KgTheme subclass.</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;">None as of now, for the reasons explained above. API reviews are welcome.
The kconf_update script in libkdegames/kgthemeprovider-migration.upd is also just a proof-of-concept.</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>trunk/KDE/kdegames/kdiamond/src/game.cpp <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/killbots/renderer.h <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/killbots/renderer.cpp <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/klickety/gamescene.cpp <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/kpat/renderer.h <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/kpat/renderer.cpp <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/libkdegames/CMakeLists.txt <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/libkdegames/includes/CMakeLists.txt <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/libkdegames/includes/KgTheme <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/includes/KgThemeProvider <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/includes/KgThemeSelector <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgamerenderer.h <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgamerenderer.cpp <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgamerenderer_p.h <span style="color: grey">(1287365)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgtheme.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgtheme.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgtheme_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgthemeprovider-migration.upd <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgthemeprovider.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgthemeprovider.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgthemeselector.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgthemeselector.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdegames/libkdegames/kgthemeselector_p.h <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/6933/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>