Differential D17945 Diff 53308 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: when divided by 2, it leaves remainder as 1. Therefore this is an odd number.")
74​ }
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​ else if (!isEven) {
76​ message.text = qsTr("There is an error: when divided by 2, it leaves remainder as 0. Therefore this is not an odd number.")
amankumarguptaUnsubmitted
Not Done

evenNumber and oddNumber can be refactored.

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

indentation

jjazeix: indentation
86​ textLabel: ""
87​ anchors {
jjazeixUnsubmitted
Not Done

missing space after ","

jjazeix: missing space after ","
88​ top: parent.top
89​ topMargin: parent.height * 0.3
90​ left: parent.left
91​ leftMargin: parent.width * 0.65
92​ }
93​ width: parent.width * 0.2
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​ height: parent.height * 0.4
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​ isCorrectAnswer: !isFirstCorrect
96​ onPressed: {
amankumarguptaUnsubmitted
Done

indentation

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

the variable name should be in camel case.

amankumargupta: the variable name should be in camel case.
101​ if(isEven) {
102​ message.text = qsTr("There is an error: when divided by 2, it leaves remainder as 1. Therefore this is an odd number.")
103​ }
104​ else if(!isEven) {
105​ message.text = qsTr("There is an error: when divided by 2, it leaves remainder as 0. Therefore this is not an odd number.")
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​ GCText {
129​ id: message
130​ anchors {
131​ top: messageRectangle.top
amankumarguptaUnsubmitted
Done

Indentation

amankumargupta: Indentation
132​ topMargin: (message.text === qsTr("Great")) ? parent.height * 0.25 : 0
133​ left: messageRectangle.left
134​ leftMargin: (message.text === qsTr("Great")) ? parent.width * 0.42 : parent.width * 0.01
135​ }
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​ text: ""
137​ fontSizeMode: Text.Fit
138​ fontSize: smallSize
139​ color: "black"
140​ horizontalAlignment: Text.AlignLeft
141​ width: parent.width
142​ height: parent.height
143​ wrapMode: Text.WordWrap
144​ z: 2
145​
146​ }
147​ }
148​}
149​
150​
151​
152​
153​
jjazeixUnsubmitted
Not Done

too much break lines

jjazeix: too much break lines