View Issue Details

IDProjectCategoryView StatusLast Update
0024924LazarusLCLpublic2014-11-11 08:55
ReporterPetr-KAssigned ToJesus Reyes 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version1.1 (SVN)Product Buildtrunk #42516 
Target Version1.4Fixed in Version1.3 (SVN) 
Summary0024924: Bad bookmarks order in dbgrid
DescriptionI use old version of zeosdb (v5) and find that order of selected rows in DBGrid is "random" and does not depend on dataset order.

In dbgrids.pas I found IMHO tamporary hack to alow compile module in fpc 2.7.1 where definitions of Bookmark was changed.

When I remove this hack, everything works ok.

Patch is attached.
Steps To ReproduceSelect some rows in the dbgrid and walk on them by

for i:=0 to DBGrid.SelectedRows.Count-1 do begin
  Dataset.GotoBookmark(Grid.SelectedRows.Items[i]);
  ...
end;

order in SelectedRows does not depend on Dataset sort.
TagsNo tags attached.
Fixed in Revision46797
LazTarget1.4
Widgetset
Attached Files
  • dbgrids.patch (1,098 bytes)
    Index: dbgrids.pas
    ===================================================================
    --- dbgrids.pas	(revision 42516)
    +++ dbgrids.pas	(working copy)
    @@ -4033,7 +4033,7 @@
       TDs=class(TDataset)
       end;
     
    -function MyCompareBookmarks(ds:Tdataset; b1,b2:pointer): Integer;
    +{function MyCompareBookmarks(ds:Tdataset; b1,b2:pointer): Integer;
     begin
       if b1=b2 then
         result := 0
    @@ -4047,7 +4047,7 @@
         // Note: Tds(ds).bookmarksize is set at creation of TDataSet and does not change
         result := CompareMemRange(b1,b2,Tds(ds).bookmarksize);
       end;
    -end;
    +end;}
     
     function TBookmarkList.Find(const Item: TBookmark; var AIndex: Integer): boolean;
     var
    @@ -4061,8 +4061,8 @@
       while (L <= R) do
       begin
         I := L + (R - L) div 2;
    -    CompareRes := MyCompareBookmarks(FDataset, pointer(Item), FList[I]);
    -    //CompareRes := FDataset.CompareBookmarks(Item, TBookmark(FList[I]));
    +    //CompareRes := MyCompareBookmarks(FDataset, pointer(Item), FList[I]);
    +    CompareRes := FDataset.CompareBookmarks(Item, TBookmark(FList[I]));
         if (CompareRes > 0) then
           L := I + 1
         else
    
    dbgrids.patch (1,098 bytes)
  • dbgridmultiselect.diff (5,218 bytes)
    Index: dbgrids.pas
    ===================================================================
    --- dbgrids.pas	(revision 46763)
    +++ dbgrids.pas	(working copy)
    @@ -131,6 +131,8 @@
         FList: TFPList; // list of TBookmark
         FGrid: TCustomDbGrid;
         FDataset: TDataset;
    +    FUseCompareBookmarks: boolean;
    +    FCanDoBinarySearch: boolean;
         function GetCount: integer;
         function GetCurrentRowSelected: boolean;
         function GetItem(AIndex: Integer): TBookmark;
    @@ -4133,9 +4135,9 @@
       Bookmark: TBookmark;
     begin
       CheckActive;
    -  Bookmark := FGrid.Datasource.Dataset.GetBookmark;
    +  Bookmark := FDataset.GetBookmark;
       Result := IndexOf(Bookmark)>=0;
    -  FGrid.Datasource.Dataset.FreeBookmark(Bookmark);
    +  FDataset.FreeBookmark(Bookmark);
     end;
     
     function TBookmarkList.GetItem(AIndex: Integer): TBookmark;
    @@ -4151,12 +4153,10 @@
       CheckActive;
     
       Bookmark := nil;
    -  TBookmark(Bookmark) := FGrid.Datasource.Dataset.GetBookmark; // fetch and increase reference count
    +  TBookmark(Bookmark) := FDataset.GetBookmark; // fetch and increase reference count
       if Bookmark = nil then
         Exit;
     
    -  FDataset := FGrid.Datasource.Dataset;
    -
       if Find(Bookmark, Index) then begin
         FDataset.FreeBookmark(Bookmark);
         {$ifndef noautomatedbookmark}
    @@ -4189,6 +4189,43 @@
       {$endif}
       if not FGrid.FDataLink.Active then
         raise EInvalidGridOperation.Create('Dataset Inactive');
    +
    +  if FGrid.DataSource.DataSet=FDataset then
    +    exit;
    +  FDataset := FGrid.DataSource.DataSet;
    +
    +  // Note.
    +  //
    +  // Some dataset descendants do not implement CompareBookmarks, for these we
    +  // use MyCompareBookmarks in the hope the allocated bookmark memory is used
    +  // to hold some kind of record index.
    +  FUseCompareBookmarks := TMethod(@FDataset.CompareBookmarks).Code<>pointer(@TDataset.CompareBookmarks);
    +
    +  // Note.
    +  //
    +  // fpc help say CompareBookmarks should return -1, 0 or 1 ... which imply that
    +  // bookmarks should be a sorted array (or list). In this scenario binary search
    +  // is the prefered method for finding a bookmark.
    +  //
    +  // The problem here is that TBufDataset and TSQLQuery (and thus TCustomSQLQuery
    +  // and TCustomBufDataset) CompareBookmarks only return 0 or -1 (some kind of
    +  // is this a valid bookmark or not), the result is that it appears as an unsorted
    +  // list (or array) and binary search should not be used.
    +  //
    +  // The weird thing is that if we use MyCompareBookmarks which deals with comparing
    +  // the memory reserved for bookmarks in the hope bookmarks are just some kind of
    +  // reocord indexes, currently work fine for TCustomBufDataset derived datasets.
    +  // however using CompareBookmarks is always the right thing to use where implemented.
    +  //
    +  // As Dbgrid should be TDataset implementation agnostic this is a way I found
    +  // to know if the dataset is derived from TCustomBufDataset or not.
    +  // Once TCustomBufDataset is fixed, remove this ugly note & hack.
    +  case FDataset.ClassName of
    +    'TSQLQuery','TBufDataset','TCustomSQLQuery','TCustomBufDataset':
    +      FCanDoBinarySearch := false;
    +    else
    +      FCanDoBinarySearch := true;
    +  end;
     end;
     
     constructor TBookmarkList.Create(AGrid: TCustomDBGrid);
    @@ -4273,33 +4310,62 @@
     function TBookmarkList.Find(const Item: TBookmark; var AIndex: Integer): boolean;
     var
       L, R, I: Integer;
    -  CompareRes: PtrInt;
    -begin
    -  {$ifdef dbgDBGrid}
    -  DebugLn('%s.Find', [ClassName]);
    -  {$endif}
    -  // From TStringList.Find() Use binary search.
    -  Result := False;
    -  L := 0;
    -  R := FList.Count - 1;
    -  while (L <= R) do
    +  CompareRes: Integer;
    +
    +  procedure BinarySearch;
       begin
    -    I := L + (R - L) div 2;
    -    CompareRes := MyCompareBookmarks(FDataset, pointer(Item), FList[I]);
    -    //CompareRes := FDataset.CompareBookmarks(Item, TBookmark(FList[I]));
    -    if (CompareRes > 0) then
    -      L := I + 1
    -    else
    +    L := 0;
    +    R := FList.Count - 1;
    +    while (L <= R) do
         begin
    -      R := I - 1;
    -      if (CompareRes = 0) then
    +      I := L + (R - L) div 2;
    +      if FUseCompareBookmarks then
    +        CompareRes := FDataset.CompareBookmarks(Item, TBookmark(FList[I]))
    +      else
    +        CompareRes := MyCompareBookmarks(FDataset, pointer(Item), FList[I]);
    +      if (CompareRes > 0) then
    +        L := I + 1
    +      else
           begin
    -         Result := True;
    -         L := I;
    +        R := I - 1;
    +        if (CompareRes = 0) then
    +        begin
    +           Result := True;
    +           L := I;
    +        end;
           end;
         end;
    +    AIndex := L;
       end;
    -  AIndex := L;
    +
    +  procedure VisitAll;
    +  begin
    +    AIndex := 0;
    +    i := 0;
    +    while i<FList.Count do begin
    +      if FUseCompareBookmarks then
    +        CompareRes := FDataset.CompareBookmarks(Item, TBookmark(FList[I]))
    +      else
    +        CompareRes := MyCompareBookmarks(FDataset, pointer(Item), FList[I]);
    +      if CompareRes=0 then begin
    +        result := true;
    +        AIndex := i;
    +        exit;
    +      end;
    +      inc(i);
    +    end;
    +  end;
    +
    +begin
    +  {$ifdef dbgDBGrid}
    +  DebugLn('%s.Find', [ClassName]);
    +  {$endif}
    +
    +  Result := False;
    +  if FCanDoBinarySearch then
    +    BinarySearch
    +  else
    +    VisitAll;
     end;
     
     function TBookmarkList.IndexOf(const Item: TBookmark): Integer;
    
    dbgridmultiselect.diff (5,218 bytes)

Relationships

related to 0021876 closedMattias Gaertner [Patch] rev 37053 breaks dbgrid multiselect 

Activities

Petr-K

2013-08-30 16:28

reporter  

dbgrids.patch (1,098 bytes)
Index: dbgrids.pas
===================================================================
--- dbgrids.pas	(revision 42516)
+++ dbgrids.pas	(working copy)
@@ -4033,7 +4033,7 @@
   TDs=class(TDataset)
   end;
 
-function MyCompareBookmarks(ds:Tdataset; b1,b2:pointer): Integer;
+{function MyCompareBookmarks(ds:Tdataset; b1,b2:pointer): Integer;
 begin
   if b1=b2 then
     result := 0
@@ -4047,7 +4047,7 @@
     // Note: Tds(ds).bookmarksize is set at creation of TDataSet and does not change
     result := CompareMemRange(b1,b2,Tds(ds).bookmarksize);
   end;
-end;
+end;}
 
 function TBookmarkList.Find(const Item: TBookmark; var AIndex: Integer): boolean;
 var
@@ -4061,8 +4061,8 @@
   while (L <= R) do
   begin
     I := L + (R - L) div 2;
-    CompareRes := MyCompareBookmarks(FDataset, pointer(Item), FList[I]);
-    //CompareRes := FDataset.CompareBookmarks(Item, TBookmark(FList[I]));
+    //CompareRes := MyCompareBookmarks(FDataset, pointer(Item), FList[I]);
+    CompareRes := FDataset.CompareBookmarks(Item, TBookmark(FList[I]));
     if (CompareRes > 0) then
       L := I + 1
     else
dbgrids.patch (1,098 bytes)

Petr-K

2013-08-30 16:33

reporter   ~0069594

This hack was added in r37053 by mattias.

zbyna

2013-12-09 01:23

reporter   ~0071826

function TBookmarkList.Find(const Item: TBookmark; var AIndex: Integer): boolean;
var
  L, R, I: Integer;
  CompareRes: PtrInt;

Changing CompareRes:PtrUInt fixed order of selected records in dbgrid (according to dataset) but only some of selected rows are displayed

zbyna

2013-12-10 17:54

reporter   ~0071855

This bug appears only when using SQLDb package.
In ZeosDB 7.1.0-beta no such issue.
Seems like bug related underlying dataset (Tbufdataset ?)than DBgrid

Installed:
Typhon IDE 4.5.0 Synchronize with Lazarus Source SVN 16-09-2013 Rev 42844.
FreePascal 2.7.1 Source from SVN 16-09-2013 Rev 25492

Jesus Reyes

2014-10-06 19:11

developer   ~0078023

Do this problem still exists in recent Lazarus versions?

Petr-K

2014-11-06 13:34

reporter   ~0078954

Problem still persist.
I tested in Lazarus r46763, fpc r27806.

Here is comparing code in zeosdb v5:
function TZZDataset.CompareBookmarks(Bookmark1, Bookmark2: TBookmark): Integer;
var
  Index1, Index2: Integer;
begin
  Result := 0;
  if not Assigned(Bookmark1) or not Assigned(Bookmark2) then
    Exit;
  Index1 := SqlBuffer.IndexOfIndex(PInteger(Bookmark1)^);
  Index2 := SqlBuffer.IndexOfIndex(PInteger(Bookmark2)^);
  if Index1 < Index2 then Result := -1
  else if Index1 > Index2 then Result := 1;
end;

It is very different from simple CompareMemRange.

Jesus Reyes

2014-11-09 04:39

developer  

dbgridmultiselect.diff (5,218 bytes)
Index: dbgrids.pas
===================================================================
--- dbgrids.pas	(revision 46763)
+++ dbgrids.pas	(working copy)
@@ -131,6 +131,8 @@
     FList: TFPList; // list of TBookmark
     FGrid: TCustomDbGrid;
     FDataset: TDataset;
+    FUseCompareBookmarks: boolean;
+    FCanDoBinarySearch: boolean;
     function GetCount: integer;
     function GetCurrentRowSelected: boolean;
     function GetItem(AIndex: Integer): TBookmark;
@@ -4133,9 +4135,9 @@
   Bookmark: TBookmark;
 begin
   CheckActive;
-  Bookmark := FGrid.Datasource.Dataset.GetBookmark;
+  Bookmark := FDataset.GetBookmark;
   Result := IndexOf(Bookmark)>=0;
-  FGrid.Datasource.Dataset.FreeBookmark(Bookmark);
+  FDataset.FreeBookmark(Bookmark);
 end;
 
 function TBookmarkList.GetItem(AIndex: Integer): TBookmark;
@@ -4151,12 +4153,10 @@
   CheckActive;
 
   Bookmark := nil;
-  TBookmark(Bookmark) := FGrid.Datasource.Dataset.GetBookmark; // fetch and increase reference count
+  TBookmark(Bookmark) := FDataset.GetBookmark; // fetch and increase reference count
   if Bookmark = nil then
     Exit;
 
-  FDataset := FGrid.Datasource.Dataset;
-
   if Find(Bookmark, Index) then begin
     FDataset.FreeBookmark(Bookmark);
     {$ifndef noautomatedbookmark}
@@ -4189,6 +4189,43 @@
   {$endif}
   if not FGrid.FDataLink.Active then
     raise EInvalidGridOperation.Create('Dataset Inactive');
+
+  if FGrid.DataSource.DataSet=FDataset then
+    exit;
+  FDataset := FGrid.DataSource.DataSet;
+
+  // Note.
+  //
+  // Some dataset descendants do not implement CompareBookmarks, for these we
+  // use MyCompareBookmarks in the hope the allocated bookmark memory is used
+  // to hold some kind of record index.
+  FUseCompareBookmarks := TMethod(@FDataset.CompareBookmarks).Code<>pointer(@TDataset.CompareBookmarks);
+
+  // Note.
+  //
+  // fpc help say CompareBookmarks should return -1, 0 or 1 ... which imply that
+  // bookmarks should be a sorted array (or list). In this scenario binary search
+  // is the prefered method for finding a bookmark.
+  //
+  // The problem here is that TBufDataset and TSQLQuery (and thus TCustomSQLQuery
+  // and TCustomBufDataset) CompareBookmarks only return 0 or -1 (some kind of
+  // is this a valid bookmark or not), the result is that it appears as an unsorted
+  // list (or array) and binary search should not be used.
+  //
+  // The weird thing is that if we use MyCompareBookmarks which deals with comparing
+  // the memory reserved for bookmarks in the hope bookmarks are just some kind of
+  // reocord indexes, currently work fine for TCustomBufDataset derived datasets.
+  // however using CompareBookmarks is always the right thing to use where implemented.
+  //
+  // As Dbgrid should be TDataset implementation agnostic this is a way I found
+  // to know if the dataset is derived from TCustomBufDataset or not.
+  // Once TCustomBufDataset is fixed, remove this ugly note & hack.
+  case FDataset.ClassName of
+    'TSQLQuery','TBufDataset','TCustomSQLQuery','TCustomBufDataset':
+      FCanDoBinarySearch := false;
+    else
+      FCanDoBinarySearch := true;
+  end;
 end;
 
 constructor TBookmarkList.Create(AGrid: TCustomDBGrid);
@@ -4273,33 +4310,62 @@
 function TBookmarkList.Find(const Item: TBookmark; var AIndex: Integer): boolean;
 var
   L, R, I: Integer;
-  CompareRes: PtrInt;
-begin
-  {$ifdef dbgDBGrid}
-  DebugLn('%s.Find', [ClassName]);
-  {$endif}
-  // From TStringList.Find() Use binary search.
-  Result := False;
-  L := 0;
-  R := FList.Count - 1;
-  while (L <= R) do
+  CompareRes: Integer;
+
+  procedure BinarySearch;
   begin
-    I := L + (R - L) div 2;
-    CompareRes := MyCompareBookmarks(FDataset, pointer(Item), FList[I]);
-    //CompareRes := FDataset.CompareBookmarks(Item, TBookmark(FList[I]));
-    if (CompareRes > 0) then
-      L := I + 1
-    else
+    L := 0;
+    R := FList.Count - 1;
+    while (L <= R) do
     begin
-      R := I - 1;
-      if (CompareRes = 0) then
+      I := L + (R - L) div 2;
+      if FUseCompareBookmarks then
+        CompareRes := FDataset.CompareBookmarks(Item, TBookmark(FList[I]))
+      else
+        CompareRes := MyCompareBookmarks(FDataset, pointer(Item), FList[I]);
+      if (CompareRes > 0) then
+        L := I + 1
+      else
       begin
-         Result := True;
-         L := I;
+        R := I - 1;
+        if (CompareRes = 0) then
+        begin
+           Result := True;
+           L := I;
+        end;
       end;
     end;
+    AIndex := L;
   end;
-  AIndex := L;
+
+  procedure VisitAll;
+  begin
+    AIndex := 0;
+    i := 0;
+    while i<FList.Count do begin
+      if FUseCompareBookmarks then
+        CompareRes := FDataset.CompareBookmarks(Item, TBookmark(FList[I]))
+      else
+        CompareRes := MyCompareBookmarks(FDataset, pointer(Item), FList[I]);
+      if CompareRes=0 then begin
+        result := true;
+        AIndex := i;
+        exit;
+      end;
+      inc(i);
+    end;
+  end;
+
+begin
+  {$ifdef dbgDBGrid}
+  DebugLn('%s.Find', [ClassName]);
+  {$endif}
+
+  Result := False;
+  if FCanDoBinarySearch then
+    BinarySearch
+  else
+    VisitAll;
 end;
 
 function TBookmarkList.IndexOf(const Item: TBookmark): Integer;
dbgridmultiselect.diff (5,218 bytes)

Jesus Reyes

2014-11-09 04:45

developer   ~0079034

Using CompareBookmarks do not work everywhere because it's not implemented in all TDataset descendants, Using it for TCustomBufDataset derived datasets (like TSQLQuery and TBufDataset) won't work also because its implementation is wrong (it only return -1 or 0).

In the attached patch I tried to solve the problem, please test.

Petr-K

2014-11-10 10:16

reporter   ~0079061

Your patch works.

Issue History

Date Modified Username Field Change
2013-08-30 16:28 Petr-K New Issue
2013-08-30 16:28 Petr-K File Added: dbgrids.patch
2013-08-30 16:33 Petr-K Note Added: 0069594
2013-08-30 21:03 Jesus Reyes Assigned To => Jesus Reyes
2013-08-30 21:03 Jesus Reyes Status new => assigned
2013-08-31 12:08 Reinier Olislagers Relationship added related to 0021876
2013-12-09 01:23 zbyna Note Added: 0071826
2013-12-10 17:54 zbyna Note Added: 0071855
2014-10-06 19:11 Jesus Reyes LazTarget => -
2014-10-06 19:11 Jesus Reyes Note Added: 0078023
2014-10-06 19:11 Jesus Reyes Status assigned => feedback
2014-11-06 13:34 Petr-K Note Added: 0078954
2014-11-06 13:34 Petr-K Status feedback => assigned
2014-11-09 04:39 Jesus Reyes File Added: dbgridmultiselect.diff
2014-11-09 04:45 Jesus Reyes Note Added: 0079034
2014-11-09 04:46 Jesus Reyes Status assigned => feedback
2014-11-10 10:16 Petr-K Note Added: 0079061
2014-11-10 10:16 Petr-K Status feedback => assigned
2014-11-10 17:37 Jesus Reyes Fixed in Revision => 46797
2014-11-10 17:37 Jesus Reyes LazTarget - => 1.4
2014-11-10 17:37 Jesus Reyes Status assigned => resolved
2014-11-10 17:37 Jesus Reyes Fixed in Version => 1.3 (SVN)
2014-11-10 17:37 Jesus Reyes Resolution open => fixed
2014-11-10 17:37 Jesus Reyes Target Version => 1.4
2014-11-11 08:55 Petr-K Status resolved => closed