GNOME Bugzilla – Bug 746218
Fix bad pgn load behaviour
Last modified: 2015-05-03 17:28:57 UTC
Cases: 1. Invalid move detected in pgn Current behaviour: => Warning printed and some moves go missing, and its pretty random. Suggesting behaviour: => print warning "Invalid move in pgn: MOVE, Can't load this game" => Cancel game load 2. Invalid Black/White time remaining Current behaviour: => Timer set to 00:59 Suggesting behaviour: => Print warning "Invalid time remaining for player [black/white] in pgn : TIME, Setting timer to infinity" => Set timer to INFINITY for both players 3.Invalid timer increment Current behaviour: => Something random happens, sometimes random time increments, sometimes 0 increment. Suggesting behaviour: =>print warning "Invalid timer increment in pgn : TIMER_INCREMENT, using a simple clock." => Change clock type to simple and don't insert timer increment tag in game.tags. 4. Invalid Time //harmless Current behaviour: => Silent with no warning. Suggesting behaviour: => print warning "Invalid Time in pgn : TIME, setting to current time." => Set tag to current time. 5. Invalid Date //harmless Current behaviour: => Silent with no warning. Suggesting behaviour: => print warning "Invalid Date in pgn : DATE" => Set tag to "??" 6. Invalid Result => Correct result is derived from the moves. 7. Invalid TimeControl => Correct behaviour, we only support simple timeouts, if this gets set to 0 we use a clock with infinite time. Nothing can be done for these: //all are harmless ---------------------------------- 7. Event: the name of the tournament or match event. 8. Site: the location of the event. 9. Round: the playing round ordinal of the game within the event. 10. White: the player of the white pieces, in "last name, first name" format. 11. Black: the player of the black pieces, same format as White. Ref for tags: http://en.wikipedia.org/wiki/Portable_Game_Notation http://www.saremba.de/chessgml/standards/pgn/pgn-complete.htm
I guess this is a good opportunity to enhance test-chess-pgn.vala. The more automated tests we have, the better. (In reply to Sahil Sareen from comment #0) > 1. Invalid move detected in pgn > Current behaviour: > => Warning printed and some moves go missing, and its pretty random. > > Suggesting behaviour: > => print warning "Invalid move in pgn: MOVE, Can't load this game" > => Cancel game load If you're going to cancel the game load (which I agree you should do), I think we need an actual in-game dialog to warn the player of what's happening. "This doesn't look like a valid chess game" or something like that. (I think we need that for bug #704990, too.) Then the command-line warning can contain the extra you propose. > 4. Invalid Time //harmless > Current behaviour: > => Silent with no warning. > > Suggesting behaviour: > => print warning "Invalid Time in pgn : TIME, setting to current time." > => Set tag to current time. > > 5. Invalid Date //harmless > Current behaviour: > => Silent with no warning. > > Suggesting behaviour: > => print warning "Invalid Date in pgn : DATE" > => Set tag to "??" Hm, I think for these two we should just leave them blank. I agree on all the rest.
Thanks for reviewing these Michael!!! (In reply to Michael Catanzaro from comment #1) > I guess this is a good opportunity to enhance test-chess-pgn.vala. The more > automated tests we have, the better. > > (In reply to Sahil Sareen from comment #0) > > 1. Invalid move detected in pgn > > Current behaviour: > > => Warning printed and some moves go missing, and its pretty random. > > > > Suggesting behaviour: > > => print warning "Invalid move in pgn: MOVE, Can't load this game" > > => Cancel game load > > If you're going to cancel the game load (which I agree you should do), I > think we need an actual in-game dialog to warn the player of what's > happening. "This doesn't look like a valid chess game" or something like > that. (I think we need that for bug #704990, too.) Then the command-line > warning can contain the extra you propose. Agree. This would be better. Thanks :D > > > 4. Invalid Time //harmless > > Current behaviour: > > => Silent with no warning. > > > > Suggesting behaviour: > > => print warning "Invalid Time in pgn : TIME, setting to current time." > > => Set tag to current time. > > > > 5. Invalid Date //harmless > > Current behaviour: > > => Silent with no warning. > > > > Suggesting behaviour: > > => print warning "Invalid Date in pgn : DATE" > > => Set tag to "??" > > Hm, I think for these two we should just leave them blank. I can't imply what you exactly prefer here: "leave them blank" = ( Keep them as they were and/or ignore with no warnings ) OR ( Replace with "" )? Keeping them as they were would pass on the invalid values to the pgn that the player saves later. For 'Unknown date' the standard[1][2] pgn behaviour suggests to put "??", though it doesn't say anything for 'Time'. ---- [1]http://en.wikipedia.org/wiki/Portable_Game_Notation Quoting the necessary: Date: the starting date of the game, in YYYY.MM.DD form. "??" are used for unknown values. [2]http://www.saremba.de/chessgml/standards/pgn/pgn-complete.htm#c8.1.1 Quoting the necessary: 8.1.1.3: The Date tag The Date tag value gives the starting date for the game. (Note: this is not necessarily the same as the starting date for the event.) The date is given with respect to the local time of the site given in the Event tag. The Date tag value field always uses a standard ten character format: "YYYY.MM.DD". The first four characters are digits that give the year, the next character is a period, the next two characters are digits that give the month, the next character is a period, and the final two characters are digits that give the day of the month. If the any of the digit fields are not known, then question marks are used in place of the digits. Examples: [Date "1992.08.31"] [Date "1993.??.??"] [Date "2001.01.01"]
My thinking was that since we don't need those fields at all, we can just ignore them. I mean, there's no point in checking dates for validity when we don't use the field for anything.
(In reply to Michael Catanzaro from comment #3) > My thinking was that since we don't need those fields at all, we can just > ignore them. I mean, there's no point in checking dates for validity when we > don't use the field for anything. Ok, Got it! Thanks. :D
Created attachment 300537 [details] [review] Fix bad pgn load behaviour This patch is for 3.16 So it doesn't include the fix for incorrect moves for which we need a dialog.
Review of attachment 300537 [details] [review]: Patch looks good, though I haven't tested it to see what might break. For the version going into master, let's mark those strings for translation. ::: lib/chess-pgn.vala @@ +257,3 @@ + if (game.tags["WhiteTimeLeft"] != null && + game.tags["BlackTimeLeft"] != null) + game.tags.insert (tag_name, tag_value); Nit: either add extra braces here, or write the condition in one line, so that it's easier to see the difference between the condition and the action. (Since the indentation is the same.)
Created attachment 300538 [details] [review] Fix bad pgn load behaviour (mcatanzaro) Putting the conditions in a single line so its easier to see the difference between the condition and the action.
Pushed into 3.16 To ssh://ssareen@git.gnome.org/git/gnome-chess e9d862c..0e9eaa8
Leaving this bug open because these are left: 1. Invalid move detected in pgn Current behaviour: => Warning printed and some moves go missing, and its pretty random. Suggesting behaviour: => print warning "Invalid move in pgn: MOVE, Can't load this game" => Cancel game load 2. Mark new strings for translation.
Created attachment 300902 [details] [review] Marking strings for translation
Review of attachment 300902 [details] [review]: For all of the makefile changes, please use tab characters rather than eight spaces. Other than that, this is good!
(In reply to Michael Catanzaro from comment #11) > Review of attachment 300902 [details] [review] [review]: > > For all of the makefile changes, please use tab characters rather than eight > spaces. Other than that, this is good! Corrected and pushed to master. To ssh://ssareen@git.gnome.org/git/gnome-chess a5e3ecc..ba7a105
Created attachment 302291 [details] [review] Fix loading PGN with invalid moves Thats the last of what was left in this bug, Don't load a pgn with invalid moves.
Review of attachment 302291 [details] [review]: Looks good! ::: lib/chess-game.vala @@ +88,3 @@ } + public ChessGame (string fen = STANDARD_SETUP, string[]? moves = null, ref bool invalid_move) Hm, let's throw an exception instead.... @@ +102,1 @@ warning ("Invalid move %s", moves[i]); Then we can make this translatable and use it as the error message to the exception.... ::: src/gnome-chess.vala @@ +2170,3 @@ + Gtk.MessageType.ERROR, + Gtk.ButtonsType.NONE, + _("Invalid move in PGN, starting a new game.")); ...and display it here, so the user can see exactly what's wrong. Instead of starting a new game, I would just cancel and leave things as-is, if that's easy to do.
(In reply to Michael Catanzaro from comment #14) > Review of attachment 302291 [details] [review] [review]: > > Looks good! > > ::: lib/chess-game.vala > @@ +88,3 @@ > } > > + public ChessGame (string fen = STANDARD_SETUP, string[]? moves = null, > ref bool invalid_move) > > Hm, let's throw an exception instead.... Sure, I'll do that. > > @@ +102,1 @@ > warning ("Invalid move %s", moves[i]); > > Then we can make this translatable and use it as the error message to the > exception.... Okay, The only thing that bothers me here is if the invalid move is too long, it will make the dialog too big. Is that good ? > > ::: src/gnome-chess.vala > @@ +2170,3 @@ > + > Gtk.MessageType.ERROR, > + > Gtk.ButtonsType.NONE, > + _("Invalid move in > PGN, starting a new game.")); > > ...and display it here, so the user can see exactly what's wrong. > > Instead of starting a new game, I would just cancel and leave things as-is, > if that's easy to do. We already loose all the state until we get to the point when the pgn parser finds out that it is bad, Though possible it would clutter the code quite a bit.
Created attachment 302727 [details] [review] Fix loading PGN with invalid moves *(mcatanzaro) Change to throwing exceptions instead of passing by reference. *(mcatanzaro) Make dialog message translatable.
Review of attachment 302727 [details] [review]: ::: lib/chess-game.vala @@ +12,3 @@ +public errordomain PGNMoveError +{ + MOVE_ERROR How about reusing PGNError.LOAD_ERROR? @@ +105,3 @@ if (!do_move (current_player, moves[i], true)) + { + throw new PGNMoveError.MOVE_ERROR (_("Can't load PGN, Starting a new game\nInvalid move %s"), moves[i]); Hm, let's avoid newlines in the message dialog, and don't forget the translator comment: /* Message when the game cannot be loaded due to an error in the file. */ throw new PGNError.LOAD_ERROR (_("Failed to load PGN: move %s is invalid")); GTK+ will deal with wrapping the text as well as possible, but if the move is impressively long it will indeed look bad. ::: src/gnome-chess.vala @@ +459,3 @@ + { + run_invalid_move_dialog (e.message); + start_new_game (); I don't see why you start a new game here? Why do anything? If GNOME Chess has just been launched and is autoloading a game, then we do need to start a new game. Otherwise, I don't see the need to do anything.
(In reply to Michael Catanzaro from comment #17) > Review of attachment 302727 [details] [review] [review]: > ::: src/gnome-chess.vala > @@ +459,3 @@ > + { > + run_invalid_move_dialog (e.message); > + start_new_game (); > > I don't see why you start a new game here? Why do anything? > > If GNOME Chess has just been launched and is autoloading a game, then we do > need to start a new game. Otherwise, I don't see the need to do anything. If I don't do anything it would leave the game in an unplayable state. If you look at src/gnome-chess.vala activate() ->autosave_filename updated ->game_file updated ->load_game() ===>pgn_game updated ===>start_game() ----->model updated ----->moves updated ----->Try parsing the moves => Problem? Boom!!! We get to parsing the moves as the last step until when all the game variables have lost their respective state. One thing that I could do is to make a copy of the autosave file and add a function like restore_game_state() where I can load_game(). The player already confirms "Abandon Game" in the load game dialog, so I thought that starting a new game would be fine.
OK, that's fine then. No need to mention it in the message dialog: it should be clear enough that no game has loaded because it failed.
Created attachment 302808 [details] [review] Fix loading PGN with invalid moves * (mcatanzaro) Reuse PGNError.LOAD_ERROR instead of defining a new MOVE_ERROR. * (mcatanzaro) Change dialog message.
Comment on attachment 302808 [details] [review] Fix loading PGN with invalid moves Thanks for iterating. :) One more suggestion: "Failed to load PGN: Move %s is invalid." ^ I prefer to lowercase the first word after a colon, especially since it's not a sentence.
Yipee! Thats the end of this bug! :D
(In reply to Michael Catanzaro from comment #21) > Comment on attachment 302808 [details] [review] [review] > Fix loading PGN with invalid moves > > Thanks for iterating. :) One more suggestion: > > "Failed to load PGN: Move %s is invalid." > > ^ I prefer to lowercase the first word after a colon, especially since it's > not a sentence. Corrected and Pushed to master. To ssh://ssareen@git.gnome.org/git/gnome-chess 3b54bf4..16037c4 master -> master