<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/102987/">http://git.reviewboard.kde.org/r/102987/</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 6th, 2011, 7:51 p.m., <b>Laszlo Papp</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;">> As the title says. There will be more changes of the UI in later commits, so it wouldn't make much sense to discuss now about the look of the achievements tab in the player.
I would personally like to see the mocup of the imagined Ui before many patches coming. I would like to agree upon something before getting any new Ui in and code then to maintain (On the other hand, the whole Ui interface of this player should be redesigned by a person with relevant skills - for future reference).</pre>
</blockquote>
<p>On November 7th, 2011, 6:25 p.m., <b>Felix Rohrbach</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;">I added a screenshot. It doesn't include everything that was discussed (fex. we talked about not show the selected icon if an achievement is locked), but you should be able to get the idea of the UI.</pre>
</blockquote>
<p>On November 10th, 2011, 8:29 a.m., <b>Laszlo Papp</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;">Right. It looks very alike to the other list items in this player application. I do not have time to investigate about the differences between "QStyledItemDelegate" and "KWidgetItemDelegate". I would personally vote for consistency (KWidgetItemDelegate).
Since as I said above, I do not have time to investigate myself about QStyledItemDelegate: Could you please present us why "QStyledItemDelegate" would be a better fit for this type of list item in your opinion ? Pros and cons. Thank you in advance.</pre>
</blockquote>
<p>On November 10th, 2011, 7:35 p.m., <b>Felix Rohrbach</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;">I just tried to replace QStyledItemDelegate with KWidgetItemDelegate. Two problems:
1. KWidgetItemDelegate forces me to implement two functions (aka pure virtual): createItemWidgets and updateItemWidgets. As I don't want to have any widgets in my delegate, I returned an empty widget list in the first one and did nothing in the second one.
2. When I try to open a game with achievements, gluon_kdeextplayer crashes. The backtrace contains KWidgetItemDelegatePrivate::initializeModel a few thousand times before crashing in QMutex::lock.
It seems to me KWidgetItemDelegate should really only be used if you want to have widgets in your delegate.</pre>
</blockquote>
<p>On November 10th, 2011, 8:48 p.m., <b>Laszlo Papp</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;">1. Well, I would like to have widgets for several reasons. Firstly, UiS are widget based anyway. ;) Secondly, it is a KDE player..using the KDE technology for convenience is not bad.. Thirdly, It gives much more flexibility for extensions in the future and since it is a KDE Player, it does not mean any overheads. Fourthly, it would bring inconsistency into the codebase, and unneccesary maintainance overhead.
2. This class has been existing for many years, proven and so on. That is just a simple misusage. If you check out the other list items, they are basically almost the same meaning that it is doable. :)
I am sorry, but I am really against this inconsistency (ie.: implement the same Ui in different modes), and the existing solution has been working for a while. I do not see the need for a replacement. Refactoring the other delegates would be more intrusive change than modifying this patch. If the need ever arises for this, we can re-think it later after all. Just keep the consistency as usual for now, please. You can find examples in other delegates how to do it.</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;">As it looks like this would require a rewrite of the delegate, I'll delay the introduction of it until all other changes are merged into master.</pre>
<br />
<p>- Felix</p>
<br />
<p>On November 7th, 2011, 6:25 p.m., Felix Rohrbach wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.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 Gluon.</div>
<div>By Felix Rohrbach.</div>
<p style="color: grey;"><i>Updated Nov. 7, 2011, 6:25 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;">As the title says. There will be more changes of the UI in later commits, so it wouldn't make much sense to discuss now about the look of the achievements tab in the player.</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>engine/achievement.cpp <span style="color: grey">(021ee24)</span></li>
<li>engine/achievementsmanager.h <span style="color: grey">(1e9e24d)</span></li>
<li>engine/achievementsmanager.cpp <span style="color: grey">(e08a05d)</span></li>
<li>player/desktop/CMakeLists.txt <span style="color: grey">(01cd3ee)</span></li>
<li>player/desktop/delegates/achievementdelegate.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/desktop/delegates/achievementdelegate.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/desktop/gamedetailsoverlay.h <span style="color: grey">(dd95f2a)</span></li>
<li>player/desktop/gamedetailsoverlay.cpp <span style="color: grey">(ca0d6a8)</span></li>
<li>player/lib/models/achievementsmodel.h <span style="color: grey">(c25c1f9)</span></li>
<li>player/lib/models/achievementsmodel.cpp <span style="color: grey">(34000ff)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102987/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/102987/s/327/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2011/11/07/achievement_5_400x100.png" style="border: 1px black solid;" alt="Achievements" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>