Difference between revisions of "Reviewing an activity"
(→Script to help reviewing a pull request) |
Petitlapin (talk | contribs) (→Indentation) |
||
Line 102: | Line 102: | ||
Indentation is made with 4 spaces | Indentation is made with 4 spaces | ||
example: | example: | ||
− | + | <pre> | |
− | + | Image { | |
+ | id: rightHand | ||
+ | </pre> | ||
==Variable names== | ==Variable names== |
Revision as of 20:43, 27 December 2017
Code level
Design / Ergonomy
Resolution independence
Activities must look nice and be usable on phones, tablets and desktops. Test must be done for the three devices.
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 are the prefered format for activities images. 'png' images to 'svg' can be converted 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.
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. example: 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. example: 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 waist of time :(
Warnings
All the compilation warnings must be solved before committing.
Script to help reviewing a pull request
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"