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

[Parker Coates]

> On 2009-11-29 21:56:48, Parker Coates wrote:
> > /trunk/KDE/kdegames/libkdegames/kgamepopupitem_p.h, lines 112-142
> > <http://reviewboard.kde.org/r/2296/diff/1/?file=15122#file15122line112>
> >
> >     Putting all this code in a header will force it to be inlined, right? Is that really what you want?
> 
> Stefan Majewsky wrote:
>     That's a private header, that has been created to improve readability. The classes in there are all internal.

Sorry. My explanation was completely stupid and not what I actually objected to. :|

I understand the concept of a private header, but it strikes me as odd to put actual function implementations there. Of course, maybe that's a common practice that I just haven't seen before. I would never go looking in a .h file for the implementation.


> On 2009-11-29 21:56:48, Parker Coates wrote:
> > /trunk/KDE/kdegames/libkdegames/kgamepopupitem_p.h, lines 42-75
> > <http://reviewboard.kde.org/r/2296/diff/1/?file=15122#file15122line42>
> >
> >     Is creating three private QGI subclasses really that much simpler then just doing everything in a single paint() method?
> >     
> >     I don't really understand what KGamePopupGraphicsObject is even needed for. Could you explain this a bit?
> >     
> >
> 
> Stefan Majewsky wrote:
>     Yes, I don't like this part of the current code, either.
>     
>     On KGPGO: Only a QGraphicsObject can be connected with QPropertyAnimation. Also, having a single child item below KGPI simplifies animation, and is actually necessary to deprecate QGraphicsItem::opacity in favor of KGPI::messageOpacity. (Thinking about this again, I can probably use the QGraphicsTextItem as this master item, it's already a QGraphicsObject.)

I think your original goal was to simplify this class, but it doesn't seem like you've actually done so. You're using newer Qt features, but the overall line count hasn't really dropped. Individual functions are simpler, but the number of classes involved is up.

Personally, if I were doing this, I would manually do all the painting myself in KGamePopupItem::paint() and do away with the subitems. The paint method would be big, but I'm not sure it'd be any worse than managing the subitems. I would make the private class a QObject and hook a QPropertyAnimation up to it. Opacity handling is super simple since Qt4.5. With only one item, event handling becomes trivial.

Maybe there's a complication that I'm not seeing, but really I think we could get away with a lot less code.


> On 2009-11-29 21:56:48, Parker Coates wrote:
> > /trunk/KDE/kdegames/libkdegames/kgamepopupitem.cpp, line 178
> > <http://reviewboard.kde.org/r/2296/diff/1/?file=15121#file15121line178>
> >
> >     No, I don't think so. KPat doesn't use KGamePopupItem, but it has Z values above 1000. Whatever value is picked will be arbitrary, but I would go with something more obviously "big", like 1e6.
> 
> Stefan Majewsky wrote:
>     Can we agree on something like "INT_MAX - 100"? ;-) By the way, this value has always been like this, I did not touch it.

zValue is a qreal not an int, so using INT_MAX would be especially arbitrary. :) It would also be platform dependent.

I would say just put it at a million and add a note to the API docs. If someone actually needs it higher, can't they just call setZValue on it?


- Parker


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


On 2009-11-28 19:44:51, Stefan Majewsky wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2296/
> -----------------------------------------------------------
> 
> (Updated 2009-11-28 19:44:51)
> 
> 
> 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 and orchestrated by a QStateMachine.
> 
> 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 1054906 
>   /trunk/KDE/kdegames/libkdegames/kgamepopupitem.cpp 1054906 
>   /trunk/KDE/kdegames/libkdegames/kgamepopupitem_p.h PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/2296/diff
> 
> 
> Testing
> -------
> 
> I have used kgamepopupitemtest to keep regressions at a minimum. I'm aware of and investigating the issue that hiding is not animated.
> 
> 
> Thanks,
> 
> Stefan
> 
>