Recent

Author Topic: unit Grids.pas SaveToCSVStream bug?  (Read 405 times)

KodeZwerg

  • Hero Member
  • *****
  • Posts: 967
  • Fifty shades of code.
    • Delphi & FreePascal
unit Grids.pas SaveToCSVStream bug?
« on: December 19, 2022, 06:26:07 am »
Hello there, I do actually not know if this is on purpose or a bug, here is what I found out.
I made a basic Grid and entered in the cells (beside fixed cells) test data.
When I did exported a file, it somehow had strange content.
The SaveToCSVStream mechanism add infront of each line a delimiter char when you turned WriteTitles off.
here is the original:
Code: Pascal  [Select][+][-]
  1. procedure TCustomStringGrid.SaveToCSVStream(AStream: TStream; ADelimiter: Char;
  2.   WriteTitles: boolean=true; VisibleColumnsOnly: boolean=false);
  3. var
  4.   i,j,StartRow: Integer;
  5.   HeaderL, Lines: TStringList;
  6.   C: TGridColumn;
  7. begin
  8.   if (RowCount=0) or (ColCount=0) then
  9.     exit;
  10.   Lines := TStringList.Create;
  11.   try
  12.     if WriteTitles then begin
  13.       if Columns.Enabled then begin
  14.         if FixedRows>0 then begin
  15.           HeaderL := TStringList.Create;
  16.           try
  17.             // Collect header column names to a temporary StringList
  18.             for i := 0 to ColCount-1 do begin
  19.               c := ColumnFromGridColumn(i);
  20.               if (c <> nil) then begin
  21.                 if c.Visible or not VisibleColumnsOnly then
  22.                   HeaderL.Add(c.Title.Caption);
  23.               end
  24.               else
  25.               if not VisibleColumnsOnly then
  26.                 HeaderL.Add(Cells[i, 0]);
  27.             end;
  28.             HeaderL.Delimiter:=ADelimiter;
  29.             Headerl.StrictDelimiter := False; //force quoting of strings that contain whitespace or Delimiter
  30.             Lines.Add(HeaderL.DelimitedText); // Add as a first row in Lines
  31.           finally
  32.             HeaderL.Free;
  33.           end;
  34.         end;
  35.         StartRow := FixedRows;
  36.       end else
  37.       if FixedRows>0 then
  38.         StartRow := FixedRows-1
  39.       else
  40.         StartRow := 0;
  41.     end else
  42.       StartRow := FixedRows;
  43.     for i:=StartRow to RowCount-1 do begin
  44.       if Columns.Enabled and VisibleColumnsOnly then begin
  45.         HeaderL := TStringList.Create;
  46.         try
  47.         for j := 0 to ColCount-1 do begin
  48.           c := ColumnFromGridColumn(j);
  49.           if c=nil then Continue;
  50.           if c.Visible then
  51.             HeaderL.Add(Cells[j,i]);
  52.         end;
  53.         HeaderL.Delimiter:=ADelimiter;
  54.         HeaderL.StrictDelimiter := False; //force quoting of strings that contain whitespace or Delimiter
  55.         Lines.Add(HeaderL.DelimitedText); // Add the row in Lines
  56.         finally
  57.           HeaderL.Free;
  58.         end;
  59.       end
  60.       else
  61.       begin
  62.       Rows[i].StrictDelimiter := False; //force quoting of strings that contain whitespace or Delimiter
  63.       Rows[i].Delimiter:=ADelimiter;
  64.       Lines.Add(Rows[i].DelimitedText);
  65.     end;
  66.     end;
  67.     Lines.SaveToStream(AStream);
  68.   finally
  69.     Lines.Free;
  70.   end;
  71. end;

here is my patch:
Code: Pascal  [Select][+][-]
  1. procedure TCustomStringGrid.SaveToCSVStream(AStream: TStream; ADelimiter: Char;
  2.   WriteTitles: boolean=true; VisibleColumnsOnly: boolean=false);
  3. var
  4.   i,j,StartRow: Integer;
  5.   HeaderL, Lines: TStringList;
  6.   C: TGridColumn;
  7. begin
  8.   if (RowCount=0) or (ColCount=0) then
  9.     exit;
  10.   Lines := TStringList.Create;
  11.   try
  12.     if WriteTitles then begin
  13.       if Columns.Enabled then begin
  14.         if FixedRows>0 then begin
  15.           HeaderL := TStringList.Create;
  16.           try
  17.             // Collect header column names to a temporary StringList
  18.             for i := 0 to ColCount-1 do begin
  19.               c := ColumnFromGridColumn(i);
  20.               if (c <> nil) then begin
  21.                 if c.Visible or not VisibleColumnsOnly then
  22.                   HeaderL.Add(c.Title.Caption);
  23.               end
  24.               else
  25.               if not VisibleColumnsOnly then
  26.                 HeaderL.Add(Cells[i, 0]);
  27.             end;
  28.             HeaderL.Delimiter:=ADelimiter;
  29.             Headerl.StrictDelimiter := False; //force quoting of strings that contain whitespace or Delimiter
  30.             Lines.Add(HeaderL.DelimitedText); // Add as a first row in Lines
  31.           finally
  32.             HeaderL.Free;
  33.           end;
  34.         end;
  35.         StartRow := FixedRows;
  36.       end else
  37.       if FixedRows>0 then
  38.         StartRow := FixedRows-1
  39.       else
  40.         StartRow := 0;
  41.     end else
  42.       StartRow := FixedRows;
  43.     for i:=StartRow to RowCount-1 do begin
  44.       if Columns.Enabled and VisibleColumnsOnly then begin
  45.         HeaderL := TStringList.Create;
  46.         try
  47.         for j := 0 to ColCount-1 do begin
  48.           c := ColumnFromGridColumn(j);
  49.           if c=nil then Continue;
  50.           if c.Visible then
  51.             HeaderL.Add(Cells[j,i]);
  52.         end;
  53.         HeaderL.Delimiter:=ADelimiter;
  54.         HeaderL.StrictDelimiter := False; //force quoting of strings that contain whitespace or Delimiter
  55.         Lines.Add(HeaderL.DelimitedText); // Add the row in Lines
  56.         finally
  57.           HeaderL.Free;
  58.         end;
  59.       end
  60.       else
  61.       begin
  62.         HeaderL := TStringList.Create;
  63.         try
  64.         for j := FixedCols to ColCount-1 do begin
  65.             HeaderL.Add(Cells[j,i]);
  66.             HeaderL.Delimiter:=ADelimiter;
  67.             HeaderL.StrictDelimiter := False; //force quoting of strings that contain whitespace or Delimiter
  68.         end;
  69.         Lines.Add(HeaderL.DelimitedText); // Add the row in Lines
  70.         finally
  71.           HeaderL.Free;
  72.         end;
  73.     end;
  74.     end;
  75.     Lines.SaveToStream(AStream);
  76.   finally
  77.     Lines.Free;
  78.   end;
  79. end;

If you think its useful feel free to update Grids.pas with my patch.
If you think it is wrong, please enlight me the truth to let me see my mistake.

In attachment are two images, the first show the unpatched Grids.pas
The second demonstrate the patched varation

My Version: Lazarus 2.2.4 (rev lazarus_2_2_4) FPC 3.2.2 x86_64-win64-win32/win64
« Last Edit: Tomorrow at 31:76:97 by KodeZwerg »

rvk

  • Hero Member
  • *****
  • Posts: 5074
Re: unit Grids.pas SaveToCSVStream bug?
« Reply #1 on: December 19, 2022, 11:09:34 am »
The SaveToCSVStream mechanism add infront of each line a delimiter char when you turned WriteTitles off.
It's not that strange. The first field is the text in the first column (your fixed column).
Setting WriteTitles to false only eliminates the FixedRow rows. Not the FixedCol columns.
You could create a WriteFixed to also eliminate the fixed column (or the WriteTitles should be renamed to WriteHeaders).

Anyway... looking at your code I see a bit of inefficiency (ever so slightly). You do the Delimiter:=ADelimiter and StrictDelimiter := False for EVERY column.
This would be better (this was probably because the original code isn't really properly indented):
Code: Pascal  [Select][+][-]
  1. HeaderL := TStringList.Create;
  2. try
  3.   for j := FixedCols to ColCount-1 do HeaderL.Add(Cells[j,i]);
  4.   HeaderL.Delimiter:=ADelimiter;
  5.   HeaderL.StrictDelimiter := False; //force quoting of strings that contain whitespace or Delimiter
  6.   Lines.Add(HeaderL.DelimitedText); // Add the row in Lines
  7. finally
  8.   HeaderL.Free;
  9. end;

Also... what if I really WANT the FixedCol exported in my CSV? It could contain really important information.
In that case you should look at WriteTitles and only start from FixedCols is that is false.

Something like (not tested):
Code: Pascal  [Select][+][-]
  1. if WriteTitles then StartCol := 0 else StartCol := FixedCols;
  2. for j := StartCol to ColCount-1 do HeaderL.Add(Cells[j,i]);

wp

  • Hero Member
  • *****
  • Posts: 10493
Re: unit Grids.pas SaveToCSVStream bug?
« Reply #2 on: December 19, 2022, 11:35:08 am »
The author of the SaveToCSVFile (not me) decided to mean with "Titles" only the headers of the columns because a stringgrid with GridColumns has "Titles" in the top-most row. If a grid contains no gridcolumns the "Titles" are the cells in the fixed rows. The cells in the fixed columns are not understood to be "Titles" in this concept. This is by design, not a bug.

The fixed column containing the row headers thus is not affected by the "WriteTitles" parameter. So what appears to be a leading semicolon to you is just the consequence of the the first column being empty.

Since your fixed column is empty, simply turn it off (Grid.FixedCols := 0), and everything will be fine.

Of course, if you have content in the fixed columns but you do not want to save it you have a problem. In this case I'd recommend to use an own procedure like yours.

It would be possible to extend the procedure by an additional parameter, "WriteFixedCols", to clarify things. This would not be very nice because the original parameters (WriteTitles, VisibleColumnsOnly) must be kept in place for compatibility reasons and therefore WriteTitles and WriteFixedCols would not be listed together which I personally would expect. One could overload the entire procedure and introduce some "CSVOptions = set of (coWriteTitles, coWriteFixedCols, csVisibleColumnsOnly)", but sincerely, I don't like this either because I think that saving to CSV should not be a method of the grids at all. What if somebody wants to save a grid to hmtl, xml, json, xls? For this the user would have to write his own procedures anyway. So why not drop the SaveToCSVFile at all and introduce a more general TGridExporter?

KodeZwerg

  • Hero Member
  • *****
  • Posts: 967
  • Fifty shades of code.
    • Delphi & FreePascal
Re: unit Grids.pas SaveToCSVStream bug?
« Reply #3 on: December 19, 2022, 11:45:04 am »
Thanks @rvk for hint with removing the wrong begin/end, yeah I just copied and pasted without seeing it  :'(
My own method does do same and on wish also like it was before i accidently felt over it  ;)
Code: Pascal  [Select][+][-]
  1. procedure SaveGridToFile(const AStringGrid: TStringGrid; const AFileName: string; const ADelimiter: Char = ','; const AIncludeFixed: Boolean = False; const AIncludeTitle: Boolean = False);
  2. var
  3.   LRow, LCol,
  4.   LRowStart, LColStart: Integer;
  5.   sl: TStringList;
  6.   s: string;
  7. begin
  8.   sl := TStringList.Create;
  9.   if AIncludeFixed then
  10.     begin
  11.       LRowStart := 0;
  12.       LColStart := 0;
  13.     end
  14.     else
  15.     begin
  16.       if AIncludeTitle then
  17.         LRowStart := 0
  18.         else
  19.         LRowStart := AStringGrid.FixedRows;
  20.       LColStart := AStringGrid.FixedCols;
  21.     end;
  22.   try
  23.     for LRow := LRowStart to Pred(AStringGrid.RowCount) do
  24.       begin
  25.         s := '';
  26.         for LCol := LColStart to Pred(AStringGrid.ColCount) do
  27.           if (LCol < Pred(AStringGrid.ColCount)) then
  28.             s := s + AStringGrid.Cells[LCol, LRow] + ADelimiter
  29.             else
  30.             s := s + AStringGrid.Cells[LCol, LRow];
  31.         sl.Add(s);
  32.       end;
  33.     sl.SaveToFile(AFilename, True);
  34.   finally
  35.     sl.Free;
  36.   end;
  37. end;
« Last Edit: Tomorrow at 31:76:97 by KodeZwerg »

rvk

  • Hero Member
  • *****
  • Posts: 5074
Re: unit Grids.pas SaveToCSVStream bug?
« Reply #4 on: December 19, 2022, 11:55:25 am »
Code: Pascal  [Select][+][-]
  1. s := s + AStringGrid.Cells[LCol, LRow] + ADelimiter
You definitely want to fix these lines  ::)
Using a TStringList.DelimitedText, there is some logic which adds protection for a delimiter character in a string.
In your code you don't.

So if one of you cells contain a ADelimiter character, the whole CSV-line would be corrupt.

KodeZwerg

  • Hero Member
  • *****
  • Posts: 967
  • Fifty shades of code.
    • Delphi & FreePascal
Re: unit Grids.pas SaveToCSVStream bug?
« Reply #5 on: December 19, 2022, 12:07:31 pm »
Since your fixed column is empty, simply turn it off (Grid.FixedCols := 0), and everything will be fine.
When I do that, it be same like on my first post screensnap, leading delimiter  ::)
Thanks for all of your text, I understand now that this is a wanted behavior and my thinking was wrong.
« Last Edit: Tomorrow at 31:76:97 by KodeZwerg »

rvk

  • Hero Member
  • *****
  • Posts: 5074
Re: unit Grids.pas SaveToCSVStream bug?
« Reply #6 on: December 19, 2022, 12:13:52 pm »
Since your fixed column is empty, simply turn it off (Grid.FixedCols := 0), and everything will be fine.
When I do that, it be same like on my first post screensnap, leading delimiter  ::)
After doing that (setting FixedCols to 0) you would need to reload the text in your grid, and your first real field will end up in column 0.
In that case you don't have a first empty column (but you will also loose a visible margin column).

wp

  • Hero Member
  • *****
  • Posts: 10493
Re: unit Grids.pas SaveToCSVStream bug?
« Reply #7 on: December 19, 2022, 12:31:43 pm »
my thinking was wrong.
Hmmm... I think there's no wrong or right here, it's just a matter of preferences. The problem is that once a procedure was written and has been released it is hard to change it without break existing code.

wp

  • Hero Member
  • *****
  • Posts: 10493
Re: unit Grids.pas SaveToCSVStream bug?
« Reply #8 on: December 19, 2022, 12:37:30 pm »
Since your fixed column is empty, simply turn it off (Grid.FixedCols := 0), and everything will be fine.
When I do that, it be same like on my first post screensnap, leading delimiter  ::)
Sorry. Setting Grid.FixedCells to 0 simply converts the cells in the first column (index 0) from fixed cells to normal cells, and then the SaveToCSVFile will still be the same because it does not distinguish between normal and fixed cells.

What I meant is: Set Grid.FixedCols=0 before you fill the grid and when you fill it make sure that every column index is decremented by 1 compared with the current code.

KodeZwerg

  • Hero Member
  • *****
  • Posts: 967
  • Fifty shades of code.
    • Delphi & FreePascal
Re: unit Grids.pas SaveToCSVStream bug?
« Reply #9 on: December 19, 2022, 12:44:22 pm »
Code: Pascal  [Select][+][-]
  1. s := s + AStringGrid.Cells[LCol, LRow] + ADelimiter
You definitely want to fix these lines  ::)
Using a TStringList.DelimitedText, there is some logic which adds protection for a delimiter character in a string.
In your code you don't.

So if one of you cells contain a ADelimiter character, the whole CSV-line would be corrupt.

I agree, I was not that deep thinking about such situation and fixed it.
Code: Pascal  [Select][+][-]
  1. // SaveGridToFile method by KodeZwerg for FreePascal forum
  2. // this method can plain save grid content into a text file where each element is seperated by a delimiter (default = ",")
  3. // argument #1: the stringgrid that you want to save
  4. // argument #2: the filepath and filename to save, no checking if valid
  5. // argument #3: optional set a delimiter, default ","
  6. // argument #4: optional shall the save include fixed cells, default False
  7. // argument #5: optional shall the save include the title cells, default False
  8. procedure SaveGridToFile(const AStringGrid: TStringGrid; const AFileName: string; const ADelimiter: Char = ','; const AIncludeFixed: Boolean = False; const AIncludeTitle: Boolean = False);
  9. var
  10.   LRow, LCol, // where are we inside loop
  11.   LRowStart, LColStart: Integer; // where do we start working at
  12.   sl, hl: TStringList; // sl is the main list with formated results from hl
  13. begin
  14.   sl := TStringList.Create; // create object, needed in the main loop
  15.   try
  16.     hl := TStringList.Create; // create object, needed in the main loop to create the sl list
  17.     try
  18.       hl.Delimiter := ADelimiter; // setup Delimiter
  19.       hl.StrictDelimiter := False; //force quoting of strings that contain whitespace or Delimiter
  20.       if AIncludeFixed then // user wants to include the fixed cells in the results
  21.         begin
  22.           if AIncludeTitle then // user wants to include the title
  23.             LRowStart := 0 // giving every row
  24.             else
  25.             if ((AStringGrid.FixedRows >= 1) and (AStringGrid.RowCount > 0)) then // giving fixed row without title
  26.               LRowStart := 1 // we cut the title off
  27.               else
  28.               LRowStart := 0; // giving all
  29.           LColStart := 0; // 0 for the fixed beginning
  30.         end // user wants to include the fixed cells in the results
  31.         else
  32.         begin // user does not want to have fixed cells
  33.           if AIncludeTitle then // user wants to include title (what results in getting all cells)
  34.             LRowStart := 0 // giving all
  35.             else
  36.             LRowStart := AStringGrid.FixedRows; // user want no fixed row data
  37.           LColStart := AStringGrid.FixedCols; // user want no fixed col data
  38.         end; // user does not want to have fixed cells
  39.       for LRow := LRowStart to Pred(AStringGrid.RowCount) do // begin outer loop for the rows
  40.         begin
  41.           hl.Clear;  // clear stringlist
  42.           for LCol := LColStart to Pred(AStringGrid.ColCount) do // begin inner loop for the columns
  43.             hl.Add(AStringGrid.Cells[LCol, LRow]); // transport string-grid-cell to stringlist
  44.           sl.Add(hl.DelimitedText); // store the generated delimited
  45.         end;
  46.     finally
  47.       hl.Free; // free the hl list
  48.     end;
  49.     if (sl.Count > 0) then // we gathered something, lets save
  50.       sl.SaveToFile(AFilename, True); // save main list to disk
  51.                                       // this method assume that you want to save and overwrite
  52.                                       // so check outside of this method your filename on disk etc...
  53.   finally
  54.     sl.Free; // free the main list
  55.   end;
  56. end;
« Last Edit: December 19, 2022, 03:03:46 pm by KodeZwerg »
« Last Edit: Tomorrow at 31:76:97 by KodeZwerg »

 

TinyPortal © 2005-2018