View Issue Details

IDProjectCategoryView StatusLast Update
0026943PatchesLCLpublic2014-11-10 11:01
ReporterBigChimpAssigned ToBart Broersma 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Platformx64 (with x86 compiler)OSWindowsOS VersionWindows 7
Product Version1.2.6Product Build 
Target Version1.4Fixed in Version 
Summary0026943: TStringgrid: InsertColRow on a cleared Grid
DescriptionInsertRowWithValues seems to insert rows without data at least in some cases.

See example program; press button several times & you'll see the vertical scrollbar appear, indicating rows have been added.
However they're empty.

Also occurs on current trunk, FPC trunk x86, Windows.

Note: this was actually a test program for a more complicated case which I cannot reproduce.
Additional InformationPlease let me know if you require .zip instead of .7z
My peazip did not like zipping up multiple files from the explorer so I'd have to create one manually.
Tagspatch
Fixed in Revisionr46766, r46782, r46787
LazTarget1.4
WidgetsetWin32/Win64
Attached Files
  • insertrowwithvalues.7z (1,504 bytes)
  • grids.addcolrow.diff (1,492 bytes)
    Index: lcl/grids.pas
    ===================================================================
    --- lcl/grids.pas	(revision 46593)
    +++ lcl/grids.pas	(working copy)
    @@ -5775,7 +5775,7 @@
     
     procedure TCustomGrid.DoOPInsertColRow(IsColumn: boolean; index: integer);
     var
    -  NewCol,NewRow: Integer;
    +  NewCol,NewRow,OldC: Integer;
     begin
       if Index<0 then
         Index:=0;
    @@ -5785,17 +5785,34 @@
       if IsColumn then begin
         if Index>ColCount-1 then
           Index := ColCount-1;
    +    if Index<0 then
    +      Index := 0;
         if columns.Enabled then begin
    -      Columns.InsertColumn(ColumnIndexFromGridColumn(index));
    +      if Columns.Count > 0 then
    +        Columns.InsertColumn(ColumnIndexFromGridColumn(index))
    +      else
    +        Columns.Add;
           ColRowInserted(true, index);
           exit;
         end else begin
    -      FCols.Insert(Index, pointer(-1));
    -      FGCache.AccumWidth.Insert(Index, nil);
    +      if ColCount > 0 then begin
    +        FCols.Insert(Index, pointer(-1));
    +        FGCache.AccumWidth.Insert(Index, nil);
    +      end else begin
    +        OldC := RowCount; //Setting ColCount can set RowCount to 1 if RowCount = 0
    +        ColCount := 1;
    +        RowCount := OldC;
    +        Index := 0;
    +      end;
         end;
       end else begin
    +    if RowCount > 0 then begin
         Frows.Insert(Index, pointer(-1));
         FGCache.AccumHeight.Insert(Index, nil);
    +    end else begin
    +      RowCount := 1;
    +      Index := 0;
    +    end;
       end;
       ColRowInserted(IsColumn, index);
       VisualChange;
    
    grids.addcolrow.diff (1,492 bytes)
  • insrow.zip (3,091 bytes)
  • insrow2.zip (2,967 bytes)
  • insrow-1.png (40,945 bytes)
    insrow-1.png (40,945 bytes)
  • insrow-2.png (39,330 bytes)
    insrow-2.png (39,330 bytes)
  • insrow-3.png (17,420 bytes)
    insrow-3.png (17,420 bytes)

Activities

Reinier Olislagers

2014-10-25 19:03

developer  

insertrowwithvalues.7z (1,504 bytes)

Bart Broersma

2014-10-26 13:37

developer   ~0078618

[Off Topic]
> Please let me know if you require .zip instead of .7z
> My peazip did not like zipping up multiple files from the explorer so I'd have
> to create one manually.

I have 7-zip on Windows, so that should not be a problem.
You know Windows Explorer (WinME and up) has built in capabilities of creating a straigth .zip?
[/Off Topic]

Bart Broersma

2014-10-26 16:06

developer  

grids.addcolrow.diff (1,492 bytes)
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 46593)
+++ lcl/grids.pas	(working copy)
@@ -5775,7 +5775,7 @@
 
 procedure TCustomGrid.DoOPInsertColRow(IsColumn: boolean; index: integer);
 var
-  NewCol,NewRow: Integer;
+  NewCol,NewRow,OldC: Integer;
 begin
   if Index<0 then
     Index:=0;
@@ -5785,17 +5785,34 @@
   if IsColumn then begin
     if Index>ColCount-1 then
       Index := ColCount-1;
+    if Index<0 then
+      Index := 0;
     if columns.Enabled then begin
-      Columns.InsertColumn(ColumnIndexFromGridColumn(index));
+      if Columns.Count > 0 then
+        Columns.InsertColumn(ColumnIndexFromGridColumn(index))
+      else
+        Columns.Add;
       ColRowInserted(true, index);
       exit;
     end else begin
-      FCols.Insert(Index, pointer(-1));
-      FGCache.AccumWidth.Insert(Index, nil);
+      if ColCount > 0 then begin
+        FCols.Insert(Index, pointer(-1));
+        FGCache.AccumWidth.Insert(Index, nil);
+      end else begin
+        OldC := RowCount; //Setting ColCount can set RowCount to 1 if RowCount = 0
+        ColCount := 1;
+        RowCount := OldC;
+        Index := 0;
+      end;
     end;
   end else begin
+    if RowCount > 0 then begin
     Frows.Insert(Index, pointer(-1));
     FGCache.AccumHeight.Insert(Index, nil);
+    end else begin
+      RowCount := 1;
+      Index := 0;
+    end;
   end;
   ColRowInserted(IsColumn, index);
   VisualChange;
grids.addcolrow.diff (1,492 bytes)

Bart Broersma

2014-10-26 16:13

developer   ~0078628

Problem seems to be in TCustomGrid.DoOPInsertColRow
If you perform this on a cleared grid, then somehow internals structures get out of sync.
(Run attached demo in insrow.zip, to see what happens.)

It is also strange that this procedure raises an exception in this case (cleared grid) if IsColumn = True, but not if isColumn = False.

Attached patch seems to fix it, but maybe not the right way to do it.
Basically when RowCount or ColCount = 0 it sets it to 1 if needed.
The strange side-effect seems to be that when doing this for a Column, under cirtain circumstances, it may then also set RowCount to 1 if RowCount was 0.

@Jesus: can you plese review the patch?

Bart Broersma

2014-10-26 16:15

developer  

insrow.zip (3,091 bytes)

Jesus Reyes

2014-11-06 20:45

developer   ~0078965

I think we have two options here: 1) raise an exception if trying to insert rows in a grid without columns, or trying to insert columns in a grid without rows, or 2) if trying to insert rows in a grid without columns add one, or trying to insert columns in a grid without rows add one row.

Either option is OK to me, but I tend to prefer option 1, because option 2 is letting the grid guessing and it could guess wrong. Feel free to commit or suggest a patch that implements either option.

On the other hand, InsertRowWithValues is a higher level function that carry information about rows and columns to be added, this information should be used to setup the grid (done in r46766).

Bart Broersma

2014-11-08 01:41

developer   ~0078994

Trying to insert a row when no columns are present, or a columns when no rows are present now raises an exception.

Please close if OK.

Reinier Olislagers

2014-11-08 09:57

developer   ~0079000

Doesn't seem fixed to me r46786, FPC trunk - the test program still inserts empty data and does not show any exceptions.
Not much of a grid/GUI guy here so I may have missed something?

Bart Broersma

2014-11-08 14:20

developer  

insrow2.zip (2,967 bytes)

Bart Broersma

2014-11-08 14:20

developer  

insrow-1.png (40,945 bytes)
insrow-1.png (40,945 bytes)

Bart Broersma

2014-11-08 14:20

developer  

insrow-2.png (39,330 bytes)
insrow-2.png (39,330 bytes)

Bart Broersma

2014-11-08 14:21

developer  

insrow-3.png (17,420 bytes)
insrow-3.png (17,420 bytes)

Bart Broersma

2014-11-08 14:24

developer   ~0079009

Uploaded a modified sample application (insrow2.zip).
Look at screenshots to see what it does (insrow-1.png, insrow-2.png, insrow-3.png: the last one uses InsertRowWithValues, and succeeds as expected, the other two raise exceptions).

Lazarus 1.3 r46782 FPC 2.6.4 32-bit on Win7-64.

Reinier Olislagers

2014-11-08 17:04

developer   ~0079012

Bart, the third button in your demo program still seems to insert empty rows instead of the expected 'col1', 'col2' if you press it multiple times?!?

Bart Broersma

2014-11-08 17:42

developer   ~0079014

Last edited: 2014-11-08 18:01

View 2 revisions

Yes, you are right.
Clear saves (a.o.) RowCount in FGridPropBackup.RowCount, which then is restored when InsertRowWithValues sets ColCount -> TCustomGrid.InternalSetColCount().

Invalidating FGridPropBackup seems not the right thing to do, so I decided to reset RowCount to 1, if it was 0 before inserting.

Bart Broersma

2014-11-08 18:03

developer   ~0079015

Please close if OK now.

Reinier Olislagers

2014-11-10 11:01

developer   ~0079062

Thanks a lot, works now!

Issue History

Date Modified Username Field Change
2014-10-25 19:03 Reinier Olislagers New Issue
2014-10-25 19:03 Reinier Olislagers Status new => assigned
2014-10-25 19:03 Reinier Olislagers Assigned To => Bart Broersma
2014-10-25 19:03 Reinier Olislagers File Added: insertrowwithvalues.7z
2014-10-26 13:37 Bart Broersma Note Added: 0078618
2014-10-26 16:06 Bart Broersma File Added: grids.addcolrow.diff
2014-10-26 16:13 Bart Broersma Note Added: 0078628
2014-10-26 16:15 Bart Broersma File Added: insrow.zip
2014-10-26 16:15 Bart Broersma Assigned To Bart Broersma => Jesus Reyes
2014-10-26 16:15 Bart Broersma Project Lazarus => Patches
2014-10-26 16:16 Bart Broersma Tag Attached: patch
2014-10-26 17:30 Bart Broersma Summary Stringgrid InsertRowWithValues inserts rows without data => TStringgrid: InsertColRow on a cleared Grid
2014-11-06 20:45 Jesus Reyes Note Added: 0078965
2014-11-06 20:47 Jesus Reyes Assigned To Jesus Reyes => Bart Broersma
2014-11-08 01:41 Bart Broersma Fixed in Revision => r46782
2014-11-08 01:41 Bart Broersma LazTarget - => 1.4
2014-11-08 01:41 Bart Broersma Note Added: 0078994
2014-11-08 01:41 Bart Broersma Status assigned => resolved
2014-11-08 01:41 Bart Broersma Resolution open => fixed
2014-11-08 01:41 Bart Broersma Target Version => 1.4
2014-11-08 09:57 Reinier Olislagers Note Added: 0079000
2014-11-08 09:57 Reinier Olislagers Status resolved => assigned
2014-11-08 09:57 Reinier Olislagers Resolution fixed => reopened
2014-11-08 14:20 Bart Broersma File Added: insrow2.zip
2014-11-08 14:20 Bart Broersma File Added: insrow-1.png
2014-11-08 14:20 Bart Broersma File Added: insrow-2.png
2014-11-08 14:21 Bart Broersma File Added: insrow-3.png
2014-11-08 14:24 Bart Broersma Note Added: 0079009
2014-11-08 14:24 Bart Broersma Status assigned => feedback
2014-11-08 17:04 Reinier Olislagers Note Added: 0079012
2014-11-08 17:04 Reinier Olislagers Status feedback => assigned
2014-11-08 17:42 Bart Broersma Note Added: 0079014
2014-11-08 17:42 Bart Broersma Assigned To Bart Broersma => Jesus Reyes
2014-11-08 18:01 Bart Broersma Note Edited: 0079014 View Revisions
2014-11-08 18:03 Bart Broersma Fixed in Revision r46782 => r46766, r46782, r46787
2014-11-08 18:03 Bart Broersma Note Added: 0079015
2014-11-08 18:03 Bart Broersma Status assigned => resolved
2014-11-08 18:03 Bart Broersma Resolution reopened => fixed
2014-11-08 18:03 Bart Broersma Assigned To Jesus Reyes => Bart Broersma
2014-11-08 18:03 Bart Broersma Status resolved => assigned
2014-11-08 18:03 Bart Broersma Status assigned => resolved
2014-11-10 11:01 Reinier Olislagers Note Added: 0079062
2014-11-10 11:01 Reinier Olislagers Status resolved => closed