Refactored code, added new Button class, reduced rerenders

[jriyyya]

#23 stated a lot of software design issues and issues with performance and also the architecture of the codebase
while this PR does not aim to be a direct resolution to that issue, this PR does fix a lot of problems mentioned in it

[jriyyya]

There's an issue I'm facing and that is the fact that in the scorewindow after a game is over when I press the back home button it does not take me back home, it does sometimes but it is unreliable and might sometimes send me to another gamemode.
I tried but was not able to find a solution to this.

@chimosky @quozl would you please help me with this?

[quozl]

Hello. I'm on leave right now, and was able to take a quick look.

• try to stick to 80 column limit for git commit messages; your messages were truncated in gitk, and badly wrapped when using Terminal to view, see our guide to contributing for suggested layout,
• the code starts out being hard to read, even before your changes, with hundreds of flake8 excursions; feel free to run autopep8 and commit the changes with a commit message that says autopep8 was run,
• our review standards are way lower for activities that don't yet work, or haven't been published, so we might not be as picky and careful as you see us in other repositories,
• sugargame 1.3 is used, but the activity does not follow the sample activity exactly; for instance it calls show_all in the toolbar builder, which implies it has been an unfinished port to 1.3,
• the way that different event loops are used makes it hard to figure out what is actually going on when the problem occurs; you may need to use print statements or an interactive debugger to find out which loop is active at any given time.

In testing 4d6dc05, I reproduced what you described, and also an exception;

Traceback (most recent call last):
  File "/usr/share/sugar/activities/MakeThemFall.activity/main.py", line 165, in run
    btn.update()
  File "/usr/share/sugar/activities/MakeThemFall.activity/button.py", line 39, in update
    self.action()
  File "/usr/share/sugar/activities/MakeThemFall.activity/main.py", line 141, in <lambda>
    self.buttons.append(Button(390, 450, "data/images/welcomescreen/2paneheart.png", lambda : self.run_game(pane2heartwindow, 6), maxcardiac))
  File "/usr/share/sugar/activities/MakeThemFall.activity/main.py", line 75, in run_game
    if scorewindow(self.gameDisplay, s, gamenumber).run():
  File "/usr/share/sugar/activities/MakeThemFall.activity/scorescreen.py", line 59, in __init__
    if score > maxscore[gamenumber - 1]:
TypeError: '>' not supported between instances of 'NoneType' and 'int'

The exception was not reproducible in repeated execution unless score.pkl was removed from the activity root directory.

Hope that helps!

[quozl]

Interesting, thanks. 4d6dc05 was 326 flake8 messages. 17bdd8b brings it down to 240. A further --experimental --aggressive --aggressive brings it down to 153.

I've looked at the problem you described. An unusual excursion from the Sugargame test activity is that Make Them Fall calls game.start() in Activity.__init__. This is wrong to begin with, as it causes Pygame initialisation at the wrong time relative to GTK initialisation. This shows incomplete port to Sugargame 1.2, a520472.

Later, game.run() renders the buttons, and the buttons call game.run_game() which calls the event loop in the other source files. After that event loop finishes, it calls scorewindow, and if the Back Home button is pressed scorewindow.run() is to return 0, and run_game() runs start() again. This makes no sense to me.

It is not a pattern I've seen in any of the other activities, so it was a pattern introduced by this activity. (Other activities that try to make a button class in Pygame are Pycut and Math Hurdler).

Further debugging will be required. 😁

[quozl]

Okay, I've found the problem with some print() statements;

--- a/main.py
+++ b/main.py
@@ -56,6 +55,7 @@ class game:
     sound = True
 
     def __init__(self):
+        print("game.__init__")
         self.crashed = False
         self.running_mode = None
         self.clock = pygame.time.Clock()
@@ -69,6 +69,7 @@ class game:
         self.maxscore = [0, 0, 0, 0, 0, 0]
 
     def run_game(self, window, gamenumber):
+        print("game.run_game entry", window, gamenumber)
         self.running_mode = window()
         self.running_mode.crashed = self.crashed
         s = self.running_mode.run(self.gameDisplay, self.info)
@@ -77,6 +78,7 @@ class game:
             self.run_game(window, gamenumber)
 
         self.start()
+        print("game.run_game return", window, gamenumber)
 
     def show_help(self):
         a = rules()
@@ -92,6 +94,7 @@ class game:
                 self.maxscore = pickle.load(inp)
 
     def start(self):
+        print("game.start")
 
         self.gameDisplay = pygame.display.get_surface()
 
@@ -155,9 +200,14 @@ class game:
 
     def run(self):
+        print("game.run entry")
         self.gameDisplay = pygame.display.get_surface()
 
         while not self.crashed:
@@ -169,6 +219,7 @@ class game:
 
             event = pygame.event.poll()
             if event.type == pygame.QUIT:
+                print("game.run quit")
                 return
 
             for btn in self.buttons:
@@ -178,6 +229,7 @@ class game:
             pygame.display.update()
             self.clock.tick(60)
 
+        print("game.run return")
         return
 

Briefly, the welcome screen buttons think they have been pressed.

In turn, because the score window button has just been pressed and the mouse is still down.

You can see this by pressing the Back Home button in the lower right corner, which corresponds to no button on the welcome screen. The welcome screen is shown.

If you click on the top right of the Back Home button, the welcome screen detects this as Cardiac game, and begins to play it. Here's the log;

game.__init__
game.start
game.run entry
game.run_game entry <class 'normal.pane2window'> 1
game.start
game.run_game return <class 'normal.pane2window'> 1
game.run_game entry <class 'normal.pane2window'> 1
game.start
game.run_game return <class 'normal.pane2window'> 1
game.run_game entry <class 'normal.pane2window'> 1
game.start
game.run_game return <class 'normal.pane2window'> 1
game.run_game entry <class 'normal.pane2window'> 1
game.start
game.run_game return <class 'normal.pane2window'> 1
game.run_game entry <class 'cardiac.pane2heartwindow'> 6
game.start
game.run_game return <class 'cardiac.pane2heartwindow'> 6

A couple of ways you could fix this;

• wait for mouse button or key release before returning from a window,
• change the button class to work like almost every other GUI on the planet; which take action when a mouse button is released not when it is pressed.

[jriyyya]

I have made some changes, I will be making a few commits for flake8 fixes.

[chimosky]

Reviewed 31af7c4, not tested.

[quozl]

Please don't put in gi.require_version calls unless they have an effect. Usually one is enough, in the main program called by the Sugar shell. Redundant calls waste time, both execution time and time taken to understand the code.

[quozl]

Let me rephrase that; use the opportunity to remove any gi.require_version calls that have no effect. 😁

[jriyyya]

I see, It was done by the original author I suppose, I haven't taken a much look in other files, except for the main.py and scorescreen.py which I refactored,

I just did auto pep8 in all files because of which it was in my commits, Fixing it now though 🫡

[chimosky]

Tested 1d2f127, no new issues were found.

I'll wait for @quozl to review before I merge.

[chimosky]

If 861d2c9 would lead to other flake8 fixes, it'll be better to put it in a separate PR else it's fine as is.

[quozl]

Sorry for the delay. Reviewed the aggregate change (not the commits), and have following comments;

• the activity __init__ builds the toolbar after creating the PygameCanvas object, which is different to how the Sugargame test activity shows how to do it; if there's not a good reason to depart from this model, it would be best to have it the same, and if there is a good reason it should be in comments,
• the activity __init__ calls game.start; instead game.start can be called from the end of game.__init__, again so that the code complies with the model implementation.

[chimosky]

I'm curious, wouldn't it be better to have a generic name for the file instead of guy.py and a generic name for the class too.

[chimosky]

While I agree with the creation of a generic Game class, these should also exist as they're supposed to handle different pygame events in different ways.

They can inherit from the generic Game class and implement their run function to handle what needs to be handled, this also makes debugging easier as the game modes are modular.

[chimosky]

Reviewed 1a8c7e0, not tested.

Also add licenses to the necessary files.

[chimosky]

Reviewed 0b6a605, not tested.

[chimosky]

Tested 77ada49, key presses aren't handled and this is seen in the logs.

Traceback (most recent call last):
  File "/home/ibiam/Activities/make-them-fall-activity/sugargame/event.py", line 128, in _keydown_cb
    return self._keyevent(widget, event, pygame.KEYDOWN)
  File "/home/ibiam/Activities/make-them-fall-activity/sugargame/event.py", line 169, in _keyevent
    mod = self._keymods()
  File "/home/ibiam/Activities/make-them-fall-activity/sugargame/event.py", line 145, in _keymods
    mod |= self.__keystate[key_val] and mod_val
IndexError: list index out of range
Traceback (most recent call last):
  File "/home/ibiam/Activities/make-them-fall-activity/sugargame/event.py", line 128, in _keydown_cb
    return self._keyevent(widget, event, pygame.KEYDOWN)
  File "/home/ibiam/Activities/make-them-fall-activity/sugargame/event.py", line 169, in _keyevent
    mod = self._keymods()
  File "/home/ibiam/Activities/make-them-fall-activity/sugargame/event.py", line 145, in _keymods
    mod |= self.__keystate[key_val] and mod_val
IndexError: list index out of range

[chimosky]

This PR seems to have achieved it's aim of refactoring the code and #27 was opened to track the poor event handling, @quozl do you have anything to add before I merge?

[quozl]

No, nothing springs to mind.