sudoku, added multiple datasets
ClosedPublic

Authored by shubhammishra on Jun 1 2020, 9:57 AM.

Diff Detail

Repository
R2 GCompris
Branch
gsoc_2020_shubham_sudoku
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27117
Build 27135: arc lint + arc unit
shubhammishra created this revision.Jun 1 2020, 9:57 AM
Restricted Application added a project: KDE Edu. · View Herald TranscriptJun 1 2020, 9:57 AM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
shubhammishra requested review of this revision.Jun 1 2020, 9:57 AM
shubhammishra added inline comments.Jun 1 2020, 10:09 AM
src/activities/sudoku/sudoku.js
27–28

Previously hardcoded symbolizeLevelMax variable makes the first 7 levels to use symbols, replaced it with symbolizeLevel variable, and added setSymbolizeLevel function which checks sudoku will form with symbols or numbers at each level.

shubhammishra marked an inline comment as done.Jun 1 2020, 11:11 AM

The handling of the "symbolizeLevel" is not good. It should be set during the initLevel function, not inside dataToImageSource

src/activities/sudoku/sudoku.js
58

missing space

78–79

here it is always "undefined"

96

missing spaces after =

Only a convention question here to decide at this beginning of coding.

src/activities/sudoku/sudoku.js
95

Here within qtcreator it gives a warning telling that we should use !== which makes more sens because we do no need to convert type.
Can you check with johnny if we really need to use !== instead os !=. For me both work but I get no warning if we use !==

jjazeix added inline comments.Jun 2 2020, 6:18 AM
src/activities/sudoku/sudoku.js
95

yes, it would be better to have a strong comparison if QtCreator warns on it

shubhammishra marked an inline comment as not done.
  • sudoku, setting symbolizeLevel in initLevel,solved space issues and replaced "!=" with "!=="
Restricted Application added a project: KDE Edu. · View Herald TranscriptJun 2 2020, 8:25 AM
shubhammishra marked 4 inline comments as done.Jun 2 2020, 8:28 AM
jjazeix added inline comments.Jun 2 2020, 8:32 AM
src/activities/sudoku/sudoku.js
62

is there a preference for not setting it in the dataset directly instead of computing it when we initialize the level?

shubhammishra added inline comments.Jun 2 2020, 9:00 AM
src/activities/sudoku/sudoku.js
62

It will cost us an extra variable in datasets for each level and then levels would not be simple arrays anymore but transform into maps and arrays and it's easy to compute it by code too,
So I went in this way. Does setting it in dataset have any extra advantage?

jjazeix added inline comments.Jun 2 2020, 10:01 AM
src/activities/sudoku/sudoku.js
62

We need to think on how we will allow teachers/users to create datasets (and if it makes sense for the activity).
The easiest way for them would be to have a graphical editor.
In this precise case, the first thing I think of is letting the user choose the grid size and if they want symbols or numbers, so it may be easier to store it directly on the dataset.

Then, for this activity specifically, we probably don't need it...

65

please add parenthesis here to avoid confusion

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Jun 4, 9:43 AM
This revision was automatically updated to reflect the committed changes.

Not sure if it was closed on purpose (or automatically because htere was a commit) but I don't see the latest required changes.

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

As discussed on the irc channel, we need to add a '.' at the end of the sentences

shubhammishra reopened this revision.Fri, Jun 5, 9:09 PM
  • sudoku, moved symbols to datasets, js treating symbols and numbers in same way
jjazeix added inline comments.Sat, Jun 6, 2:02 PM
src/activities/sudoku/resource/1/Data.qml
29

is it still useful to separate imgName and extension? Just having imgName: "circle.svg"?

echarruau added inline comments.Sat, Jun 6, 6:03 PM
src/activities/sudoku/resource/1/Data.qml
29

Do you know why we did it initialy? I would prefer to have rectangle.svg. We have to think that some of these data will be filled up by teachers a day, the easiest the better ;)

  • sudoku, removed 'extention' attribute from symbols

Working like a charm.
Once you will have made it tested by your "test group" as stated in the timeline it will certainly be ready.
Nice and clean job!

jjazeix accepted this revision.Tue, Jun 9, 7:21 PM

If you don't have more changes to do on this activity, this is good for me.
@AkshayCHD, @echarruau, @timotheegiet : do you plan on testing? If yes, I'll wait before merging else I'll merge it when I have time (next week)

This revision is now accepted and ready to land.Tue, Jun 9, 7:21 PM

@jjazeix : I'll test tomorrow.

@johnny: I tested it by using it, nothing has been broken, working fine on my side.

timotheegiet accepted this revision.Wed, Jun 10, 1:08 PM

All tested, looks good to merge.

AkshayCHD accepted this revision.Wed, Jun 10, 8:39 PM

Ran through all the datasets, looks good to go, just one thing can we add a third level in dataset 3, which contains both symbols and numbers. Currently all levels either contain symbols or numbers, which may lead to someone thinking that he can use only one of them at a time in a sublevel, which is not the case actually.

Ran through all the datasets, looks good to go, just one thing can we add a third level in dataset 3, which contains both symbols and numbers. Currently all levels either contain symbols or numbers, which may lead to someone thinking that he can use only one of them at a time in a sublevel, which is not the case actually.

That it is technically possible to use both symbols and numbers in a level doesn't mean it is necessarily a good idea to do it by default.

While we can give the possibility in the future admin interface to mix both in custom-made datasets, I would prefer to keep them separated in datasets provided by default.

As Timothée just said I would also prefer to keep them separated in datasets provided by default.
Symbols are more aimed at children or beginners as number is more for advanced users.
The actual code is now perfectly generic and numbers are in fact symbols.

Sure, works fine for me, just a thought I had :)

jjazeix closed this revision.Sat, Jun 13, 12:44 PM

Merged, thank you!