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

[Stefan Majewsky]

> On 2009-12-02 15:36:22, Dmitry Suzdalev wrote:
> > /trunk/KDE/kdegames/libkdegames/kgamepopupitem.h, line 123
> > <http://reviewboard.kde.org/r/2296/diff/2/?file=15265#file15265line123>
> >
> >     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.

I have inserted a resetMessageIcon() because it allows the appropriate syntax in the Q_PROPERTY macro to be used, should we ever insert that.


> On 2009-12-02 15:36:22, Dmitry Suzdalev wrote:
> > /trunk/KDE/kdegames/libkdegames/kgamepopupitem.h, line 155
> > <http://reviewboard.kde.org/r/2296/diff/2/?file=15265#file15265line155>
> >
> >     Same here. In setBackgroundBrush() we can check that QBrush() is passed (it'll have style() == QBrush::NoStyle) and reset brush then.

A game programmer could want to use setBackgroundBrush(QBrush()); to make the box transparent. Also, resetBackgroundBrush() is much easier to understand when you read other people's code.


> On 2009-12-02 15:36:22, Dmitry Suzdalev wrote:
> > /trunk/KDE/kdegames/libkdegames/kgamepopupitem_p.h, line 23
> > <http://reviewboard.kde.org/r/2296/diff/2/?file=15267#file15267line23>
> >
> >     Let's have things grouped nicely:
> >     
> >     First 
> >     
> >     #includes <...>
> >     
> >     then
> >     
> >     class ForwardDeclaration;
> >     
> >     Why mess them up? :)
> 
> Parker Coates wrote:
>     The advantage of this system is that things are alphabetical. If you need to convert and include to a forward declaration (or vice versa) you just edit that line in place.
>     
>     I personally prefer this way, but I can see reasons to group the way you suggest. In this particular case, I think it makes sense to do whatever the kdelibs coding style recommends.

kdelibs coding style says nothing about how includes should be ordered. My personal preference is sorting by position of the library in the framework stack (e. g. first Linux headers and STL, then Qt, then KDE), then in the second order alphabetic sorting. If you want me to split forward declarations and includes, I can also do that.


> On 2009-12-02 15:36:22, Dmitry Suzdalev wrote:
> > /trunk/KDE/kdegames/libkdegames/kgamepopupitem.cpp, line 266
> > <http://reviewboard.kde.org/r/2296/diff/2/?file=15266#file15266line266>
> >
> >     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 :)

That's what Ctrl+F+"QGraphicsView*" is for. ;-)


- Stefan


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


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
> 
>