GNOME Bugzilla – Bug 733415
Rework Preferences dialog and add --level command-line option.
Last modified: 2014-07-21 12:53:21 UTC
Created attachment 281194 [details] [review] Switch notation As discussed on [1], there is a need to rework the Preferences dialog (and the way the settings work) in order to implement correctly jumplist. Here is a set of patches that does this, and some cleaning: * 0001 is a notation switch: using 2 for dark player (who plays first) was quite strange; * 0002 change Preferences dialog, as explained previously; * 0003 syncs the sound checkbox and the sound setting; * 0004 is a cleaning; * 0005 adds --level command-line option; * 0006 is a probably good thing, if I understood correctly. [1] https://bugzilla.gnome.org/show_bug.cgi?id=733224
Created attachment 281195 [details] [review] Change Preferences dialog
Created attachment 281196 [details] [review] Sync sound setting with Preferences checkbox
Created attachment 281197 [details] [review] Remove unused variable.
Created attachment 281198 [details] [review] Add --level command-line option.
Created attachment 281199 [details] [review] Respect gtk_list_store_set() syntax. Not sure it’s necessary… needs a check.
Review of attachment 281194 [details] [review]: Eh, I don't want to reject this since it's no worse than what was here before, but this should really be an enum. See Chess if you need an example of how to use enums in UI files; you can access them from the code as if they were strings.
Review of attachment 281195 [details] [review]: OK, minor changes only here: * Let's use enums instead of ints in the gschema file here as well. * For the Game mode combo, let's put "Two players" on the bottom of the list * Let's rename "Player plays first" to "Play first (Dark)" and "Player plays second" to "Play second (Light)" * I think we can rename "Computer's level" to simply "Computer" (aside: see [1] for the right apostrophe character) [1] https://wiki.gnome.org/Design/HIG/Typography
Review of attachment 281196 [details] [review]: Is this so that the checkbox gets changed properly if you change the setting manually while the game is running, or from the command line on a remote instance?
Comment on attachment 281197 [details] [review] Remove unused variable. OK, but I spotted this unused variable before I spotted this patch, so I already pushed an identical commit.
Review of attachment 281198 [details] [review]: Need to print a warning if the user selects an invalid level. Also, in the man page, mention whether 1 is the easiest level and 3 the hardest, or vice-versa.
Review of attachment 281199 [details] [review]: It's handled for us automatically; see how it's declared in the vapi: [CCode (sentinel = "-1")] public void @set (Gtk.TreeIter iter, ...);
Created attachment 281262 [details] [review] Use enum for play-as setting. Yeah, the enum is a good thing here.
Created attachment 281263 [details] [review] Change Preferences combos and the way settings work. I really don’t see why we should use an enum for the computer level: * we have to deal with it as an integer in the command-line; * we have to deal with it as an integer in the computer’s AI creation; * it doesn’t help to understand anything, “1” for “level 1” is clear; * a number is more universaly understood than an english word. I changed the other things. I’ll try to remember to use the normal apostrophe (bépo keyboard here…) in the code, even if I usually don’t use UTF-8 chars in it.
Created attachment 281264 [details] [review] Sync in Preferences dialog the main settings. > Is this so that the checkbox gets changed properly if you change the > setting manually while the game is running, or from the command line > on a remote instance? It permits to update the dialog (checkbox, 2 of the 3 comboboxes, the tileset is not needed and is hard to code…) when the settings is changed externaly, via dconf-editor, a gsettings command-line option, or another instance with command-line options like --level or --mute.
Created attachment 281265 [details] [review] Add --level command-line option.
Created attachment 281266 [details] [review] Add --first, --second and --two-players command-line options. Oh, two more things… ^^’
Created attachment 281267 [details] [review] Add jumplist.
Review of attachment 281262 [details] [review]: ok
Review of attachment 281263 [details] [review]: ok
Review of attachment 281264 [details] [review]: ok
Review of attachment 281265 [details] [review]: Sucks that we need that extra member variable, but we do
Review of attachment 281266 [details] [review]: ok ::: src/iagno.vala @@ +210,3 @@ + /* The game mode is set for the next game. */ + if (options.contains("second")) Missing a space here (and copypasted below) I'll commit with it fixed, since I'm releasing now
Review of attachment 281267 [details] [review]: OK
Thanks! Hope this jumplist works, cause I haven't tried it. I'm not even sure which version of GNOME Shell added jumplists.... Attachment 281262 [details] pushed as 8228c5b - Use enum for play-as setting. Attachment 281263 [details] pushed as c2da2d3 - Change Preferences combos and the way settings work. Attachment 281264 [details] pushed as ba7beaf - Sync in Preferences dialog the main settings. Attachment 281265 [details] pushed as f105c75 - Add --level command-line option. Attachment 281266 [details] pushed as fd02c26 - Add --first, --second and --two-players command-line options. Attachment 281267 [details] pushed as ff5b1ab - Add jumplist.
For future reference, the actions list should end in a semicolon
Thank’s! Jumplists are here frome 3.11.5-3.12[1], it’s quite new. Concerning the Actions list, if I read correctly[2], it’s optionnal. [1] https://bugzilla.gnome.org/show_bug.cgi?id=669603 [2] http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s03.html
Eh, OK, must be a desktop-file-validate bug then... I don't care enough to report it rather than just add a semicolon :)