[Kde-games-devel] Review Request: Adapt KGamePopupItem to Qt 4.5 and 4.6 improvements

[Dmitry Suzdalev]

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



/trunk/KDE/kdegames/libkdegames/kgamepopupitem.h
<http://reviewboard.kde.org/r/2296/#comment2701>

    I think it makes sense to just enchance setMessageIcon() to be able to reset icon to default one if pix argument is a QPixmap() (i.e. invalid pixmap)
    
    Of course this needs to be explicitely documented in api dox.
    
    We already have a lot of functions IMO. Better to add them only when really necessary.



/trunk/KDE/kdegames/libkdegames/kgamepopupitem.h
<http://reviewboard.kde.org/r/2296/#comment2703>

    Same here. In setBackgroundBrush() we can check that QBrush() is passed (it'll have style() == QBrush::NoStyle) and reset brush then.



/trunk/KDE/kdegames/libkdegames/kgamepopupitem.h
<http://reviewboard.kde.org/r/2296/#comment2704>

    And here :)
    !QColor().isValid() can be used to reset the color



/trunk/KDE/kdegames/libkdegames/kgamepopupitem.cpp
<http://reviewboard.kde.org/r/2296/#comment2705>

    Just wanted to write that this line needs to handle views().isEmpty() check (my bug), and noticed some TODO's on that below. great.
    Do not forget to use that new function here, this line lacks TODO :)



/trunk/KDE/kdegames/libkdegames/kgamepopupitem.cpp
<http://reviewboard.kde.org/r/2296/#comment2706>

    Ah, so you already on it :) Great.



/trunk/KDE/kdegames/libkdegames/kgamepopupitem_p.h
<http://reviewboard.kde.org/r/2296/#comment2707>

    Let's have things grouped nicely:
    
    First 
    
    #includes <...>
    
    then
    
    class ForwardDeclaration;
    
    Why mess them up? :)


- Dmitry


On 2009-11-30 23:16:02, Stefan Majewsky wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2296/
> -----------------------------------------------------------
> 
> (Updated 2009-11-30 23:16:02)
> 
> 
> Review request for KDE Games and Dmitry Suzdalev.
> 
> 
> Summary
> -------
> 
> Apart from some general cleanup, the most interesting changes are:
> * Use QGraphicsItem::opacity. (KGamePopupItem::messageOpacity is deprecated now.)
> * Use QGraphicsPathItem and QGraphicsPixmapItem instead of painting primitives.
> * Improve event handling of the QGraphicsTextItem.
> * Radically simplify internal layouting code.
> * Instead of QTimeLine, animations are now implemented as QPropertyAnimations.
> * Introduce methods to read and reset the appearance properties (messageIcon, textColor, backgroundBrush).
> The last change introduces new public API. The patch includes an appropriate update to the unit test.
> 
> Note that the changes are rather extensive. (I estimate that 80% of the codebase has already changed, and I'm not finished yet.) If you prefer that, I can also send you 12 separate patches of the single commits I made in my git-svn repository.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdegames/libkdegames/kgamepopupitem.h 1056063 
>   /trunk/KDE/kdegames/libkdegames/kgamepopupitem.cpp 1056063 
>   /trunk/KDE/kdegames/libkdegames/kgamepopupitem_p.h PRE-CREATION 
>   /trunk/KDE/kdegames/libkdegames/tests/kgamepopupitemtest.h 1056063 
>   /trunk/KDE/kdegames/libkdegames/tests/kgamepopupitemtest.cpp 1056063 
>   /trunk/KDE/kdegames/libkdegames/tests/kgamepopupitemtest.ui 1056063 
> 
> Diff: http://reviewboard.kde.org/r/2296/diff
> 
> 
> Testing
> -------
> 
> I have used kgamepopupitemtest and did not find any regressions yet.
> 
> 
> Thanks,
> 
> Stefan
> 
>