Added multiple dataset in algebra_by activity
ClosedPublic

Authored by shubhammishra on Feb 2 2020, 7:29 PM.

Details

Summary

added initial levels

final level done

added ok button and circular shift

enable and disable functionality for ok button

added more datasets

fixed identation

Diff Detail

Repository
R2 GCompris
Branch
enumerate
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22465
Build 22483: arc lint + arc unit
shubhammishra created this revision.Feb 2 2020, 7:29 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptFeb 2 2020, 7:29 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
shubhammishra requested review of this revision.Feb 2 2020, 7:29 PM
shubhammishra retitled this revision from Added multiple dataset to Added multiple dataset in algebra_by activity.Feb 2 2020, 7:33 PM
shubhammishra edited the test plan for this revision. (Show Details)
shubhammishra edited projects, added KDE Applications; removed KDE Edu.
shubhammishra removed a subscriber: kde-edu.
Restricted Application added a project: KDE Edu. · View Herald TranscriptFeb 2 2020, 7:33 PM

Hi, added data set same as we discussed but other 3 activities are broken now, i will fix them later. do i need to make same button for data set options and slider? they have separate buttons right now.

jjazeix added a subscriber: jjazeix.Feb 3 2020, 7:34 AM

I didn't check the behaviour yet, but I added some comments.
I think the dataset will need more information than just the operand.
The aim is to reduce the code too to have the maximum of specificities in the dataset and have a generic code where we can add other data without having to change the code.
For example, we should be able to add a modulo activity (it's just an example, it is too complicated for children but it should be possible to do so).

src/activities/algebra_by/Algebra.qml
192

the size should be relative to the bar size to be coherent (like in calendar for example) or fixed like in digital electricity.

192

okButton.enabled === (answer != "")
seems better to read

src/activities/algebra_by/algebra.js
85

remove space before )

91

no need for this extra line

98

coherency in the name (camelcase)

102

remove extra spaces in this function

shubhammishra edited the test plan for this revision. (Show Details)
  • done all required changes, size of okButton still needs to be set
  • removed extra spaces
  • added height and width in ok button
  • changed variable operations name to subLevelData

The other algebra activities are broken so we won't be able to merge this patch unless all the other activities are fixed.
It can be in this diff directly, either by adding multiple dataset or fixing the errors. But as we plan to change all to multiple dataset, it makes sense to convert them directly.

src/activities/algebra_by/Algebra.qml
64

This should be merged within the DialogChooseLevel (and dialogActivityConf should be renamed dialogActivityConfig)

src/activities/algebra_by/algebra.js
98

add some comment to know why the last level is different and how it's created

  • merge configs
  • added dataset to all algebra

After testing, I noticed an issue with Options:
-When changing some options and then click "save" or "save and start", the changes are not applied directly (it keeps the settings that were there while opening the Settings page).
Wether the Activity Settings is opened from the menu or from inside the the activity doesn't make a difference.

Though, it seems to be a global issue with Options-inside-Settings page, as the same problem happens for example with the options of "Simple letters" activity.

I'm adding that to the "General TODO for multi-dataset" task.

I just made a commit on the branch to fix the issue with settings change not applied directly, and applied the fix accordingly to all activities using the DialogChooseLevel.
Note: now inside the Activities DialogChooseLevel, in onStartActivity, we need both background.stop() and background.start() , and in onSaveActivity we don't need anymore the background.stop() and background.start() .

In the process I also noticed some other general issues when pressing Cancel button:
-Dataset selection in the dialog are not restored to previous state
-In Gletters, all the options are restored correctly to previous state, except the Speed slider, so I guess we will have same issue in Algebra/other activities with a speed slider.
I'm now investigating to try to fix the second problem in Gletters, I'll let you know if I find the solution to apply it here too.

Good, I could fix the issue with Speed slider value not restored correctly after pressing Cancel in Gletters.

Just adding this line near the beginning of the fuction setDefaultValues() :

speedSlider.value = Qt.binding(function() {return activityConfiguration.speedSetting;})

see commit c10b3e0ef4f9cce4b315f6dbd18a9242207e61a0

I guess the same will be needed for algebra activities.

I added inline comments for all the needed changes after the changes I made on last commit, and all options editing will work properly.

src/activities/algebra_by/ActivityConfig.qml
58

you need to add at the beginning of setDefaultValues() :

speedSlider.value = Qt.binding(function() {return activityConfiguration.speedSetting;})
src/activities/algebra_by/Algebra.qml
82

Those comments apply after last changes I made on the multi_dataset branch:

you need to remove background.stop() and background.start() from onSaveData

85

you need to add:

onLoadData: {
    if(activityData && activityData["speedSetting"]) {
        activity.speedSetting = activityData["speedSetting"];
    }
}
87

you need to add background.stop() here before the background.start()

timotheegiet requested changes to this revision.Feb 13 2020, 5:39 PM
This revision now requires changes to proceed.Feb 13 2020, 5:39 PM
  • rebased and made requested changes
shubhammishra marked 4 inline comments as done.Feb 13 2020, 6:45 PM
shubhammishra edited the test plan for this revision. (Show Details)Feb 13 2020, 7:27 PM

It looks mostly good for me now.

I still have one question:
When all datasets are selected, is it intended that table of 10 is at second level, after table of 1?
I feel this is weird, and I didn't find in the code where that is defined.

timotheegiet requested changes to this revision.Feb 13 2020, 8:59 PM

I just added a little thing to improve the look of the ok button (I could do it myself afterward, but thought it was good for you to know about this subtlety).

Also, two more things you could add if you want:

-When validating with the keyboard, a little animation to scale down and up again the OK button (like when clicking on it) would be nice.
-The balloon goes up when next question appears; it would be better to make it go up as soon as answer is validated.

src/activities/algebra_by/Algebra.qml
130

please add:

sourceSize.height: height
sourceSize.width: height

to make sure the svg is always rendered at perfect size for the button and not scaled up or down

This revision now requires changes to proceed.Feb 13 2020, 8:59 PM
  • fixed level misorder problem and made other changes

I haven't tested yet but it seems good to be, only a few minor coding conventions to fix.
Thank you!

src/activities/algebra_by/Algebra.qml
104

missing space at the end

137

space + indentation

144

space + indentation

src/activities/algebra_by/algebra.js
66

remove space before var

98

will be generated
there is a useless " at the end

src/activities/algebra_by/resource/1/Data.qml
68

the comma is not needed at the end of the list

  • fixed minor issue and removed comma from all data files
shubhammishra planned changes to this revision.Feb 14 2020, 8:05 AM
shubhammishra requested review of this revision.
shubhammishra marked 15 inline comments as done.
jjazeix accepted this revision.Feb 16 2020, 10:53 AM

Commited in https://commits.kde.org/gcompris/ef0ffda2a553d12f9162516b140e4b476fc1889c.

Thank you!

ps: can't be closed until @timotheegiet accept the revision too

timotheegiet accepted this revision.Mar 25 2020, 5:01 PM

Thanks, closing it now.

This revision is now accepted and ready to land.Mar 25 2020, 5:01 PM
timotheegiet closed this revision.Mar 25 2020, 5:01 PM