[Kde-games-devel] Review of KF5 ports: KJumpingCube, KGoldrunner and Kubrick

[Ian Wadham]

Hi Laurent,

On 19/01/2015, at 3:35 AM, laurent Montel wrote:
> Le Sunday 18 January 2015 17:26:08 Ian Wadham a écrit :
>> I have walked through all the KF5 changes to KJumpingCube,
>> KGoldrunner and Kubrick and (almost) everything looks fine… :-)
>> That is to say, I think the functionality of the games will be the same
>> as before, except perhaps for a few worries.  I am the author or
>> co-author of those three games.
>> 
>> As you know, I am unable to build and test the games in KF5 yet.
>> 
>> KJumpingCube port to KF5:
>> --------------------------------------
>> 
>> Just one query: in file kjumpingcube.cpp, around lines 131-135, this change
>> appears:
>> 
>> -   KAction * b = actionCollection()->addAction (QLatin1String
>> ("action_button")); -   b->setDefaultWidget (actionButton);	// Show the
>> button on the toolbar. +   QAction * b = actionCollection()->addAction
>> (QLatin1String ("action_button")); +   //QT5 b->setDefaultWidget
>> (actionButton);	// Show the button on the toolbar.
> 
> Normal it's not ported yet. As you see there is “//QT5” to inform that we (me) 
> need to port to QT5 api.

Ah, my mistake.  I misunderstood all the //QT5 and #if changes.

>> The last line is commented out, but equivalent code may still be needed,
>> in KF5, for making sure that actionButton, a specially-styled QPushButton,
>> is shown on the toolbar.
>> 
>> Using setDefaultWidget() works in KDE 4 because KAction inherits
>> QWidgetAction, which contains that method.  The preceding 15 lines of my
>> code show the setup of the actionButton widget.  Maybe use QWidgetAction,
>> instead of QAction here?
>> 
>> KGoldrunner port to KF5:
>> ----------------------------------
>> 
>> Thanks for commenting out the many (now unnecessary) debugging messages as
>> you ported them from kDebug() to qDebug()… :-)  I have just five queries...
>> 
>> 1. Why is DBus support added (main.cpp)?
> 
> Because it was created by kapplication/kaboutdata in kde4 => same feature as 
> in kde4
> 
>> KGoldrunner does not use DBus
>> directly. 
>> 2. In file src/kgoldrunner.cpp, some code is #iffed out around
>> line 240.  That would prevent Key_Escape being used as an alternate
>> Pause/Resume key.  Esc has been available in KGr from the very first and in
>> its ancestor game since the 1980s.
> 
> Not ported "//QT5"
> 
>> 3. In file src/kgoldrunner.cpp, some
>> code is #iffed out around line 720.
> 
> //QT5
> 
>> This is needed to translate the names
>> of keys that appear in the Pause/Resume message in the game's status bar (a
>> fix I included in 2008, at Albert's request IIRC).
>> 4. The method
>> KGoldrunner::getDirectories() in file kgoldrunner.cpp is archaic code for
>> getting both read-only and writable (local) data locations, where all the
>> games, levels and other data files are stored.  After it runs, the paths
>> are passed everywhere they are needed, via the QString variables called
>> systemDataDir (read-only dir) and userDataDir (writable dir).  This code
>> should be using QStandardPaths. 
> 
> Yes because there is still kdelibs4support.

I have edited the wiki to indicate that [1].

>> Maybe something like:
>>        systemDataDir =
>> QStandardPaths::locate(QStandardPaths::AppDataLocation, "system",
>> QStandardPaths::LocateDirectory); userDataDir =
>> QStandardPaths::writableLocation(QStandardPaths::AppDataLocation) +
>> "/user/"; 
> 
> 
>> 5. When systemDataDir is found, it will need a "/" to be
>> appended.  If userDataDir is not found or it has a name but does not exist
>> yet, it will need to be created.  So will .../user/levels.  The rest of
>> KGoldrunner needs all that to be in place before the game or game editor
>> can run.
>> 
>> Kubrick port to KF5:
>> --------------------------
>> 
>> Thanks for putting in the qCDebug() code to log errors, should they occur. 
>> The ported code is looking very nice.  I have just three queries...
>> 
>> 1. The code following line 64 in file src/gameglview.cpp does not look
>> right.  Should it be using QStandardPaths?
> 
> Because there is still kdelibs4support.
> 
>> If it succeeds, Kubrick should
>> have wavy lines in its background.  If it fails, Kubrick will put a 4-way
>> gradient in the background and it will appear as if the game is working
>> correctly.
>> 2. In the file kubrick.cpp, several lines that should put messages in
>> statusBar() are #iffed out.
> 
> //QT5
> 
>> They need to appear.  The texts are drawn from
>> const structures and arrays which appear around line 100 and after.
>> 3. The said consts have I18N_NOOP around the C-string literals.  Is that
>> valid in KF5?
>> 
>> Thanks again for all your help on these games, Laurent.
> 
> I told that I make it compile not that they were ready to merge because there 
> is still kdelibs4support and some commented code which needs to be convert to 
> QT5 api :)

OK, understood.

Also I have edited the wiki to note that KGoldrunner, Kubrick and Palapeli need
to migrate user-data as well as config files [1].

I do not know what other games can have user-data.  High-scores are quite
commonly saved.

Cheers, Ian W.

[1] https://techbase.kde.org/Projects/Games/kdegame_apps_porting_kf5_status