Multiplle datasets for money activity
AbandonedPublic

Authored by AkshayCHD on May 28 2019, 12:57 PM.

Details

Summary

The following changes have been made

  • Changed the ActivityInfo.qml file of money activity and its subactivities to contain multiple datasets.
  • Removed the hardcoded data from the js file and moved to it Data.qml files in the respective resource folders.
  • Created multiple dataset structure inside resource directory (in the form of folders numbered 1, 2, 3 and so on)

Diff Detail

Repository
R2 GCompris
Lint
Lint Skipped
Unit
Unit Tests Skipped
AkshayCHD created this revision.May 28 2019, 12:57 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptMay 28 2019, 12:57 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
AkshayCHD requested review of this revision.May 28 2019, 12:57 PM
AkshayCHD updated this revision to Diff 58785.May 28 2019, 1:05 PM

This is good start :).
I did some small comments. Please check with your mentors how you prefer to work: either work by diffs, or you create your own branch (do commits when you want) and once it's good create a diff of the set of commits you want to check.

src/activities/money/moneyConstants.js
48

This is more of an improvement but can you put here the full path to the images?
This way, if someone wants to make a dataset with external files (without embed images in rcc), it will still be possible?

src/activities/money/resource/1/Data.qml
28

Is it possible to have a better objective text? The aim is for the child to know what they will learn or what will the dataset contains. For example, "Learn how to pay up to 10 euros (using money between 1 euro and 5 euros)." (last part may be unnecessary).

jjazeix added inline comments.May 28 2019, 2:36 PM
src/activities/money/resource/1/Data.qml
28

Note that the money is localized so using "euros" is not good as you can be using other currency

AkshayCHD updated this revision to Diff 58810.May 29 2019, 6:11 AM

Added full paths to images
Changed currency variable names to remove euros
changed objective text(not accurate just for example purposes as of now)

AkshayCHD marked 3 inline comments as done.May 29 2019, 6:12 AM

@jjazeix Thanks for the review I have made the required changes

AkshayCHD updated this revision to Diff 58819.May 29 2019, 8:07 AM

Few corrections with respect to with cents subactivity

AkshayCHD updated this revision to Diff 58862.May 29 2019, 7:34 PM

Add 3rd dataset for money activity to enable children to count till 1000 units

AkshayCHD updated this revision to Diff 58863.May 29 2019, 8:09 PM
AkshayCHD updated this revision to Diff 58864.
AkshayCHD updated this revision to Diff 58926.May 30 2019, 6:22 PM

The following changes have been made
Added 2 new currency worth units 100 and 200(Temporary graphics used later to be replaced with graphics of same format as previous )
Added dataset number 2 and 3 for money_back activity
Made changes in previous datasets to use the new currencies.

AkshayCHD updated this revision to Diff 58964.May 31 2019, 5:48 PM

Added multiple datasets to money_cents and money_back_cents activity.

I let the other mentors do a complete review, I only point out one improvement that would be great to have

src/activities/money/money.js
141

can this part be made generic (computed from the moneyItems of the current dataset)? For example, if we take the previous Italian money (https://en.wikipedia.org/wiki/Italian_lira), 1€ is around 2000 Lira so the prices had higher values.
If we do a switch case between 5 and 400, we won't be able to add a dataset for these prices.

AkshayCHD updated this revision to Diff 59121.Jun 4 2019, 11:08 AM

Made the selection process of total money in money_back and money_back_cents activity generic

AkshayCHD marked an inline comment as done.Jun 4 2019, 11:10 AM
AkshayCHD added inline comments.
src/activities/money/money.js
141

@jjazeix I have done an implementation for it. You may have a look whenever you have time :)

jjazeix added inline comments.Jun 4 2019, 11:49 AM
src/activities/money/money.js
73

small typo: available

74

I would use the dataset moneyItems, not directly the Constants one. Else the specific items won't be used.
I guess you can use slice to copy the array (https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Objets_globaux/Array/slice)

86

I think the part: (avaliableCurrency[i].val > amountToBeCovered) is not needed because it should always be the case before breaking as we start from the higher value in a sorted array.

AkshayCHD marked an inline comment as done.Jun 4 2019, 3:05 PM
AkshayCHD added inline comments.
src/activities/money/money.js
74

@jjazeix But there is a problem with using the dataset's moneyItems. If suppose we need to find change for 100 units paid, then the pocket values might not contain a 100 units note, it might instead contain one 50 units, two 20 units and one 10 units notes. So instead of just one 100 units note, tux will now hold 4 notes will would require alot of space and would hinder the visibility of individual notes.

86

The dataset is sorted in ascending order that is why i added that condition, but yes it won't be needed if i sort it in descending order. Will do that

jjazeix added inline comments.Jun 4 2019, 3:16 PM
src/activities/money/money.js
74

It will be up to the person doing the dataset to take care of not having too much notes for the layout.
But for now, if someone has a money worth 50000 unit, it won't be used in the algorithm to compute.

86

Then, it's the other condition that's not needed (one of the two is not).

AkshayCHD updated this revision to Diff 59142.Jun 4 2019, 5:55 PM

-use only pocket currencies for paid amount instead of all available currencies
-fix typos

AkshayCHD marked 3 inline comments as done.Jun 4 2019, 5:57 PM

@jjazeix I've made the changes

scagarwal requested changes to this revision.Jun 7 2019, 6:27 AM
scagarwal added a subscriber: scagarwal.
scagarwal added inline comments.
src/activities/money/money.js
26

Do rename Constants as moneyConstants. Clear names are much better.

src/activities/money/resource/2/Data.qml
25

Same Constants . Do rename to moneyConstants

src/activities/money/resource/3/Data.qml
24 ↗(On Diff #59142)

moneyConstants reference

src/activities/money_back/resource/1/Data.qml
25

moneyConstants reference

src/activities/money_back/resource/2/Data.qml
24 ↗(On Diff #59142)

moneyConstants reference

src/activities/money_back/resource/3/Data.qml
24 ↗(On Diff #59142)

moneyConstants reference

src/activities/money_back_cents/resource/1/Data.qml
25

moneyConstants reference

src/activities/money_back_cents/resource/2/Data.qml
24 ↗(On Diff #59142)

moneyConstants reference

src/activities/money_back_cents/resource/3/Data.qml
24 ↗(On Diff #59142)

moneyConstants reference

src/activities/money_cents/resource/1/Data.qml
25

moneyConstants reference

src/activities/money_cents/resource/2/Data.qml
24 ↗(On Diff #59142)

moneyConstants reference

src/activities/money_cents/resource/3/Data.qml
24 ↗(On Diff #59142)

moneyConstants reference

This revision now requires changes to proceed.Jun 7 2019, 6:27 AM
AkshayCHD updated this revision to Diff 59365.Jun 7 2019, 7:17 PM

Fixed layout issues
Equalised number of levels in datasets.
Changed variable name "Constant" to "MoneyConstants"

amankumargupta added inline comments.
src/activities/money/money.js
47

Indentation for break. Same for the following breaks..

88

Please follow the coding style everywhere and write a complete version of "for". Having "for" written in different ways everywhere is not uniform.

185

better rename it as "itemList" or "objectList"?

amankumargupta requested changes to this revision.Jun 8 2019, 6:32 AM
This revision now requires changes to proceed.Jun 8 2019, 6:32 AM
AkshayCHD updated this revision to Diff 59385.Jun 8 2019, 8:00 AM

Fixed errors in DialogChooseLevel.qml

AkshayCHD updated this revision to Diff 59393.Jun 8 2019, 11:02 AM

Remove coding style issues and make datasets more precise

AkshayCHD marked 3 inline comments as done.Jun 8 2019, 11:03 AM

@echarruau can you test this when you have some time to see if it is good on the pedagogic level? Instead of applying the diff, checkout the branch gsoc_akshaychd_multiple_datasets where the work is done

scagarwal accepted this revision.Jul 8 2019, 7:40 AM

@AkshayCHD Do rename the title of this task. Multiple is spelled wrong.

AkshayCHD abandoned this revision.Jul 16 2019, 1:08 PM