Adding feature to change letters to capital and back (Ref: T5936)
ClosedPublic

Authored by rohitdas on Dec 16 2017, 3:36 PM.

Details

Summary

Adding config options to change letter to be searched in words in capital.

Diff Detail

Repository
R2 GCompris
Lint
Lint Skipped
Unit
Unit Tests Skipped
rohitdas created this revision.Dec 16 2017, 3:36 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptDec 16 2017, 3:36 PM
Restricted Application added a subscriber: KDE Edu. · View Herald Transcript
rohitdas requested review of this revision.Dec 16 2017, 3:36 PM
rohitdas added a reviewer: jjazeix.
jjazeix added inline comments.Dec 26 2017, 7:37 PM
src/activities/letter-in-word/LetterInWord.qml
69

not a good variable name

143

We can have a combobox with the 3 possible values instead: lowercase, uppercase and mixed (we randomly choose if lower or upper) and we can be more precise telling it's only the letter to find that will be in lower/uppercase (the words won't be affected by this setting).

152

you should only reload the level on onSaveData where you need to store the value

src/activities/letter-in-word/letter-in-word.js
121

if I'm not wrong (I didn't tested), there is a global configuration option to set all the texts in uppercase.
If we choose lowercase for this activity (but there is uppercase in global configuration), we should check here that the letter is displayed in lowercase

rohitdas updated this revision to Diff 24448.Dec 29 2017, 9:49 AM

Added combo box. Changes made as requested. Please review.

jjazeix added inline comments.Dec 29 2017, 5:43 PM
src/activities/letter-in-word/LetterInWord.qml
69

why don't you directly use the string value (not the text displayed, but a value associated to the combobox text)? It will be more explicit when used than the index

70

as it is only used in the combobox (and not in the js file), it is better to declare it in the combobox

rohitdas updated this revision to Diff 24591.Jan 2 2018, 2:07 PM

Changes made as requested. Please review.

jjazeix added inline comments.Jan 2 2018, 2:30 PM
src/activities/letter-in-word/LetterInWord.qml
26

There is an enum in http://doc.qt.io/qt-5/qml-font.html for the capitalization, it would be better to use them than magic numbers (check the other parts of the code using it too).

rohitdas updated this revision to Diff 24652.Jan 3 2018, 9:53 AM

Changes made as requested. Used enumerated font values. Please review.

jjazeix added inline comments.Jan 3 2018, 8:17 PM
src/activities/letter-in-word/LetterInWord.qml
152

If I put lowercase, leave the activity and return it is saved as mixed case. Same if I put mixed case, restart GCompris, it will be lowercase.

293

the dataset is fully on lowercase so you should keep the rand()%2 here in mixed case

rohitdas updated this revision to Diff 24711.Jan 4 2018, 1:20 PM

Changes made as requested. Please review.

@jjazeix Hi, please review the changes here.

Sorry for delay, there is still an issue with mixed case

src/activities/letter-in-word/LetterInWord.qml
69

The default value should be the same as ApplicationSettings.fontCapitalization

293

after testing, it does not work as expected for MixedCase.
Basically, it's only computed once when changing (so it will either always be uppercase or lowercase) and it won't change when changing level/sublevel.
To handle all the cases, the capitalization should be set on the initLevel function.

rohitdas updated this revision to Diff 29171.Mar 10 2018, 4:24 PM

Changes made as requested. Please review.

jjazeix added inline comments.Mar 10 2018, 4:42 PM
src/activities/letter-in-word/LetterInWord.qml
29

why has it changed again?

src/activities/letter-in-word/letter-in-word.js
120

why not directly using Font.MixedCase (you may need a small adaptation to not have an error)?

Hi. I didn't do any changes on line 29. What are the changes showing?

Hi. I didn't do any changes on line 29. What are the changes showing?

https://phabricator.kde.org/D9360?vs=24711&id=29171&whitespace=ignore-most#toc
the letterCaseBox model changed again from enum values (good) to magic numbers (not good)

jjazeix accepted this revision.Mar 10 2018, 5:46 PM

Committed in https://commits.kde.org/gcompris/7c769594a5372e560904511404e2d1e156904050
don't hesitate to check if it works fine,
thank you

This revision is now accepted and ready to land.Mar 10 2018, 5:46 PM
jjazeix closed this revision.Mar 10 2018, 5:46 PM

It's perfect. Thanks.

rohitdas updated this revision to Diff 32055.Apr 13 2018, 11:52 AM
This comment was removed by rohitdas.