View Issue Details

IDProjectCategoryView StatusLast Update
0031061LazarusPatchpublic2017-01-24 16:05
ReporterGabor BorosAssigned ToJesus Reyes 
PrioritynormalSeverityminorReproducibilityN/A
Status closedResolutionfixed 
Product Version1.7 (SVN)Product Build 
Target Version1.8Fixed in Version1.7 (SVN) 
Summary0031061: DBGrid - Implement dgRowMove
DescriptionI try to develop a row reordering feature for my application but the TDBGrid not have TStringGrid like row moving feature. The attached patch and the example project works for me (one row can be moved by the indicator column). But don't know where is the correct place for fGridState:=gsRowMoving. I know only one problem (double click?), sometimes ToIndex=0 in OnRowMoved event.
TagsNo tags attached.
Fixed in Revision53837, 53986
LazTarget1.8
Widgetset
Attached Files
  • dbgrids.pas.patch (2,608 bytes)
    Index: lcl/dbgrids.pas
    ===================================================================
    --- lcl/dbgrids.pas	(revision 53521)
    +++ lcl/dbgrids.pas	(working copy)
    @@ -73,7 +73,8 @@
         dgCellEllipsis,                     // show ... if cell text is truncated
         dgRowHighlight,                     // Highlight current row
         dgThumbTracking,
    -    dgDblClickAutoSize                  // dblclicking columns borders (on hdrs) resize col.
    +    dgDblClickAutoSize,                 // dblclicking columns borders (on hdrs) resize col.
    +    dgRowMove
       );
       TDbGridOptions = set of TDbGridOption;
     
    @@ -314,6 +315,7 @@
         FKeySign: Integer;
         FSavedRecord: Integer;
         FOnGetCellHint: TDbGridCellHintEvent;
    +    FOnRowMoved: TMovedEvent;
         procedure EmptyGrid;
         function GetColumns: TDBGridColumns;
         function GetCurrentColumn: TColumn;
    @@ -494,6 +496,7 @@
         property OnTitleClick: TDBGridClickEvent read FOnTitleClick write FOnTitleClick;
         property OnUserCheckboxBitmap: TDbGridCheckboxBitmapEvent read FOnCheckboxBitmap write FOnCheckboxBitmap;
         property OnUserCheckboxState: TDbGridCheckboxStateEvent read FOnCheckboxState write FOnCheckboxState;
    +    property OnRowMoved: TMovedEvent read FOnRowMoved write FOnRowMoved;
       public
         constructor Create(AOwner: TComponent); override;
         procedure AutoAdjustColumns; override;
    @@ -615,6 +618,7 @@
         property OnMouseWheelDown;
         property OnMouseWheelUp;
         property OnPrepareCanvas;
    +    property OnRowMoved;
         property OnSelectEditor;
         //property OnStartDock;
         property OnStartDrag;
    @@ -1168,6 +1172,11 @@
         else
           Exclude(OldOptions, goDblClickAutoSize);
     
    +    if dgRowMove in fOptions then
    +      Include(OldOptions, goRowMoving)
    +    else
    +      Exclude(OldOptions, goRowMoving);
    +
         if (dgIndicator in ChangedOptions) then begin
           if (dgIndicator in FOptions) then
             FixedCols := FixedCols + 1
    @@ -1987,7 +1996,7 @@
         end;
         if Assigned(OnColumnMoved) then
           OnColumnMoved(Self, FromIndex, ToIndex);
    -  end;
    +  end else if Assigned(OnRowMoved) then OnRowMoved(Self, FromIndex, ToIndex);
     end;
     
     function TCustomDBGrid.ColumnEditorStyle(aCol: Integer; F: TField): TColumnButtonStyle;
    @@ -2624,8 +2633,10 @@
               else begin
                 if Button=mbLeft then
                   ClearSelection(true);
    -            if gz=gzFixedRows then
    -              doMouseDown
    +            if gz=gzFixedRows then begin
    +              fGridState:=gsRowMoving;
    +              doMouseDown;
    +            end
                 else
                   doInherited;
               end;
    
    dbgrids.pas.patch (2,608 bytes)
  • DBGrid_RowMoving.tar.gz (2,081 bytes)
  • DBGrid_OnRowMoved.tar.gz (1,942 bytes)
  • OnRowMoved.png (27,190 bytes)
    OnRowMoved.png (27,190 bytes)
  • OnRowMoved.patch (1,018 bytes)
    Index: lcl/dbgrids.pas
    ===================================================================
    --- lcl/dbgrids.pas	(revision 53964)
    +++ lcl/dbgrids.pas	(working copy)
    @@ -2643,6 +2643,7 @@
                   ClearSelection(true);
                 if gz=gzFixedRows then begin
                   fGridState:=gsRowMoving;
    +              MoveLast:=Point(-1,-1);
                   doMouseDown;
                 end
                 else
    Index: lcl/grids.pas
    ===================================================================
    --- lcl/grids.pas	(revision 53964)
    +++ lcl/grids.pas	(working copy)
    @@ -1215,6 +1215,7 @@
         property OnGetCellHint : TGetCellHintEvent read FOnGetCellHint write FOnGetCellHint;
         property OnSaveColumn: TSaveColumnEvent read FOnSaveColumn write FOnSaveColumn;
         property OnLoadColumn: TSaveColumnEvent read FOnLoadColumn write FOnLoadColumn;
    +    property MoveLast: TPoint read FMoveLast write FMoveLast;
       public
         constructor Create(AOwner: TComponent); override;
         destructor Destroy; override;
    
    OnRowMoved.patch (1,018 bytes)
  • dbgrid-move_row_by_drag_and_drop.zip (2,699 bytes)

Activities

Gabor Boros

2016-12-02 19:48

reporter  

dbgrids.pas.patch (2,608 bytes)
Index: lcl/dbgrids.pas
===================================================================
--- lcl/dbgrids.pas	(revision 53521)
+++ lcl/dbgrids.pas	(working copy)
@@ -73,7 +73,8 @@
     dgCellEllipsis,                     // show ... if cell text is truncated
     dgRowHighlight,                     // Highlight current row
     dgThumbTracking,
-    dgDblClickAutoSize                  // dblclicking columns borders (on hdrs) resize col.
+    dgDblClickAutoSize,                 // dblclicking columns borders (on hdrs) resize col.
+    dgRowMove
   );
   TDbGridOptions = set of TDbGridOption;
 
@@ -314,6 +315,7 @@
     FKeySign: Integer;
     FSavedRecord: Integer;
     FOnGetCellHint: TDbGridCellHintEvent;
+    FOnRowMoved: TMovedEvent;
     procedure EmptyGrid;
     function GetColumns: TDBGridColumns;
     function GetCurrentColumn: TColumn;
@@ -494,6 +496,7 @@
     property OnTitleClick: TDBGridClickEvent read FOnTitleClick write FOnTitleClick;
     property OnUserCheckboxBitmap: TDbGridCheckboxBitmapEvent read FOnCheckboxBitmap write FOnCheckboxBitmap;
     property OnUserCheckboxState: TDbGridCheckboxStateEvent read FOnCheckboxState write FOnCheckboxState;
+    property OnRowMoved: TMovedEvent read FOnRowMoved write FOnRowMoved;
   public
     constructor Create(AOwner: TComponent); override;
     procedure AutoAdjustColumns; override;
@@ -615,6 +618,7 @@
     property OnMouseWheelDown;
     property OnMouseWheelUp;
     property OnPrepareCanvas;
+    property OnRowMoved;
     property OnSelectEditor;
     //property OnStartDock;
     property OnStartDrag;
@@ -1168,6 +1172,11 @@
     else
       Exclude(OldOptions, goDblClickAutoSize);
 
+    if dgRowMove in fOptions then
+      Include(OldOptions, goRowMoving)
+    else
+      Exclude(OldOptions, goRowMoving);
+
     if (dgIndicator in ChangedOptions) then begin
       if (dgIndicator in FOptions) then
         FixedCols := FixedCols + 1
@@ -1987,7 +1996,7 @@
     end;
     if Assigned(OnColumnMoved) then
       OnColumnMoved(Self, FromIndex, ToIndex);
-  end;
+  end else if Assigned(OnRowMoved) then OnRowMoved(Self, FromIndex, ToIndex);
 end;
 
 function TCustomDBGrid.ColumnEditorStyle(aCol: Integer; F: TField): TColumnButtonStyle;
@@ -2624,8 +2633,10 @@
           else begin
             if Button=mbLeft then
               ClearSelection(true);
-            if gz=gzFixedRows then
-              doMouseDown
+            if gz=gzFixedRows then begin
+              fGridState:=gsRowMoving;
+              doMouseDown;
+            end
             else
               doInherited;
           end;
dbgrids.pas.patch (2,608 bytes)

Gabor Boros

2016-12-02 19:48

reporter  

DBGrid_RowMoving.tar.gz (2,081 bytes)

Jesus Reyes

2016-12-19 08:07

developer   ~0096927

The problem I see with this patch is that the name "OnRowMoved" gives the impression that it works like OnColumnMoved, but it is a false impression. I think that because dgColumnMoved not only works like a notification about the fact that a column was already moved, the grid actually performs the column movement. This is precisely the difference with the OnRowMoved: the grid will not perform the row movement, it will be just a notification. For this reason I think this patch should not be applied.

Gabor Boros

2016-12-19 08:52

reporter   ~0096929

If you have naming problem only with my patch, please modify it. OnRowMove?

Jesus Reyes

2016-12-19 18:34

developer   ~0096938

The naming is not a problem because it describes very well the notification, as said the problem is that the grid doesn't implement the record reordering it self, a grid user might be surprised when hooking the OnRowMoved and finding out that it will not work like the OnColumnMoved event. That is, the grid implements moving the column but it will not implement the row moving action.

What we can do is implement and document the OnRowMoved event as a public property, so it doesn't appear in the object inspector but it will remain available for those interested on it by wiring it in code.

Gabor Boros

2016-12-19 19:37

reporter   ~0096939

Now I understand what is your problem. OnRowMoved as a public (and not published) property is enough from my POV.

Jesus Reyes

2017-01-02 19:01

developer   ~0097248

Applied with changes, please test.

Gabor Boros

2017-01-03 19:00

reporter  

DBGrid_OnRowMoved.tar.gz (1,942 bytes)

Gabor Boros

2017-01-03 19:06

reporter   ~0097275

Created and attached a new example project for the committed patch. Start it, double click on the indicator and the returned ToIndex value is 0 (displayed on the form's caption). Is this the expected behavior?

Jesus Reyes

2017-01-03 20:07

developer   ~0097277

AFAIU if there is not movement, the OnRowMoved should not be triggered. In your example I implemented OnColumnMoved and it doesn't trigger on column double click, so something there is not working right...

Gabor Boros

2017-01-04 09:49

reporter   ~0097282

If put a simple ShowMessage('OnRowMoved'); into the OnRowMoved event then start the example project and double click on the indicator got the attached (OnRowMoved.png) result. The red line drawed above the column titles.
At doubleclick value of ToIndex in TCustomGrid.DoOPMoveColRow is 0. If move a column before row indicator doubleclick value of ToIndex is invalid. For example if move NAME column to the first place then doubleclick on the row indicator value of ToIndex in DoOPMoveColRow is 12.

Gabor Boros

2017-01-04 09:49

reporter  

OnRowMoved.png (27,190 bytes)
OnRowMoved.png (27,190 bytes)

Gabor Boros

2017-01-17 19:45

reporter   ~0097558

Last edited: 2017-01-17 19:46

View 2 revisions

I have a simple test case for the earlier described problem. Start the attached example project, move NAME column to the first place (before the ID column) then click on the black indicator triangle. The result is "Grid index out of range". Tried to debug the problem and found the source of it. I created and attached a patch (OnRowMoved.patch) which solve the problem.

Gabor Boros

2017-01-17 19:46

reporter  

OnRowMoved.patch (1,018 bytes)
Index: lcl/dbgrids.pas
===================================================================
--- lcl/dbgrids.pas	(revision 53964)
+++ lcl/dbgrids.pas	(working copy)
@@ -2643,6 +2643,7 @@
               ClearSelection(true);
             if gz=gzFixedRows then begin
               fGridState:=gsRowMoving;
+              MoveLast:=Point(-1,-1);
               doMouseDown;
             end
             else
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 53964)
+++ lcl/grids.pas	(working copy)
@@ -1215,6 +1215,7 @@
     property OnGetCellHint : TGetCellHintEvent read FOnGetCellHint write FOnGetCellHint;
     property OnSaveColumn: TSaveColumnEvent read FOnSaveColumn write FOnSaveColumn;
     property OnLoadColumn: TSaveColumnEvent read FOnLoadColumn write FOnLoadColumn;
+    property MoveLast: TPoint read FMoveLast write FMoveLast;
   public
     constructor Create(AOwner: TComponent); override;
     destructor Destroy; override;
OnRowMoved.patch (1,018 bytes)

wp

2017-01-17 22:26

developer   ~0097559

Last edited: 2017-01-17 22:28

View 2 revisions

I think user-defined row-reorderung should not be possible in a DBGrid at all because the order of the rows is defined by the order in the dataset. Whenever the dataset is refreshed the user-defined order will be lost. This will be VERY confusing!

Delphi's DBGrid doesn't have an OnRowMoved event either.

Gabor Boros

2017-01-17 22:55

reporter   ~0097560

wp, the OnRowMoved event just a helper to accomplish the needed task. Please try the second example project (DBGrid_OnRowMoved.tar.gz) with current trunk and will see.

wp

2017-01-17 23:26

developer   ~0097561

If I understand this correctly you want to double click on an indicator cell and this should resort the dataset, and you want to use the new OnRowMoved event to write a new order index into the db?

But why don't you use the OnDblClick event to trigger your action? You only have to query where the double click occurs and then you can do the same that you do in OnRowMoved.

I think having this event here - even if it is only public - will be very confusing. This looks very wrong to me.

Gabor Boros

2017-01-18 08:02

reporter   ~0097564

Last edited: 2017-01-18 08:06

View 2 revisions

Resort with double click? No. The double click thing just a bug which solved by the second patch if apply on the current trunk. I try to accomplish a reordering feature in my application with row moving just like a TStringGrid. The OnRowMoved event needed to know which row go to which place. Please try the second example project. You can reorder the records by moving rows (grab the indicator triangle then move).

Michl

2017-01-18 09:32

developer   ~0097567

> I think user-defined row-reorderung should not be possible in a DBGrid at all
> because the order of the rows is defined by the order in the dataset.
> Whenever the dataset is refreshed the user-defined order will be lost. This
> will be VERY confusing!

+ 1

I think, a DBGrid should hold data, getting from dataset. If someone will move rows in the grid, the sorting entry of the dataset should be changed. I do it so in some cases.


> I try to accomplish a reordering feature in my application with row moving just
> like a TStringGrid.

This is also a possibility. You can use a TStringGrid or TDrawGrid and fill it with your data from dataset and take it for sorting.

Just curious (I don't tested your patches/examples), did you try to derive from TDBGrid and implement such a feature in your own TMyDBGrid?

wp

2017-01-18 09:50

developer   ~0097568

Michl, I think the point is that he wants to reorder the table visually. The change in the order parameter is written back into the database so that the next dataset refresh will return records in the new order.

Gabor, I missed that you want to reorder by some kind of drag and drop (maybe I did not read carefully enough, but you never said that explicitly). But anyway, can't you use the standard drag and drop events for your purpose?

Gabor Boros

2017-01-18 10:32

reporter   ~0097570

Michl, wp, I try to follow this TDBGrid patching way because of it's ancestor (TCustomGrid) have all needed feature (row moving, draw red line, etc.). I din't know this is an "evil" thing from Lazarus POV. I don't understand why confusing this event because a dataset have records not rows. OnRowMoved or OnRowMove or OnRowDragDrop... I just try to follow the existing features of TCustomGrid. The whole feature is "VERY confusing" or just its naming?

Michl

2017-01-18 10:59

developer   ~0097571

> I try to follow this TDBGrid patching way because of it's ancestor
> (TCustomGrid) have all needed feature (row moving, draw red line, etc.).

Generally I like that idea! But what happened after a DataSet.Refresh?

Gabor Boros

2017-01-18 11:16

reporter   ~0097572

Last edited: 2017-01-18 11:17

View 2 revisions

The OnRowMoved is just a "notification" like OnClick. In the real life application (with a Firebird database) I need a reordering feature and I just to know from Lazarus/DBGrid the user want to move the actual second row to the fourth place and all database/dataset magic will be in my application. So the DBGrid will do nothing with the dataset.

wp

2017-01-18 12:14

developer  

dbgrid-move_row_by_drag_and_drop.zip (2,699 bytes)

wp

2017-01-18 12:22

developer   ~0097573

Last edited: 2017-01-18 12:35

View 3 revisions

The attached demo "dbgrid_move_row_by_drag_and_drop" shows how to move rows in the grid by drag and drop machinery only, without any OnRowMoved event. You can initiate drag and drop from anywhere within the row, but of course you can add restrictions to initiate from the indicator column only. I did not manage so far to display the thick insertion line. However, it should be possible to display some drag information in a hovering drag object window (like in some components of the tvPlanIt package).

> I din't know this is an "evil" thing from Lazarus POV. I don't understand why
> confusing this event because a dataset have records not rows. OnRowMoved or
> OnRowMove or OnRowDragDrop...

I would not say the event is an "evil" thing. I just fear that user may conclude that the row has been moved (like Jesus said above). There is a fundamental difference between TStringGrid and TDBGrid: The stringgrid contains the data by its own while the DBGrid is "virtual" in the sense that it displays data which come from somewhere else, from the dataset. So, the DBGrid cannot move rows by its own, it must execute additional code to transfer the new order back to the dataset. You do this, but this code does not belong to the grid, and in fact, it should not belong to it because of the differences in the database types. Without this code, OnRowMoved does not do anything, and this is why I think the event is confusing. The fact that the event is not visible in the Object Inspector just obscures, or maybe even increases, confusion, because everybody will ask himself: Why didn't they publish this event?

Gabor Boros

2017-01-18 14:41

reporter   ~0097574

wp, first of all... Thank you for the drag and drop example!
I don't want to reinvent the wheel and TCustomGrid have almost everything what I need. Just see the revision link below. I tried your example and if decrease the size of the form to see only three rows (ID 1,2,3) then move the first (ID 1) row to the empty space after third (ID 3) row the new place for the first (ID 1) row is the last place. With the OnRowMoved event inserted between third (ID 3) and fourth (ID 4).

For me the name of the event is no problem. I am will be happy with OnRowSomething too. In my patch the OnRowMoved is a published property but Jesus not like it and committed a modified (include published->public) version of my patch. But he not had naming problem.

http://svn.freepascal.org/cgi-bin/viewvc.cgi/trunk/lcl/dbgrids.pas?root=lazarus&r1=53837&r2=53836&pathrev=53837

wp

2017-01-18 15:50

developer   ~0097576

First of all: It it Jesus' decision. If he can live with the present solution (which he probably can because otherwise he would not have committed the patch) I will accept it as well. I just wanted to point out an issue which I had not seen in the previous discussion; people are often very enthusiastic to request changes of the LCL which they easily could achieve in other ways without modification of the LCL.

As for the issues with my example: Yes I see the problem, I just did not fully test it. The problem is the call of MouseCoord for conversion of x/y coordinates to column/row indexes. It returns a -1 if x/y is below the last row complete in height (I thought -1 would result only below the very last row).
I am sure that this certainly can be fixed - there is also a MouseToCell method.

Gabor Boros

2017-01-18 18:59

reporter   ~0097580

wp, created the patch and an example project for it because I think this feature is will be good to others. Just a new event, no more. Is it bad if LCL have more and more features? In this case why not exists a base LCL for example with a blunt Delphi level TDBGrid and an extended LCL for example with TDBGridEx with all other features? Okay, okay, historic reasons, harder to maintain, etc. Development of Lazarus is "VERY confusing" (I quoted you) for me. Sometimes read in the commit log "changed for Delphi compatibility" etc. sometimes read "Lazarus is not Delphi". You wrote earlier "Delphi's DBGrid doesn't have an OnRowMoved event either.", but have many others. I follow the development of Lazarus from years but don't know what is the direction.

Jesus Reyes

2017-01-23 21:22

developer   ~0097647

Implemented in r53986 other alternative, please test.

Jesus Reyes

2017-01-23 21:44

developer   ~0097648

As you may have noted, using the Drag/Drop events may in part solve this feature but not fully an it would really duplicate in part the already existing functionality.

I decided to implement this feature because almost all the needed code was already in place, no even an option for it was necessary. It is something that should be documented in order to avoid misunderstandings and the first step in that direction was to lowered the visibility of the OnRowMoved event.

Gabor Boros

2017-01-24 16:05

reporter   ~0097669

Tested with trunk 53993. Thank you very much!

Issue History

Date Modified Username Field Change
2016-12-02 19:47 Gabor Boros New Issue
2016-12-02 19:48 Gabor Boros File Added: dbgrids.pas.patch
2016-12-02 19:48 Gabor Boros File Added: DBGrid_RowMoving.tar.gz
2016-12-17 22:59 Jesus Reyes Assigned To => Jesus Reyes
2016-12-17 22:59 Jesus Reyes Status new => assigned
2016-12-19 08:07 Jesus Reyes Note Added: 0096927
2016-12-19 08:52 Gabor Boros Note Added: 0096929
2016-12-19 18:34 Jesus Reyes Note Added: 0096938
2016-12-19 19:37 Gabor Boros Note Added: 0096939
2017-01-02 19:01 Jesus Reyes Fixed in Revision => 53837
2017-01-02 19:01 Jesus Reyes LazTarget => 1.8
2017-01-02 19:01 Jesus Reyes Note Added: 0097248
2017-01-02 19:01 Jesus Reyes Status assigned => resolved
2017-01-02 19:01 Jesus Reyes Fixed in Version => 1.7 (SVN)
2017-01-02 19:01 Jesus Reyes Resolution open => fixed
2017-01-02 19:01 Jesus Reyes Target Version => 1.8
2017-01-03 19:00 Gabor Boros File Added: DBGrid_OnRowMoved.tar.gz
2017-01-03 19:06 Gabor Boros Note Added: 0097275
2017-01-03 20:07 Jesus Reyes Note Added: 0097277
2017-01-04 09:49 Gabor Boros Note Added: 0097282
2017-01-04 09:49 Gabor Boros File Added: OnRowMoved.png
2017-01-17 19:45 Gabor Boros Note Added: 0097558
2017-01-17 19:45 Gabor Boros Status resolved => assigned
2017-01-17 19:45 Gabor Boros Resolution fixed => reopened
2017-01-17 19:46 Gabor Boros File Added: OnRowMoved.patch
2017-01-17 19:46 Gabor Boros Note Edited: 0097558 View Revisions
2017-01-17 22:26 wp Note Added: 0097559
2017-01-17 22:28 wp Note Edited: 0097559 View Revisions
2017-01-17 22:55 Gabor Boros Note Added: 0097560
2017-01-17 23:26 wp Note Added: 0097561
2017-01-18 08:02 Gabor Boros Note Added: 0097564
2017-01-18 08:06 Gabor Boros Note Edited: 0097564 View Revisions
2017-01-18 09:32 Michl Note Added: 0097567
2017-01-18 09:50 wp Note Added: 0097568
2017-01-18 10:32 Gabor Boros Note Added: 0097570
2017-01-18 10:59 Michl Note Added: 0097571
2017-01-18 11:16 Gabor Boros Note Added: 0097572
2017-01-18 11:17 Gabor Boros Note Edited: 0097572 View Revisions
2017-01-18 12:14 wp File Added: dbgrid-move_row_by_drag_and_drop.zip
2017-01-18 12:22 wp Note Added: 0097573
2017-01-18 12:34 wp Note Edited: 0097573 View Revisions
2017-01-18 12:35 wp Note Edited: 0097573 View Revisions
2017-01-18 14:41 Gabor Boros Note Added: 0097574
2017-01-18 15:50 wp Note Added: 0097576
2017-01-18 18:59 Gabor Boros Note Added: 0097580
2017-01-23 21:22 Jesus Reyes Fixed in Revision 53837 => 53837, 53986
2017-01-23 21:22 Jesus Reyes Note Added: 0097647
2017-01-23 21:22 Jesus Reyes Status assigned => resolved
2017-01-23 21:22 Jesus Reyes Resolution reopened => fixed
2017-01-23 21:44 Jesus Reyes Note Added: 0097648
2017-01-24 16:05 Gabor Boros Note Added: 0097669
2017-01-24 16:05 Gabor Boros Status resolved => closed