Differential D17945 Diff 51082 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 evenNumber: evenNumber.text
amankumarguptaUnsubmitted
Done

Extra empty line.

amankumargupta: Extra empty line.
29​ property alias oddNumber: oddNumber.text
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 alias question: question.text
32​ property bool isEven: 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: qsTr("Choose the even 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​ Row {
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​ spacing: parent.height * 0.8
56​ anchors.centerIn: parent
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​ Rectangle {
58​ color: "transparent"
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
59​ border.color: "black"
60​ width: 170
61​ height: 130
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​ MouseArea {
63​ anchors.fill: parent
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​ onClicked: {
65​ if(isEven) {
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?
66​ message.text = qsTr("Great")
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…
67​ }
68​ else {
amankumarguptaUnsubmitted
Done

extra 1 space after ':' and above too.

amankumargupta: extra 1 space after ':' and above too.
69​ message.text = qsTr("Great")
70​
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​ messageRectangle.visible = true
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
73​ message.visible = true
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​ }
76​ }
amankumarguptaUnsubmitted
Not Done

evenNumber and oddNumber can be refactored.

amankumargupta: evenNumber and oddNumber can be refactored.
77​ Rectangle {
78​ color: "transparent"
79​ border.color: "black"
80​ width: 170
81​ height: 130
82​ MouseArea {
83​ anchors.fill: parent
84​ onClicked: {
85​ if(isEven) {
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
86​ message.text = qsTr("No,this is not an even number.")
jjazeixUnsubmitted
Not Done

missing space after ","

jjazeix: missing space after ","
87​ }
88​ else {
89​ message.text = qsTr("No,this is not an odd number.")
90​ }
91​ messageRectangle.visible = true
92​ message.visible = true
93​ }
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​ }
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​ }
96​ }
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
97​
98​
99​ Row {
100​ id: number
amankumarguptaUnsubmitted
Done

the variable name should be in camel case.

amankumargupta: the variable name should be in camel case.
101​ spacing: parent.height * 0.8
102​ anchors.centerIn: parent
103​
104​ GCText {
105​ id: evenNumber
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
106​ text: " "
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…
107​ fontSize: hugeSize
108​ }
jjazeixUnsubmitted
Not Done

indentation

jjazeix: indentation
109​
110​ GCText {
111​ id: oddNumber
112​ text: " "
113​ fontSize: hugeSize
114​ }
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
115​}
amankumarguptaUnsubmitted
Not Done

rightMessage and wrongMessage can be refactored

amankumargupta: rightMessage and wrongMessage can be refactored
116​
117​
amankumarguptaUnsubmitted
Done

camel case

amankumargupta: camel case
118​ Rectangle {
119​ id: messageRectangle
120​ anchors {
121​ top: parent.top
122​ topMargin: parent.height * 0.7
amankumarguptaUnsubmitted
Done

indentation

amankumargupta: indentation
123​ left: parent.left
124​ leftMargin: parent.height * 0.7
125​ }
126​ opacity: 0.8
127​ radius: 10
128​ border.width: 6
129​ color: "white"
130​ border.color: "#87A6DD"
131​ width: parent.width * 0.4
amankumarguptaUnsubmitted
Done

Indentation

amankumargupta: Indentation
132​ height: parent.height * 0.3
133​ visible: false
134​ MouseArea {
135​ anchors.fill: parent
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​ onClicked: {
137​ messageRectangle.visible = false
138​ message.visible = false
139​ }
140​ }
141​
142​ GCText {
143​ id: message
144​ anchors {
145​ top: parent.top
146​ topMargin: parent.height * 0.2
147​ left: parent.left
148​ leftMargin: parent.height * 0.08
149​ }
150​ text: ""
151​ fontSize: largeSize
152​ fontSizeMode: Text.Fit
153​ color: "black"
154​ horizontalAlignment: Text.AlignLeft
155​ width: parent.width
156​ height: parent.height
157​ visible: false
158​ z: 2
159​
jjazeixUnsubmitted
Not Done

too much break lines

jjazeix: too much break lines
160​ }
161​ }
162​}
163​
164​
165​
166​
167​