Details
- Reviewers
jjazeix echarruau rahulyadav scagarwal - Group Reviewers
GCompris
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
I have made some comments for small changes also so that you have a todo list to work with. But the initial port is good.
| src/activities/multiplication_tables/ActivityInfo.qml | ||
|---|---|---|
| 31 ↗ | (On Diff #10154) | use "math" |
| src/activities/multiplication_tables/Multiplication_tables.qml | ||
| 7 | If its your own activity and not a port from the GTK version . I dont think you need to write (Qt quick port).Just in case ask Johnny also. | |
| 74 | I personally liked the previously layout of two columns. I think you should use that instead current layout. and add focus with arrow keys also it would be handy.. | |
| 196 | Not sure about the timer concept. we skipped it in guesscount activity ( to avoid dicouragement) . But i think a seperate mode will be better. You can talk to johnny about it. | |
| 211 | these errors occur as soon as i change mode. (qrc:/gcompris/src/activities/multiplication_tables/Multiplication_tables.qml:222:25: QML CheckBox: Binding loop detected for property "implicitHeight" you have misused property . ping me if you are not able to solve it i.ll make a patch. | |
| src/activities/multiplication_tables/Question.qml | ||
| 1 | Please use the same comments as present used in Multiplication_tables.qml . Same goes for any new file js or qml. | |
| 26 | I dont think you needed flow here Rectangle will be fine so that you can provide alignment . | |
| 33 | Put all the components inside a rectangle(instead of flow) and then add vertical alignment. (it you see the text input feild its bottom aligns with the bottom of the text) | |
| 35 | you have automate using GCText . Its pretty easy . If you have any problem ping me. | |
| 42 | Add validator : so that only numbers can be added. you can also add range. | |
| 59 | put a conditional statement in the Image component. | |
| src/activities/multiplication_tables/drawnumber_dataset.js | ||
| 1 ↗ | (On Diff #10154) | there is no need for a dataset bcz the level no will be in sync with the Table No. |
| src/activities/multiplication_tables/multiplication_tables.js | ||
| 66 | no need for this also. Verification can be done in the in Question.qml itself. | |
| 76 | try to use properties rather than writing explicit code in js. I added comment for this in Question.qml. | |
Hi, I don't have time to do a complete review.
@rahulyadav: the activity should be generic and not only focused on mathematical exercise (you can see it as a MCQ (multiple-choice questionnaire) so the layout must be the more generic possible (to handle long questions/answers) and we can't have validator for the answer and we need specific datasets
thanks @rahulyadav for your feedback.I'll make the changes after discussing them with Johnny & emmanuel.
@jjazeix & @echarruau , it would be great if you could also suggest some changes & modifications.
regards.
nitish
A suggestion not related to the content of the code: the title of the review will become the commit message in git, so I suggest using something different than "SOK_multiplicationtables_REVIEW".
REVIEW is implicit, SOK is not relevant here (you can land the work on a separate branch).
For more details please see http://chris.beams.io/posts/git-commit/
Hi,
you also need to add a README file with all the image links and licenses.
It also misses the virtual keyboard (and potentially, if we know the answer can only be numeric, think of using the Numpad).
I tested it and it looked better than the previous code (the code also is ;)). Have you tested it with a "bigger size" dataset? Long questions and potentially long answers to be sure it's correctly displayed?
Johnny
| src/activities/multiplication_tables/ActivityInfo.qml | ||
|---|---|---|
| 32 ↗ | (On Diff #10154) | 8000 |
| src/activities/multiplication_tables/Multiplication_tables.qml | ||
| 81 | right? | |
| 105 | Finish, no need to have spaces before and after and uppercase | |
| 135 | you don't need 2 variables | |
| 145 | If the button theme is the same as the other one, maybe you can create an Item for it. | |
| 147 | same as above | |
| src/activities/multiplication_tables/Question.qml | ||
| 22 | do you need this? | |
| src/activities/multiplication_tables/drawnumber_dataset.js | ||
| 2 ↗ | (On Diff #10154) | This file should be deleted, it is not used, the one being used should be multiplication_dataset.js |
| 22 ↗ | (On Diff #10154) | tableName (no need for uppercase for first letter) |
| src/activities/multiplication_tables/multiplication_tables.js | ||
| 36 | scoreCounter | |
| 62 | no. Either the question has to be translated and you put qsTr() directly in the dataset or it does not have and you don't need it. | |
| 79 | I'm not sure you need the toString() call | |
| 85 | no need for qsTr() for empty strings, what do you want the translator translate :)? | |
| 86 | The variable name is misleading, it's the opacity not visible | |
| 87 | why do you reset it both in the loop and outside? | |
| 99 | can resetValue, and the 3 lines be always called in the initLevel function to avoid code duplication? | |
| 102 | it won't be translated, you can put "--" always on this case, without qsTr() | |
@nitish i checked the activity . you should make some changes.
- center allign the cursor in the text area
- put some space between the buttons. Highlight or zoom in effect on the buttons
- there is an error when you change state and sometime you can choose both the states simultaneously.
error -: qrc:/gcompris/src/activities/multiplication_tables/Multiplication_tables.qml:257: TypeError: Type error
4.And i think this should not be present - : "Both point size and pixel size set. Using pixel size." use relative ones like
smallsize etc.
- Improve layout also link -: https://drive.google.com/open?id=0B25oBy2F8e4jUThMV3M1SFdONWc
You removed ltoscano from reviewers (also, by principle, you should not, if he has subscribed, there is a reason ;)) but didn't applied what he said: your commits should have a real description/name, basically: "activity name, description of the changes" that would be in your case: "multiplication_tables, code cleanup" (if there are no "major" changes)
| src/activities/multiplication_tables/Multiplication_tables.qml | ||
|---|---|---|
| 43 | remove the added spaces at the end | |
| 60 | why mode2? | |
| 124 | use camelCase to name variables | |
| 226 | you can take a look at categorisation activity to see how to have an exclusive group | |
| src/activities/multiplication_tables/resource/README | ||
| 2 ↗ | (On Diff #10677) | do you have the link for this image? |
@rahulyadav & @jjazeix thanks for your suggestions, I will try to remove these bugs.
Also, in order to test this activity in android , what will be the steps to convert this into an apk file ??
@ltoscano thank you for pointing out my mistake. I will keep this thing in mind for future :)
| src/activities/multiplication_tables/Multiplication_tables.qml | ||
|---|---|---|
| 43 | sure, i will do that. | |
| 60 | I will change the variable name. This variable is for checking which the mode (either normal or school mode). | |
| 124 | sure :) | |
| 226 | I will have a look at it :) | |
| src/activities/multiplication_tables/resource/README | ||
| 2 ↗ | (On Diff #10677) | actually I have taken this image from memory activity. There, in the README for this image only credit was given to the author without any link. So, i also included the name. |
Hi,
Sorry for these messages that repeat themself, I did not use the right place to fill up my comments in phabricator.
It should now be ok.
I think we should rename the settings modes to something like :
- Present all the data to learn
- Choose data you want to learn and train.
The school mode is not started yet if I get it right. This is for me the main intersting part of the activity.
Could you add your actibvity to the workboard? I did not find it. Thanks :)
Emmanuel
Have you tried the layout with real questions as dataset? The aim of the activity is to be extensible to various subjects (not necessarely mathematical ones) so the layout must handle that.
The activity should also handle virtual keyboard.
| src/activities/multiplication_tables/Multiplication_tables.qml | ||
|---|---|---|
| 80 | why 800 everywhere? | |
| 154 | remove space at the end (not only there, please check all the diff for the other ones) | |
| 203 | don't put - after : | |
| src/activities/multiplication_tables/Question.qml | ||
| 37 | spaces | |
Hi Nitish,
After discussion with Johnny, we think that we do not requiere an additional diplay to set the "custom" questions marks.
We can put it directly within the setting display like it is done in guesscount settings.
I let you have a look to it and do not hesitate if you have questions.
Emmanuel