sugarlabs / fifty-two-activity

GNU General Public License v2.0
0 stars 5 forks source link

Fixed stop activity button #11

Open SudoSu-bham opened 1 month ago

SudoSu-bham commented 1 month ago

activity used to stuck in while loop Even after pressing Stop button activity was running in background

SudoSu-bham commented 1 month ago

closes #7

quozl commented 1 month ago
SudoSu-bham commented 1 month ago

I am not able to understand what do you mean by there are game states that you may not have tested the stop button with. I don't know how to play card games. Please let me know If I am missing something.

There was an issue that Stop button was not responding when camera was opened and it is fixed now.

quozl commented 1 month ago

Okay, here's one way to do it; please go through the line by line changes in https://github.com/sugarlabs/fifty-two-activity/commit/9ebc08d32339115aaf9bebbf76d60be6cd116472, deduce their effect, check if you have them in your patch, and where you do not have them explain why they are not needed?

SudoSu-bham commented 1 month ago

modifications in crazyheights.py is not needed.

original code were

while 1:
elif event.type == QUIT:
   return

new changes through 9ebc08d

while running:
elif event.type == QUIT:
    running = False
    return running

these changes have no effect, because ultimately changing the value of running depends on event.type == QUIT. while in original code if QUIT event is detected than return will break the while 1: loop. Therefore I haven't made this change in my PR.

however the state of running is used in run.py but inside run.py

new changes at line 126

if not self.running:
    break

seems unreachable because once self.running = False while loop at line 121 would not be executed

and

there is one more function photo() in run.py at line 230 there is while loop which is not stopped when stop button is clicked, which was causing the activity to stuck in loop when camera is opened.

And all these are fixed in my PR.

I also noticed that once there is a winner label on the screen than fiftytwo activity becomes unresponsive (for 2-3 seconds) until the label vanish on its own.

quozl commented 1 month ago

Sorry, but I don't see how your explanation fits together.

Reviewing the source code as at your commit 033f131, this is what I've learned;

Therefore the implementation is incomplete; there are two game states in which the game will not respond if the stop button is pressed.

On the other hand, the other pull request 9ebc08d adds a local flag in crazyeights.py instead of using the same flag used by activity.py.

Also it is not clear why run.py photo() and run.py run() use different variables running and playing.