Reviewing an activity

This is a checklist to verify before asking for a review.

Design / Ergonomy

Resolution independence

Activities must look nice and be usable on phones, tablets and desktops. Test must be done for the three devices. The resolution and dpi value may differ a lot.

Images and Graphics

Setting image size according ApplicationInfo.ratio

On mobile with high dpi the size of your images and graphics will look smaller. Initial size must be set related to ApplicationInfo.ratio like this:

Image {
    id: ball
    source: "qrc:/gcompris/src/activities/ballcatch/resource/ball.svg"
    sourceSize.height: 100 * ApplicationInfo.ratio
}

Convert png images to svg

Svg format is the prefered format for activities images/icons.

A temporary solution, if you can't find images from openclipart is to convert 'png' images to 'svg' using the following way: 1) Copy all the png images inside a folder.
2) Copy this python script inside the same folder.
3) Run the above python script.

Voila! New converted svg images are created in the same directory.

Fonts

The situation for fonts is comparable. The font.pointSize property of Text-like QML elements handles device independence to a certain degree. An exception are devices with relativ low-dpi and a high resolution (i.e. many tablet devices >= 10'), where fonts are rendered too small when using pointSize.

For this case we use calculated constant ApplicationInfo.fontRatio, that must be used a factor to a font.pointSize value. This is done automatically when the size is set via the fontSize property of the GCFont type, which is the recommended way to specify font-sizes. Example:

GCText {
    id: text
    text: "Text"
    fontSize: 14
    font.weight: Font.DemiBold
    color: "white"
}

You can also use the internal pseudo enum values of GCText, that specify some often used font-sizes:

    readonly property int tinySize:     10.0
    readonly property int smallSize:    12.0
    readonly property int regularSize:  14.0
    readonly property int mediumSize:   16.0
    readonly property int largeSize:    24.0
    readonly property int hugeSize:     32.0

GCText {
...
    fontSize: regularSize
...
}

Window resize

Activities must adapt their content properly when the window is resized.

Screen rotation

Activities must support screen rotation. To do this it can be related to the window width. If items coordinates are absolute, there may be a need for reset when the screen is changed. To detect a rotation the following code can be added in ActivityBase:

    onWidthChanged: Activity.widthChanged()

Audio

Creating Audio items is rather slow. If lots of items are present on the screen, there should not be an Audio item for each. Instead a single Audio should be passed to all the items.

In order to play audio only if audio has been enabled in the preferences, the core GCAudio must be used instead of Audio. Make sure that import "../../core" has been used.

Background image

Background image can not be used to bring useful informations. It is not possible to keep the background aligned with items when the resolution is changed.

Adding a configuration for a specific activity

Each activity can have a specific configuration (for example, changing the locale of the activity, having different modes...). A simple example can be found on Traffic activity where you can change between images and colors to display the cars.

To add configuration, the "config" value in the Bar needs to be added. Then a DialogActivityConfig item needs to be added where the component item of the dialog is defined. The following functions needs to be defined:

• setDefaultValues to set the values when displaying the configuration.
• onLoadData which is called to load the existing data from configuration.
• onSaveData which is called to save the configuration in configuration file and where you can also send signals to dynamically change current activity settings.

The data should be set in "dataToSave" attribute which is a map (pairs of key, values) where the keys are strings and values are javascript var (can be strings, lists...).

Accessing the component items can be done using: dialogActivityConfig.configItem.

For the data to be loaded at start of activity, "dialogActivityConfig.getInitialConfiguration()" needs to be called on Component.onCompleted() of the pageComponent item.

Code Level

Javascript and QML files

Only small Javascript in the QML code must be kept, all the game logic must be in your Javascript file. This makes it easier to read the activity logic. It is possible to have several Javascript files if needed. The QML files must be seen as the graphical interface description and the Javascript the logic of the game.

Copyright update

Make sure all the files have a header with the good license and the updated names for copyright.

Indentation

Indentation is made with 4 spaces example:

Image {
    id: rightHand

Variable names

Variable names must tell what the variable is doing to allow reader to understand the code. example: activity.audioEffects.play("qrc:/gcompris/src/core/resource/sounds/"+ identifier + ".wav") shows clearly that an audio effect is played, which would not be the case of activity.audio.start("qrc:/gcompris/src/core/resource/sounds/"+ identifier + ".wav") Code must be easily understandable by every one

Factorisation

If you read something like (seeing a repeated pattern):

function processKeyLeft() {
    leftShiftPressed();
    items.leftPressed = true
}
function processKeyRight() {
     rightShiftPressed();
     items.rightPressed = true
}

if(event.key == Qt.Key_Left) {
    processKeyLeft()
}
if(event.key == Qt.Key_Right) {
    processKeyRight()
}

it can be more readable, maintainable, and expendable if it is factorised:

function processKey(event) {
    if(event.key == Qt.Key_Left) {
        // left
        leftShiftPressed();
        items.leftPressed = true
    }
    else if(event.key == Qt.Key_Right) {
        // right
        rightShiftPressed();
        items.rightPressed = true
    }
}

Translation

Each time a string appears in the activity it must be written using qsTr(), in order to be present in the translation tools.

GCText {
    text: ApplicationInfo.isMobile ?
                          qsTr("Tap both hands at the same time, " +
                               "to make the ball go in a straight line.") :
                          qsTr("Press left and right arrow keys at the same time, " +
                               "to make the ball go in a straight line.")
}

Anchors

Items positions need to set as much as possible using anchors to be able to to define relationships between different items.

GCText {
    id: instructions
    anchors {
        left: parent.left
        right: parent.right
    }
}

If it is impossible, x and y need to be defined using x and y relatively to other positions.

ActivityInfo.qml

ActivityInfo.qml needs to be completed. It is the file which will describe the nature of your activity.

GCText

Use GCText when displaying text. This ensures the homogeneity of the text displayed in the application (fonts and sizes).

Commiting temporary files

Do not commit temporary file such as genre Dataset.qml~ ou Dataset.qml.autosave.

Cleaning before Review

Before committing there are often not so much energy left for cleaning. It is sometime better to postpone the commit in order to make a nice cleaning phase after a break before sending the PR. Reviewing an uncleaned PR is very frustrating :( and source of waste of time :(

Warnings

All the compilation warnings must be solved before committing.

Script to retrieve a specific github branch from a specific user to do a code review

The following script allows with a simple command line to retrieve a given pull request and start the application. This gains some time and allow to test more efficiently.

---

#!/bin/sh

repo=$1
branch=$2
echo "$repo $branch"
mkdir $1
cd $1
git clone https://github.com/$1/GCompris-qt.git
cd GCompris-qt
git submodule init && git submodule update
git checkout $2
mkdir build
cd build
cmake ..
make
./bin/gcompris-qt

---

To run it, copy paste it in a sh file.

Then for example to pull the following pull request:

https://github.com/gcompris/GCompris-qt/pull/194

Go to the pull request page, then look for the following line:

amitsagtani97 wants to merge 48 commits into gcompris:master from amitsagtani97:calender_activity

This gives you the informations to use the script :

"./retrieveGithub.sh amitsagtani97 calender_activity"

Command line to retrieve a remote branch to be tested from a gcompris fork on invent.kde.org

In order to locally create a branch to test a remote forked repository, you can use the following git commands:

git remote add contributor_name "git@invent.kde.org:contributer_name/gcompris.git"
git fetch contributor_name
git checkout branch_name

It will automatically take the branch from the contributor_name fork because it's the only remote that has a branch called that way.

contributor_name and branch_name need to be replaced with their equivalents found on invent.kde.org gcompris forked page.