Review Request: Achievements 3: AchievementsAsset

[Laszlo Papp]

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



engine/assets/other/achievements/CMakeLists.txt
<http://git.reviewboard.kde.org/r/101909/#comment3976>

    I think I bumped it to 2.8 in master since even the very old debian has that and they told me it is safe enough to bump after a discussion with them.



engine/assets/other/achievements/achievementsasset.h
<http://git.reviewboard.kde.org/r/101909/#comment3977>

    GLUON_ENGINE_* ?



engine/assets/other/achievements/achievementsasset.h
<http://git.reviewboard.kde.org/r/101909/#comment3978>

    For instance I like the order as it is in the audio asset:
    1) Qt related macros
    2) Gluon related macros like GLUON_OBJECT



engine/assets/other/achievements/achievementsasset.h
<http://git.reviewboard.kde.org/r/101909/#comment3979>

    Reimplementated ? :) If you use spell checker that will highlight the issue for you in case of the comments, documentation.



engine/assets/other/achievements/achievementsasset.cpp
<http://git.reviewboard.kde.org/r/101909/#comment3980>

    Other assets use new *Private, and not new *Private(). I think that is the vast majority of the cases also inside the Qt codebase.



engine/assets/other/achievements/achievementsasset.cpp
<http://git.reviewboard.kde.org/r/101909/#comment3985>

    Even if it is highly unlike to be changed for now, I would use a const QString for the "gluonachievements" part.



engine/assets/other/achievements/achievementsasset.cpp
<http://git.reviewboard.kde.org/r/101909/#comment3981>

    empty line



engine/assets/other/achievements/achievementsasset.cpp
<http://git.reviewboard.kde.org/r/101909/#comment3982>

    i2 ? :) You can use "j" ;)



engine/assets/other/achievements/achievementsasset.cpp
<http://git.reviewboard.kde.org/r/101909/#comment3983>

    Mmh, it did not match with "i2", so it was buggy :) But just use "j".



engine/assets/other/achievements/achievementsasset.cpp
<http://git.reviewboard.kde.org/r/101909/#comment3986>

    Same as above, I would use const QString, so if it changes anytime, it needs to be done only once and the name is separated from the code.


- Laszlo


On July 10, 2011, 6:07 p.m., Felix Rohrbach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101909/
> -----------------------------------------------------------
> 
> (Updated July 10, 2011, 6:07 p.m.)
> 
> 
> Review request for Gluon.
> 
> 
> Summary
> -------
> 
> Adds the AchievementsAsset class, which cares about the Achievement objects and saves them in a file. Also add mime-type information for this file type.
> 
> Note: This patch also adds the file "achievements_template.gluonachievements" at engine/assets/other/achievements/. As this file is empty, ReviewBoard doesn't show it.
> 
> 
> Diffs
> -----
> 
>   creator/x-gluon-mimetypes.xml 378876d 
>   engine/assets/CMakeLists.txt b8c087b 
>   engine/assets/other/achievements/CMakeLists.txt PRE-CREATION 
>   engine/assets/other/achievements/achievementsasset.h PRE-CREATION 
>   engine/assets/other/achievements/achievementsasset.cpp PRE-CREATION 
>   engine/gluon_engine_export.h 17fc00c 
> 
> Diff: http://git.reviewboard.kde.org/r/101909/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Felix
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/gluon/attachments/20110710/c0c8c17e/attachment-0001.htm