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.
Details
- Reviewers
rudranilbasu jjazeix - Group Reviewers
GCompris: Improvements GCompris
Diff Detail
- Repository
- R2 GCompris
- Lint
Lint Skipped - Unit
Unit Tests Skipped
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. | |
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 |
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. |
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.
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.
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,
src/activities/penalty/GoalZone.qml | ||
---|---|---|
61 ↗ | (On Diff #21174) | Should I change this function back to the ternary expression? |
commited in https://commits.kde.org/gcompris/e5b68f68657cf1ac25a6aa31924acfaf4a180f0b.
Changes were made, please check them