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
multipledataset_smallnumbers2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21146
Build 21164: 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

133

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.

165–166

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
131

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

133

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

134

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

160

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.

165–166

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.

201–204

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
133

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.

134

See previous comment.

160

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

201–204

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
133

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.

160

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?

201–204

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
133

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.

201–204

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
201–204

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
211

algorithm

212

separately

215

remove useless spaces before/after parenthesis

217

for(var i = 0 ...)

218

probability

227

an element

228

indentation

src/activities/smallnumbers2/resource/1/Data.qml
39–41

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
141

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
141

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
141

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
217

add { }

218

remove space before )

223

add { }

224

remove space before )

237

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
134

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