Tstringgrid PrepareCanvas works unstable lazarus / 31-LCL ---------------------------------------------------------------------- guywest · 2020-07-28 00:15 I want to import rows from a list with a date in their last column to a new grid. The date of the rows with the date of yesterday and earlier, should get their date in a cell with green/red background in the new grid. For drawing the background color I use PrepareCanvas but this works completely unreliable. Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i procedure Tfrm_main.grid_nowPrepareCanvas(sender: TObject; aCol, aRow: Integer;  aState: TGridDrawState);var tn,td:tdatetime;    s:string;begin  tn:=today;  td:=0;  s:= (Sender as TStringGrid).Cells[acol,arow];  //s:=ins_dat;  if ((arow>0) and (acol=2)) then     td:=strtodate(s);    //skip header row  if tn=td then (Sender as TStringGrid).Canvas.Brush.Color := clMoneyGreen else if arow>0 then (Sender as TStringGrid).Canvas.Brush.Color:=$8080FF;  if acol<>2 then (Sender as TStringGrid).Canvas.brush.color:=clWhite;end;         This works sometimes, or sometimes not, so very unstable. Problem is that the s from s:= (Sender as TStringGrid).Cells[acol,arow]; (tried also s:=grid_now.cells[acol,arow] but same result) is sometimes empty even when the cell is not! And then strtodate of course causes an EConvertError. Sometimes this happens at the start of the program, sometimes after several lines. So very unreliable. I tried to circumvent this by using a global variable (ins_dat) to pass the value for s, but then I get REALLY strange results! The value of ins_dat (global variable!) in the PrepareCanvas is different from its value in the main program!! How can that be? It seems that PrepareCanvas has BIG unstabilities when using variables other than given in the procedure call. Is this known? Thanks. ---------------------------------------------------------------------- jamie · 2020-07-28 01:10 You may need to look at the TGridDrawState  You need to condition the colors according to what the state is..  Look up the possible values ---------------------------------------------------------------------- Handoko · 2020-07-28 01:15 @guywest Can you please provide a compile-able demo? Create a new folder, copy and paste all the necessary files except: the binary (exe file), *.bak, lib and backup folders. Compress the folder and send the zip here. ---------------------------------------------------------------------- guywest · 2020-07-28 10:59 Thanks all! @Jamie: I don't understand the link between TGridDrawState and my problem. Nothing gets selected when this happens, no mouse above it, ... It is just filled with data programmatically, not by the user. @Handoko: please find the zip somewhere here ;-). Sometimes you get the EConvertError (because of empty cell according to the variable being read in the PrepareCanvas block, which is not true) immediately at start of the program. I've also included the datafile as this is being read at program start. Sometimes you get it after adding a few "trades" (usage is keyboard based, ALT-A and TAB to switch from field to field, in the WHEN-field: add a number (=days + or -)). Most "success" to get the error is trying to add a few existings "tickers". Hope this helps. Thanks for checking. ---------------------------------------------------------------------- Handoko · 2020-07-28 11:20 Quote from: guywest on July 28, 2020, 12:59:05 pmSometimes you get the EConvertError ... The EConvertError can be easily fixed by adding a TryStrToInt before converting the string and addding an Exit if duplicate found. For example: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i procedure Tfrm_main.ed_whenKeyDown(Sender: TObject; var Key: Word;  Shift: TShiftState);var ts:longint;    td:tdatetime;    Z: Integer;begin  if ((Key=VK_TAB) or (Key=VK_RETURN)) then     begin         inc(nr_rows_upc);         grid_upc.rowcount:=nr_rows_upc;         if not(TryStrToInt(ed_when.text, Z)) then ed_when.text := '0';         ts:=datetimetounix(IncDay(Today,strtoint(ed_when.text)));         ed_when.text:=DateToStr(IncDay(Today,strtoint(ed_when.text)));       if not duplicate(ed_ticker.text) then grid_upc.InsertRowWithValues(nr _rows_upc,[ed_ticker.text,ed_why.Text,ed_when.text,inttostr(ts)])       else         begin           showmessage('Duplicate ticker, not inserted');           Key:=0;           Exit;         end;         Key:=0;         grid_upc.SortColRow(true,3);         update_to_check;         ed_when.text:='0';         ed_ticker.setfocus;     end;end; Although my suggestion above seems to solve the EConvertError issue but I suggest you to rewrite the procedure. Your increase the grid rowcount before checking duplicate, that is not the correct order. You should increase the rowcount only after you are sure the data is not duplicate. ---------------------------------------------------------------------- guywest · 2020-07-28 12:11 The problem is not the duplicate part. Before I put in the check on duplicates part I had the same behaviour. The duplicate part works OK, no problem with that. Besides that: the duplicate part works on the grid_upc which works fine (no PrepareCanvas on that). The problem is with the grid_now and its PrepareCanvas. ---------------------------------------------------------------------- Handoko · 2020-07-28 12:18 It seems you didn't get it. When the code run into KeyDown if pressed using VK_Tab or VK_Return you set the grid is set to editing mode by increasing the rowcount. But if the data is not correct (for example duplicate) you abandon it, while it is still in editing mode you call SortColRow, bad thing happens. Any way, my workaround seems to work, you can test it. What I mentioned above is only a potential bug. It may not happens now but maybe later. Your orignal code (downloaded from post #3) has many serious issues. For example. Simply run it and press tab 5 times, error pops up immediately. Tested on Linux GTK2. PrepareCanvas event is working good as far as I know. If you want to test it you can try to download and run the codes #5 - Searchable String Grid: https://forum.lazarus.freepascal.org/index.php/topic,48264.msg352057.h tml#msg352057 ---edit--- wrong link ---------------------------------------------------------------------- guywest · 2020-07-28 13:04 Thanks for the information. I wrongly understood editing mode, I supposed that editing mode was when the user was typing in a cell. Now I also understand the remark of @jamie. So now it works better. When I do the start up test: Before: gave the error about 1 in 5 times Now: gives the error about 1 in 20 times But how can it be that a fault with the grid_upc that is NOT using a PrepareCanvas block, has an influence on: - the PrepareCanvas of another grid (grid_now)? That are two totally different items. - a global variable that gets assigned a correct value, has a wrong value when read in the PrepareCanvas block? Thanks. I will clean up the experiments in the code now also ;-). ---------------------------------------------------------------------- howardpc · 2020-07-28 13:24 You want routines like OnPrepareCanvas which are called repeatedly many times to be as lean as possible, and to avoid continually resetting properties to the same value they already have. I suggest you rewrite your routine something like this: Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i procedure Tfrm_main.grid_nowPrepareCanvas(Sender: TObject; aCol, aRow: Integer; aState: TGridDrawState);var  grid: TStringGrid absolute Sender;  dt: TDateTime;begin  if not (Sender is TStringGrid) then    Exit;  case ((aCol = 2) and (aRow > 0)) of    True: case (TryStrToDate(grid.Cells[2, aRow], dt) and (SameDate(dt, Today))) of          True: if grid.Canvas.Brush.Color <> clMoneyGreen then                  grid.Canvas.Brush.Color := clMoneyGreen;            False: if grid.Canvas.Brush.Color <> $8080FF then                     grid.Canvas.Brush.Color := $8080FF;          end;    False: if grid.Canvas.Brush.Color <> clWhite then             grid.Canvas.Brush.Color := clWhite;  end;end;Also, looking at the rest of your code, you are asking for trouble by programmatically changing the focus arbitrarily in the middle of key events. This is not the way to program GUIs. Clicking on a label to change the focus is much better done by setting the label's FocusControl property to the control you want to be given focus. ---------------------------------------------------------------------- Handoko · 2020-07-28 13:42 Quote from: guywest on July 28, 2020, 03:04:18 pmNow: gives the error about 1 in 20 times Don't be shy show us the code.  The more people inspect your code, the better quality it will be. And please tell us the steps to reproduce the 1/20 error. ---------------------------------------------------------------------- guywest · 2020-07-28 14:32 Quote from: howardpc on July 28, 2020, 03:24:24 pmYou want routines like OnPrepareCanvas which are called repeatedly many times to be as lean as possible, and to avoid continually resetting properties to the same value they already have. I suggest you rewrite your routine something like this: Done and works :-), thanks. Any philosophy about using case statements in stead of IF THEN's? Quote from: howardpc on July 28, 2020, 03:24:24 pmAlso, looking at the rest of your code, you are asking for trouble by programmatically changing the focus arbitrarily in the middle of key events. This is not the way to program GUIs. Clicking on a label to change the focus is much better done by setting the label's FocusControl property to the control you want to be given focus. I like the view and design of GUIs, but I hate changing between keyboard and mouse. Continuously switching between keyboard and mouse is a major waste of time. That is why I try to make programs keyboard first ;-). So I don't want to click, but using an ALT-combination. I changed it to FocusControl and this works also. Quote from: Handoko on July 28, 2020, 03:42:16 pmQuote from: guywest on July 28, 2020, 03:04:18 pmNow: gives the error about 1 in 20 times Don't be shy show us the code.  The more people inspect your code, the better quality it will be. And please tell us the steps to reproduce the 1/20 error. The steps were easy: just start the program and close it again. When you did that about 20 times, the error was back. I made changes howardpc suggested and I've started and closed the program now more than 80 times and no error. I believe that this is true (howardpc): "You want routines like OnPrepareCanvas which are called repeatedly many times to be as lean as possible", it seems that OnPrepareCanvas is very sensible to this. Based in the input I received here, I'm going to work on a version 0.2 and will show the code :-). THANK you ALL! ---------------------------------------------------------------------- Handoko · 2020-07-28 14:47 Quote from: guywest on July 28, 2020, 04:32:17 pmAny philosophy about using case statements in stead of IF THEN's? Case statement is more readable than if-then-else statement, especially multiple levels of if-then-else. Code: Pascal  [Select][+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i   case not(SomeBoolean) of    True:  if Something = bad then DoOtherThing;  False: Case aVar < 7 of              True:  Inc(Var);              False: aVar := 7           End;   End;   if not(SomeBoolean) then    if Something = bad then       DoOtherThing  else    if aVar < 7 then     Inc(Var)     else       aVar := 7; The else on the line #12 belong to which if? A small mistake on the line #11 can make it hard to notice but still valid to the compiler. And some said case statement is a bit faster then conditional if. I'm not sure. ---------------------------------------------------------------------- howardpc · 2020-07-28 17:54 In time-critical code (such as painting routines) there may be a tiny advantage to case-of rather than if-then-else because where the compiler can determine distinct branches at compile time, it might decide to use a jump table, which might be slightly faster than conditional jumps. That said, except for quite complex if-then-else statements the difference in speed is insignificant. But as Handoko points out, whether or not there may be a speed advantage, there is often a readability advantage. This matters less when you write code, though it can help clarity of thought as you are writing. But increased readability pays dividends months later, when you maintain/debug/add new features etc. Mind you, readability is also rather subjective. You might be someone who finds nested if-then-else statements easier to understand than case statements.