note_names
ClosedPublic

Authored by amankumargupta on Aug 6 2018, 2:54 PM.

Details

Summary

This is the patch file for Note_names activity to be submitted for final evaluation of GSoC.

Diff Detail

Repository
R2 GCompris
Lint
Lint Skipped
Unit
Unit Tests Skipped
amankumargupta created this revision.Aug 6 2018, 2:54 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptAug 6 2018, 2:54 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
jjazeix added a subscriber: jjazeix.Aug 6 2018, 3:17 PM
jjazeix added inline comments.
src/activities/note_names/AdvancedTimer.qml
22

2.6

src/activities/note_names/NoteNames.qml
54

code should be factorised

58

why Z? Isn't there a cleaner way to have an error?

241

need a translator comment to understand the string

314

should have a better id than piano2.
And a comment to tell why you need 2 pianos, and not only one.
Can using a Repeater here be better?

342

shouldn't it be bar_previous?

src/activities/note_names/note_names.js
23

version

44

you should never use += for appending strings as the order can change depending on the language.

60

first compute items.background.clefType and use it in items.piano.coloredKeyLabels

88

hardcoded 25, 50 is not good

126

4 is harcoded

137

hardcode

amankumargupta marked 12 inline comments as done.
jjazeix added inline comments.Aug 6 2018, 6:52 PM
src/activities/note_names/AdvancedTimer.qml
22

1.5

src/activities/note_names/NoteNames.qml
224

can it be an alias of the value?

241

it still needs qsTr(). In French, there is a space between the number and the %

echarruau added inline comments.
src/activities/note_names/NoteNames.qml
280

I have a problem when I want to play a B3. I should have two keyboards present on the screen. C3-B3 keyboard under C4-B4, I can this way play a C4 and B3 without using the arrows. At the moment I have just the C3-B3 alone on the top position and this is not playable.

Same problem in horizontal mode where I have only C3-B3 on the right of the screen. I should see the two octaves C3-B3 and C4-B4.

283

It would be good to rename Piano element as PianoOctaveKeyboard

362

can you try to make the arrows bigger and put them over the keyboard, you gain some place to extend the size of the keyboardon the left and on the right, bigger the keys will be better it will be.

amankumargupta marked 4 inline comments as done.Aug 7 2018, 2:29 AM
amankumargupta added inline comments.
src/activities/note_names/NoteNames.qml
224

I had tried but it doesn't allow.

362

you mean above the keyboard?

echarruau added inline comments.Aug 7 2018, 7:24 AM
src/activities/note_names/NoteNames.qml
362

Yes above. Three is enough Place.

amankumargupta edited the summary of this revision. (Show Details)Aug 11 2018, 10:44 AM
jjazeix added inline comments.Aug 12 2018, 4:35 PM
src/activities/note_names/ActivityInfo.qml
34

just leave it empty if none is needed

src/activities/note_names/NoteNames.qml
54

you can use a map of <Qt.Key_*, key> to simplify more

241

you can just say it's a percentage, translators won't know what is parent.value

src/activities/note_names/resource/dataset_01.qml
24

version

amankumargupta marked 3 inline comments as done.Aug 13 2018, 1:57 PM

Factorised keyboard binding code.

jjazeix accepted this revision.Aug 13 2018, 2:49 PM
This revision is now accepted and ready to land.Aug 13 2018, 2:49 PM
jjazeix closed this revision.Mar 14 2019, 11:04 AM