PGNDATABASE - An idea at a chess pgn database double-click to load games #3
Conversation
There was a problem hiding this comment.
The feature looks useful, and the implementation looks good as a proof-of-concept. However, to merge this, some adjustments should be made.
Considering the code:
- Please follow the code style and the naming conventions (for variable names, unit names, method names, etc.). Also, it's a good idea to reformat your code (
Code > JEDI Code Formatin Lazarus) - Also, see all the comments in the review
- I didn't dive into the details on how the rendering part works, though
Considering the UI:
- All the other UI things are integrated into the main window, see
PseudoDockedForms. It would be good to integrate the PGN selection window also, but it's not a necessary part. - It crashes under Qt5 widgetset, though I didn't have time to investigate the exact reasons, unfortunately
- It seems that resizing the window doesn't work properly, at least under GTK2 widgetset
- On the grid itself, it would be good to indicate somehow, which game is currently loaded
- The rendering is slow because the grid with the games contains too many information (all the moves, for example). Also, I don't think that the entire move sequence and the entire PGN tags must be shown in the grid (including line breaks).
| protected | ||
| procedure AddWindow(AForm: TApplicationForm; APanel: TCustomPanel; | ||
| ASplitter: TSplitter; CanHide: boolean); | ||
| public | ||
| FGame: TChessGame; |
There was a problem hiding this comment.
It is not a good idea to promote the field from private to public. Passing either a callback to change the current game or a reference to TChessGame into the new form is acceptable, though.
| on E: Exception do | ||
| MessageDlg(E.Message, mtError, [mbOK], 0); | ||
| end; | ||
| SList.Free(); |
There was a problem hiding this comment.
// create SList
try
// do something
finally
FreeAndNil(SList);
end;| RegexObj.Expression := '((((\[.*?\])\s*)+)(1\..+?(0-1|1-0|1/2-1/2)))'; | ||
| if dlgOpenPGN.Execute() then | ||
| begin | ||
| frmPGNDatabase.Show(); |
There was a problem hiding this comment.
Using () after function names is not required in Pascal, and is never used in Chess256.
| @@ -203,6 +208,77 @@ procedure TMainForm.AboutActionExecute(Sender: TObject); | |||
| AboutBox.ShowModal; | |||
| end; | |||
|
|
|||
| procedure TMainForm.actLoadPGNFileExecute(Sender: TObject); | |||
There was a problem hiding this comment.
- It is not a good idea to mix GUI creation and parsing in one function. Parsing should go to some other unit (like
NotationLists.pas,ChessNotation.pasand the related things are done). GUI creation should go to the new form unit itself instead of creating everything in the main form. - I don't think that splitting PGNs with regular expressions is reliable. While Chess256 parser currently assumes that there is only one PGN in the file, the splitter ideally should use this parser, without relying on regular expressions.
| for i := 0 to SList.Count-1 do | ||
| begin | ||
| s2 := SList[i]; | ||
| s += s2 + #13#10; |
There was a problem hiding this comment.
Never use #13#10. There is a LineEnding constant AFAIR.
| end; | ||
|
|
||
| try | ||
| CreateNewGame(); |
There was a problem hiding this comment.
AFAIR CreateNewGame will just create the new game with the current new game dialog settings. So, if you are not in the analysis mode now, then the following will happen:
- the current game will be restarted (not in analysis mode)
- in the game mode, all the attempts to replace the current notation will be disallowed
So, it's necessary to disallow using this in the analysis mode.
| procedure TfrmPGNDatabase.gridPGNDatabaseDrawCell(Sender: TObject; aCol, | ||
| aRow: Integer; aRect: TRect; aState: TGridDrawState); | ||
| begin | ||
| If Sender is TStringGrid then |
There was a problem hiding this comment.
if not Sender is TStringGrid then
Exit;And, can it happen that Sender is not TStringGrid and is not equal to gridPGNDatabase?
| implementation | ||
|
|
||
| uses | ||
| MainUnit, GraphUtil, LCLIntf; |
There was a problem hiding this comment.
Empty line between uses and {$R *.lfm}
| BoardForm.Caption := gridPGNDatabase.Rows[gridPGNDatabase.Row].Strings[1]; | ||
| if (BoardForm.Caption = '') then | ||
| begin | ||
| BoardForm.Caption := 'Board'; |
There was a problem hiding this comment.
I am unsure whether it's a good idea to change the board form caption. Also, i didn't notice whether these changes actually do anything.
There was a problem hiding this comment.
No they don't actually do anything, I was trying to show some game info at the top.
| var | ||
| P: TPoint; | ||
| c, r: LongInt; | ||
| obe: Boolean; |
There was a problem hiding this comment.
Use full and descriptive variable names where possible. OldAllowOutBoundEvents, for example.
Hi, I'm just showing you what I did in my spare time. I made a chess pgn database viewer for your program.
What do you think?