Progress bar corresponding to clicked zone only will be animated
ClosedPublic

Authored by rohitdas on Sep 16 2017, 4:38 PM.

Details

Summary

A new variable was introduced, saveBallState in items, to save the current state of the ball outside the function changeBallState().
The zones are only accessed when they are clicked first after the position of the ball has been reset. Please review.

Diff Detail

Repository
R2 GCompris
Lint
Lint Skipped
Unit
Unit Tests Skipped
rohitdas created this revision.Sep 16 2017, 4:38 PM

Your diff works fine to only take in account one direction.
The code should only take in account the two first clicks and should not take any other until the ball is in its initial position (now we can click as soon as the ball is starting to reposition).

src/activities/penalty/Penalty.qml
108

I would remove the if.
For (items.saveBallState == "" || items.saveBallState == "LEFT"), I would directly test it in the changeBallState function (more generic, avoid duplication).

172

I would put "INITIAL" as default value to do the same as ball.state

438

I would put "INITIAL" as default value to do the same as ball.state

rohitdas updated this revision to Diff 19897.Sep 25 2017, 2:43 PM

The code might be a bit messed up. Please review. The game seems to work fine.

jjazeix added inline comments.Sep 25 2017, 7:43 PM
src/activities/penalty/Penalty.qml
4

why not var progress = items.progress directly?
we usually use undefined instead of null

10

you need to think more at binding level instead of manually do the updates

47

you don't need to check the mouse.button

rohitdas updated this revision to Diff 19966.Sep 27 2017, 5:30 AM

Please review.

The patch works as expected but it would be better to improve the code to avoid duplication before merging it.
Not related to your patch, but as we are on this file, can you put the "{" besides the PropertyAnimation in the animationRight animation (there is a line break for now)?

src/activities/penalty/Penalty.qml
48

it would be better to create a new file for the Rectangle where you can define the state value.
Then for the enabled it would look like: items.saveBallState === "INITIAL" ? (ball.x === items.ballX && ball.y === items.ballY) : (items.saveBallState !== rectangle.state)) which is simplier.

rohitdas updated this revision to Diff 20636.Oct 12 2017, 12:31 PM

Please review.

Restricted Application added a project: KDE Edu. · View Herald TranscriptOct 12 2017, 12:31 PM
Restricted Application added a subscriber: KDE Edu. · View Herald Transcript
rohitdas removed a subscriber: KDE Edu.
rohitdas added a subscriber: KDE Edu.
rohitdas updated this revision to Diff 20649.Oct 12 2017, 3:30 PM

Please review. Changed filename.

It is not only about changing the filename. Zones is not good also, each object is a zone (not plural). The copyright is also missing.
isEnabled and changeBallState should be in the zone too, not in the background.

rohitdas updated this revision to Diff 20896.Oct 17 2017, 9:17 AM

Added Progress.qml. Renamed Zones.qml to GoalZone.qml. Please review.

rudranilbasu requested changes to this revision.Oct 22 2017, 2:06 PM
rudranilbasu added a subscriber: rudranilbasu.

Looks good, I made few inline comments for some minor changes.

src/activities/penalty/GoalZone.qml
1

"Zone.qml" => "GoalZone.qml"

62

Can it be deleted it if it is not needed?

This revision now requires changes to proceed.Oct 22 2017, 2:06 PM

Refactoring in a new file is not jsut copy/pasting the previous code to a new file, it can now be simplified too. Please try to simplify what can be in the GoalZone.

rohitdas updated this revision to Diff 21174.Oct 23 2017, 1:09 PM

Did some cleaning up. Please review.

dmadaan added inline comments.Oct 27 2017, 2:25 PM
src/activities/penalty/GoalZone.qml
91

ball.state = progress.ratio > 100 ? items.saveBallState : "FAIL" ?

src/activities/penalty/Penalty.qml
94

space before and after "*" and "/"

src/activities/penalty/penalty.js
34

why this empty line inserted?

About the blank line: I though it makes the code more readable.
About the ternary expression: I thought it makes the code look obscure, so didn't do it. Also have to change in another place.
About the gaps: Thanks for noticing,

rohitdas added inline comments.Oct 27 2017, 2:32 PM
src/activities/penalty/GoalZone.qml
62

Should I change this function back to the ternary expression?

rohitdas abandoned this revision.Nov 1 2017, 2:32 PM
This comment was removed by rohitdas.
rohitdas reclaimed this revision.Nov 1 2017, 2:37 PM
rudranilbasu accepted this revision.Nov 1 2017, 2:37 PM
This revision is now accepted and ready to land.Nov 1 2017, 2:37 PM
rohitdas closed this revision.Nov 1 2017, 2:38 PM