[Kde-games-devel] Review Request: new KgTheme and KgThemeProvider classes to replace and generalize KGameTheme

[Albert Astals Cid]

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



trunk/KDE/kdegames/libkdegames/kgamerenderer.h
<http://svn.reviewboard.kde.org/r/6933/#comment13286>

    If you plan to use this from QML having a NOTIFY signal is quite important



trunk/KDE/kdegames/libkdegames/kgamerenderer.h
<http://svn.reviewboard.kde.org/r/6933/#comment13288>

    Don't you need to Q_DECLARE_METATYPE the KgThemeProvider * if you want this to work in all cases (i.e. read from QML?)



trunk/KDE/kdegames/libkdegames/kgtheme.h
<http://svn.reviewboard.kde.org/r/6933/#comment13290>

    You should still have the NOTIFY signals otherwise when you do something like
    Mylabeltext: theme.name
    in QML it'll complain you are depending on a property that doesn't have NOTIFY support



trunk/KDE/kdegames/libkdegames/kgtheme.h
<http://svn.reviewboard.kde.org/r/6933/#comment13291>

    This class seems abstract enough that it could work for non svg themes, why not call it themePath? or filepath?



trunk/KDE/kdegames/libkdegames/kgthemeprovider.cpp
<http://svn.reviewboard.kde.org/r/6933/#comment13287>

    Do you need this given we already have Q_DECLARE_METATYPE(const KgTheme*) ?


- Albert Astals Cid


On April 1, 2012, 5:55 p.m., Stefan Majewsky wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6933/
> -----------------------------------------------------------
> 
> (Updated April 1, 2012, 5:55 p.m.)
> 
> 
> Review request for KDE Games.
> 
> 
> Description
> -------
> 
> KgTheme is a simple generalization (with QML-ready API) of KGameTheme. The main generalization lies in that it can be used without an installed desktop file.
> 
> KgThemeProvider implements a selection logic on top of KgTheme. You give it a bunch of KgTheme instances, and it determines which one is selected at the moment using the application config (if you want). KgThemeProvider includes discovery logic for themes installed somewhere in KStandardDirs, but (unlike e.g. the old KGameThemeSelector) is not restricted to ${DATA_INSTALL_DIR}/$APP/themes.
> 
> KGameRenderer, which previously used KGameTheme internally, now relies on a KgThemeProvider for its theme selection.
> 
> KgThemeSelector is an update to KGameThemeSelector, which borrows its appearance from Palapeli's puzzle list.
> 
> The current diff is just a snapshot of my work. It does not compile because I could not find a way to fully emulate old-style KGameRenderer usage with the new API. The full diff will thus need to port KGameRenderer usages to KgTheme(Provider) immediately.
> 
> Compared to the minimalistic KGameTheme, the KgTheme(Provider) API is quite complex, for the following reasons:
> 1. I plan to replace app-local theming logic and KGameThemeSelector copies by KgTheme usage. Most prominent example: libkmahjongg.
> 2. The KgThemeProvider::generatePreview method *can* be used to compose preview images dynamically, so the theme author does not need to care about the preview anymore.
> 3. KgTheme can be subclassed for app-specific behavior. I want to use this feature in KDiamond: Two of three KDiamond themes include bitmap canvas backgrounds, which amount for >50% of the SVG size and greatly increase SVG loading time and cache burden. I want to optionally pull these bitmaps out into separate files, using logic in a custom KgTheme subclass.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdegames/kdiamond/src/game.cpp 1287365 
>   trunk/KDE/kdegames/killbots/renderer.h 1287365 
>   trunk/KDE/kdegames/killbots/renderer.cpp 1287365 
>   trunk/KDE/kdegames/klickety/gamescene.cpp 1287365 
>   trunk/KDE/kdegames/kpat/renderer.h 1287365 
>   trunk/KDE/kdegames/kpat/renderer.cpp 1287365 
>   trunk/KDE/kdegames/libkdegames/CMakeLists.txt 1287365 
>   trunk/KDE/kdegames/libkdegames/includes/CMakeLists.txt 1287365 
>   trunk/KDE/kdegames/libkdegames/includes/KgTheme PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/includes/KgThemeProvider PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/includes/KgThemeSelector PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/kgamerenderer.h 1287365 
>   trunk/KDE/kdegames/libkdegames/kgamerenderer.cpp 1287365 
>   trunk/KDE/kdegames/libkdegames/kgamerenderer_p.h 1287365 
>   trunk/KDE/kdegames/libkdegames/kgtheme.h PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/kgtheme.cpp PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/kgtheme_p.h PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/kgthemeprovider-migration.upd PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/kgthemeprovider.h PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/kgthemeprovider.cpp PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/kgthemeselector.h PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/kgthemeselector.cpp PRE-CREATION 
>   trunk/KDE/kdegames/libkdegames/kgthemeselector_p.h PRE-CREATION 
> 
> Diff: http://svn.reviewboard.kde.org/r/6933/diff/
> 
> 
> Testing
> -------
> 
> None as of now, for the reasons explained above. API reviews are welcome.
> 
> The kconf_update script in libkdegames/kgthemeprovider-migration.upd is also just a proof-of-concept.
> 
> 
> Thanks,
> 
> Stefan Majewsky
> 
>

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