Differential D26549

clockgame multiple dataset
ClosedPublic
Actions

Authored by dekumar on Jan 9 2020, 4:51 PM.

Details

Reviewers
jjazeix
echarruau
timotheegiet
Group Reviewers
GCompris: Improvements
Maniphest Tasks
T12328: Improve multipledataset in clockgame
Summary

I have done the following changes:

  1. Added a OK button to check answer
  2. Added level selection for full hours

Diff Detail

Repository
R2 GCompris
Branch
clockgame_mdataset
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20999
Build 21017: arc lint + arc unit
dekumar created this revision.Jan 9 2020, 4:51 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptJan 9 2020, 4:51 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
dekumar requested review of this revision.Jan 9 2020, 4:51 PM
dekumar retitled this revision from level with full hours and ok button added to clockgame multiple dataset.Jan 9 2020, 4:54 PM
dekumar edited the summary of this revision. (Show Details)
dekumar added reviewers: jjazeix, echarruau.
dekumar added a task: T12328: Improve multipledataset in clockgame.
dekumar edited the summary of this revision. (Show Details)Jan 9 2020, 5:00 PM
dekumar updated this revision to Diff 73218.EditedJan 10 2020, 5:57 PM
dekumar edited the summary of this revision. (Show Details)

I have added all the different configurations as defined for the dataset of clock-game.

echarruau added a comment.Jan 10 2020, 10:07 PM

Can you modify the options messages to make them shorter. I commented two of them in the code.

src/activities/clockgame/resource/1/Data.qml
24

Please change to "Full hours".

src/activities/clockgame/resource/2/Data.qml
24–25

Please change to "Full and half hours".

dekumar updated this revision to Diff 73293.EditedJan 11 2020, 7:22 PM

I have done the following changes:

  1. Added the 24 hours format to some levels as discussed.
  2. Made the minutes hand visible for all of the levels.

@echarruau @jjazeix Please review.
Thanks!

dekumar updated this revision to Diff 73334.Jan 12 2020, 2:09 PM
  • goal modified
jjazeix added inline comments.Jan 12 2020, 2:11 PM
src/activities/clockgame/Clockgame.qml
458

it would be better to have the button on the right side else it may override with the clock

src/activities/clockgame/clockgame.js
43–52

it would be easier to do :

items.targetH = Math.floor(Math.random() * 12)
if(!items.levels[currentLevel].twentyfourHoursFormat) {
    items.targetH += 12
}
99

you don't need to duplicate all the function.
Something like:

if (((items.currentH === items.targetH) || (items.currentH === (items.targetH - 12)))
                && items.currentM === items.targetM
                && items.currentS === items.targetS) {
            items.bonus.good("gnu")
        }
        else {
            items.bonus.bad("gnu")
        }
dekumar updated this revision to Diff 73335.Jan 12 2020, 2:12 PM
  • typo fixed
jjazeix added a comment.EditedJan 12 2020, 2:22 PM

For the 24 hours, I would revert the change from this diff.
Multiple things need to be taken in account:

  • Should the hint go to 24h (from 0h to 24h)?
  • Should we display AM/PM for locale that use them instead of 13h->24h?

...
I think we should use https://doc.qt.io/qt-5/qml-qtqml-date.html to format the time correctly (but it is work for the other task)

dekumar added a comment.Jan 12 2020, 2:27 PM
In D26549#592560, @jjazeix wrote:

For the 24 hours, I would revert the change from this diff.
Multiple things need to be taken in account:

  • Should the hint go to 24h (from 0h to 24h)?
  • Should we display AM/PM for locale that use them instead of 13h->24h? ...

I think for 24 hrs format there is no AM/PM. As 20hr is equal to 8 pm using this format.
Let me know if you want me to remove 24hrs format from the diff for now.

jjazeix added a comment.Jan 12 2020, 2:33 PM
In D26549#592563, @dekumar wrote:
In D26549#592560, @jjazeix wrote:

For the 24 hours, I would revert the change from this diff.
Multiple things need to be taken in account:

  • Should the hint go to 24h (from 0h to 24h)?
  • Should we display AM/PM for locale that use them instead of 13h->24h? ...

I think for 24 hrs format there is no AM/PM. As 20hr is equal to 8 pm using this format.
Let me know if you want me to remove 24hrs format from the diff for now.

For me, it needs more discussion on how to handle it in a clean way

dekumar added a comment.Jan 12 2020, 4:00 PM
In D26549#592564, @jjazeix wrote:
In D26549#592563, @dekumar wrote:
In D26549#592560, @jjazeix wrote:

For the 24 hours, I would revert the change from this diff.
Multiple things need to be taken in account:

  • Should the hint go to 24h (from 0h to 24h)?
  • Should we display AM/PM for locale that use them instead of 13h->24h? ...

I think for 24 hrs format there is no AM/PM. As 20hr is equal to 8 pm using this format.
Let me know if you want me to remove 24hrs format from the diff for now.

For me, it needs more discussion on how to handle it in a clean way

Sure, than I would revert the changes and update the diff.

dekumar updated this revision to Diff 73341.Jan 12 2020, 4:10 PM
  • 24 four hours format reverted back
dekumar added a comment.Jan 12 2020, 4:12 PM

I have made all the changes as requested.

jjazeix added inline comments.Jan 12 2020, 5:25 PM
src/activities/clockgame/resource/3/Data.qml
4

need to be updated (also in level 5)

jjazeix accepted this revision.Jan 12 2020, 5:32 PM
This revision is now accepted and ready to land.Jan 12 2020, 5:32 PM
echarruau added a comment.Jan 12 2020, 9:46 PM

On my side I do not see anything to correct so far. I think next test will be done by real pupils :) Great work!

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

Here change to 4 (teaching clock is started around 6 years old).

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

Here change to 4.

src/activities/clockgame/resource/4/Data.qml
26

Here change to 5.

src/activities/clockgame/resource/5/Data.qml
26

Here change to 6.

jjazeix closed this revision.Jan 13 2020, 7:36 AM

Thank you, pushed in https://commits.kde.org/gcompris/730ff4e546edb0227ff5f376ebc1feb77869c91e (I did the change @echarruau mentioned on difficulties)

Revision Contents
Changeset List

Diff 73334

View Options

src/activities/clockgame/ActivityInfo.qml

Loading...
View Options

src/activities/clockgame/Clockgame.qml

Loading...
View Options

src/activities/clockgame/clockgame.js

Loading...
View Options

src/activities/clockgame/resource/1/Data.qml

Loading...
View Options

src/activities/clockgame/resource/2/Data.qml

Loading...
View Options

src/activities/clockgame/resource/3/Data.qml

Loading...
View Options

src/activities/clockgame/resource/4/Data.qml

Loading...
View Options

src/activities/clockgame/resource/5/Data.qml

Loading...