[Kde-games-devel] Review Request 110137: PopupItem QML component for KGamePopupItem

[Viranch Mehta]

> On April 23, 2013, 10:12 p.m., Albert Astals Cid wrote:
> > My initail review gives me a bit of warning about those "hardcoded" color defaults/sizes. Sorry I didn't have time to check how the cpp version does it. Are they hardcoded too?

colors are not hardcoded, cpp code has used a KStatefulBrush(KColorScheme::Tooltip, KColorScheme::NormalBackground) for background and KStatefulBrush(KColorScheme::Tooltip, KColorScheme::NormalText) for text. as for sizes and animation duration, yes, they are hardcoded in cpp.


> On April 23, 2013, 10:12 p.m., Albert Astals Cid wrote:
> > declarativeimports/qml/PopupItem.qml, line 40
> > <http://git.reviewboard.kde.org/r/110137/diff/1/?file=140650#file140650line40>
> >
> >     Is this hardcoded to that color in the cpp too?

this is the color of KStatefulBrush, I took the color specs by printing it in cpp.


> On April 23, 2013, 10:12 p.m., Albert Astals Cid wrote:
> > declarativeimports/qml/PopupItem.qml, line 49
> > <http://git.reviewboard.kde.org/r/110137/diff/1/?file=140650#file140650line49>
> >
> >     This hardcoded 32 feels a bit wrong, can we get something better? Is this what the cpp code is using?
> >     
> >     Also i guess that instead duplicating the logic is easier if you do somehing like
> >     height: width

yes, the cpp is using "dialog-information" kicon with 32x32 hardcoded default size.

although the cpp also lets us pass a custom QPixmap (of whatever size) to be used as the icon, which qml doesn't. dunno if such a flexibility is necessary as no game is using it. QML is already providing option to show the icon or not, and to specify an alternate KIcon name. but this would not let us set any other desired size of icon than 32x32.

should the QML allow us to pass any hand-made Image element to be used as the icon? that way we can control pretty much anything about the icon image.

+1 on height: width


> On April 23, 2013, 10:12 p.m., Albert Astals Cid wrote:
> > declarativeimports/qml/PopupItem.qml, line 66
> > <http://git.reviewboard.kde.org/r/110137/diff/1/?file=140650#file140650line66>
> >
> >     is this hardcoded to white in the cpp version?

KStatefulBrush again


- Viranch


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


On April 23, 2013, 7:51 p.m., Viranch Mehta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110137/
> -----------------------------------------------------------
> 
> (Updated April 23, 2013, 7:51 p.m.)
> 
> 
> Review request for KDE Games.
> 
> 
> Description
> -------
> 
> Implements a QML component KgCore.PopupItem for KGamePopupItem widget. Most of the properties of KGamePopupItem are provided by PopupItem, except ability to set the background brush (can set background color instead), and ability to set a custom QPixmap as icon (can set KIcon's name instead and switch showing icon on/off instead).
> 
> Although one of the critical downside is it cannot set theme based background and text colors. Not sure how to overcome this. But at least we have the component for starting to use in ports.
> 
> 
> Diffs
> -----
> 
>   declarativeimports/qml/PopupItem.qml PRE-CREATION 
>   declarativeimports/qml/qmldir 79aae32 
> 
> Diff: http://git.reviewboard.kde.org/r/110137/diff/
> 
> 
> Testing
> -------
> 
> The KMines port (viranch/qtquick branch in kmines) uses this component (src/qml/main.qml) for showing "paused" message when game is paused/unpaused. works as expected except themed colors.
> 
> 
> Thanks,
> 
> Viranch Mehta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20130423/7d147c55/attachment-0001.html>