Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update sugargame to v1.3 #15

Closed
wants to merge 8 commits into from
Closed

update sugargame to v1.3 #15

wants to merge 8 commits into from

Conversation

ayushnawal
Copy link
Contributor

No description provided.

@ayushnawal
Copy link
Contributor Author

Tested.
looks good.
@quozl please review

@quozl
Copy link
Contributor

quozl commented Feb 7, 2020

Thanks, but given your other pull requests involving the Sugargame API, I'd prefer to see you fix them before I review any further changes.

@ayushnawal
Copy link
Contributor Author

I reviewed and fixed them as you suggested, you can review these changes now

remove stop accelerator
add exit test
detect gtk stop button
Copy link
Member

@srevinsaju srevinsaju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ayushnawal Great. Tested works perfectly on Sugar Fedora SOAS 31 other than occassional black screen which I recreated on master too. Thanks. addressed in #16

@quozl
Copy link
Contributor

quozl commented Feb 25, 2020

@srevinsaju, how do you recreate black screen? I think the Activity class should not call show_all, as that would begin a race between GTK+ and Pygame initialisation. The Sugargame test activity does not call it, and we saw black screen problems in some activities if it were called.

@ayushnawal
Copy link
Contributor Author

@srevinsaju, how do you recreate black screen?

There is a small wait (black screen) when the play button is pressed on the toolbar.

Fixes #16 now, @srevinsaju, please review 😁

@srevinsaju
Copy link
Member

@ayushnawal tested. Fundamentally the code is the same. Now, the black screen is no longer seen. The black screen was added to force redraw the screen contents. On play button press, now, the same content remains on the screen, (but actually it should redraw the entire screen). I have provided a clue in my review. Hope you will get a click and find the solution, alternatively, I shall tell what is to be done. Because, you might learn something more of sugargame if you fix this issue, which will help to solve 10+ open PRs which aims at almost the same issue.

Thanks

@srevinsaju
Copy link
Member

@srevinsaju, how do you recreate black screen? I think the Activity class should not call show_all, as that would begin a race between GTK+ and Pygame initialisation. The Sugargame test activity does not call it, and we saw black screen problems in some activities if it were called.

@quozl The black screen can be recreated on 8a8a6ac by pressing the play button and keeping the mouse on the toolbar and not moving it over the sugargame widget. This will make the black screen persistent. Once the cursor is moved over the sugargame widget, the black screen disappears, because the whole area is force - redrawn because of the mouse cursor

@ayushnawal
Copy link
Contributor Author

@srevinsaju please review 😁

I have fixed the error that the same content remains on screen even after pressing the play button to restart, I have also fixed the same issue for the next round button.

Thanks

@ayushnawal
Copy link
Contributor Author

@quozl @srevinsaju

@srevinsaju
Copy link
Member

@ayushnawal Perfect. Tested dd3b834 . Works nice, no errors or warnings. Hope you are getting into groove with pygame and sugargame
Thanks, we will have a final review from @quozl before merge

@ayushnawal
Copy link
Contributor Author

Thanks 😁 and yeah feeling more comfortable now

@quozl
Copy link
Contributor

quozl commented Feb 27, 2020

Thanks. Tested and reviewed as at dd3b834.

  • there's no explanation for the user how to play the game,
  • the user interface elements in the canvas don't have any mouseover tooltips,
  • there are two play buttons in the toolbar; perhaps the "new game" button from other activities should be used in place of the first one,
  • the winning smile is not centred,
  • in hard level the number entry does not fit on my display (1024x768 pixels),
  • the CPU is heavily used redrawing the entire screen with no frame rate limitation; I suggest 15 to 25 frames per second should be adequate,
  • the Activity class should not call show_all, as in my experience this may cause a race condition depending on number of CPUs and system load,
  • the VIDEORESIZE event is only handled on the first call to the event loop function,
  • the activity is not yet ported to Python 3, so can't be used,

@ayushnawal
Copy link
Contributor Author

ayushnawal commented Mar 7, 2020

Thanks @quozl, reviewed back.

Work in Progress

the user interface elements in the canvas don't have any mouseover tooltips

elements such as buttons have mouseover tooltips, displaying on hovering the mouse as expected.

there are two play buttons in the toolbar; perhaps the "new game" button from other activities should be used in place of the first one

Done as suggested, added new-game.svg , took it from colordeducto

the winning smile is not centered

I found it perfect james, as it is not centered with respect to screen but at mid-point(centered) with respect to the output results and input field. Looking nice as it is now.

Screenshot from 2020-03-07 23-29-46

@ayushnawal
Copy link
Contributor Author

the CPU is heavily used redrawing the entire screen with no frame rate limitation; I suggest 15 to 25 frames per second should be adequate.

Set fps to 20 as suggested.

the Activity class should not call show_all

Done as suggested, called child widgets explicitly using show , working

the VIDEORESIZE event is only handled on the first call to the event loop function,

Done, I guess this was only causing number entry not to fit in the hard level. looking nice on my system now.

Screenshot from 2020-03-08 15-16-02

there's no explanation for how to play the game.

Improved gameplay instructions.

Also improved readme.md in general

removed show_all to avoid any race condition
@quozl
Copy link
Contributor

quozl commented Mar 12, 2020

Thanks. Tested as at 666fb3a. Good progress. Updated previous comment.

@ayushnawal
Copy link
Contributor Author

there's no explanation for the user how to play the game

I am assuming that you want me to add a help button which on clicking displays you some basic rules/instructions, it is taking some time 😅, but I am on it.

there are two play buttons in the toolbar; perhaps the "new game" button from other activities should be used in place of the first one,

This is fixed here at
160ecd7

the user interface elements in the canvas don't have any mouseover tooltips
the winning smile is not centred,

I reviewed and commented about these issues here

the activity is not yet ported to Python 3, so can't be used,

after all these fixes reviewed and approved, I can port the activity quickly.

@quozl
Copy link
Contributor

quozl commented Mar 16, 2020

Yes, a help button is one way. An introduction screen is another way.

I don't remember any mouseover tooltips for the buttons on the canvas.

@JuiP
Copy link
Member

JuiP commented Jun 10, 2020

@quozl in this comment, I think

  • the smile is not centered

should be ticked, as @ayushnawal mentioned before, it is not centered with respect to screen but at mid-point(centered) with respect to the output results and input field.

Also cherry-picked the commits in #17 also ported to python3

Edited - Correction, it does not center the smile for medium and hard levels. Shouldn't be ticked

@JuiP
Copy link
Member

JuiP commented Aug 8, 2020

@quozl @chimosky with #17 merged, this PR can be closed.

@chimosky
Copy link
Member

chimosky commented Aug 8, 2020

Thanks for noticing.

@chimosky chimosky closed this Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants