Recent

Author Topic: AutoAdjustColumns() or Col 0 header double-click causes range-check error  (Read 2812 times)

russeld

  • New Member
  • *
  • Posts: 17
Hi,

With FpSpreadsheet V1.12, 32 bit Lazarus V2.0.10 and FPC V3.2.0 on Windows 8.1, if TsWorksheetGrid.AutoAdjustColumns() is called, or if grid option goDblClickAutoSize is enabled and column zero's header is double clicked in the worksheet grid then a range check error is raised.

This was traced to method TsCustomWorksheetGrid.AutoAdjustColumn() where Worksheet.Cells.GetColEnumerator() is called with parameter GetWorkSheetCol(ACol); if ACol is zero this returns UNASSIGNED_ROW_COL_INDEX ($FFFFFFFF) which causes the range check error.

The problem can be fixed by either changing the early return at line 1452 of unit fpspreadhseetgrid from
Code: Pascal  [Select][+][-]
  1. if Worksheet = nil then exit;
to
Code: Pascal  [Select][+][-]
  1. if (Worksheet = nil) or (ACol = 0) then exit;
or probably better by modifying GetColEnumerator() in unit fpsclasses:
Code: Pascal  [Select][+][-]
  1. function TsCells.GetColEnumerator(ACol: Cardinal; AStartRow: Cardinal = 0;
  2.   AEndRow: Cardinal = $7FFFFFFF): TsCellEnumerator;
  3. begin
  4.   if (ACol = UNASSIGNED_ROW_COL_INDEX) then
  5.     Result := nil
  6.   else
  7.     Result := TsCellEnumerator.Create(Self, AStartRow, ACol, AEndRow, ACol, false);
  8. end;

wp

  • Hero Member
  • *****
  • Posts: 11858
I cannot reproduce this issue. Can you post a simple demo project to guide me? How can you double-click the fixed column to auto-resize it? And there is no TsWorksheetGrid.AutoAdjustColumns() (plural).

In your patch there is maybe also an issue with column 0 which refers to a non-fixed cell when DisplayFixedColRow is false.

russeld

  • New Member
  • *
  • Posts: 17
Apologies for only replying now. You are right, there is no AutoAdjustColumns in TsWorksheetGrid, however AutoAdjustColumns is inherited from TCustomGrid which calls the overriden TsCustomWorksheetGrid.AutoAdjustColumn.
Code: Pascal  [Select][+][-]
  1. procedure TCustomGrid.AutoAdjustColumns;
  2. var
  3.   i: Integer;
  4. begin
  5.   For i:=0 to ColCount  do
  6.     AutoAdjustColumn(i);
  7. end;
Quote
Can you post a simple demo project to guide me?
A form with a worksheet grid and three buttons to allow auto-resize all, hide/show grid headers and one to resize col 0:
Code: Pascal  [Select][+][-]
  1. unit Unit1;
  2. {$mode objfpc}{$H+}
  3.  
  4. interface
  5. uses
  6.   Classes, SysUtils, Forms, Controls, Graphics, Dialogs, Grids, StdCtrls, fpspreadsheetgrid;
  7. type
  8.   TForm1 = class(TForm)
  9.     Button1: TButton;
  10.     Button2: TButton;
  11.     Button3: TButton;
  12.     sWorksheetGrid1: TsWorksheetGrid;
  13.     procedure Button1Click(Sender: TObject);
  14.     procedure Button2Click(Sender: TObject);
  15.     procedure Button3Click(Sender: TObject);
  16.     procedure FormCreate(Sender: TObject);
  17.   private
  18.   public
  19.   end;
  20.  
  21. var
  22.   Form1: TForm1;
  23.  
  24. implementation
  25. {$R *.lfm}
  26. { TForm1 }
  27. const
  28.   Button2Caption: array[false..true] of string = ('DisableFixedColRow', 'EnableFixedColRow');
  29.  
  30. procedure TForm1.FormCreate(Sender: TObject);
  31. const
  32.   strings: array[1..3] of string = ('String 1', 'String 2', 'String 3');
  33. var
  34.   i: Integer;
  35. begin
  36.   for i := low(strings) to high(strings) do
  37.     sWorksheetGrid1.Cells[i, 1] := strings[i];
  38.  
  39.   sWorksheetGrid1.Options := sWorksheetGrid1.Options + [goColSizing, goDblClickAutoSize, goFixedColSizing];
  40.   sWorksheetGrid1.DisplayFixedColRow := true;
  41.  
  42.   Button1.Autosize := true;
  43.   Button2.Autosize := true;
  44.   Button3.AutoSize := true;
  45.   Button1.Caption := 'Autosize All';
  46.   Button2.Caption := Button2Caption[not sWorksheetGrid1.DisplayFixedColRow];
  47.   Button3.Caption := 'Autosize Col 0';
  48. end;
  49.  
  50. procedure TForm1.Button1Click(Sender: TObject);
  51. begin
  52.   sWorksheetGrid1.AutoAdjustColumns;
  53. end;
  54.  
  55. procedure TForm1.Button2Click(Sender: TObject);
  56. begin
  57.   Button2.Caption := Button2Caption[sWorksheetGrid1.DisplayFixedColRow];
  58.   sWorksheetGrid1.DisplayFixedColRow := not sWorksheetGrid1.DisplayFixedColRow;
  59. end;
  60.  
  61. procedure TForm1.Button3Click(Sender: TObject);
  62. begin
  63.   sWorksheetGrid1.AutoColWidth(0);
  64. end;
  65.  
  66. end.
  67.  
Quote
How can you double-click the fixed column to auto-resize it?
If the options include goFixedColSizing then it resizes with a double click.
Quote
In your patch there is maybe also an issue with column 0 which refers to a non-fixed cell when DisplayFixedColRow is false
I agree that the first solution I proposed doesn't work in this case, however the patch to TsCells.GetColEnumerator does work with and without headers as the demo project shows.

« Last Edit: April 13, 2021, 02:22:47 pm by russeld »

wp

  • Hero Member
  • *****
  • Posts: 11858
Cannot reproduce the range check error with the attached project created following your instructions (it would be better if you'd attach a compilable project rather than pasting code because there are various ways to set up a project in detail). The fpspreadsheet used is my development version on ccr trunk. So, maybe this bug already has been fixed? Please check out the trunk version (if you don't have svn, download the snapshot from https://sourceforge.net/p/lazarus-ccr/svn/HEAD/tree/components/fpspreadsheet/)

russeld

  • New Member
  • *
  • Posts: 17
Sorry about not attaching a full project.

I downloaded and installed rev 7988, I still see the same issue. Unit fpsclasses hasn't changed from V1.12 and that's where the problem appears to be.

In function TsCells.GetColEnumerator, if parameter ACol has a value of UNASSIGNED_ROW_COL_INDEX, then the call to TsCellEnumerator fails with a range check error. In GetColEnumerator the ACol parameter is a Cardinal, while in TsRowColEnumerator.Create the corresponding parameters are LongInts. If I change the parameter types in the constructor to Cardinal then it no longer raises an error at the constructor call, but it does at line 508 of fpsClasses where FStartCol is assigned. FStartCol is a LongInt, while UNASSIGNED_ROW_COL_INDEX, at least on my system with FPC V3.2.0, is evaluated as a Cardinal with a value of $FFFFFFFF and this is what is causing the range check error.

If I change UNASSIGNED_ROW_COL_INDEX to -1, then compile time errors are raised in TsWorksheet.Create because fields FFirstRowIndex, FFirstColIndex etc. are all Cardinals. It might be easier for TsCells.GetColEnumerator to just return nil if the function is called when ACol contains UNASSIGNED_ROW_COL_INDEX.

russeld

  • New Member
  • *
  • Posts: 17
Update: I had Lazarus built for debugging, when built without debugging the issue is understandably gone.

wp

  • Hero Member
  • *****
  • Posts: 11858
This is not the first time to get the impression that it was a bad decision to declare column and row indices as Cardinal in the initial days of fpspreadsheet. Now it is impossible to revert because it would break lots of methods...

Nevertheless, I think that the declarations within the entire library should be consistent, and as you showed, they are not consistent within the enumerator classes. Therefore, I'd propose to declare FStartRow, FStartCol, FEndRow, FEndCol in TsRowColEnumerator as Cardinal, and propagate this to all related code locations. Having done this the crash of my demo program does not occur any more, and also all 1800+ tests of the test suite are passed successfully.

I prefer this solution rather than returning nil in the TsCells.GetColEnumerator - who knows which can this opens...

russeld

  • New Member
  • *
  • Posts: 17
You are of course right that returning nil from GetColEnumerator could have all sorts of unintended consequences.

FpSpreadsheet is a huge project and at times I suppose it must feel like a millstone around your neck. I would like to thank you and other contributors for the huge effort it has taken to make it what is is and for all the support you have given people on this forum

Russell

 

TinyPortal © 2005-2018