Tutorial screen added for odd even activity
ClosedPublic

Authored by dekumar on Jan 3 2019, 5:50 PM.

Diff Detail

Repository
R2 GCompris
Branch
tutorial
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7897
Build 7915: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
dekumar updated this revision to Diff 50909.Feb 5 2019, 7:55 AM
  • Requested changes made

Can you also please do the other changes (files that shouldn't be updated, typos...)?

Thank you

src/activities/numbers-odd-even/resource/TutorialBase.qml
65

this is not a maintainable way to check this. Let's assume we want to add a new question, we would think that we only need to add a new Tutorial like Tutorial5 but then we will probably forget to update here.
It would be better to have a boolean telling if we want to find the odd or even number at TutorialBase level, and use it to both set the text and tell if it is good or not on click.

Both button should share the same code (there is AnswerButton that could maybe be used? It also have user feedback).

dekumar updated this revision to Diff 51076.Feb 7 2019, 7:45 AM
  • Typos fixed
jjazeix added inline comments.Feb 7 2019, 8:21 AM
src/activities/numbers-odd-even/resource/Tutorial4.qml
29

there is no need for two booleans, it's either odd or even, it can't be both at same time

src/activities/numbers-odd-even/resource/Tutorial5.qml
28

2 false wouldn't happen with only one bool

dekumar updated this revision to Diff 51082.Feb 7 2019, 11:30 AM
  • Boolean set to one instead of two
jjazeix added inline comments.Feb 10 2019, 2:14 PM
src/activities/numbers-odd-even/resource/Tutorial4.qml
30

the question should be handled directly in TutorialBase.
isEven ? qsTr() : qsTr()

src/activities/numbers-odd-even/resource/TutorialBase.qml
66

are you sure you need to display the same text in if and in else?

86–87

missing space after ","

107

it should not be a space. Either don't have a default value or set "" (which is the same but better not put anything if not needed)

dekumar updated this revision to Diff 51341.Feb 10 2019, 6:46 PM
  • Requested changes made

Can you try using "AnswerButton.qml" to select the choices? It is used in leftright activity if you need some example.

I have "qrc:/gcompris/src/core/Tutorial.qml:136: TypeError: Cannot read property 'length' of undefined" when starting planegame or alphabet-sequence activity.

Please check them everytime before commiting, the 3 activities share the same code and there shouldn't be side effects.

src/activities/numbers-odd-even/resource/Tutorial4.qml
30

This is not good. It can't be assumed that the first rectangle is always the good answer to have and twist the definition of even for this.

src/activities/numbers-odd-even/resource/TutorialBase.qml
58

indentation

61

having hardcoded width/height make overriding (if you reduce the width at maximum, the rectangles will b eoutside the background gray one).

72

indentation

85

indentation

159

too much break lines

src/activities/planegame/planegame.js
55

indentation

112

indentation

119

indentation

I have many small changes to request, could you document the changes refering to their number each time one of the item is done? Otherwise it will be difficult to me to see what has been done.
I will describe the changes refering to tutorial pages.
1- tutorial page n2
Can you insert the following page before the page 2 in order to explain what a remainder is
https://snag.gy/tDoGi8.jpg
2- tutorial page n2
Even numbers are numbers which leave remainder 0 when divisible by 2. -> Even numbers are numbers which leave remainder 0 when divided by 2.
For example: 12, 38, 52, 68, 102, 118, 168, 188, 502, 532, 700, 798, 842, 892 ,1000. All of these numbers are even numbers as they leave remainder 0 when divisible by 2. -> For example: 12, 38, 52, 68, 102, 118, 168, 188, 502, 532, 700, 798, 842, 892 ,1000. All of these numbers are even numbers as they leave remainder 0 when divided by 2.
3- tutorial page n3
Even numbers are numbers which leave remainder 0 when divisible by 2 -> Even numbers are numbers which leave remainder 0 when divisided by 2
4- tutorial page n3
For example: 15, 19, 51, 65, 103, 119, 169, 185, 505, 533, 701, 799, 845, 897, 1001. All of these numbers are odd numbers as they do not leave remainder 0 when divisible by 2. -> For example: 15, 19, 51, 65, 103, 119, 169, 185, 505, 533, 701, 799, 845, 897, 1001. All of these numbers are odd numbers as they do not leave remainder 0 when divided by 2.
5- tutorial page 4
Under choose an even number
Please center the number in the box
6- tutorial page 4
Replace "No, this is a wrong anwer by: Your made an error: When divided by 2, 13 is equal to 6 and the remainder is 1. Therefore this is an odd number.
All this must fit int he box.
7- tutorial page 4
Please center the great in the box.
8- tutorial page 5
Same comments that for points 6 and 7.
9- NumbersOddEven.qml content should be moved to directory ressources
10- NumbersOddEven.qml tutorialInstructions should be dispatched in tutorialsx.qml files.

@echarruau Yaa sure, I would update the diff by making the requested changes and document them too :)

6- tutorial page 4
Replace "No, this is a wrong anwer by: Your made an error: When divided by 2, 13 is equal to 6 and the remainder is 1. Therefore this is an odd number.
All this must fit in the box.

What do you think about using AnswerButton (the same as readingh/readingv/leftright/missing-letter activities) and avoid adding the text: "Great" or "Error"?

7- tutorial page 4
Please center the great in the box.
8- tutorial page 5
Same comments that for points 6 and 7.
9- NumbersOddEven.qml content should be moved to directory ressources
10- NumbersOddEven.qml tutorialInstructions should be dispatched in tutorialsx.qml files.

I would let them in the NumbersOddEven file as they are directly inherited from the parent Item, else it would mean add more variables to get the path of the instructions for not a big gain.

echarruau added a comment.EditedFeb 17 2019, 8:20 AM

What do you think about using AnswerButton (the same as readingh/readingv/leftright/missing-letter activities) and avoid adding the text: "Great" or "Error"?

7- tutorial page 4

This would be a nice way. Less stressfull than getting a "NO" :)
I would still like to have a message explaining why the answer is wrong, this would be a nice way to help the notion to be acquired.

What do you think?

This would close points 6 7 and 8.

dekumar updated this revision to Diff 51957.Feb 18 2019, 1:21 PM
  • Typos fixed
  • Requested changes made
dekumar updated this revision to Diff 51958.Feb 18 2019, 1:23 PM
  • typos fixed
dekumar added a comment.EditedFeb 18 2019, 1:40 PM

I have made all the changes as requested by @echarruau @jjazeix
Requested changes:

  1. Can you insert the following page before the page 2 in order to explain what a remainder is

    I have added the tutorial page for remainder https://pasteboard.co/I1JCGBr.png
  1. Requested changes made to the definition of odd and even numbers

For even number - https://pasteboard.co/I1JDfjO.png
For odd number - https://pasteboard.co/I1JDUVT.png

  1. The message text "Great" centered in the message box - https://pasteboard.co/I1JESzH.png

4.Error message changed - https://pasteboard.co/I1JFm7O.png

I have also tested it for vertical and horizontal screens
https://pasteboard.co/I1JFLtC.png
https://pasteboard.co/I1JG0Dy.png
https://pasteboard.co/I1JGduW.png
https://pasteboard.co/I1JGKBZ.png
https://pasteboard.co/I1JGVSt.png
https://pasteboard.co/I1JH6CR.png

Great job. I haven't looked at the code yet but the behaviour is what I was looking for.
Can you do some small adjustments?
We need to add an other tutorial page before to explain what is a remainder.
Adding a first page with the question of this page: https://pasteboard.co/I1JDfjO.png
But not the example, this will lead to the remainder explanation which is at the moment in page 1 but will be moved to page 2.

Can you slighly reduce the size of the font in the error message so that the message can fit?
https://pasteboard.co/I1JG0Dy.png

Can you put a space between the : and the when and replace the capital W by a w?

Thanks,

In https://pasteboard.co/I1JH6CR.png, there should be some margin so the text do not override with the border.
In https://pasteboard.co/I1JGVSt.png, can you center the "Great" text?
In https://pasteboard.co/I1JGKBZ.png, it should be a generic definition of "remainder", not division by 2.

Thanks

src/activities/numbers-odd-even/resource/Tutorial2.qml
2

do not forget to update these info when you rename a file

34

892, 1000

src/activities/numbers-odd-even/resource/Tutorial5.qml
31

there is no need for2 bools

32

it should be renamed firstChoice or firstNumber, having a variable called "evenNumber" and setting it to 59 is not a good practice

src/activities/numbers-odd-even/resource/TutorialBase.qml
70

the numbers shouldn't be harcoded here, else it won't work for the examples

135

missing qsTr(). Is there a better way to check that the text has a specific value to set the margin?
With translation, we should not assume anything about the text length

jjazeix added inline comments.Feb 20 2019, 7:43 AM
src/activities/numbers-odd-even/resource/TutorialBase.qml
56

needs to be renamed to be generic too

dekumar updated this revision to Diff 52359.Feb 23 2019, 8:46 AM
  • Requested changes made

I have made the following changes:

  1. Centered the great text https://pasteboard.co/I2sXluZ.png
  1. Fix the issue of error message which were overriding the border https://pasteboard.co/I2sZ6aJ.png

3.Added the general definition of remainder https://pasteboard.co/I2sZjqq.png

dekumar updated this revision to Diff 52530.Feb 25 2019, 1:05 PM
  • Typo fixed
dekumar updated this revision to Diff 52532.Feb 25 2019, 1:30 PM
  • Typo fixed
jjazeix added inline comments.Feb 27 2019, 12:20 PM
src/activities/numbers-odd-even/resource/TutorialBase.qml
33

I still don't understand why you need 2 bools?
isEven (should be renamed to isEvenExpected) is set to true if the question is "choose the even number".
then, when you click on a button, if the number inside is even you should tell if it is good or not.

70

missing space after the '.'?

93

why is there 3 cases here and only one on the other button? Shouldn't they be "symetrical"?

94

The checks order seems not logical.
There is a check on !isCorrectAnswer to display a text and two lines below, if it's not even, the text changes directly.
It would look more readable having:

if(isCorrect) {
}
else {
  if(even) {
  }
  else {
  }
}
dekumar updated this revision to Diff 53034.Mar 3 2019, 6:32 AM
  • Requested changes made

I have changed the condition to check to be symmetrical in the same way as suggested in the comments.

src/activities/numbers-odd-even/resource/TutorialBase.qml
33

I need two bools for changing the order of the AnswerButton to be correct.As we need to display three different messages and it should not be assumed that the first box is always the correct answer and also to set the (isCorrectAnswer: ) in answer button we need the second bool.I think the message good or bad can't be set using a single bool because we need to handle a case of odd number too.

@jjazeix Regarding the error "Cannot defined property of length defined" on running the Alphabet Sequence.The line 136 of src/core/Tutorial.qml is set to some condition to check the visibility of the tutorial.I think that removing that line would resolves the error and would have no effect on tutorial.qml

@jjazeix Regarding the error "Cannot defined property of length defined" on running the Alphabet Sequence.The line 136 of src/core/Tutorial.qml is set to some condition to check the visibility of the tutorial.I think that removing that line would resolves the error and would have no effect on tutorial.qml

Please read better to which component the visible property would affect and on which usecase, make a few test to get to this case on another activity that has a tutorial and see why it's needed :)

dekumar updated this revision to Diff 53220.Mar 5 2019, 5:07 PM
  • Extraspaces removed
jjazeix added inline comments.Mar 5 2019, 6:37 PM
src/activities/numbers-odd-even/resource/TutorialBase.qml
74

there will be 2 spaces after ':', you need to remove the one before when on the next line (same comment applies to other sentences below

aacid added a subscriber: aacid.Mar 5 2019, 8:03 PM
aacid added inline comments.
src/activities/numbers-odd-even/resource/Tutorial1.qml
34

Don't use + for concatenating strings that go on qsTr i'm 96% sure this breaks translations.

src/activities/numbers-odd-even/resource/Tutorial2.qml
35

same comment about qsTr and using +

src/activities/numbers-odd-even/resource/Tutorial3.qml
34

same

dekumar updated this revision to Diff 53308.EditedMar 6 2019, 6:47 PM
  • Removed + used for concatenation of string

@jjazeix I tried using loader to resolve the error " Cannot read property of length undefined" but I couldn't resolve it. I think the visibilty of the "next" in core should be set to some other parameters instead of tutorialDetails.

@dekumar, @echarruau: Please take a look at these changes: https://pastebin.com/1TMefRZ9 and check if it still works and what's expected.
On code side, the last change that could be required would be to use a Repeater for handling the AnswerButton more cleanly but it's not mandatory...

@jjazeix Yes, I have applied the changes. It produces the same behavior as before.

dekumar updated this revision to Diff 56088.Apr 12 2019, 6:42 PM
  • typo fixed
  • Minor chnages
  • new changes applied
  • typo fixed
  • extraspaces removed
  • All changes applied

@jjazeix @echarruau I have updated the diff by making all the changes. Please have a look and let me know if there's still any changes to be done.

Thanks!

I finally managed to run openssl and could review your diff.
This is what I was looking for.
Code is ok for me, I find it generic enough to be reused in other programs.
One thing thought. The identation of brackets { and } is broken.
For example in TutorialBase.qml.

GCText {

 id: message
 anchors {
     centerIn: parent
     margins: parent.border.width+1
 }
 text: ""
 fontSizeMode: Text.Fit
 ........
    }
}

Closing brackets should be at the level of the first character of the element it belongs too.
Could you have a look through your code and tidy the indentation?
Emmanuel

I finally managed to run openssl and could review your diff.
This is what I was looking for.
Code is ok for me, I find it generic enough to be reused in other programs.
One thing thought. The identation of brackets { and } is broken.
For example in TutorialBase.qml.

GCText {

 id: message
 anchors {
     centerIn: parent
     margins: parent.border.width+1
 }
 text: ""
 fontSizeMode: Text.Fit
 ........
    }
}

Closing brackets should be at the level of the first character of the element it belongs too.
Could you have a look through your code and tidy the indentation?
Emmanuel

Thanks for the review!
Sure, I would update the diff by fixing the indentation issues.
Thanks!

We shouldn't allow changing the level in the bar when the tutorial is going on.
On UI side, there are some improvements to be made: https://pasteboard.co/IaJRr7Y.png

dekumar updated this revision to Diff 56679.Apr 21 2019, 7:37 PM
  • Indentation fixed

@echarruau @amankumargupta I have made the requested changes.

Thanks!

This revision is now accepted and ready to land.Apr 27 2019, 4:17 PM
jjazeix closed this revision.Apr 27 2019, 4:18 PM