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

[Laszlo Papp]

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


There is a post-sprint implementation ongoing I started in Munich. You might need to rebase this patch on top of those patches, but it should not be a big work since it is mostly just about transferring. I will put you into the CC field of those patches, and you can get easily notified. It should not be more than 1-2 days from now on. I have not unfortunately had too much time to review this patch in details because of my deadlines and personal issues. It seems okay to me for a fast skim, though. I do not object to it, but please wait for the ongoing work first.


engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/102987/#comment6674>

    For reminding: Please always put a space after the description. I am sometimes just interested in that, and not the parameter types and so on. It is better if they are separated.
    
    You do not need to change it for now since It is already done without space in this file, just for future reference. :)



player/kdeext/delegates/achievementdelegate.cpp
<http://git.reviewboard.kde.org/r/102987/#comment6668>

    Please use <KDE/X> style.



player/kdeext/delegates/achievementdelegate.cpp
<http://git.reviewboard.kde.org/r/102987/#comment6669>

    How about the term "Invalid" instead of "Wrong" in general ?
    
    I would recommend using the same pattern as in other delegates:
    kDebug() << "Warning - Invalid Model!; 
    
    You could write something, like:
    kDebug() << "Warning - Invalid Model! AchievementModel expected.";



player/kdeext/delegates/achievementdelegate.cpp
<http://git.reviewboard.kde.org/r/102987/#comment6670>

    Start the comments with capital letters, if it is not a direct function name and so on (where it should be left as is). :) It is mixed in the codebase either way..



player/kdeext/delegates/achievementdelegate.cpp
<http://git.reviewboard.kde.org/r/102987/#comment6675>

    Please use if { .. } since you use { ... } for the "else" branch as well.



player/kdeext/delegates/achievementdelegate.cpp
<http://git.reviewboard.kde.org/r/102987/#comment6673>

    On different lines, please.



player/kdeext/delegates/achievementdelegate.cpp
<http://git.reviewboard.kde.org/r/102987/#comment6671>

    Same, capital letter.



player/kdeext/delegates/achievementdelegate.cpp
<http://git.reviewboard.kde.org/r/102987/#comment6672>

    Same, capital letter.


- Laszlo Papp


On Oct. 29, 2011, 1:25 p.m., Felix Rohrbach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102987/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2011, 1: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/kdeext/CMakeLists.txt 01cd3ee 
>   player/kdeext/delegates/achievementdelegate.h PRE-CREATION 
>   player/kdeext/delegates/achievementdelegate.cpp PRE-CREATION 
>   player/kdeext/gamedetailsoverlay.h dd95f2a 
>   player/kdeext/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
> -------
> 
> 
> Thanks,
> 
> Felix Rohrbach
> 
>

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