After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762016 - Pause gets very broken if used during countdown
Pause gets very broken if used during countdown
Status: RESOLVED FIXED
Product: gnome-nibbles
Classification: Applications
Component: general
3.19.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-nibbles-maint
gnome-nibbles-maint
Depends on:
Blocks:
 
 
Reported: 2016-02-14 02:49 UTC by Michael Catanzaro
Modified: 2016-02-18 13:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Disable new game/pause button during countdowns (785 bytes, patch)
2016-02-14 12:03 UTC, Gabriel Ivașcu
none Details | Review
Fix New Game/Pause actions during countdowns (4.26 KB, patch)
2016-02-16 17:01 UTC, Gabriel Ivașcu
none Details | Review
Fix New Game/Pause actions during countdowns (4.87 KB, patch)
2016-02-17 12:36 UTC, Gabriel Ivașcu
none Details | Review
Fix New Game/Pause actions during countdowns (4.85 KB, patch)
2016-02-17 19:03 UTC, Gabriel Ivașcu
committed Details | Review

Description Michael Catanzaro 2016-02-14 02:49:50 UTC
The pause button is disabled during the countdown on level one, but on subsequent levels it's not disabled; clicking it immediately ends the countdown, starts the game, and gets the state messed up as the game is running though the button still says pause... clicking the button again pauses the game very briefly, then immediately unpauses it.

Let's make sure the pause action is always disabled during countdowns.
Comment 1 Gabriel Ivașcu 2016-02-14 12:03:40 UTC
Created attachment 321108 [details] [review]
Disable new game/pause button during countdowns

Finishing a level disables the new game and pause actions, however only until the 'Next Level' button is clicked (see the set_enabled (true) calls connected to the button's clicked signal).

Considering that these actions are also enabled back as soon as the countdown reaches zero (see set_enabled (true) calls inside start_game_with_countdowns()) I say that discarding the set_enabled statements connected to the 'Next Level' button should do the trick.

Another way to solve this would be by disabling them when the countdown starts but I see no sense in having them enabled when clicking 'Next Level' only to disable them few moments later.
Comment 2 Gabriel Ivașcu 2016-02-14 12:09:04 UTC
(In reply to Michael Catanzaro from comment #0)
> Let's make sure the pause action is always disabled during countdowns.

Oops, the above patch disables the new game action too.

Let me know if you want this behavior only for the pause action.
Comment 3 Michael Catanzaro 2016-02-14 15:36:04 UTC
(In reply to Gabriel Ivascu from comment #2)
> (In reply to Michael Catanzaro from comment #0)
> > Let's make sure the pause action is always disabled during countdowns.
> 
> Oops, the above patch disables the new game action too.
> 
> Let me know if you want this behavior only for the pause action.

I think it makes sense to allow New Game, but we should make sure the button works properly in that case (i.e. nothing weird happens if the countdown expires a couple seconds into the new game).
Comment 4 Gabriel Ivașcu 2016-02-16 17:01:02 UTC
Created attachment 321424 [details] [review]
Fix New Game/Pause actions during countdowns

(In reply to Michael Catanzaro from comment #3)
> I think it makes sense to allow New Game, but we should make sure the button
> works properly in that case (i.e. nothing weird happens if the countdown
> expires a couple seconds into the new game).

Clicking 'New Game' -> 'New Game' works just fine, the game will switch to the opening screen where you choose the number of players and start a new game.

Yet clicking 'New Game' -> 'Cancel' is the one that is causing some trouble: it immediately starts the game (while the countdown is still running) therefore causing the game to start again once more when the countdown reaches zero. This oddly results in an increased move speed of the worms and some labels overlaying the game screen which is bad.

After having a closer look I've noticed that this is caused by automatically firing the game.start() method when the new game dialog is destroyed regardless of the fact that the countdown hasn't expired yet.

This patch provides a fix for this unwanted behavior by freezing the countdown while the new game dialog is open and making sure that clicking 'Cancel' (or hitting Esc) will not start the game unless the countdown has reached zero, assuring that way the game will only start when the countdown expires.

As for the 'Pause' button, it will remain disabled during countdowns.

Please let me know what you think of this.
Comment 5 Michael Catanzaro 2016-02-16 18:04:40 UTC
Review of attachment 321424 [details] [review]:

::: src/gnome-nibbles.vala
@@ +76,3 @@
     private const int COUNTDOWN_TIME = 3;
+    private bool clicked_new_game = false;
+    private int seconds = 0;

I don't think you need either of these, see below.

@@ +361,3 @@
         countdown_id = Timeout.add (1000, () => {
+            if (!clicked_new_game)
+                seconds--;

But now, when is this timeout source ever removed from the main loop? Only in show_new_game_screen_cb(), which won't occur until the next time the user clicks the new game button, correct? So you leave this useless source causing wakeups every 1ms indefinitely. Instead...

@@ +394,3 @@
     private void new_game_cb ()
     {
+        clicked_new_game = true;

...just remove the source here.

Source.remove (countdown_id);
countdown_id = 0;

Then it won't get called again.

You might look at https://developer.gnome.org/glib/unstable/glib-The-Main-Event-Loop.html to get some background on how the main loop works.

@@ +415,3 @@
             {
+                if (seconds == 0)
+                    game.start ();

I know this was here previously, but I'm not sure this is right; why should you ever start a new game if the user pressed cancel?
Comment 6 Gabriel Ivașcu 2016-02-16 19:38:10 UTC
(In reply to Michael Catanzaro from comment #5)
> @@ +361,3 @@
>          countdown_id = Timeout.add (1000, () => {
> +            if (!clicked_new_game)
> +                seconds--;
> 
> But now, when is this timeout source ever removed from the main loop? Only
> in show_new_game_screen_cb(), which won't occur until the next time the user
> clicks the new game button, correct? So you leave this useless source
> causing wakeups every 1ms indefinitely. Instead...
> 
> @@ +394,3 @@
>      private void new_game_cb ()
>      {
> +        clicked_new_game = true;
> 
> ...just remove the source here.
> 
> Source.remove (countdown_id);
> countdown_id = 0;
> 
> Then it won't get called again.

My point was to stall the countdown while the new game dialog is open, so that's why I chose to only decrement the number of seconds if (clicked_new_game == false).

Of course, this means the source will continue wakeups every 1s until either 'Cancel' or 'New Game' is pressed. If clicked 'Cancel' then the countdown will continue and the source will be removed when reached zero; if clicked 'New Game' then the source will be removed in show_new_game_screen_cb().

Are you suggesting to remove the source once the new game dialog is open and add it back when you press 'Cancel'? (but keeping the number of seconds in countdown the same of course).

> @@ +415,3 @@
>              {
> +                if (seconds == 0)
> +                    game.start ();
> 
> I know this was here previously, but I'm not sure this is right; why should
> you ever start a new game if the user pressed cancel?

This does not mean a new game will be started when pressed cancel; it is only supposed to unpause the game (given that clicking 'New Game' while playing automatically pauses the game).

It matches the stop call found here:

388        if (game.is_running)
389	       game.stop ();
Comment 7 Michael Catanzaro 2016-02-17 00:01:07 UTC
(In reply to Gabriel Ivascu from comment #6)
> My point was to stall the countdown while the new game dialog is open, so
> that's why I chose to only decrement the number of seconds if
> (clicked_new_game == false).
> 
> Of course, this means the source will continue wakeups every 1s

There will actually be a processor wakeup every 1ms when using g_timeout_add, which is hard on power usage; it's usually better to use g_timeout_add_seconds to avoid this and reduce power usage, but then the first execution of the source will be less precise as it could start anytime within the next 1-2 seconds rather than anytime within the next 1-2 milliseconds; that might be why we're not using it here, if not it should be changed.

> Are you suggesting to remove the source once the new game dialog is open and
> add it back when you press 'Cancel'? (but keeping the number of seconds in
> countdown the same of course).

Yes please, that's much cleaner. It's true you do need to keep seconds as a member variable, so I was wrong in saying you can get rid of that; there's no other way to set it to count down from the right number.

You will have to split the code from the lambda out into a separate function, so you can call it from two places.

> This does not mean a new game will be started when pressed cancel; it is
> only supposed to unpause the game (given that clicking 'New Game' while
> playing automatically pauses the game).

It's only supposed to unpause the game, in the condition that !game.is_paused... yeah, I see what Iulian was saying the other day about the start/stop pause/unpause being too confusing. :)
Comment 8 Gabriel Ivașcu 2016-02-17 12:36:15 UTC
Created attachment 321479 [details] [review]
Fix New Game/Pause actions during countdowns

(In reply to Michael Catanzaro from comment #7)
> There will actually be a processor wakeup every 1ms when using
> g_timeout_add, which is hard on power usage; it's usually better to use
> g_timeout_add_seconds to avoid this and reduce power usage, but then the
> first execution of the source will be less precise as it could start anytime
> within the next 1-2 seconds rather than anytime within the next 1-2
> milliseconds; that might be why we're not using it here, if not it should be
> changed.

Oh, I wasn't aware of that; I think it should be okay now :)

(I've also replaced g_timeout_add with g_timeout_add_seconds and it appears to work fine so I think we can keep it this way)

@@ -350,29 +374,13 @@
+        new_game_action.set_enabled (true);

In case you wonder why I've added that here, it is to have the new game action enabled during the countdown on level one too.

> It's only supposed to unpause the game, in the condition that
> !game.is_paused... yeah, I see what Iulian was saying the other day about
> the start/stop pause/unpause being too confusing. :)

Yes, that's one thing but I guess it will be the subject of another patch.
Comment 9 Michael Catanzaro 2016-02-17 17:53:04 UTC
Review of attachment 321479 [details] [review]:

Much better :)

Typo fiollowing -> following in the commit message

::: src/gnome-nibbles.vala
@@ +257,3 @@
     }
 
+    private bool countdown_handler ()

countdown_cb
Comment 10 Gabriel Ivașcu 2016-02-17 19:03:17 UTC
Created attachment 321536 [details] [review]
Fix New Game/Pause actions during countdowns

(In reply to Michael Catanzaro from comment #9)
> Typo fiollowing -> following in the commit message

Oops :D

> @@ +257,3 @@
>      }
>  
> +    private bool countdown_handler ()
> 
> countdown_cb

Done.
Comment 11 Iulian Radu 2016-02-18 13:22:52 UTC
Attachment 321536 [details] pushed as 984d3b2 - Fix New Game/Pause actions during countdowns