Differential D17945 Diff 53220 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 isEven: true
32​ property bool isFirstCorrect: true
33​
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​ GCText {
35​ id: question
36​ anchors {
37​ left: parent.left
38​ verticalCenter: parent.verticalCenter
amankumarguptaUnsubmitted
Done

Missing space before {

amankumargupta: Missing space before {
39​ leftMargin: parent.height * 0.1
40​ centerIn: parent.Center
41​ top: parent.top
42​ topMargin: parent.height * 0.05
43​ }
44​ text: isEven ? qsTr("Choose the even number") : qsTr("Choose the odd number")
45​ fontSize: mediumSize
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​ color: "black"
amankumarguptaUnsubmitted
Done

extra space after (

amankumargupta: extra space after (
47​ horizontalAlignment: Text.AlignLeft
48​ width: 1.8 * parent.width
49​ height: 1.8 * parent.height
50​ wrapMode: Text.WordWrap
51​ z: 2
52​ }
53​
54​ AnswerButton {
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​ id: firstNumber
56​ textLabel: ""
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​ anchors {
58​ top: parent.top
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
59​ topMargin: parent.height * 0.3
60​ left: parent.left
61​ leftMargin: parent.width * 0.2
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​ }
63​ width: parent.width * 0.2
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​ height: parent.height * 0.4
65​ isCorrectAnswer: isFirstCorrect
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​ if (!isCorrectAnswer) {
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​
72​ if(isEven) {
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
73​ message.text = qsTr("There is an error: " +
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…
74​ " when divided by 2, it leaves remainder as 1. " +
75​ "Therefore this is an odd number. ")
76​ }
amankumarguptaUnsubmitted
Not Done

evenNumber and oddNumber can be refactored.

amankumargupta: evenNumber and oddNumber can be refactored.
77​ else if (!isEven) {
78​ message.text = qsTr("There is an error: " +
79​ " when divided by 2, it leaves remainder as 0. " +
80​ "Therefore this is not an odd number. ")
81​ }
82​ }
83​ message.visible = true
84​ messageRectangle.visible = true
85​ }
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
86​ }
87​
jjazeixUnsubmitted
Not Done

missing space after ","

jjazeix: missing space after ","
88​ AnswerButton {
89​ id: secondNumber
90​ textLabel: ""
91​ anchors {
92​ top: parent.top
93​ topMargin: parent.height * 0.3
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​ left: parent.left
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​ leftMargin: parent.width * 0.65
96​ }
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
97​ width: parent.width * 0.2
98​ height: parent.height * 0.4
99​ isCorrectAnswer: !isFirstCorrect
100​ onPressed: {
amankumarguptaUnsubmitted
Done

the variable name should be in camel case.

amankumargupta: the variable name should be in camel case.
101​
102​ if(isCorrectAnswer) {
103​ message.text = qsTr("Great")
104​ }
105​ if(!isCorrectAnswer) {
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
106​ if(isEven) {
107​ message.text = qsTr("There is an error: " +
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​ " when divided by 2, it leaves remainder as 1. " +
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
109​ "Therefore this is an odd number.")
110​ }
111​ else if(!isEven) {
112​ message.text = qsTr("There is an error:" +
113​ " when divided by 2, it leaves remainder as 0. " +
114​ "Therefore this is not an odd number.")
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
115​ }
amankumarguptaUnsubmitted
Not Done

rightMessage and wrongMessage can be refactored

amankumargupta: rightMessage and wrongMessage can be refactored
116​ }
117​ message.visible = true
amankumarguptaUnsubmitted
Done

camel case

amankumargupta: camel case
118​ messageRectangle.visible = true
119​ }
120​ }
121​
122​ Rectangle {
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
123​ id: messageRectangle
124​ anchors {
125​ horizontalCenter: parent.horizontalCenter
126​ top: parent.top
127​ topMargin: parent.height * 0.75
128​ }
129​ opacity: 0.8
130​ radius: 10
131​ border.width: 6
amankumarguptaUnsubmitted
Done

Indentation

amankumargupta: Indentation
132​ color: "white"
133​ border.color: "#87A6DD"
134​ width: parent.width * 1.15
135​ height: parent.height * 0.4
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​ visible: false
137​ GCText {
138​ id: message
139​ anchors {
140​ top: messageRectangle.top
141​ topMargin: (message.text === qsTr("Great")) ? parent.height * 0.25 : 0
142​ left: messageRectangle.left
143​ leftMargin: (message.text === qsTr("Great")) ? parent.width * 0.42 : parent.width * 0.01
144​ }
145​ text: ""
146​ fontSizeMode: Text.Fit
147​ fontSize: smallSize
148​ color: "black"
149​ horizontalAlignment: Text.AlignLeft
150​ width: parent.width
151​ height: parent.height
152​ wrapMode: Text.WordWrap
153​ z: 2
154​
155​ }
156​ }
157​}
158​
159​
jjazeixUnsubmitted
Not Done

too much break lines

jjazeix: too much break lines
160​
161​
162​