Thanks. Reviewed. Some patches added. Briefly tested. Waiting for you to proceed with or update your TODO in opening comment.

@ayushnawal, please test and review, thanks.

Tested, reviewed.

I agree with caa381a, thanks.

Regarding the AttributeError;

• sugarlabs/sugar-toolkit-gtk3@2330ef8
• sugarlabs/activity-turtle-confusion@5d3199f
• sugarlabs/write-activity@ae857bb

Same broken code pattern also still used in Fototoon, GetBooks, Memorize, Speak, and Chess.

@quozl Thanks, will review and test it again.

Please see #17 also 😁

Regarding #17 the commit 4da7b31 can't be easily cherry-picked because it is based on much older commit;

$ git fetch origin pull/17/head
$ git cherry-pick 4da7b31473813098e21cc2497d5613ebf53d60b1
error: could not apply 4da7b31... Fix source_remove warnings
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
$ git diff

In turn this is because this pull-request has changed from GObject to GLib for those calls.

$ git diff

@@@ -610,8 -610,9 +625,14 @@@
          self.total_time = 0
          self._edit_card = None
          self._dead_key = None
++<<<<<<< HEAD
 +        if hasattr(self, 'timeout_id') and self.timeout_id:
 +            GLib.source_remove(self.timeout_id)
++=======
+         if hasattr(self, 'timeout_id') and self.timeout_id is not None:
+             GObject.source_remove(self.timeout_id)
+             self.timeout_id = None 
++>>>>>>> 4da7b31... Fix source_remove warnings
  
          # Fill the grid with custom cards.
          self.grid.restore(self.deck, CUSTOM_CARD_INDICIES)

When you want to contribute to an existing pull-request like this, please base your branch on the latest commit in the pull-request.

This conflict means the two pull-requests are also in conflict, so when we merge this one you'll have to rebase yours.

So I rebased as 315b67c.

Our new focus on the use of timeouts made me review them in general. I suggest others review too.

• there's no need to check with hasattr if the variable is always defined,
• the __init__ function is the best place to define variables that will always be present,

Also I noticed the improper use of the set_cursor method; current versions of GTK do not flush the cursor change to the display automatically, so the busy cursor won't be seen; we added busy to the Activity class to fix this.

When you want to contribute to an existing pull-request like this, please base your branch on the latest commit in the pull-request.

Got it sir, will rebase from next time. your experience helps me learn 😁

Reviewed and Tested the PR again after the last 2 commits, no warnings or error.

• there's no need to check with hasattr if the variable is always defined,

I agree, thought of removing them but I decided to focus on other things.

• the init function is the best place to define variables that will always be present,

Agreed.

Tested 315b67c and it doesn't fix source_remove warnings, tracing the calls show that source_remove for some reason is called before any timeout_add calls.

Thanks for testing. I took a close look, and there were certainly ways in which the timeout source IDs were not cleared despite the source having been automatically removed. 5cea479.

Tested. Added a few more commits based on observations.

Tested 7291292, source_remove warnings are gone.
In master, clicking on a card in the card box returns it back to a different position but that doesn't happen anymore and logs show nothing.

In master, clicking on a card in the card box returns it back to a different position but that doesn't happen anymore and logs show nothing.

Thanks for testing. I don't know quite what you mean; can you explain how to reproduce? Or can you do a git bisect to find which commit made this change?

I did review the aggregate change again and spotted a mistake, fixed in 2948f82. I don't know if it relates to what you saw.

Thanks for testing. I don't know quite what you mean; can you explain how to reproduce?

• Open activity, select any category to play.
• Select cards and they'll glide to card box below.
• At c9ba20e, clicking on the first two cards in the card boxes below returns them back to the other
  cards.

I think the error was introduced at 06f0162 as this behaviour is no longer noticed there.

Thanks.

I've reviewed 06f0162 again. The CairoPlot changes can be ignored as the problem is unrelated to the plotting. None of the other changes seem likely to affect the decision-making of the program.

I've tested 2948f82 and reproduced a behaviour almost the same as what you report; the left-most card in the boxes can be returned, but not the centre card. The right-most card can never be returned because the win or loss sequence blocks it.

Also, I'm not sure if the method of selecting the cards is significant; I've only just discovered they can be dragged.

I've tested 2948f82 and reproduced a behaviour almost the same as what you report; the left-most card in the boxes can be returned, but not the centre card. The right-most card can never be returned because the win or loss sequence blocks it.

Yeah, edited my comment accordingly.

Also, I'm not sure if the method of selecting the cards is significant; I've only just discovered they can be dragged.

No the method of selecting the cards isn't significant - just discovered they can be dragged too -, just clicking on them in the card boxes are.
The earlier source_remove - the ones fixed at 5cea479 - warnings were reported as a result of these clicks.

A git bisect shows f1dfbdd is rearrangement of the match box to be underneath the grid.

A git bisect shows 06f0162 causes the card grid to be misaligned; instead of in rows and columns, the cards are in diagonals.

• fix the grid misalignment.

@quozl kindly review.

• fix grid misalignment.

#19. There was a excess print() statement, which I demoted to logger. Please review and merge

@chimosky: Thanks. Tested. Fixed XML parse error. Please continue.

TODO

• Fix source_remove warning.
• Fix collaboration - i think it was broken before this PR
• Fix turn in activity, at the moment game state is shared but both instances share the same player.