Review Request: Achievements 15: A better UI for Achievements in the KDE Extended player.

[Laszlo Papp]

> On Nov. 6, 2011, 7:51 p.m., Laszlo Papp wrote:
> > > 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).
> 
> Felix Rohrbach wrote:
>     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.
> 
> Laszlo Papp wrote:
>     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.
> 
> Felix Rohrbach wrote:
>     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.

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.


- Laszlo


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102987/#review7981
-----------------------------------------------------------


On Nov. 7, 2011, 6:25 p.m., Felix Rohrbach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102987/
> -----------------------------------------------------------
> 
> (Updated Nov. 7, 2011, 6:25 p.m.)
> 
> 
> Review request for Gluon.
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   engine/achievement.cpp 021ee24 
>   engine/achievementsmanager.h 1e9e24d 
>   engine/achievementsmanager.cpp e08a05d 
>   player/desktop/CMakeLists.txt 01cd3ee 
>   player/desktop/delegates/achievementdelegate.h PRE-CREATION 
>   player/desktop/delegates/achievementdelegate.cpp PRE-CREATION 
>   player/desktop/gamedetailsoverlay.h dd95f2a 
>   player/desktop/gamedetailsoverlay.cpp ca0d6a8 
>   player/lib/models/achievementsmodel.h c25c1f9 
>   player/lib/models/achievementsmodel.cpp 34000ff 
> 
> Diff: http://git.reviewboard.kde.org/r/102987/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Achievements
>   http://git.reviewboard.kde.org/r/102987/s/327/
> 
> 
> Thanks,
> 
> Felix Rohrbach
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/gluon/attachments/20111110/3a86a5c2/attachment.html>