Changeset View
Changeset View
Standalone View
Standalone View
src/activities/letter-in-word/LetterInWord.qml
| Context not available. | |||||
| 24 | 24 | | |||
|---|---|---|---|---|---|
jjazeix: you shouldn't change anything on checking/unchecking the box, it should only be done when the… | |||||
| 25 | import QtQuick 2.6 | 25 | import QtQuick 2.6 | ||
| 26 | import QtGraphicalEffects 1.0 | 26 | import QtGraphicalEffects 1.0 | ||
| 27 | import QtQuick.Controls 1.5 | ||||
jjazeix: still needed? | |||||
| 27 | import GCompris 1.0 | 28 | import GCompris 1.0 | ||
| 28 | import "../../core" | 29 | import "../../core" | ||
| 29 | import "letter-in-word.js" as Activity | 30 | import "letter-in-word.js" as Activity | ||
you should use ExclusiveGroup (https://doc.qt.io/qt-5/qml-qtquick-controls-exclusivegroup.html) jjazeix: you should use ExclusiveGroup (https://doc.qt.io/qt-5/qml-qtquick-controls-exclusivegroup.html) | |||||
if I'm not wrong, you never restore the reduceWords when starting the activity jjazeix: if I'm not wrong, you never restore the reduceWords when starting the activity | |||||
why do you change it here? It should only be changed when clicking on the cross (save method). Also, the names of the states are not the good ones. jjazeix: why do you change it here? It should only be changed when clicking on the cross (save method). | |||||
| Context not available. | |||||
| 64 | property Item main: activity.main | 65 | property Item main: activity.main | ||
| 65 | property alias bar: bar | 66 | property alias bar: bar | ||
| 66 | property alias background: background | 67 | property alias background: background | ||
jjazeix: remove whitespaces | |||||
| 67 | property alias wordsModel: wordsModel | 68 | property alias wordsModel: wordsModel | ||
| 68 | property bool reduceWords: false | | |||
| 69 | property string mode: "normal" | 69 | property string mode: "normal" | ||
why do you keep two variables? You keep mode and two booleans easy normal mode, they both do the same work? dmadaan: why do you keep two variables? You keep mode and two booleans easy normal mode, they both do… | |||||
why not directly store the number of words as int? It would have more sense than a string "easy", "normal"and would be directly be useable in the js file jjazeix: why not directly store the number of words as int? It would have more sense than a string… | |||||
you can rename it to be more precise on what it does jjazeix: you can rename it to be more precise on what it does
and have "readonly int" variables to name… | |||||
| 70 | property bool reduceWords: false | ||||
| 70 | property GCAudio audioVoices: activity.audioVoices | 71 | property GCAudio audioVoices: activity.audioVoices | ||
| 71 | property alias parser: parser | 72 | property alias parser: parser | ||
| 72 | property alias animateX: animateX | 73 | property alias animateX: animateX | ||
| Context not available. | |||||
| 93 | animateX.restart(); | 94 | animateX.restart(); | ||
| 94 | } | 95 | } | ||
| 95 | 96 | | |||
| 97 | ExclusiveGroup{ | ||||
jjazeix: missing space | |||||
| 98 | id: configOptions | ||||
| 99 | } | ||||
| 100 | | ||||
| 96 | DialogActivityConfig { | 101 | DialogActivityConfig { | ||
| 97 | id: dialogActivityConfig | 102 | id: dialogActivityConfig | ||
| 98 | currentActivity: activity | 103 | currentActivity: activity | ||
the names can be better what is easy mode should be signigicant from the variable name. dmadaan: the names can be better what is easy mode should be signigicant from the variable name. | |||||
| Context not available. | |||||
| 119 | width: column.width - 50 | 124 | width: column.width - 50 | ||
| 120 | text: qsTr("Reduce Visible Words to 5") | 125 | text: qsTr("Reduce Visible Words to 5") | ||
| 121 | checked: (items.mode == "easy") ? true : false | 126 | checked: (items.mode == "easy") ? true : false | ||
| 122 | onCheckedChanged: { | 127 | exclusiveGroup: configOptions | ||
jjazeix: you still use the numbers instead of the variables you defined | |||||
| 123 | if(easyMode.checked) { | 128 | onCheckedChanged: { | ||
jjazeix: I still don't understand why you change mode here | |||||
| 129 | if (easyMode.checked){ | ||||
| 124 | items.mode = "easy" | 130 | items.mode = "easy" | ||
| 125 | items.reduceWords = true | | |||
| 126 | } | 131 | } | ||
| 127 | else{ | 132 | } | ||
| 128 | items.mode = "normal" | | |||
| 129 | items.reduceWords = false | | |||
| 130 | } | | |||
| 131 | normalMode.checked = !easyMode.checked | | |||
| 132 | } | | |||
| 133 | } | 133 | } | ||
| 134 | 134 | | |||
| 135 | GCDialogCheckBox { | 135 | GCDialogCheckBox { | ||
| Context not available. | |||||
| 137 | width: column.width - 50 | 137 | width: column.width - 50 | ||
| 138 | text: qsTr("Get All Available Words") | 138 | text: qsTr("Get All Available Words") | ||
| 139 | checked: (items.mode == "normal") ? true : false | 139 | checked: (items.mode == "normal") ? true : false | ||
| 140 | exclusiveGroup: configOptions | ||||
| 140 | onCheckedChanged: { | 141 | onCheckedChanged: { | ||
| 141 | if(normalMode.checked) { | 142 | if (normalMode.checked){ | ||
| 142 | items.mode = "normal" | 143 | items.mode = "normal" | ||
| 143 | items.reduceWords = false | | |||
| 144 | } | 144 | } | ||
| 145 | else{ | | |||
| 146 | items.mode = "easy" | | |||
| 147 | items.reduceWords = true | | |||
| 148 | } | | |||
| 149 | easyMode.checked = !normalMode.checked | | |||
| 150 | } | 145 | } | ||
| 151 | } | 146 | } | ||
| 152 | 147 | | |||
jjazeix: missing space | |||||
| Context not available. | |||||
| 168 | onLoadData: { | 163 | onLoadData: { | ||
| 169 | if(dataToSave && dataToSave["mode"]){ | 164 | if(dataToSave && dataToSave["mode"]){ | ||
| 170 | items.mode = dataToSave["mode"] | 165 | items.mode = dataToSave["mode"] | ||
| 171 | console.log(items.mode + " mode loaded") | 166 | items.reduceWords = items.mode == "easy" ? true : false | ||
| 172 | //Activity.initLevel() | 167 | console.log("Mode: " + items.mode); | ||
| 173 | } | 168 | } | ||
| 174 | 169 | | |||
| 175 | if(dataToSave && dataToSave["locale"]) { | 170 | if(dataToSave && dataToSave["locale"]) { | ||
jjazeix: "modes" can be renamed to be more precise | |||||
| Context not available. | |||||
| 185 | newLocale = newLocale.substring(0, newLocale.indexOf('.')) | 180 | newLocale = newLocale.substring(0, newLocale.indexOf('.')) | ||
| 186 | } | 181 | } | ||
| 187 | dataToSave = {"locale": newLocale} | 182 | dataToSave = {"locale": newLocale} | ||
| 183 | items.reduceWords = items.mode == "easy" ? true : false | ||||
I still don't understand why you use 2 different variables for the same data jjazeix: I still don't understand why you use 2 different variables for the same data | |||||
| 188 | dataToSave["mode"] = items.mode | 184 | dataToSave["mode"] = items.mode | ||
| 189 | console.log(items.mode + " mode saved") | 185 | | ||
| 186 | console.log("Mode: " + items.mode) | ||||
| 187 | Activity.initLevel() | ||||
| 190 | 188 | | |||
| 191 | background.locale = newLocale; | 189 | background.locale = newLocale; | ||
| 192 | 190 | | |||
| Context not available. | |||||
qsTr needs to be used for translatable texts. jjazeix: qsTr needs to be used for translatable texts.
Easy does not help the child, we should find a… | |||||
jjazeix: you should use a GCDialogCheckBox instead of directly a GCText | |||||
jjazeix: no need to change the text color | |||||
jjazeix: you should not change the text directly | |||||
it should only be changed when clicking on the cross (so in onSaveData). jjazeix: it should only be changed when clicking on the cross (so in onSaveData).
You should also check… | |||||
you shouldn't change anything on checking/unchecking the box, it should only be done when the user clicks on the Cross to go back to the activity (onSaveData).