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

[Dmitry Suzdalev]

> 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.
> 
> Stefan Majewsky wrote:
>     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.

Hmm, ok. I won't insist, that's just how I feel about it - I like #includes grouped with #includes and forward declarations grouped with forward declarations. If you have your reasons to have them this way, let it be.

Btw, i can make up a situation where
#include "smth.h"
class MyClass;
#include "smthelse.h"

would make compiler not to detect a missing forward decl of MyClass in smthelse.h, but that's more of a made up situation, such thing will be quickly caught anyway I think.


> 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.
> 
> Stefan Majewsky wrote:
>     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.

He could use QBrush(Qt::transparent) for that.

Well, okay. But I must say that I still feel bad about having API growing and growing. I don't want to have a zillion of functions for every possible or not very possible situation.
Do you really feel these reset* thingies are needed by *many* apps and will be much used?
So far no one used them and no requests I heard of, why do you think this will change in future?

I'm not trying to be totally against, i'm trying to understand the real value of having another set of API methods.


- Dmitry


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