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 724439 - Pause app menu entry does not display a toggle
Pause app menu entry does not display a toggle
Status: RESOLVED FIXED
Product: gnome-nibbles
Classification: Applications
Component: general
3.10.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-nibbles-maint
gnome-nibbles-maint
Depends on:
Blocks:
 
 
Reported: 2014-02-15 21:05 UTC by gQuigs
Modified: 2014-04-01 23:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
How it should look (124.27 KB, image/png)
2014-02-15 21:05 UTC, gQuigs
  Details
how it shouldn't look (27.02 KB, image/png)
2014-02-15 21:05 UTC, gQuigs
  Details
Rebuild the app menu when the paused state changes (6.61 KB, patch)
2014-02-15 22:21 UTC, Michael Catanzaro
none Details | Review
rhythmbox in 3.10 (137.20 KB, image/png)
2014-02-16 15:19 UTC, Michael Catanzaro
  Details
Test application to demonstrate properly-functioning boolean menu item (2.16 KB, application/octet-stream)
2014-03-19 02:04 UTC, Michael Catanzaro
  Details
Properly change the state of the pause action (803 bytes, patch)
2014-03-19 03:16 UTC, Michael Catanzaro
committed Details | Review

Description gQuigs 2014-02-15 21:05:18 UTC
Created attachment 269231 [details]
How it should look

Some environments show an off slider for the Pause button. Others do not, which makes it seem less responsive.

Either we should:
 * Move it to the header bar as an icon when we make that.
 * Make it change to resume [see https://bugzilla.gnome.org/show_bug.cgi?id=666816 for details]
 * Force the slider to always appear somehow
Comment 1 gQuigs 2014-02-15 21:05:49 UTC
Created attachment 269232 [details]
how it shouldn't look
Comment 2 Michael Catanzaro 2014-02-15 22:20:35 UTC
(In reply to comment #0)
>  * Make it change to resume [see
> https://bugzilla.gnome.org/show_bug.cgi?id=666816 for details]

I gave this a try, but apparently you can only set the app menu once... this is a little frustrating... nonfunctional patch attached, in case you spot something I'm doing wrong (don't think so)

(In reply to comment #0)
> Created an attachment (id=269231) [details]
>  * Move it to the header bar as an icon when we make that.
It'd really be better to have this in the window; we don't want to use the header bar for game controls unless absolutely necessary.
Comment 3 Michael Catanzaro 2014-02-15 22:21:05 UTC
Created attachment 269234 [details] [review]
Rebuild the app menu when the paused state changes

This allows us to change the Pause/Resume label as needed.

DO NOT PUSH - THIS PATCH DOES NOT WORK!
Comment 4 gQuigs 2014-02-16 05:49:43 UTC
I haven't been able to figure out why this doesn't work... :/
Comment 5 Michael Catanzaro 2014-02-16 15:19:13 UTC
Created attachment 269305 [details]
rhythmbox in 3.10

Maybe it's expected that we can only set the app menu once? I'm not sure....

But I do not think it's expected that GNOME Shell displays the boolean state in 3.8 but not 3.10. GMenuModel guarantees:

"While a wide variety of stateful actions is possible, the following is the minimum that is expected to be supported by all users of exported menu information:

an action with no parameter type and no state
an action with no parameter type and boolean state
an action with string parameter type and string state"

And I know from Calculator that string parameter type, string state still works fine. It looks like no parameter, boolean state broke between 3.8 and 3.10, unless there's something wrong on the Nibbles end (I don't think so?).

But in this bug report, you say you do NOT want to see a toggle box. What exactly is the behavior you are expecting here -- does Unity show a check box next to that menu item when the game is paused? GNOME 3.8 uses toggles, but 3.10 uses checkboxes, if that looks better to you. (Rhythmbox uses GSettingsAction, not GSimpleAction, which is why Rhythmbox's app menu isn't broken.)

[1] https://developer.gnome.org/gio/stable/GMenuModel.html
Comment 6 gQuigs 2014-02-16 16:38:30 UTC
Sorry... I *do* want it to display a Toggle Box or a Checkbox, (I have no big preference which.
Comment 7 Michael Catanzaro 2014-02-16 17:53:17 UTC
Let's make a simple test program to demonstrate the issue, then file a bug against gnome-shell: the shell developers will let us know if it's their fault, ours, or another component's.  I'll be happy to do this if you don't want to, but no promises how soon I'll get to it.
Comment 8 Michael Catanzaro 2014-02-22 17:13:48 UTC
FYI we're not alone here: this is broken in the Sudoku Vala port, as well.
Comment 9 gQuigs 2014-03-18 02:53:47 UTC
Pushing to gnome-shell as it affects multiple apps.
Comment 10 Jasper St. Pierre (not reading bugmail) 2014-03-18 12:14:21 UTC
Do you have a simple test application?
Comment 11 gQuigs 2014-03-18 14:14:55 UTC
Not yet, still working on it... but since it affected multiple apps, I thought it made sense to at least make you aware of it..
Comment 12 Michael Catanzaro 2014-03-18 23:39:22 UTC
(In reply to comment #10)
> Do you have a simple test application?

Coming right up
Comment 13 Michael Catanzaro 2014-03-19 01:49:50 UTC
Er, in my simple test application, it works. :)

Still trying to figure out what Nibbles and Sudoku are doing wrong, but let's ping-pong this back in the meantime.
Comment 14 Michael Catanzaro 2014-03-19 02:04:12 UTC
Created attachment 272343 [details]
Test application to demonstrate properly-functioning boolean menu item

A mutilated version of Application4 from the GTK+ tutorial, with the on_toggle_toggled() function copied straight from Nibbles. Compile:

gcc exampleapp.c `pkg-config --cflags --libs glib-2.0` `pkg-config --cflags --libs gtk+-3.0`
Comment 15 Michael Catanzaro 2014-03-19 02:23:17 UTC
Bleh, the only difference is that Nibbles has a callback connected to the action's change-state signal, but my test app doesn't:

"If no handler is connected to this signal then the default behaviour is to call g_simple_action_set_state() to set the state to the requested value. If you connect a signal handler then no default action is taken. If the state should change then you must call g_simple_action_set_state() from the handler."

We got unlucky by comparing with Sudoku, since I bet it made the same mistake.

(P.S. I still think a toggle item is not really a good long-term way to implement Pause.)
Comment 16 Michael Catanzaro 2014-03-19 03:16:33 UTC
Created attachment 272351 [details] [review]
Properly change the state of the pause action

We were failing to change the state of the pause action on state change
requests, so the Pause item in the app menu was forever set to false.

FYI: to push this before 3.12 requires emailing release-team@gnome.org and waiting for two approvals. Should be easy to get for a one-line patch. Or you could just wait until after 3.12 is released to push.
Comment 17 gQuigs 2014-03-19 05:06:11 UTC
Damn.. sorry about all this.. :/

>(P.S. I still think a toggle item is not really a good long-term way to
implement Pause.)
Yea... I'm thinking that one of the first things I want to fix properly in 3.13.. Would just having a tookbar pause button be much better?

I'm thinking this can wait until 3.12.1.. 

Oh, also confirming the patch works..
Comment 18 Michael Catanzaro 2014-03-19 13:44:32 UTC
(In reply to comment #17)
> Damn.. sorry about all this.. :/
> 
> >(P.S. I still think a toggle item is not really a good long-term way to
> implement Pause.)
> Yea... I'm thinking that one of the first things I want to fix properly in
> 3.13.. Would just having a tookbar pause button be much better?

No, I think we really don't want to add a toolbar. The designers don't want the games to have toolbars any more than menu bars. Toolbars aren't fun!

Nibbles has few enough buttons that we can put Pause directly in the window, next to a New Game button.  Look at how Tetravex (and several other games) does it in 3.12. I also want to move the scoreboard out of the status bar and into the window, similar to what Iagno does in 3.12, but hopefully nicer-looking than that.

> I'm thinking this can wait until 3.12.1.. 

Agreed.
Comment 19 Michael Catanzaro 2014-04-01 23:31:38 UTC
Attachment 272351 [details] pushed as 5fa8b7a - Properly change the state of the pause action