Differential D17945 Diff 56088 src/activities/numbers-odd-even/resource/TutorialBase.qml

Changeset View

Standalone View

View Options

src/activities/numbers-odd-even/resource/TutorialBase.qml

  • This file was added.
1​/* GCompris - TutorialBase.qml
2​ *
3​ * Copyright (C) 2019 Deepak Kumar <deepakdk2431@gmail.com>
4​ *
5​ * Authors:
6​ * Deepak Kumar <deepakdk2431@gmail.com>
7​ *
8​ * This program is free software; you can redistribute it and/or modify
9​ * it under the terms of the GNU General Public License as published by
10​ * the Free Software Foundation; either version 3 of the License, or
11​ * (at your option) any later version.
12​ *
13​ * This program is distributed in the hope that it will be useful,
14​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
15​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
16​ * GNU General Public License for more details.
17​ *
18​ * You should have received a copy of the GNU General Public License
19​ * along with this program; if not, see <https://www.gnu.org/licenses/>.
20​ */
21​import QtQuick 2.6
22​import "../../../core"
23​
24​Rectangle {
25​ id: tutorialRectangle
26​ anchors.fill: parent
27​ color: "#80FFFFFF"
28​ property alias firstNumber: firstNumber.textLabel
amankumarguptaUnsubmitted
Done

Extra empty line.

amankumargupta: Extra empty line.
29​ property alias secondNumber: secondNumber.textLabel
30​ property alias message: message.text
amankumarguptaUnsubmitted
Done

Missing space after "id:" and an id should be a meaningful name.

amankumargupta: Missing space after "id:" and an id should be a meaningful name.
31​ property bool isEvenExpected: true
32​
33​ GCText {
jjazeixUnsubmitted
Not Done

I still don't understand why you need 2 bools?
isEven (should be renamed to isEvenExpected) is set to true if the question is "choose the even number".
then, when you click on a button, if the number inside is even you should tell if it is good or not.

jjazeix: I still don't understand why you need 2 bools? isEven (should be renamed to isEvenExpected) is…
dekumarAuthorUnsubmitted
Not Done

I need two bools for changing the order of the AnswerButton to be correct.As we need to display three different messages and it should not be assumed that the first box is always the correct answer and also to set the (isCorrectAnswer: ) in answer button we need the second bool.I think the message good or bad can't be set using a single bool because we need to handle a case of odd number too.

dekumar: I need two bools for changing the order of the AnswerButton to be correct.As we need to display…
34​ id: question
35​ anchors {
36​ left: parent.left
37​ verticalCenter: parent.verticalCenter
38​ leftMargin: parent.height * 0.1
amankumarguptaUnsubmitted
Done

Missing space before {

amankumargupta: Missing space before {
39​ centerIn: parent.Center
40​ top: parent.top
41​ topMargin: parent.height * 0.05
42​ }
43​ text: isEvenExpected ? qsTr("Choose the even number:") : qsTr("Choose the odd number:")
44​ fontSize: mediumSize
45​ color: "black"
jjazeixUnsubmitted
Not Done

It would be better to also have on one of the question, the choice to choose the odd number.
Also, having them being on random order (not always the even first, the odd second).

jjazeix: It would be better to also have on one of the question, the choice to choose the odd number.
46​ horizontalAlignment: Text.AlignLeft
amankumarguptaUnsubmitted
Done

extra space after (

amankumargupta: extra space after (
47​ width: 1.8 * parent.width
48​ height: 1.8 * parent.height
49​ wrapMode: Text.WordWrap
50​ z: 2
51​ }
52​
53​ AnswerButton {
54​ id: firstNumber
jjazeixUnsubmitted
Not Done

it would be better to use a Row to present both numbers and have them take the whole space of the remaining space.
Actually, there are too small.

jjazeix: it would be better to use a Row to present both numbers and have them take the whole space of…
55​ textLabel: ""
56​ anchors {
amankumarguptaUnsubmitted
Done

space after id:

amankumargupta: space after id:
jjazeixUnsubmitted
Not Done

needs to be renamed to be generic too

jjazeix: needs to be renamed to be generic too
57​ top: parent.top
58​ topMargin: parent.height * 0.3
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
59​ left: parent.left
60​ leftMargin: parent.width * 0.2
61​ }
jjazeixUnsubmitted
Not Done

having hardcoded width/height make overriding (if you reduce the width at maximum, the rectangles will b eoutside the background gray one).

jjazeix: having hardcoded width/height make overriding (if you reduce the width at maximum, the…
62​ width: parent.width * 0.2
63​ height: parent.height * 0.4
jjazeixUnsubmitted
Not Done

it would be better to have a color surrounding the rectangle so the user knows the click boundaries.
Using a Repeater for both element would avoid duplicating the Rectangle item.

jjazeix: it would be better to have a color surrounding the rectangle so the user knows the click…
64​ isCorrectAnswer: (isEvenExpected && Number(textLabel) % 2 === 0) ||
65​ (!isEvenExpected && Number(textLabel) % 2 !== 0)
66​ onPressed: {
jjazeixUnsubmitted
Not Done

this is not a maintainable way to check this. Let's assume we want to add a new question, we would think that we only need to add a new Tutorial like Tutorial5 but then we will probably forget to update here.
It would be better to have a boolean telling if we want to find the odd or even number at TutorialBase level, and use it to both set the text and tell if it is good or not on click.

Both button should share the same code (there is AnswerButton that could maybe be used? It also have user feedback).

jjazeix: this is not a maintainable way to check this. Let's assume we want to add a new question, we…
jjazeixUnsubmitted
Not Done

are you sure you need to display the same text in if and in else?

jjazeix: are you sure you need to display the same text in if and in else?
67​ if(isCorrectAnswer) {
68​ message.text = qsTr("Great")
amankumarguptaUnsubmitted
Done

extra 1 space after ':' and above too.

amankumargupta: extra 1 space after ':' and above too.
69​ }
70​ else {
jjazeixUnsubmitted
Not Done

the numbers shouldn't be harcoded here, else it won't work for the examples

jjazeix: the numbers shouldn't be harcoded here, else it won't work for the examples
jjazeixUnsubmitted
Not Done

missing space after the '.'?

jjazeix: missing space after the '.'?
71​ if(isEvenExpected) {
72​ message.text = qsTr("There is an error: when divided by 2, %1 leaves remainder as 1. Therefore this is an odd number.").arg(textLabel)
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
73​ }
74​ else {
jjazeixUnsubmitted
Not Done

there will be 2 spaces after ':', you need to remove the one before when on the next line (same comment applies to other sentences below

jjazeix: there will be 2 spaces after ':', you need to remove the one before when on the next line (same…
75​ message.text = qsTr("There is an error: when divided by 2, %1 leaves remainder as 0. Therefore this is an even number.").arg(textLabel)
76​ }
amankumarguptaUnsubmitted
Not Done

evenNumber and oddNumber can be refactored.

amankumargupta: evenNumber and oddNumber can be refactored.
77​ }
78​ message.visible = true
79​ messageRectangle.visible = true
80​ }
81​ }
82​
83​ AnswerButton {
84​ id: secondNumber
85​ textLabel: ""
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
86​ anchors {
87​ top: parent.top
jjazeixUnsubmitted
Not Done

missing space after ","

jjazeix: missing space after ","
88​ topMargin: parent.height * 0.3
89​ left: parent.left
90​ leftMargin: parent.width * 0.65
91​ }
92​ width: parent.width * 0.2
93​ height: parent.height * 0.4
jjazeixUnsubmitted
Not Done

why is there 3 cases here and only one on the other button? Shouldn't they be "symetrical"?

jjazeix: why is there 3 cases here and only one on the other button? Shouldn't they be "symetrical"?
94​ isCorrectAnswer: (isEvenExpected && Number(textLabel) % 2 === 0) ||
jjazeixUnsubmitted
Not Done

The checks order seems not logical.
There is a check on !isCorrectAnswer to display a text and two lines below, if it's not even, the text changes directly.
It would look more readable having:

if(isCorrect) {
}
else {
  if(even) {
  }
  else {
  }
}
jjazeix: The checks order seems not logical. There is a check on !isCorrectAnswer to display a text and…
95​ (!isEvenExpected && Number(textLabel) % 2 !== 0)
96​ onPressed: {
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
97​ if(isCorrectAnswer) {
98​ message.text = qsTr("Great")
99​ }
100​ else {
amankumarguptaUnsubmitted
Done

the variable name should be in camel case.

amankumargupta: the variable name should be in camel case.
101​ if(isEvenExpected) {
102​ message.text = qsTr("There is an error: when divided by 2, %1 leaves remainder as 1. Therefore this is an odd number.").arg(textLabel)
103​ }
104​ else {
105​ message.text = qsTr("There is an error: when divided by 2, %1 leaves remainder as 0. Therefore this is an even number.").arg(textLabel)
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
106​ }
107​ }
jjazeixUnsubmitted
Not Done

it should not be a space. Either don't have a default value or set "" (which is the same but better not put anything if not needed)

jjazeix: it should not be a space. Either don't have a default value or set "" (which is the same but…
108​ message.visible = true
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
109​ messageRectangle.visible = true
110​ }
111​ }
112​
113​ Rectangle {
114​ id: messageRectangle
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
115​ anchors {
amankumarguptaUnsubmitted
Not Done

rightMessage and wrongMessage can be refactored

amankumargupta: rightMessage and wrongMessage can be refactored
116​ horizontalCenter: parent.horizontalCenter
117​ top: parent.top
amankumarguptaUnsubmitted
Done

camel case

amankumargupta: camel case
118​ topMargin: parent.height * 0.75
119​ }
120​ opacity: 0.8
121​ radius: 10
122​ border.width: 6
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
123​ color: "white"
124​ border.color: "#87A6DD"
125​ width: parent.width * 1.15
126​ height: parent.height * 0.4
127​ visible: false
128​
129​ GCText {
130​ id: message
131​ anchors {
amankumarguptaUnsubmitted
Done

Indentation

amankumargupta: Indentation
132​ centerIn: parent
133​ margins: parent.border.width+1
134​ }
135​ text: ""
jjazeixUnsubmitted
Not Done

missing qsTr(). Is there a better way to check that the text has a specific value to set the margin?
With translation, we should not assume anything about the text length

jjazeix: missing qsTr(). Is there a better way to check that the text has a specific value to set the…
136​ fontSizeMode: Text.Fit
137​ fontSize: smallSize
138​ color: "black"
139​ horizontalAlignment: Text.AlignHCenter
140​ verticalAlignment: Text.AlignVCenter
141​ width: parent.width
142​ height: parent.height
143​ wrapMode: Text.WordWrap
144​ z: 2
145​ }
146​ }
147​ }
148​
jjazeixUnsubmitted
Not Done

too much break lines

jjazeix: too much break lines