TStringGrid.SaveToCSVFile does not save all Fixedrows

[ademir.uem]

Dear friends

I realized that TStringGrid.SaveToCSVFile does not save all rows when I have FixedRows>1.

See this below example you will see that the row 2 is not saved:

procedure TForm1.BitBtn1Click(Sender: TObject);
  var
    l,c : integer;
  begin
    stringgrid1.FixedRows:=2;
    stringgrid1.FixedCols:=2;
    for l := 0 to pred(stringgrid1.RowCount) do
       for c := 0 to pred(stringgrid1.colCount) do
          stringgrid1.cells[c,l] := inttostr(random(100))
  end;

procedure TForm1.BitBtn2Click(Sender: TObject);
begin
  stringgrid1.SaveToCSVFile('test1.csv');
end;

Ademir

I use Lazarus 1.6 for Win32 (Windows 10)

[Bart]

Which Lazarus version?

[wp]

Using trunk and Laz 1.4.4 I do see that not all rows are saved, but it is row 0 which is missing.

But, I think this behavior is not consistent anyway: Look at this fragment of the SaveToCSVStream code (from trunk):

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.         // ...
15.       end else
16.       if FixedRows>0 then
17.         StartRow := FixedRows-1    // <-- Note: This results in saving of the last fixed row only
18.       else
19.         StartRow := 0;
20.     end else
21.       StartRow := FixedRows;
22.     for i:=StartRow to RowCount-1 do begin
23.       if Columns.Enabled and VisibleColumnsOnly then begin
24.         //...
25.       end else begin
26.         Rows[i].StrictDelimiter := False; //force quoting of strings that contain whitespace or Delimiter
27.         Rows[i].Delimiter:=ADelimiter;
28.         Lines.Add(Rows[i].DelimitedText);         // <--- Note: This writes ALL columns even if WriteTitles=false!
29.       end;
30.     end;
31.     Lines.SaveToStream(AStream);
32.   finally
33.     Lines.Free;
34.   end;
35. end;

I see in the output of ademir.uem's sample code - and this is confirmed by these sources - that

• if "WriteTitles" is true only the last fixed row is written, and all fixed columns are written.
• if "WriteTitles" is false, no fixed rows are written. This looks ok, but note that all fixed columns are written.

I would expect to see only the non-fixed cells in the file if WriteTitles is false and all cells if WriteTitles is true. I could write a patch for this but it will break the code of people who already had used the method SaveToCSV*.

So, what to do? Keep it as it is? Or emphasize it in the docs and add a sentence saying that FixedCols/FixedRows must be 0 if full grid is to be written? Or add another parameter which defaults to the old behavior and activates the new behavior?

[Bart]

If WriteTitles = True I would expect to write all rows, otherwise I would expect to skip the first row (assuming titles ar in the first row).
So StartRow would be 0 if WriteTitles = True otherwise 1.

Bart

[wp]

assuming titles ar in the first row

What are "Titles"? In my understanding, they are in the FixedRows (not just in the "first row", since there may be several FixedRows). And the next question: are the FixedCols "titles" as well?

[Bart]

In hindsight the

Code: [Select]

procedure TCustomStringGrid.SaveToCSVStream(AStream: TStream; ADelimiter: Char;
  WriteTitles: boolean=true; VisibleColumnsOnly: boolean=false);

should have been

Code: [Select]

procedure TCustomStringGrid.SaveToCSVStream(AStream: TStream; ADelimiter: Char;
  WriteFixedRows: boolean=true; VisibleColumnsOnly: boolean=false);

It would have been clear from the start what it meant.
"WriteTitles" is just a little bit too ambiguous.

Current behaviour as decsribed by TS is undesired IMO and it should be rewritten like in the second code block.

Unfortunately not exporting FixedCols was never considered, and the current function signature does not allow for a "natural" extension like

Code: [Select]

procedure TCustomStringGrid.SaveToCSVStream(AStream: TStream; ADelimiter: Char;
  WriteFixedRows: boolean=true; WriteFixedCols: boolean=true; VisibleColumnsOnly: boolean=false);
since it would change the meaning of

Code: [Select]

SaveToCSVStream(AStream, ADelimiter, True; True);

A possible solution would be to deprecate the old implementation and implement a new one:

Code: [Select]

procedure TCustomStringGrid.SaveToCSVStream(AStream: TStream; ADelimiter: Char;
  WriteFixedRows: boolean; WriteFixedCols: boolean; VisibleColumnsOnly: boolean=false);

and the implementation of the old one would call

Code: [Select]

  SaveToCSVStream(AStream, ADelimiter, WriteTitles, True, VisibleColumnsOnly)

Bart

[ademir.uem]

My particular solution:

Grid.pas, line 11038  (Lazarus 1.6)
    for i:= StartRow to RowCount-1 do begin

I changed to:

    for i:= 0 to RowCount-1 do begin

[Bart]

I changed to:

    for i:= 0 to RowCount-1 do begin

Which is equivalent to my first suggestion:

If WriteTitles = True I would expect to write all rows, otherwise I would expect to skip the first row (assuming titles ar in the first row).
So StartRow would be 0 if WriteTitles = True otherwise 1.

 

Bart

[ademir.uem]

Anyway, does this issues is a bug?

[Bart]

Anyway, does this issues is a bug?

Not really, but it would benefit from some improvements (and proper documentation).

Bart

[ademir.uem]

If you use SaveToCSVFile(true), you expects that whole grid must be saved, but it is not true if you have FixedRow>1. So it does not work for all situation. So, it is a bug for me. I am wrong?  If so, haw include this fix to next version of the Lazarus?

[Bart]

Please open a ticket in the bugtracker about it.
This will ensure it will not be forgotten.

I cannot promise it will be fixed, nor that it will be fixed before the next release though.

Bart

[wp]

Anyway, does this issues is a bug?

Not really, but it would benefit from some improvements (and proper documentation).

Bart

In fact the current behavior is correctly documented: http://wiki.lazarus.freepascal.org/Grids_Reference_Page#procedure_SaveToCSVFile.28AFileName:_string.3B_ADelimiter:Char.3D.27.2C.27.3B_WithHeader:boolean.3Dtrue.29.3B

The WithHeader parameter is used to decide if a "row header" should be included or not. The row header is a list of field names at the beginning of the output file; its content comes from the last fixed row in the grid. There is an exception to this rule: if the grid has custom columns, the row header content comes from the custom column titles and not from fixed row cell content.

Although most grids only have a single fixed row I still think that this definition is a bit counter-intuitive... I am working on a patch to implement the method as proposed by Bart above, but now I realized that I destroyed the old behavior this way. So, please be patient.

[Michl]

Maybe you can create a new flag for that case in TSaveoptions.

[Bart]

TSaveOptions is meant for use with TCustomGrid.SaveContent, not fo exporting to CSV.
Adding options there that have no meaning in TCustomGrid.SaveContent should not be done IMO.

Having TCsvEportOptions would be an alternative to adding more and more parameters to SaveToCSVStream though.

One could also build an abstract exporter class for grids and derive TCsvExporter class from that. It would make exporting to other formats (e.g. tabulated text, HTML) easier.
Each class should implement Import and Export, and options can be controlled by TGridExport options.

Something like:

Code: [Select]

  TCustomGridExporter = Class
  public
    procedure ExportToStream(AStream: TStream; AGrid: TCustomGrid); virtual; abstract;
    procedure ExportToFile(const Fn: String; AGrid: TCustomGrid); virtual; abstract;
  end;

  TGridExporterClass = class of TCustomGridExporter;

Exporting would then be as simple as:

Code: [Select]

TCsutomGrid.ExportToStream(AExporterClass: TGridExporterClass; AStream: TStream);
var
  Exp: TCustomGridExporter;
begin
  Exp := AExporterClass.Create;
  try
    Exp.ExportToStream(AStream, Self);
  finally
    Exp.Free;
  end;
end;
(I implemented such a feature for my personal adressbook once.)

Such an idea should NOT be implemented in grids.pas unit though. It's bloated as it is right now, and Jesus is not going to like adding it in there.

Bart