<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://git.reviewboard.kde.org/r/112873/">http://git.reviewboard.kde.org/r/112873/</a>
</td>
</tr>
</table>
<br />
<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 don't seem to need ${QT_QTXML_LIBRARY}, no?
Also on startup i get
QDeclarativeExpression: Expression "(function $color() { return useCustomBackgroundColor ? backgroundColor : colorScheme.background })" depends on non-NOTIFYable properties:
ColorScheme::background
QDeclarativeExpression: Expression "(function $color() { return colorScheme.foreground })" depends on non-NOTIFYable properties:
ColorScheme::foreground
QDeclarativeExpression: Expression "(function $color() { return useCustomBorderColor ? borderColor : colorScheme.border })" depends on non-NOTIFYable properties:
ColorScheme::border
QDeclarativeExpression: Expression "(function $color() { return useCustomBorderColor ? borderColor : colorScheme.border })" depends on non-NOTIFYable properties:
ColorScheme::border
QDeclarativeExpression: Expression "(function $color() { return colorScheme.foreground })" depends on non-NOTIFYable properties:
ColorScheme::foreground
QDeclarativeExpression: Expression "(function $color() { return useCustomBackgroundColor ? backgroundColor : colorScheme.background })" depends on non-NOTIFYable properties:
ColorScheme::background
i guess it's "ok" but looks scary to Joe Random User, so may as well add the notify signals even if you never send them
>From a look at the code, the only thing i don't like it's all those QMetaObject::invokeMethod
A more QML-y way to do that would be some objects emmitting some signals and the qml side running the function as answer to those signals, but in essence "it's the same", so I'm not going to argue for you to change it if you don't want.
Now, the thing is, next wednesday is the freeze for 4.12, and we probably want this in, don't want you to do a lot of work for nothing.
Question, is there anyone else that feels like looking at the code? Or shall we just commit it?
Also, I understand you are going to stay around to fix any possible issues that your code shows, right?</pre>
<br />
<p>- Albert Astals Cid</p>
<br />
<p>On October 22nd, 2013, 5:42 p.m. UTC, Denis Kuplyakov wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Games and Viranch Mehta.</div>
<div>By Denis Kuplyakov.</div>
<p style="color: grey;"><i>Updated Oct. 22, 2013, 5:42 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kreversi
</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;">QML port for KReversi.
List of changes:
1) KReversiChip and KReversiScene classes were deleted as they not needed
2) default_theme.desktop files is now installed to make KgThemeProvider discover it
3) Added folowing QML files:
Table (contains Board and Popup, draws background)
Popup (implements KgPopupItem)
Board (draws board with labels and grid of Cells)
Cell (represents cell of board, contains Chip and displays various marks)
Chip (draws chip and it's animation)
CanvasItem (wrapper to acces themeProvider allover the code),
globals (used to store constants)
4) Fixed bug: "If you undo move, last move marker isn't showing"
5) KReversiView was compeletely rewritten to use QML implementation.
6) Minor changes at other files: added credits to me, deleted QGraphicsScene specific things
7) Introduced ColorScheme class to access KColorScheme color from QML.
8) Documentation is uptodate now.
Current bugs: I haven't found any ;)
Future plans:
See http://kreversiqml.blogspot.ru/2013/08/new-kreversi-design.html . All of this is implemented at deniskup/gsoc2013/newdesign branch. Now I am working on
1) post a patch to kdelibs that will make possible using of ColorScheme class (I mean newdesign implementation here)
2) export ColorScheme class to kdelibs as KColorSchemeToken
* search for "KDE theme colors API for QML" thread at kdelibs mailinglists *
3) export Popup.qml to libkdegames
4) apply newdesign</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;">Have played it many times, it seems to work right.</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>AUTHORS <span style="color: grey">(93f0925)</span></li>
<li>CMakeLists.txt <span style="color: grey">(8f650f8)</span></li>
<li>DESIGN <span style="color: grey">(d2a1320)</span></li>
<li>TODO <span style="color: grey">(97aedcd)</span></li>
<li>colorscheme.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>colorscheme.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kreversichip.h <span style="color: grey">(37695b9)</span></li>
<li>kreversichip.cpp <span style="color: grey">(f6e909d)</span></li>
<li>kreversigame.h <span style="color: grey">(d6b8a43)</span></li>
<li>kreversigame.cpp <span style="color: grey">(4fb231b)</span></li>
<li>kreversiscene.h <span style="color: grey">(6880a67)</span></li>
<li>kreversiscene.cpp <span style="color: grey">(2731f78)</span></li>
<li>kreversiview.h <span style="color: grey">(ff3db89)</span></li>
<li>kreversiview.cpp <span style="color: grey">(1c80e4b)</span></li>
<li>main.cpp <span style="color: grey">(dd05bd4)</span></li>
<li>mainwindow.h <span style="color: grey">(c9c3160)</span></li>
<li>mainwindow.cpp <span style="color: grey">(1855b16)</span></li>
<li>pics/CMakeLists.txt <span style="color: grey">(744ec7b)</span></li>
<li>qml/Board.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>qml/CanvasItem.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>qml/Cell.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>qml/Chip.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>qml/Popup.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>qml/Table.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>qml/globals.js <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112873/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/10/22/36a61da1-6ed3-4729-8e7a-adcbea5b3026__master_qtquick.diff">No space diff</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>