Add multiple datasets to calendar activity
Needs ReviewPublic

Authored by AkshayCHD on Jul 4 2019, 1:04 PM.

Details

Summary

Status: Needs Review

Changes Done:

  • Added multiple datasets to Calendar and find the day activity.
  • Created a generic algorithm to generate template questions as per the parameters it is provided
Test Plan

Apply the diff on multiple_dataset branch or pull the changes from the branch gsoc_akshaychd_calendar_multiple_datasets

Diff Detail

Repository
R2 GCompris
Branch
gsoc_akshaychd_calendar_multiple_datasets
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14095
Build 14113: arc lint + arc unit
AkshayCHD created this revision.Jul 4 2019, 1:04 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptJul 4 2019, 1:04 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
AkshayCHD requested review of this revision.Jul 4 2019, 1:04 PM
AkshayCHD retitled this revision from calendar, add multiple dataset 1 to Add multiple datasets to calendar activity.Jul 4 2019, 1:06 PM
AkshayCHD edited the summary of this revision. (Show Details)
AkshayCHD edited the test plan for this revision. (Show Details)
AkshayCHD edited projects, added GCompris; removed KDE Edu.
AkshayCHD edited subscribers, added: GCompris: Improvements, GCompris; removed: kde-edu.
Restricted Application added a project: KDE Edu. · View Herald TranscriptJul 4 2019, 1:06 PM
AkshayCHD updated this revision to Diff 61171.Jul 4 2019, 7:25 PM
  • find_the_day, add multiple datasets
This revision was not accepted when it landed; it landed in state Needs Review.Jul 4 2019, 7:37 PM
This revision was automatically updated to reflect the committed changes.
AkshayCHD reopened this revision.Jul 4 2019, 7:39 PM
AkshayCHD updated this revision to Diff 61174.Jul 4 2019, 7:39 PM
  • Merge branch 'gsoc_akshaychd_calendar_multiple_datasets' of git.kde.org:gcompris into gsoc_akshaychd_calendar_multiple_datasets
AkshayCHD updated this revision to Diff 61231.Jul 5 2019, 5:30 PM
  • Calendar, add DialogChooseLevel to activity
  • Calendar, improve date formats
AkshayCHD updated this revision to Diff 61234.Jul 5 2019, 5:48 PM
  • Calendar, add dataset configuration documentation comments
AkshayCHD edited the summary of this revision. (Show Details)Jul 5 2019, 6:37 PM
amankumargupta added inline comments.
src/activities/calendar/calendar.js
113

Indentation

125

Shouldn't Math.floor((maxMonth - minMonth)) be Math.floor(maxMonth - minMonth + 1)?

Let's say minMonth is 3 and maxMonth is 5. I should get any random among 0, 1, 2 so that minMonth + Math.floor(Math.random() * Math.floor((maxMonth - minMonth))) can be anything between 3, 4, 5.

But, Math.floor(Math.random() * Math.floor((maxMonth - minMonth))) limits the values among 0, 1 (as minMonth - maxMonth = 2 in our case). So you should probably do maxMonth - minMonth + 1?

128

Same comment about choosing random value as above.

jjazeix added a subscriber: jjazeix.Jul 7 2019, 3:41 PM
jjazeix added inline comments.
src/activities/calendar/calendar.js
112

qsTr() missing

161

qsTr() + bad format
Can you please check if the format is the same if we change locale?

scagarwal requested changes to this revision.Jul 8 2019, 7:44 AM
scagarwal added a subscriber: scagarwal.
scagarwal added inline comments.
src/activities/calendar/calendar.js
112

Do maintain spaces between elements in an array. The formatting is wrong.

This revision now requires changes to proceed.Jul 8 2019, 7:44 AM
AkshayCHD updated this revision to Diff 61853.Jul 16 2019, 12:25 PM
  • enclose strings in qsTr
AkshayCHD marked 5 inline comments as done.Jul 16 2019, 12:32 PM
AkshayCHD added inline comments.
src/activities/calendar/calendar.js
161

@jjazeix When I change the locale only the names of months and years change, not the question text. As we used hardcoded strings for questions.

AkshayCHD updated this revision to Diff 61911.Jul 17 2019, 1:45 PM
  • add tutorials
AkshayCHD updated this revision to Diff 61913.Jul 17 2019, 1:53 PM
  • change file copyrights
jjazeix added inline comments.Jul 17 2019, 4:57 PM
src/activities/calendar/calendar.js
114

this should be depending on the country. We may want to display month day year on some locales, it needs to be configurable for the translator (and have a comment)

160

no space before ?

src/activities/calendar/resource/Tutorial5.qml
27

qstr

AkshayCHD updated this revision to Diff 61995.Jul 18 2019, 6:32 PM
  • enclose strings in qsTr
AkshayCHD marked 3 inline comments as done.Jul 18 2019, 6:33 PM
amankumargupta added inline comments.Jul 22 2019, 6:29 AM
src/activities/calendar/calendar.js
136

Why a new variable "currentOffset"? Why not directly use "offset"? It's not used anywhere in this function.

168

Please add translators comment above explaining what the arguments will be. It'll be easier for them to understand while translating.

amankumargupta requested changes to this revision.Jul 22 2019, 6:29 AM
This revision now requires changes to proceed.Jul 22 2019, 6:29 AM
AkshayCHD updated this revision to Diff 62315.Jul 22 2019, 4:01 PM
  • add comments, remove unnecessary variables
AkshayCHD marked 2 inline comments as done.Jul 22 2019, 4:50 PM
AkshayCHD updated this revision to Diff 63050.Aug 4 2019, 10:53 AM
  • calendar, add full date format in find the day questions
timotheegiet requested changes to this revision.Aug 20 2019, 12:17 PM
timotheegiet added a subscriber: timotheegiet.

I tested the patch on multiple_dataset branch, and noticed several issues.

In Find the date:

-some questions, the answer can not be entered (it is out of the accessible range)
Reproduced with first dataset at level 3 and 5 (see screenshots 1, 2 and 3)

-a question was twice the same, happened with second dataset at first level.

-some question did not accept the correct answer (see on screenshot 4 and 5, the answer to the question is correct and still the answer is not accepted).

In Calendar activity:

  • In first dataset, at level one, the instructions shouldn't have a ? mark.

-At level 3, the questions mention the month and year which is not visible. Better do like in level 4 and say "of given month" instead.

Another note: the datasets have difficulty stars 1 and 3, while the activities initial difficulty level were 1 and 3 red stars (meaning level 4 and 6 respectively)
It would be better to adapt the stars of the datasets accordingly.

This revision now requires changes to proceed.Aug 20 2019, 12:17 PM
AkshayCHD updated this revision to Diff 64151.Aug 20 2019, 6:43 PM
  • calendar, resolve minor bugs

@timotheegiet Thanks for the review, I've updated the diff trying to resolve all the issues, can you try it out once again :)

timotheegiet accepted this revision.Aug 21 2019, 12:14 PM

Thanks, I tested again and can not replicate those issues anymore.
Good for me, waiting for @scagarwal and @amankumargupta to confirm it's ok for them too.