Removed dependency of ball direction on mouse buttons
AbandonedPublic

Authored by rohitdas on Aug 10 2017, 3:45 PM.

Details

Reviewers
jjazeix
Group Reviewers
GCompris
GCompris: Improvements
Summary

As was being discussed, tabs don't have a mouse. So please check this diff.

Diff Detail

Repository
R2 GCompris
Lint
Lint Skipped
Unit
Unit Tests Skipped
rohitdas created this revision.Aug 10 2017, 3:45 PM

Hi,

I just applied a patch and got some errors:
qrc:/gcompris/src/activities/penalty/Penalty.qml:51:27: Unable to assign QQuickAnchorLine to QQuickItem*
qrc:/gcompris/src/activities/penalty/Penalty.qml:158:27: Unable to assign QQuickAnchorLine to QQuickItem*

If you click on the ball, it does not work anymore. Also, instructions (ActivityInfo.qml) needs to be updated.

Can you fix them?

Ok I am looking into it.

rohitdas updated this revision to Diff 18207.Aug 15 2017, 6:45 PM

Please check the current patch I uploaded. Also the ball will not work when about to be kicked. You will have to double tap or double click on the goal to score.

rohitdas updated this revision to Diff 18610.Aug 23 2017, 4:37 PM

Sorry, I had uploaded the wrong patch. I should have noticed earlier. Let me know if anything is out of order.

jjazeix added inline comments.Aug 23 2017, 7:10 PM
src/activities/penalty/Penalty.qml
5

add a space between the type and { (coding style rules)

6

what does this mean? The variable should have understandable names

8

hardcoded values (width, height, x, y), will not work in all devices

71

the code should be factorised, it's the same as above and below. Copy/paste are bad.

194

is it still useful (I haven't read the full code but it seems to be the previous MouseArea for the whole screen)?

447

I guess we can keep the previous final positions, no need to add random

rohitdas updated this revision to Diff 18803.Aug 26 2017, 10:46 AM
rohitdas marked an inline comment as done.

Changes made as requested. Let me know if anything is lacking.

why are there patch files?
Also the patch does not apply anymore (seems to not be based on master branch code):

git apply penalty.diff

penalty.diff:47: trailing whitespace.

penalty.diff:78: trailing whitespace.

penalty.diff:255: trailing whitespace.

penalty.diff:315: trailing whitespace.

penalty.diff:375: trailing whitespace.

error: patch failed: src/activities/penalty/Penalty.qml:39
error: src/activities/penalty/Penalty.qml: patch does not apply

src/activities/penalty/Penalty.qml
42

use real names, not abbreviated ones (and if the name is correct, you don't need a comment to explain it)

49

coding convention, space between () and { (needs to be updated not only there)
put comment above the function, the name should be more explicit on what the function does

rohitdas updated this revision to Diff 18850.Aug 27 2017, 4:23 PM

Uploaded git diff master..my_branch of that directory. Hope it helps.

I can't test for now but I have a doubt on the behaviour on this case:
you click first on right then on left side, where does the ball go?
(the potential same issue on previous code was if you do a left click followed by a right one)
Can you test and see if it is ok? The aim of the activity is to teach double click (on same button), so for me it should not work and count as a win.

src/activities/penalty/Penalty.qml
62

coding rule.
You already initialize it to TOP, just check for "RIGHT" and don't have a default else

102

where does this value comes from? Is it the same on every screen resolutions or should it be relative to it?

111

is it necessary to check this and the button list below?
The only accepted buttons are these 3 so I don't think it's necessary and the mouse.button should always exist.

113

do you need to store it in local or can't you directly pass it to the function and store it in ball.state?

rohitdas updated this revision to Diff 18896.Aug 28 2017, 6:02 PM

The case you mentioned won't work here as the direction of the ball doesn't depend on the mouse button anymore. So long as you double click or double tap on the corresponding side of the goal, the ball will go. Anything else will fail. Let me know your feedback.

penalty.diff:10: trailing whitespace.

signal stop

penalty.diff:33: trailing whitespace.

}

penalty.diff:52: trailing whitespace.

id: rectLeft

penalty.diff:86: trailing whitespace.

id: rectRight

error: patch failed: src/activities/penalty/Penalty.qml:38
error: src/activities/penalty/Penalty.qml: patch does not apply

I actually had the white spaces removed in a later commit. It seemed it had been included in the patch. I'll have to look more into creating patches. Sorry for the inconvenience. I'll send you the diff instead.

rohitdas updated this revision to Diff 18897.Aug 28 2017, 7:03 PM

Adding diff instead.

still not working on master:
git apply penalty.patch
error: patch failed: src/activities/penalty/Penalty.qml:39
error: src/activities/penalty/Penalty.qml: patch does not apply

rohitdas updated this revision to Diff 19086.Sep 2 2017, 11:03 AM

Sorry for the delay. Please review.

jjazeix added inline comments.Sep 2 2017, 3:52 PM
src/activities/penalty/Penalty.qml
97

harcoded values?

rohitdas added a comment.EditedSep 5 2017, 1:49 PM

Created Diff 7684 as requested. Please review. Sorry for the delays. Busy prepping for placements.

jjazeix requested changes to this revision.Sep 5 2017, 2:41 PM
This revision now requires changes to proceed.Sep 5 2017, 2:41 PM
rohitdas abandoned this revision.Nov 8 2017, 12:59 PM