| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0031061 | Lazarus | Patch | public | 2016-12-02 19:47 | 2017-01-24 16:05 |
| Reporter | Gabor Boros | Assigned To | Jesus Reyes | ||
| Priority | normal | Severity | minor | Reproducibility | N/A |
| Status | closed | Resolution | fixed | ||
| Product Version | 1.7 (SVN) | Product Build | |||
| Target Version | 1.8 | Fixed in Version | 1.7 (SVN) | ||
| Summary | 0031061: DBGrid - Implement dgRowMove | ||||
| Description | I 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. | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | 53837, 53986 | ||||
| LazTarget | 1.8 | ||||
| Widgetset | |||||
| Attached Files |
| ||||
|
|
dbgrids.pas.patch (2,608 bytes) |
|
|
DBGrid_RowMoving.tar.gz (2,081 bytes) |
|
|
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. |
|
|
If you have naming problem only with my patch, please modify it. OnRowMove? |
|
|
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. |
|
|
Now I understand what is your problem. OnRowMoved as a public (and not published) property is enough from my POV. |
|
|
Applied with changes, please test. |
|
|
DBGrid_OnRowMoved.tar.gz (1,942 bytes) |
|
|
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? |
|
|
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... |
|
|
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. |
|
|
OnRowMoved.png (27,190 bytes) |
|
|
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. |
|
|
OnRowMoved.patch (1,018 bytes) |
|
|
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. |
|
|
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. |
|
|
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. |
|
|
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). |
|
|
> 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? |
|
|
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? |
|
|
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? |
|
|
> 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? |
|
|
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. |
|
|
dbgrid-move_row_by_drag_and_drop.zip (2,699 bytes) |
|
|
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? |
|
|
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 |
|
|
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. |
|
|
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. |
|
|
Implemented in r53986 other alternative, please test. |
|
|
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. |
|
|
Tested with trunk 53993. Thank you very much! |
