GNOME Bugzilla – Bug 655761
Promotion type selection shouldn't be a setting but an in-game choice.
Last modified: 2012-12-20 02:52:08 UTC
when a pawn is promoted, the user should be able to choose the type of piece it is promoted to (according to chess rules). Currently, it's only possible from settings, which doesn't allow a use during the game. This might be done using a dialog asking the user to choose the piece for the promotion (queen, rook, bishop or knight). This dialog shouldn't hide the chess board to allow the user to analyze the situation before he choose the piece. A GtkInfoBar might be used. The Promotion type option has then to be removed from the setting window.
Right, thanks for bringing this to notice. Robert, do we want that as a pop up as soon as a pawn reaches the 8th level?
also, can that choice be different for both players and for every pawn which is promoted?
quoting Wikipedia: " In some fairy chess variants, promotions to pieces of the opponent's color are also possible" Do we want to consider that too?
Yes, every time a pawn reaches the 8th level we need to prompt you can choose which piece to convert the pawn to. We only support the standard FIDE rules (http://www.fide.com/component/handbook/?id=124&view=article).
Ok, I'll modify my branch keeping this in mind.
I broke this bit off and pushed here: https://gitorious.org/glassrose-gnome/gnome-games/commits/glchess-in-game-promotion-type-selector-655761 I have my other work rebased on top of it so I can merge this as soon as reviewed.
Notes: Icons to load need this fixed: https://bugs.freedesktop.org/show_bug.cgi?id=48264 (until then you may manually have to edit your build of shared-mime-info/or edit the svg image files.) Also, you may ignore warnings like: Gtk-WARNING **: Unknown property: GtkGrid.n-rows (going by follow-ups here- https://mail.gnome.org/archives/gtk-app-devel-list/2011-September/msg00004.html)
Chandni, can you convert this patch using git format-patch and attach here? Then it is easier to review.
Created attachment 213158 [details] [review] [glchess] Install private themable icons for promotion types In dir: $(DESTDIR)$(pkgdatadir)/icons/hicolor/scalable/actions Fixes:
Created attachment 213159 [details] [review] [glchess] Prompt user for selecting promotion type when a pawn reaches 8th rank Fixes:
Created attachment 213160 [details] [review] [glchess] Stop promotion-type selector from appearing on loading moves Fixes:
Review of attachment 213158 [details] [review]: The icons seem to be missing from the patch so I can't review them. However, it is better to render them in-game so they look exactly the same as in the 2D version. I'm happy to accept the patch when working with the icons but the long term solution would be better to use the same images.
Review of attachment 213159 [details] [review]: The UI for picking the promotion type seems good, but it needs some work on structuring how this UI is initiated and where the code for it lives. ::: glchess/src/Makefile.am @@ +57,2 @@ glchess_CFLAGS = \ + -DGNOMEGAMESDATADIR=\"@datadir@/gnome-games\" \ Why are there files installed into /usr/share/gnome-games? All the data files for chess should be in /usr/share/glchess ::: glchess/src/chess-game.vala @@ +249,3 @@ public class ChessState { + GLib.Settings settings = new GLib.Settings ("org.gnome.glchess.Settings"); This class should not load the settings - the settings should be controlled externally to this class. In fact, this file should not perform any UI at all - it only contains the chess game state. The code that enters the move from the UI should block until it gets the promotion type to provide to the chess game. I'd expect to see chess-view-2d.vala / chess-view-3d.vala get the promotion type (probably using a signal that is intercepted in glchess.vala)
Review of attachment 213160 [details] [review]: You can't put global variables to work out a synchronisation issue. Needs rethinking. ::: glchess/src/chess-game.vala @@ +1273,3 @@ + +public bool is_loaded = true; Global variable - a sign something is wrong.
(In reply to comment #12) > Review of attachment 213158 [details] [review]: > > The icons seem to be missing from the patch so I can't review them. However, > it is better to render them in-game so they look exactly the same as in the 2D > version. I'm happy to accept the patch when working with the icons but the > long term solution would be better to use the same images. I took the same icons as provided in the directory of the Makefile edited (i.e. glchess/data/pieces/fancy/). No new ones.
(In reply to comment #13) > Review of attachment 213159 [details] [review]: > > The UI for picking the promotion type seems good, but it needs some work on > structuring how this UI is initiated and where the code for it lives. > > ::: glchess/src/Makefile.am > @@ +57,2 @@ > glchess_CFLAGS = \ > + -DGNOMEGAMESDATADIR=\"@datadir@/gnome-games\" \ > > Why are there files installed into /usr/share/gnome-games? All the data files > for chess should be in /usr/share/glchess OK, I followed https://live.gnome.org/ThemableAppSpecificIcons. It expects me to install them in ${prefix}/share/<appname>/icons/hicolor/. I thought that would make it look more integrated with gnome-games? Isn't "gnome-games" the appname? Are we allowed to install glchess out of it too? I'll fix the remaining issues.
I have modified my work to fit the needs and changed the installation directory. Hopefully it looks better now.
I have modified my work to fit the needs and changed the installation directory. Hopefully it looks better now. Patches follow
Created attachment 213511 [details] [review] [glchess] Install private themable icons for promotion types In dir: $(DESTDIR)$(pkgdatadir)/icons/hicolor/scalable/actions Fixes: 655761
Created attachment 213512 [details] [review] [glchess] Prompt user for selecting promotion type when pawn reaches 8th rank Fixes: 655761
Created attachment 213513 [details] [review] [glchess] No need of promotion-type combo box and related settings Fixes: 655761
Created attachment 224003 [details] [review] Prompt user for selecting promotion type when pawn reaches 8th rank I've taken your patches and merged them together and made them apply against master so they're easier to review.
Review of attachment 224003 [details] [review]: Patch looks generally good, I'd just like the following changes: - Encode the promotion type into the move string - Update the patch so the changes to preferences.ui aren't so complicated. - Use the theme icons instead of installing your own copies. In the case of 3D view just use the default 2D theme. This means the icons are consistent with the theme (which they won't be in the current patch) and it makes the patch simpler. - Minor whitespace changes: - Use correct indentation - Don't limit to 80 characters I've accidentally dropped promotion-type-selector.ui from the patch, but it looks fine. Please include it in any updated patch. ::: glchess/data/preferences.ui @@ +2,3 @@ <interface> <requires lib="gtk+" version="2.16"/> + <object class="GtkListStore" id="custom_duration_units_model"> As you can see there are a huge number of changes in the UI dialog due to the way Glade works (which is very annoying). You really need to manually adjust this patch so these changes are minimised. In the current patch it's almost impossible to see what has changed. For example, manually moving the custom_duration_units_model back to the bottom of the file to match the old version would start to make this simpler. If the changes are caused by a more recent version of Glade adjusting the file can you do a separate patch that updates the current version, then this patch will be simpler. ::: glchess/src/chess-game.vala @@ +10,3 @@ public signal void start_turn (); + public signal bool do_move (string move, bool apply, PieceType + promotion_type = PieceType.QUEEN); You don't have to limit the lines to 80 characters, please leave them on one line. ::: glchess/src/chess-scene.vala @@ +208,3 @@ + PieceType promotion_selection = choose_promotion_type (); + game.current_player.move_with_coords (selected_rank, + selected_file, rank, file, true, promotion_selection); You don't need a separate parameter for promotion type. The move string can contain the promotion type using '=', e.g. a7a8=N This simplifies a lot of the other changes in chess-game.vala @@ +213,2 @@ if (game.current_player.move_with_coords(selected_rank, selected_file, rank, file)) + selected_rank = selected_file = -1; You shouldn't have random whitespace changes in patches. ::: glchess/src/glchess.vala @@ +198,3 @@ + if (pixbuf != null) + ((Gtk.Image) image_bishop).set_from_pixbuf (pixbuf); + } This line is indented 9 characters... @@ +1738,3 @@ + (Config.PKGDATADIR, Path.DIR_SEPARATOR_S, "icons")); + + You have two empty lines here instead of 1.
(In reply to comment #23) > Review of attachment 224003 [details] [review]: > > Patch looks generally good, I'd just like the following changes: > - Encode the promotion type into the move string Hey, I am sure I had tried that and there was some major unexpected behaviour which took me a lot of time to debug in this approach because of move string could be modified by both robot or local users, if I remember right! To resolve that I had to make a distinction between the promotion-type in move string and the one explicitly selected by user. I had to take the approach as the following in ChessState.move () function and I attempted to document it there too: public bool move (string move, bool apply = true, 497 PieceType promotion_type_selected = PieceType.QUEEN) 498 { 499 int r0, f0, r1, f1; 500 PieceType promotion_type; 501 502 if (!decode_move (current_player, move, out r0, out f0, out r1, out f1, out promotion_type)) 503 return false; 504 505 if (promotion_type_selected != PieceType.QUEEN) 506 { 507 /* Promotion type selected is not the default means it was selected by 508 * the user and user selected move does not specify it either */ 509 promotion_type = promotion_type_selected; 510 } 511 512 if (!move_with_coords (current_player, r0, f0, r1, f1, promotion_type, apply)) 513 return false; 514 515 return true; 516 } To do it the way one would have done in the first sight would have meant that I would have needed to change your current design to provide me information I needed. I took the simpler way. I thought you would not want me to create additional properties or do other bulky design changes for a trivial thing. Unfortunately, rethinking the design again would require some time so if it's working fine for you, can we postpone this to the next cycle or create a separate bug unblocking this one? > - Update the patch so the changes to preferences.ui aren't so complicated. Done! and that has already created a havoc in all branches based on this one. I also hate glade for misbehaving so much! > - Use the theme icons instead of installing your own copies. These are the icons already provided by games. They are just not themable presently. I just needed to have them iinstalled in the correct place for them to be themable. In the case of 3D > view just use the default 2D theme. Right, If you find them more pretty! done! > This means the icons are consistent with > the theme (which they won't be in the current patch) and it makes the patch > simpler. None of the pieces icons are themable right now. My corrected patch makes the 2D peices themable. > - Minor whitespace changes: > - Use correct indentation > - Don't limit to 80 characters Checked. > > I've accidentally dropped promotion-type-selector.ui from the patch, but it > looks fine. Please include it in any updated patch. > > ::: glchess/data/preferences.ui > @@ +2,3 @@ > <interface> > <requires lib="gtk+" version="2.16"/> > + <object class="GtkListStore" id="custom_duration_units_model"> > > As you can see there are a huge number of changes in the UI dialog due to the > way Glade works (which is very annoying). You really need to manually adjust > this patch so these changes are minimised. In the current patch it's almost > impossible to see what has changed. > > For example, manually moving the custom_duration_units_model back to the bottom > of the file to match the old version would start to make this simpler. Yeah, moved all models to the bottom. > > If the changes are caused by a more recent version of Glade adjusting the file > can you do a separate patch that updates the current version, then this patch > will be simpler. I tried all approaches. nothing seems to work. It was very difficult to work with the UI. > > ::: glchess/src/chess-game.vala > @@ +10,3 @@ > public signal void start_turn (); > + public signal bool do_move (string move, bool apply, PieceType > + promotion_type = PieceType.QUEEN); > > You don't have to limit the lines to 80 characters, please leave them on one > line. My tiny monitor screen makes working on longer lines so difficult! Done nevertheless to keep up with the format of the file! > > ::: glchess/src/chess-scene.vala > @@ +208,3 @@ > + PieceType promotion_selection = choose_promotion_type (); > + game.current_player.move_with_coords (selected_rank, > + selected_file, rank, file, true, promotion_selection); > > You don't need a separate parameter for promotion type. The move string can > contain the promotion type using '=', e.g. > a7a8=N > This simplifies a lot of the other changes in chess-game.vala Changing design and redebugging would require sometime for good testing again as stated above. We can figure the exact issue out later, can't we? > > @@ +213,2 @@ > if (game.current_player.move_with_coords(selected_rank, > selected_file, rank, file)) > + selected_rank = selected_file = -1; > > You shouldn't have random whitespace changes in patches. Checked! > > ::: glchess/src/glchess.vala > @@ +198,3 @@ > + if (pixbuf != null) > + ((Gtk.Image) image_bishop).set_from_pixbuf (pixbuf); > + } > > This line is indented 9 characters... > Oops. Corrected. > @@ +1738,3 @@ > + (Config.PKGDATADIR, Path.DIR_SEPARATOR_S, "icons")); > + > + > > You have two empty lines here instead of 1. Corrected. Following are corrected patches.
yes, it should be fine now! I hope you'll like the branch.
Created attachment 224751 [details] [review] [glchess] Install private themable icons for promotion types In dir: $(DESTDIR)$(datadir)/glchess/icons/hicolor/scalable/actions Fixes:
Created attachment 224752 [details] [review] [glchess] Prompt user for selecting promotion type when the pawn reaches 8th rank Fixes:
Created attachment 224753 [details] [review] [glchess] No need of promotion-type combo box and related settings
Created attachment 224754 [details] [review] [glchess] SVG correction: Move <svg> tags above licencing info Workaround for https://bugs.freedesktop.org/show_bug.cgi?id=48264
(In reply to comment #24) > (In reply to comment #23) > > Review of attachment 224003 [details] [review] [details]: > > > > Patch looks generally good, I'd just like the following changes: > > - Encode the promotion type into the move string > > Hey, I am sure I had tried that and there was some major unexpected behaviour > which took me a lot of time to debug in this approach because of move string > could be modified by both robot or local users, if I remember right! To resolve > that I had to make a distinction between the promotion-type in move string and > the one explicitly selected by user. > > I had to take the approach as the following in ChessState.move () function and > I attempted to document it there too: > > public bool move (string move, bool apply = true, > 497 PieceType promotion_type_selected = PieceType.QUEEN) > 498 { > 499 int r0, f0, r1, f1; > 500 PieceType promotion_type; > 501 > 502 if (!decode_move (current_player, move, out r0, out f0, out r1, > out f1, out promotion_type)) > 503 return false; > 504 > 505 if (promotion_type_selected != PieceType.QUEEN) > 506 { > 507 /* Promotion type selected is not the default means it was > selected by > 508 * the user and user selected move does not specify it either > */ > 509 promotion_type = promotion_type_selected; > 510 } > 511 > 512 if (!move_with_coords (current_player, r0, f0, r1, f1, > promotion_type, apply)) > 513 return false; > 514 > 515 return true; > 516 } > > To do it the way one would have done in the first sight would have meant that I > would have needed to change your current design to provide me information I > needed. I took the simpler way. I thought you would not want me to create > additional properties or do other bulky design changes for a trivial thing. > > Unfortunately, rethinking the design again would require some time so if it's > working fine for you, can we postpone this to the next cycle or create a > separate bug unblocking this one? > Yes! If I recall right, the issue was that the promotion-type-selector dialog was getting displayed in case we loaded an unfinished game in which we already had made a move which required us to select a promotion-piece. If we encode the piece in the move string, and we load the game from a saved file, it will get parsed exactly and will cause the promotion type selector to show up which will be unexpected since we actually don't want to make a move. We are just loading what we already played! Please let me know if these set of patches are good to go!
Hello! I won't mind if this work gets into master for 3.7 along with my networking branch which depends on this work and comes with a revolutionary design. But still I love reviews and my mistakes being pointed out so give them all to me so that this work looks truly profesional! Eagerly waiting.. Do you want me to drop in additional comments in source explaining the codewalk?
Review of attachment 224751 [details] [review]: As stated before, please use the existing images, not install new images.
Review of attachment 224752 [details] [review]: Still need to fix the following issues... ::: glchess/src/chess-game.vala @@ +28,2 @@ { string move = "%c%d%c%d".printf ('a' + f0, r0 + 1, 'a' + f1, r1 + 1); Here you should do: switch (promotion_type) { case PieceType.Queen; /* Default is queen so don't add anything */ break; case PieceType.Rook: move += "=R"; break; etc... } ::: glchess/src/glchess.vala @@ +159,3 @@ + try + { + Gtk.IconTheme icon_theme = Gtk.IconTheme.get_default (); As stated, you should use the existing images from the 2D view. Since this is delaying this patch, just use buttons with the piece names inside them for now and we can update this with a following patch for the icons.
Review of attachment 224753 [details] [review]: This is correct, but should really be merged with the other changes into one patch.
Review of attachment 224754 [details] [review]: Looks good, committed. Thanks Chandni!
(In reply to comment #32) > Review of attachment 224751 [details] [review]: > > As stated before, please use the existing images, not install new images. Yes, I can do that. It's not that I can't use those images but those wouldn't be themable in that directory I read. But that can be done separately yes. I'll postpone that patch. (In reply to comment #33) > Review of attachment 224752 [details] [review]: > > Still need to fix the following issues... > > ::: glchess/src/chess-game.vala > @@ +28,2 @@ > { > string move = "%c%d%c%d".printf ('a' + f0, r0 + 1, 'a' + f1, r1 + 1); > > Here you should do: > > switch (promotion_type) > { > case PieceType.Queen; > /* Default is queen so don't add anything */ > break; > case PieceType.Rook: > move += "=R"; > break; > etc... > } > I didn't add that line. Are you sure you want me to edit it? > ::: glchess/src/glchess.vala > @@ +159,3 @@ > + try > + { > + Gtk.IconTheme icon_theme = Gtk.IconTheme.get_default (); > > As stated, you should use the existing images from the 2D view. > Since this is delaying this patch, just use buttons with the piece names inside > them for now and we can update this with a following patch for the icons. No, no delaying. I just didn't know we wanted to postpone having these images themable or can do without them. Will update shortly.
Hi! I see the existing images for piece icons which are installed in $(datadir)/glchess/pieces/simple are used by calling Rsvg.Handle.from_file (Path.build_filename (Config.PKGDATADIR, "pieces", scene.theme_name, name + ".svg", null)); or new TDSModel (File.new_for_path (Path.build_filename (Config.PKGDATADIR, "pieces", "3d", "knight.3ds", null))); But I cannot use images installed in $(datadir)/glchess/pieces/simple by just providing their names (like I did in glchess.vala): Gdk.Pixbuf pixbuf = icon_theme.load_icon (queen_icon, 80, Gtk.IconLookupFlags.GENERIC_FALLBACK); where queen_icon = "whiteQueen"; or "blackQueen" depending on player colour. The advantage here besides having these UI images themable is that I don't have to hardcode their path explicitly. These can actually even be different from the theme being used in the game. According to https://live.gnome.org/ThemableAppSpecificIcons: "You have to install application-specific named icons in a special location in filesystem. This location is ${prefix}/share/<appname>/icons/hicolor/<size>/<context>" (In reply to comment #32) > Review of attachment 224751 [details] [review]: > > As stated before, please use the existing images, not install new images. I have already modified that patch to include the images from the 2D view like you wanted. There are no new images involved just a new installation dir. Is it that you are not able to get the 2D images loaded as you already have 3D images loaded with these names in the defalt icon theme? That could be tricky. This happened with me because of the previous patches and was solved only after I made some change to glchess.vala and recompiled. Or are you getting a warning like: "Failed to load image: Icon 'whiteQueen' not present in theme" I saw this error with no image displayed when I switched my installation directory to a new one and it was rectified only after I did a : $jhbuild build -acn librsvg If its an "Unrecognized image file format" even on using Gtk.IconLookupFlags.FORCE_SVG in icon_theme.load_icon(), you might not be having librsvg installed in the same prefix as the gdk-pixbuf in use. The above command would fix that. It would be helpful if you could confirm if you are able to produce the 2D images or not or if you get some warning while we are at it. I am not quite acquainted to the rsvg library or any other way to load images but I will dig in if that's necessary. I am seeing 2D icons getting loaded. Attaching screenshots for you to see if they are as expected. If you still are unable to get them loaded, we'll unfortunately have to let it go imageless.. (In reply to comment #36) > > Here you should do: > > > > switch (promotion_type) > > { > > case PieceType.Queen; > > /* Default is queen so don't add anything */ > > break; > > case PieceType.Rook: > > move += "=R"; > > break; > > etc... > > } > > > This is working now. Donno what changed meanwhile but it helped. Updated: https://gitorious.org/glassrose-gnome/gnome-games/commits/glchess-in-game-promotion-type-selector-655761-latest
Created attachment 230727 [details] 2D white icons
Created attachment 230728 [details] 2D black icons
Created attachment 230729 [details] [review] [glchess] Prompt user for selecting promotion type when the pawn reaches 8th rank Fixes:
Review of attachment 230729 [details] [review]: Hi Chandni, I've fixed up this patch to apply to the gnome-chess repository and applied it. I've made a second patch immediately that loads the icons directly from the files (i.e. so they are not treated as an icon theme) and uses the correct theme. Thanks! ::: glchess/src/glchess.vala @@ +1745,3 @@ + Gtk.IconTheme icon_theme = Gtk.IconTheme.get_default (); + icon_theme.append_search_path ("%s%s%s".printf + (Config.PKGDATADIR, Path.DIR_SEPARATOR_S, "icons")); Path.build_filename would have been simpler here. This is not the valid location of the icons.
If you like that, well, its well! :) Thanks! Do let me know if we can have it merged to master.
Also, it would be great it you could confirm the other commits in the branch at the end of in Comment 37 above for fitness too.
It is in master: http://git.gnome.org/browse/gnome-chess/commit/?id=7e16e130a1434a901da0cd6b91ead1744b9ab6dc
Thanks, Oh! \o/
(In reply to comment #34) > Review of attachment 224753 [details] [review]: > > This is correct, but should really be merged with the other changes into one > patch. For this left-out patch, I'd like to have it merged directly to master. There seems to be no other place to fit it in. Is there anything to do more here?
I have updated master. Closing finally. This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.