D14653: note_names

[Johnny Jazeix]

jjazeix added inline comments.

INLINE COMMENTS

> AdvancedTimer.qml:21
> + */
> +import QtQuick 2.1
> +import QtQuick.Controls 1.0

2.6

> NoteNames.qml:53
> +                // If the key pressed matches the note, pass the correct answer as parameter, else pass a wrong answer.
> +                if(event.key === Qt.Key_1) {
> +                    if(Activity.newNotesSequence[Activity.currentNoteIndex][0] === 'C')

code should be factorised

> NoteNames.qml:57
> +                    else
> +                       Activity.checkAnswer("Z")
> +                }

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

> NoteNames.qml:240
> +            property int percentage: 0
> +            readonly property string message: qsTr("%1%").arg(value)
> +

need a translator comment to understand the string

> NoteNames.qml:313
> +            Piano {
> +                id: piano2
> +                width: piano.width

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?

> NoteNames.qml:341
> +            id: shiftKeyboardLeft
> +            source: "qrc:/gcompris/src/core/resource/bar_next.svg"
> +            sourceSize.width: piano.width / 7

shouldn't it be bar_previous?

> note_names.js:22
> +.pragma library
> +.import QtQuick 2.0 as Quick
> +.import "qrc:/gcompris/src/core/core.js" as Core

version

> note_names.js:43
> +    var objective = dataset.objective
> +    objective += qsTr("<br>Note: Reference notes are red in color")
> +    items.introMessage.intro = [objective]

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

> note_names.js:59
> +    items.piano.coloredKeyLabels = dataset.referenceNotes[levels[currentLevel]["clef"]]
> +    items.background.clefType = levels[currentLevel]["clef"]
> +    items.piano.currentOctaveNb = 1

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

> note_names.js:87
> +    targetNotes = JSON.parse(JSON.stringify(levels[currentLevel]["sequence"]))
> +    for(var i = 0; i < currentLevel && newNotesSequence.length < 25; i++) {
> +        if(levels[currentLevel]["clef"] === levels[i]["clef"]) {

hardcoded 25, 50 is not good

> note_names.js:125
> +
> +    items.progressBar.percentage = Math.max(0, items.progressBar.percentage - 4)
> +    items.multipleStaff.resumeNoteAnimation()

4 is harcoded

> note_names.js:136
> +    items.multipleStaff.resumeNoteAnimation()
> +    items.progressBar.percentage += 2
> +    if(items.progressBar.percentage === 100) {

hardcode

REPOSITORY
  R2 GCompris

REVISION DETAIL
  https://phabricator.kde.org/D14653

To: amankumargupta, #gcompris_activities
Cc: jjazeix, kde-edu, #gcompris_activities, harrymecwan, ganeshredcobra, nityanandkumar, echarruau, rahulyadav, narvaez, scagarwal, apol, timotheegiet, hkaelberer, bcoudoin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20180806/6164dc29/attachment-0001.html>