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

[Stefan Majewsky]

> On April 1, 2012, 9:10 p.m., Albert Astals Cid wrote:
> > trunk/KDE/kdegames/libkdegames/kgtheme.h, line 72
> > <http://svn.reviewboard.kde.org/r/6933/diff/1/?file=47787#file47787line72>
> >
> >     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

I've added a dummy signal which is never emitted and published this as the NOTIFY signal of all readonly properties.


> On April 1, 2012, 9:10 p.m., Albert Astals Cid wrote:
> > trunk/KDE/kdegames/libkdegames/kgtheme.h, line 126
> > <http://svn.reviewboard.kde.org/r/6933/diff/1/?file=47787#file47787line126>
> >
> >     This class seems abstract enough that it could work for non svg themes, why not call it themePath? or filepath?

"Theme" is ambiguous: Is the theme file the ".desktop" or the ".svgz" file? I've called it graphicsPath now.


> On April 1, 2012, 9:10 p.m., Albert Astals Cid wrote:
> > trunk/KDE/kdegames/libkdegames/kgthemeprovider.cpp, line 51
> > <http://svn.reviewboard.kde.org/r/6933/diff/1/?file=47792#file47792line51>
> >
> >     Do you need this given we already have Q_DECLARE_METATYPE(const KgTheme*) ?

Yes, this is necessary. Q_DECLARE_METATYPE and qRegisterMetaType complement each other. The documentation for qRegisterMetaType has the details.


- Stefan


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


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/20120411/be0a357f/attachment.html>