multiple datasets in share pieces of candy
Needs ReviewPublic

Authored by shubhammishra on Jun 13 2020, 11:42 AM.

Details

Summary

share, added activity configuration

share, added dataset 1

share, added multiple datasets

Diff Detail

Repository
R2 GCompris
Branch
gsoc_2020_shubham_share_pieces_of_candy
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27153
Build 27171: arc lint + arc unit
shubhammishra created this revision.Jun 13 2020, 11:42 AM
Restricted Application added a project: KDE Edu. · View Herald TranscriptJun 13 2020, 11:42 AM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
shubhammishra requested review of this revision.Jun 13 2020, 11:42 AM
  • share, Increased left widget width bit because of double digit numbers
  • share, made no. of candies in instruction uniform for randomised and non randomised levels
shubhammishra retitled this revision from share, added activity configuration to multiple datasets in share pieces of candy.Jun 13 2020, 12:17 PM
shubhammishra edited the summary of this revision. (Show Details)
shubhammishra edited projects, added GCompris: Improvements; removed KDE Edu.
shubhammishra removed a subscriber: kde-edu.
Restricted Application added a project: KDE Edu. · View Herald TranscriptJun 13 2020, 12:17 PM
shubhammishra added inline comments.Jun 13 2020, 12:28 PM
src/activities/share/Share.qml
245

Increased left widget size, to fit double digit numbers of candy

src/activities/share/share.js
119

Previously for non randomised levels, Instructions show how many candies child needs to distribute while for random levels instructions shows total no. of candies without considering placed in candies.

More globally, do we need to specify how many children are girls or boys?
"Paul wants to equally share 2 pieces of candy between 2 of his friends: one girl and one boy." -> is there any addition of knowing there is a boy and a girl?

src/activities/share/ActivityConfig.qml
53

indentation

src/activities/share/resource/1/Data.qml
36

why is it a string and not a boolean?

63

missing dot at the end of the sentence

87

Shouldn't there be no rest on the first dataset?

98

should be no rest?

155

missing .

177

missing .

291

rest?

302

rest?

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

to be discussed, but if the objective says there is a rest, I don't see any rest in some levels.
Maybe rephrase the objective to say there is maybe a rest?

35–36

bool

74

11-2 = 9 :)

212

four boys and one girl.
pieces of candy

223

candy

269

two girls and two boys

"Paul wants to equally share 2 pieces of candy between 2 of his friends: one girl and one boy." -> is there any addition of knowing there is a boy and a girl?

If you only take the aim to teach "sharing" there is no addition interest of knowing that there is a boy and a girl.
But solving problem is also a matter of taking multiple informations, deciding if they are important or not, keeping only the relevant ones.
Here it is interesting to add a bit of difficulties which is placing a girl and a boy and not only two boys.
We have the beginning of what we call a multiple phases problem.

After discussion I think that this should be proposed after a first level that proposes only boys or girls. Sharing is the main goal we are aiming at here.

@shubham: could you please make some modifications to be sure that the first questions of each level have only girls and no boys?

I do not know why i was subscribed.

shubham removed a subscriber: shubham.Jun 14 2020, 4:36 AM

Hi, Sorry I think our same names lead to this confusion.

  • share, fixed instructions, remove boys from first sublevel of each level
shubhammishra marked 13 inline comments as done.Jun 14 2020, 9:28 AM
AkshayCHD added inline comments.Jun 14 2020, 7:09 PM
src/activities/share/resource/1/Data.qml
25

Can't we make it simply "Split candies equally between kids with no extra candies left."

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

I think a better objective would be "Split candies equally between kids with extra candies may or may not be left."

src/activities/share/resource/3/Data.qml
25

Again this can be reduced to "Split candies equally between kids."

AkshayCHD added inline comments.Jun 14 2020, 7:44 PM
src/activities/share/Share.qml
65

What is the purpose of this check, as in the share.js file we are not making any checks based on the case when the value of levels is null.

shubhammishra added inline comments.Jun 18 2020, 10:38 AM
src/activities/share/Share.qml
65

nothing specific, just to make this line the same across all activities but yes, we don't need this check here. So removing it.

  • updated dataset objectives
timotheegiet requested changes to this revision.Jun 18 2020, 1:22 PM

For the datasets descriptions, I think it would be more efficient to make it shorter and to the point, something like:
"Maximum n candies and n kids, no rest.", "Maximum n candies and n kids, possible rest.".... (replacing n with the actual values of course)

This revision now requires changes to proceed.Jun 18 2020, 1:22 PM
  • share, made dataset description shorter
jjazeix accepted this revision.Jun 20 2020, 10:11 AM

Good for me. Please double check all the missing dots at the end of the sentences

src/activities/share/resource/2/Data.qml
62

missing .

143

missing .

165

missing .

src/activities/share/share.js
94

Played it extensively. Could not break and I find the new dataset messages much clearer.
Good for me.

  • share, added missing full stop in instructions
jjazeix added a comment.EditedJun 20 2020, 6:57 PM

We have 2 conflicting options for the "show counter"... If we set it on the configuration, I would expect to always have it but on some level, we have an attribute: "showCount" which overrides the configuration option. @timotheegiet, @echarruau I think it was already previously the case but it may be better to find a better wording of the option if we want to keep this behaviour?

jjazeix accepted this revision.EditedJun 20 2020, 7:04 PM

waiting for @timotheegiet to validate the diff so we can close it. Once done, @shubhammishra please close the diff and the related tasks.

Commited in https://invent.kde.org/education/gcompris/-/commit/1f616fbdf03f594ded8ab4631805ced9f42ed75c
Thank you!

We have 2 conflicting options for the "show counter"... If we set it on the configuration, I would expect to always have it but on some level, we have an attribute: "showCount" which overrides the configuration option. @timotheegiet, @echarruau I think it was already previously the case but it may be better to find a better wording of the option if we want to keep this behaviour?

Indeed, looks like I overlooked this.
And, the "showCount" in datasets doesn't exactly override the config option, as if the config says to not show it, it will not show it even if dataset says to show it (so, if any of the config or the dataset says to not show, it won't show it).

After thinking more about it, for default datasets, I think we should not use the "showCount" in datasets and only rely on the config option to show/hide it.
It would be good to still have this variable available for teachers if they need to specify it per custom dataset; but then this dataset-variable should really override the config option in any case.

Either this, or just remove the config option and only use the dataset variable to handle it.

I confirm my first idea from previous comment: better keep the "show counter" as an Option, and not part of the datasets.

(similar to the comment I just made for categorization on https://phabricator.kde.org/T13112 )

Hi,

The "showCount" in datasets and "show counter" option in config behaves differently. When we disable counter from config, "totalCandies" becomes invalid and it is possible to drag unlimited candies that also means candies never become unavailable. I thought it was intended not a bug so didn't raise this before.

Hi,

The "showCount" in datasets and "show counter" option in config behaves differently. When we disable counter from config, "totalCandies" becomes invalid and it is possible to drag unlimited candies that also means candies never become unavailable. I thought it was intended not a bug so didn't raise this before.

Good catch, indeed this looks like a bug that should be fixed then.

I checked the code and now I can say it is intended because of line 305 in Share.qml

total: background.easyMode ? items.totalCandies : 8 * items.totalChildren + 1

Maximum limit of candies in any box is 8. Now, the question is, do we really want behavior like this or not?

For consistency, as we already always limit the number of boys/girls to place according to the numbers in the instructions, it would be logical to do the same for the number of candies.
So, unless others disagree, I think we don't really want this behavior, and it can be changed.