added datasets in smallnumbers2
ClosedPublic

Authored by shubhammishra on Jan 9 2020, 8:06 AM.

Details

Summary

Added datasets, follow same pattern as for smallnumbers in https://phabricator.kde.org/T12390

Test Plan

apply diff from adc807115b1a83c13a2968e9f42d201253cca6e1 (origin/multiple_dataset)

Diff Detail

Repository
R2 GCompris
Branch
smallnumbers2_multidataset
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20841
Build 20859: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
shubhammishra edited subscribers, added: jjazeix, echarruau; removed: kde-edu.Jan 9 2020, 8:11 AM
shubhammishra edited the test plan for this revision. (Show Details)
  • typo fixed
Restricted Application added a project: KDE Edu. ยท View Herald TranscriptJan 9 2020, 8:19 AM
  • typo fixed on datset 3
  • added sublevel1 in dataset1, modified number of stars
  • modified gletters.js to repreat each domino thrice
  • modified gletters.js to repeat each domino thrice

Please use a hard copy of the data you want to modify to solve the problems you have. Be careful to not break gletters.

src/activities/gletters/gletters.js
35

as seen in our discussion you do not need this value

131

Here you are modifying the source itself, this means that next time you come back to a level you already did you will have the concatenation in the data presented and not what is in the json file.
To solve this problem you need to work on a hard copy of the variable.
I find this approach interesting as we will be able to present randomized data to the user, as long as the datas to be taught in the level is appearing much more often than already known elements. Let say 50% of the time. We could say that the first data presented is the new one to be taught or find an other way. To be discussed with the others.

156โ€“157

why is this diff here?

  • further changes in gletter.js with new hard copy
shubhammishra marked 3 inline comments as done.Jan 10 2020, 11:57 AM
shubhammishra added inline comments.
src/activities/gletters/gletters.js
129

it creates new variable, which is a hard copy not a reference,as we discussed.

131

this line will concatenate levelData to itself thrice, so we will get each copy of dominoes three times.

132

this will restrict the number of dominoes in each level to 15.

151

in line 137,156,159,167,170,171,178 just replaced the level.words with new variable levelData as we have to do every operation using this concatenated variable.

156โ€“157

The previous code is treating all letters like a alphabets, and sometimes enabling shift even for digits. i have just added isDigit variable in if condition to ensure not to enable shift because of digit. counter case here is if levelData=[3,3]. the previous code will enable shift for this array.

192

in previous code,function initRandomWord(currentLevel + 1) shuffles the items.word, here we have to take care to do every operation with levelData. i checked the defination of this function and generated the random list by shyuffling levelData.

Sorry forgot to submit my comments yesterday. Here are then my comments.

src/activities/gletters/gletters.js
131

we said yesterday that it does not work. If you have 1 data at the beginning you don't obtain 15 data. We said you need to create an array containing 15 data randomly taken from the available data.

132

See previous comment.

151

Good, did you make sure it does not breack gletters? It should not but please check.

192

Not sure we need this since you will create our own random list, but here we may have a problem keeping gletters working.

shubhammishra marked an inline comment as done and an inline comment as not done.Jan 12 2020, 12:03 PM

can i go with this way?

src/activities/gletters/gletters.js
131

do i have to make data set of 15 for each level regardless of initial data size as current approach is only increasing it size three times? can i use a loop here which randomly pick a element from level.words 15 times and push it into levelData variable.

151

i test it by playing all falling * activities and even sure with code changes which i have done, is there any more formal way to check?

192

levelData is already shuffled, and we can't use initRandomWord function for smallumbers2 as it will shuffle level.words, so i can make changes like this? -

 if(items.ourActivity.activityName !== "smallnumbers2"){
       items.wordlist.initRandomWord(currentLevel + 1)
else
       items.wordlist.randomWordList = levelData

See new comments about data choice algorithme.

src/activities/gletters/gletters.js
131

we need to have a dataset made from data given in levelswords. The data should be randomly chosen but still have abotu equally distributed.
Let say we have 1 3 5 in our data, it would be good to have 1 3 5 5 3 1 3 5 1.
We should be able to have a non hard coded value given though the qml to replace 15. We can call it count.
And we need to have a special dataname to descripbe the list of the data that are really targeted for a given level
Let say we introduce 4 and 5 we need to have 50% of the time 4 and 5 and 50% of the the rest of the data.

192

I need to investigate this one furthut is is not clear to me :(

Please build the mecanism which creates the data to be given to the user, it will solve most of our problems. Good luck :)

src/activities/gletters/gletters.js
192

At the present moment this discussion is not necessary anymore as our current problem will be soved once you will have created your input data array containing the data to give to the user.

  • added data building mechanism
shubhammishra added a comment.EditedJan 13 2020, 7:10 PM

sorry for delay, everything is working fine now, i have added sublevels size inside json and added setLevelData() and addNewElement() functions to create data. i have also tried to describe everything in comments and tested other activities too, they are not affected by this change.

shubhammishra marked 15 inline comments as done.Jan 13 2020, 7:13 PM
jjazeix added inline comments.Jan 13 2020, 7:47 PM
src/activities/gletters/gletters.js
199

algorithm

200

separately

203

remove useless spaces before/after parenthesis

205

for(var i = 0 ...)

206

probability

215

an element

216

indentation

src/activities/smallnumbers2/resource/1/Data.qml
34

I think it's not needed to have "level" (unused)

commented on maxsublevel which is not self explanable variable name.

src/activities/gletters/gletters.js
132

Here can you have a detailed comment to explain what maxSubLevel is used for. Something to say that it gives the number of falling elements presented for each level. ?

shubhammishra marked 2 inline comments as done.Jan 13 2020, 9:52 PM
shubhammishra added inline comments.
src/activities/gletters/gletters.js
132

Yes, maxSubLevel stores the number of dominoes to fall down in each level, in previous code it is equals to level.words.length but this time i added attribute sublevels in json to pass it a value.
Reason for choosing this name is that activity previously also using this variable for storing number of falling elements as on line no. 128, 139,141,452.
and i choose the name sublevels for json after looking the defination of function
items.wordlist.getMaxSubLevel(currentLevel + 1) .

see my comment about comment :)

src/activities/gletters/gletters.js
132

I mean a comment in the code, not for me :)
I understood this part, but it tool me time, a detailed comment will help maintenainers and other coder.

shubhammishra marked an inline comment as done.
  • fixed typos, identation and removed "levels"
  • added detialed comment for maxSubLevel
jjazeix added inline comments.Jan 14 2020, 5:51 PM
src/activities/gletters/gletters.js
205

add { }

206

remove space before )

211

add { }

212

remove space before )

225

remove space before )

  • fixed spacing
  • dataset for smallnumbers updated
  • updated Smallnumbers.qml
jjazeix added inline comments.Jan 14 2020, 7:49 PM
src/activities/gletters/gletters.js
132

it would be better to have an attribute named useDataset in the Activity.qml that is set to true for smallnumbers and smallnumbers2.
This way, if we update another activity, we will only have to update this variable instead of changing all "if" conditions in the js.

  • added useDataset attribute

Hi,
I tested it looked at the code and this is for me ready to be merged. Well done!
Emmanuel

This revision is now accepted and ready to land.Jan 19 2020, 5:05 PM
jjazeix closed this revision.Jan 19 2020, 5:05 PM