View Issue Details

IDProjectCategoryView StatusLast Update
0034141LazarusLCLpublic2018-08-24 18:44
Reporterwp Assigned ToBart Broersma  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Target Version1.10Fixed in Version1.10 
Summary0034141: TValueListEditor: inherited public method SortColRow not working
DescriptionSorting of rows in a TValueListEditor presumably is supposed to be done by calling the method Sort of its Strings. However, TValueListEditor, inherits also the public method SortColRow from its ancestors. As demonstrated in the attached project, this method has no effect (I've also seen cases when it crashes the application).

Debugging the issue I found that TValueListEditor.SortByColRow calls TCustomDrawGrid.SortByColRow which in turn calls the virtual method TCustomGrid.Sort. The problem is in the call to DoExchangeColRow which is not implemented by TValueListEditor for its dedicated strings storage. In fact, this method not even is virtual.

There are two ways to fix the issue:
(1) Override the method Sort by the TValueListEditor by calling its special CustomSort which correctly handles its strings. - This is provided in the attached patch.
(2) Replace the DoExchangeColRow in TCustomGrid.Sort by ExchangeColRow (which just calls DoExchangeColRow). Make TCustomGrid.ExchangeColRow virtual. Override it in TValueListEditor such that it exchanges the two strings.
Steps To ReproduceRun attached demo. There is a TValueListEditor at the left and a TStringGrid at the right. Both are populated by the same dummy strings. Click on the button "Sort" to sort by the 1st column (using SortColRow): Nothing happens for the TValueListEditor while the TStringGrid is correctly sorted.
Additional InformationSee also forum discussion https://forum.lazarus.freepascal.org/index.php/topic,42249.msg294625.html
TagsNo tags attached.
Fixed in Revisionr58773
LazTarget1.10
Widgetset
Attached Files

Relationships

has duplicate 0034160 resolvedBart Broersma TValueListEditor Cell Content not Change at Sort 

Activities

wp

2018-08-17 17:09

developer  

wp

2018-08-17 17:09

developer  

valedit.pas.patch (1,391 bytes)   
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58715)
+++ lcl/valedit.pas	(working copy)
@@ -162,6 +162,7 @@
     procedure SetEditText(ACol, ARow: Longint; const Value: string); override;
     procedure SetFixedRows(const AValue: Integer); override;
     procedure SetRowCount(AValue: Integer);
+    procedure Sort(ColSorting: Boolean; index,IndxFrom,IndxTo:Integer); override;
     procedure TitlesChanged(Sender: TObject);
     function ValidateEntry(const ACol,ARow:Integer; const OldValue:string; var NewValue:string): boolean; override;
   public
@@ -181,7 +182,6 @@
     function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
-
     property Modified;
     property Keys[Index: Integer]: string read GetKey write SetKey;
     property Values[const Key: string]: string read GetValue write SetValue;
@@ -1336,6 +1336,14 @@
   end;
 end;
 
+procedure TValueListEditor.Sort(ColSorting: Boolean;
+  index,IndxFrom,IndxTo:Integer);
+begin
+  if not ColSorting then
+    raise Exception.Create('TValueListEditor: Only sorting by columns allowed.');
+  Strings.Sort;
+end;
+
 procedure TValueListEditor.TitlesChanged(Sender: TObject);
 begin
   // Refresh the display.
valedit.pas.patch (1,391 bytes)   

Bart Broersma

2018-08-17 23:45

developer   ~0110095

Last edited: 2018-08-18 00:01

View 2 revisions

What about the index,IndxFrom,IndxTo parameters?
Maybe better override SortColRow() as well?

This component should have been as not inherited from TCustomStringGrid, but the Greek did it that way, so we must follow.

wp

2018-08-18 00:35

developer  

wp

2018-08-18 00:36

developer  

valuelisteditor_sorting_v2.patch (4,366 bytes)   
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 58733)
+++ lcl/grids.pas	(working copy)
@@ -1282,6 +1282,7 @@
 
     procedure EndUpdate(aRefresh: boolean = true);
     procedure EraseBackground(DC: HDC); override;
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); virtual;
     function  Focused: Boolean; override;
     function  HasMultiSelection: Boolean;
     procedure InvalidateCell(aCol, aRow: Integer); overload;
@@ -1379,7 +1380,7 @@
     procedure DeleteColRow(IsColumn: Boolean; index: Integer);
     procedure DeleteCol(Index: Integer); virtual;
     procedure DeleteRow(Index: Integer); virtual;
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     procedure InsertColRow(IsColumn: boolean; index: integer);
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     procedure SortColRow(IsColumn: Boolean; index:Integer); overload;
@@ -3156,7 +3157,7 @@
               (ColSorting     and (DoCompareCells(index, I, index, J)<>0)) or
               (not ColSorting and (DoCompareCells(I, index, J, index)<>0))
             then
-              DoOPExchangeColRow(not ColSorting, I,J);
+              ExchangeColRow(not ColSorting, I,J);
 
           if P=I then
             P:=J
@@ -8143,6 +8144,11 @@
   //
 end;
 
+procedure TCustomGrid.ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+begin
+  //
+end;
+
 function TCustomGrid.Focused: Boolean;
 begin
   Result := CanTab and (HandleAllocated and
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58733)
+++ lcl/valedit.pas	(working copy)
@@ -80,6 +80,8 @@
   private
     FGrid: TValueListEditor;
     FItemProps: TItemPropList;
+    FSortRow1: Integer;
+    FSortRow2: Integer;
     function GetItemProp(const AKeyOrIndex: Variant): TItemProp;
     procedure QuickSortStringsAndItemProps(L, R: Integer; CompareFn: TStringListSortCompare);
     function CanHideShowingEditorAtIndex(Index: Integer): Boolean;
@@ -115,6 +117,8 @@
 
   { TValueListEditor }
 
+  TValueListEditorColIndex = 0..1;
+
   TValueListEditor = class(TCustomStringGrid)
   private
     FTitleCaptions: TStrings;
@@ -162,6 +166,7 @@
     procedure SetEditText(ACol, ARow: Longint; const Value: string); override;
     procedure SetFixedRows(const AValue: Integer); override;
     procedure SetRowCount(AValue: Integer);
+//    procedure Sort(ColSorting: Boolean; index,IndxFrom,IndxTo:Integer); override;
     procedure TitlesChanged(Sender: TObject);
     function ValidateEntry(const ACol,ARow:Integer; const OldValue:string; var NewValue:string): boolean; override;
   public
@@ -176,12 +181,13 @@
     procedure InsertColRow(IsColumn: boolean; index: integer);
     function InsertRow(const KeyName, Value: string; Append: Boolean): Integer;
     procedure InsertRowWithValues(Index: Integer; Values: array of String);
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     function IsEmptyRow: Boolean; {Delphi compatible function}
     function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
-
+    procedure SortColumn(AIndex: TValueListEditorColIndex; AFromRow, AToRow: Integer); overload;
+    procedure SortColumn(AIndex: TValueListEditorColIndex); overload;
     property Modified;
     property Keys[Index: Integer]: string read GetKey write SetKey;
     property Values[const Key: string]: string read GetValue write SetValue;
@@ -910,6 +916,17 @@
   end;
 end;
 
+procedure TValueListEditor.SortColumn(AIndex: TValueListEditorColIndex;
+  AFromRow, AToRow: Integer);
+begin
+  Sort(true, Integer(AIndex), AFromRow, AToRow);
+end;
+
+procedure TValueListEditor.SortColumn(AIndex: TValueListEditorColIndex);
+begin
+  Sort(true, Integer(AIndex), FixedRows, RowCount-1);
+end;
+
 procedure TValueListEditor.StringsChange(Sender: TObject);
 begin
   //Since we never call inherited SetCell, this seems the logical place to do it

wp

2018-08-18 00:52

developer   ~0110096

Last edited: 2018-08-18 00:58

View 3 revisions

I uploaded a new patch which follows the idea that I mentioned above as (2).

It is based on the only problem of TCustomGrid.Sort that it calls a method DoOpExchangeColRow which does not fit to the way TValueListEditor handles its data. The patch replaces here DoOpExchangeColRow by ExchangeColRow which is already implemented for TCustomDrawGird and TValueListEditor. Declaring these as "override" and introducing a virtual TCustomGrid.ExchangeColRow makes that construction compile.

With these changes the standard sorting method of the most elemental grid can be used, and the ValueListEditor can apply its own ExchangeColRows method which is needed to carry the Strings.ItemProps along.

This way TValueListEditor.SortColRow sorts correctly.

But of course, this method is a bit dangerous because the user could misuse it to sort by rows. Therefore I implemented a new method TValueListEditor.SortColumn. Here the index of the column to be sorted can be selected (0 or 1, for keys or values, respectively), and even a range of row indexes is possible.

The new demo (_v2) allows to play with the new possibilities.

wp

2018-08-18 00:54

developer   ~0110097

Last edited: 2018-08-18 00:56

View 2 revisions

> This component should have been as not inherited from TCustomStringGrid, but the
> Greek did it that way, so we must follow.

I just looked: No - it inherits from TCustomDrawGrid in D7 and 10.2CE.

Bart Broersma

2018-08-18 10:02

developer   ~0110107

We should check if altering DoOpExchangeColRow/ExchangeColRow is OK with Jesus.
The advantage of your approach is that it allows the use of OnCompareCells, which would be difficult when using Strings.Sort.

Must we rebase the whole thing to inherit from TCustomDrawGrid?
Why they did not design the whole thing from the ground up (with the grid as a private component, like we have TButtonEdit etc.) only Zeus knows.
The full functionality of the grid component should not be visible for TValueListEditor at all.
(OK, our StringGrid has way more functionality than D's)

Bart Broersma

2018-08-18 10:44

developer   ~0110113

Last edited: 2018-08-18 11:01

View 3 revisions

Less intrusive: make DoOpExchangeColRow virtual and in TValueListEditor override it to just call ExchangeColRow?

See attached: valedit.sort.diff
Can you test that?
My simple tests run OK with it. It calls OnCompareCells too.

Maybe we should rename Sort to InternalSort, so TValueListEditor can have Sort, which just calls SortColRow(True,0)?

B.t.w. You say SortColRow (as it is now) has no effect, but in my sample app it cripples the Strings content.

Bart Broersma

2018-08-18 10:57

developer  

valedit.sort.diff (2,108 bytes)   
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 58638)
+++ lcl/grids.pas	(working copy)
@@ -989,7 +989,7 @@
       const AXProportion, AYProportion: Double); override;
     procedure DoOnChangeBounds; override;
     procedure DoOPDeleteColRow(IsColumn: Boolean; index: Integer);
-    procedure DoOPExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure DoOPExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); virtual;
     procedure DoOPInsertColRow(IsColumn: boolean; index: integer);
     procedure DoOPMoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     procedure DoPasteFromClipboard; virtual;
@@ -3174,6 +3174,7 @@
     until I>=R;
   end;
 begin
+  debugln('TCustomGrid.Sort');
   if RowCount>FixedRows then begin
     CheckIndex(ColSorting, Index);
     CheckIndex(not ColSorting, IndxFrom);
@@ -6236,7 +6237,7 @@
 var
   aColRow: integer;
 begin
-
+  debugln('TCustomGrid.DoOPExchangeColRow: IsColumn=',dbgs(IsColumn));
   if IsColumn and Columns.Enabled then begin
     Columns.ExchangeColumn( ColumnIndexFromGridColumn(Index),
       ColumnIndexFromGridColumn(WithIndex));
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58638)
+++ lcl/valedit.pas	(working copy)
@@ -150,6 +150,7 @@
     procedure ShowColumnTitles;
     procedure AdjustRowCount; virtual;
     procedure DefineCellsProperty(Filer: TFiler); override;
+    procedure DoOPExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     procedure InvalidateCachedRow;
     procedure GetAutoFillColumnInfo(const Index: Integer; var aMin,aMax,aPriority: Integer); override;
     function GetEditText(ACol, ARow: Integer): string; override;
@@ -1104,6 +1105,12 @@
 begin
 end;
 
+procedure TValueListEditor.DoOPExchangeColRow(IsColumn: Boolean; index,
+  WithIndex: Integer);
+begin
+  ExchangeColRow(IsColumn, index, WithIndex);
+end;
+
 procedure TValueListEditor.InvalidateCachedRow;
 begin
   if (Strings.Count = 0) then
valedit.sort.diff (2,108 bytes)   

wp

2018-08-18 13:39

developer   ~0110117

Fine, but I'd just rename TValueListEditor.ExchangeRowCol to .DoIoExchangeRow and move its declaration as "override" into the protected section.

---------------

As for renaming "Sort" to "InternalSort": Yes, that would have been correct in the first place. But now we have the problem that this is a protected method which may be called by third-party grids. FpSpreadsheet's TsWorksheetgrid uses it for example - well, I can fix it easily but it would add another "$IF LCLVersion...". But wouldn't it be sufficient to just overload it with a version without the IsColumn parameter:

  procedure TValueListEditor.Sort(ColumnIndex: TValueListEditorColIndex;
    AFirstRow: Integer = -1; ALastRow: Integer = -1);

-----------------

As for "SortColRow having no effect": Maybe my words were not very precise. I've seen cases when nothing changed, when value cells were duplicated, when the program crashed with an - IIRC - "index out bounds" error.

-----------------

As for inheritance from TCustomDrawGrid or TCustomStringGrid:
I don't think that this makes a big difference. The basic functionality of our grids is introduced in TCustomGrid and TCustomDrawGrid. TCustomStringGrid just adds the functionality to have cell content in TStringGridStrings.

What I don't like is that TValueListEditor adds duplicate storage. I think this is not necessary. In my eyes, the only reason for TValueListStrings is that it provides storage for the ItemProps.

What about inheriting TValueListStrings from TStringGridStrings (instead of TStringList) providing an extra field ItemProps? This would leave all strings access as it is with the stringgrid, keep the Object field, but provide a way to hook the ItemProps in. Another advantage (not tested, just guessing): the specialized QuickSort and CustomSort methods could be removed.

---------------

I am attaching another patch which is based on yours, but
- renames TValueList.ExchangeColRow as described above,
- overrides the inherited (existing) Sort method to raise an exception when sorting by rows is attempted
- adds a new overloaded Sort method to determine the column and row range to be sorted (as described above).

--------------------

I also looked at the ItemProps and found another annoyance: while the cell content of the ValueListEditor is indexed like the other grids, i.e. including with the fixed rows (--> the first data row has index 1 for FixedRows=1), the ItemProps ignore the FixedRows and have index 0 at the first data row! Very confusing!

wp

2018-08-18 13:39

developer  

valuelisteditor_sorting_v3.patch (4,313 bytes)   
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 58735)
+++ lcl/grids.pas	(working copy)
@@ -991,7 +991,7 @@
       const AXProportion, AYProportion: Double); override;
     procedure DoOnChangeBounds; override;
     procedure DoOPDeleteColRow(IsColumn: Boolean; index: Integer);
-    procedure DoOPExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure DoOPExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); virtual;
     procedure DoOPInsertColRow(IsColumn: boolean; index: integer);
     procedure DoOPMoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     procedure DoPasteFromClipboard; virtual;
@@ -3175,6 +3175,7 @@
     until I>=R;
   end;
 begin
+  debugln('TCustomGrid.Sort');
   if RowCount>FixedRows then begin
     CheckIndex(ColSorting, Index);
     CheckIndex(not ColSorting, IndxFrom);
@@ -6274,7 +6275,7 @@
 var
   aColRow: integer;
 begin
-
+  debugln('TCustomGrid.DoOPExchangeColRow: IsColumn=',dbgs(IsColumn));
   if IsColumn and Columns.Enabled then begin
     Columns.ExchangeColumn( ColumnIndexFromGridColumn(Index),
       ColumnIndexFromGridColumn(WithIndex));
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58735)
+++ lcl/valedit.pas	(working copy)
@@ -115,6 +115,8 @@
 
   { TValueListEditor }
 
+  TValueListEditorColIndex = 0..1;
+
   TValueListEditor = class(TCustomStringGrid)
   private
     FTitleCaptions: TStrings;
@@ -150,6 +152,7 @@
     procedure ShowColumnTitles;
     procedure AdjustRowCount; virtual;
     procedure DefineCellsProperty(Filer: TFiler); override;
+    procedure DoOPExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     procedure InvalidateCachedRow;
     procedure GetAutoFillColumnInfo(const Index: Integer; var aMin,aMax,aPriority: Integer); override;
     function GetEditText(ACol, ARow: Integer): string; override;
@@ -162,6 +165,7 @@
     procedure SetEditText(ACol, ARow: Longint; const Value: string); override;
     procedure SetFixedRows(const AValue: Integer); override;
     procedure SetRowCount(AValue: Integer);
+    procedure Sort(ColSorting: Boolean; index,IndxFrom,IndxTo:Integer); override;
     procedure TitlesChanged(Sender: TObject);
     function ValidateEntry(const ACol,ARow:Integer; const OldValue:string; var NewValue:string): boolean; override;
   public
@@ -176,11 +180,12 @@
     procedure InsertColRow(IsColumn: boolean; index: integer);
     function InsertRow(const KeyName, Value: string; Append: Boolean): Integer;
     procedure InsertRowWithValues(Index: Integer; Values: array of String);
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
     function IsEmptyRow: Boolean; {Delphi compatible function}
     function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
+    procedure Sort(ColIndex: TValueListEditorColIndex; FromRow: Integer = -1;
+      ToRow: Integer = -1);
 
     property Modified;
     property Keys[Index: Integer]: string read GetKey write SetKey;
@@ -845,7 +850,7 @@
   Strings.InsertItem(Index, AKey + Strings.NameValueSeparator + AValue);
 end;
 
-procedure TValueListEditor.ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+procedure TValueListEditor.DoOPExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
 begin
   if not IsColumn then
     Strings.Exchange(Index - FixedRows, WithIndex - FixedRows)
@@ -910,6 +915,21 @@
   end;
 end;
 
+procedure TValueListEditor.Sort(ColSorting: Boolean; index,IndxFrom,IndxTo:Integer);
+begin
+  if not ColSorting then
+    raise Exception.Create('TValueListEditor: Sorting by rows not allowed.');
+  inherited;
+end;
+
+procedure TValuelistEditor.Sort(ColIndex: TValueListEditorColIndex;
+  FromRow: Integer = -1; ToRow: Integer = -1);
+begin
+  if FromRow = -1 then FromRow := FixedRows;
+  if ToRow = -1 then ToRow := RowCount - 1;
+  SortColRow(true, ColIndex, FromRow, ToRow);
+end;
+
 procedure TValueListEditor.StringsChange(Sender: TObject);
 begin
   //Since we never call inherited SetCell, this seems the logical place to do it

Bart Broersma

2018-08-18 14:05

developer   ~0110118

TValueList.ExchangeColRow is public. So we can't simply rename that, it'll break backwards compatibility.

W.r.t. duplicate storage: this also is the case in Delphi: TValueListStrings inherits from TStringList and holds/duplicates the content of the grid.

Bart Broersma

2018-08-18 14:23

developer  

valedit.sort.2.diff (3,160 bytes)   
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 58638)
+++ lcl/grids.pas	(working copy)
@@ -989,7 +989,7 @@
       const AXProportion, AYProportion: Double); override;
     procedure DoOnChangeBounds; override;
     procedure DoOPDeleteColRow(IsColumn: Boolean; index: Integer);
-    procedure DoOPExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure DoOPExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); virtual;
     procedure DoOPInsertColRow(IsColumn: boolean; index: integer);
     procedure DoOPMoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     procedure DoPasteFromClipboard; virtual;
@@ -6236,7 +6236,6 @@
 var
   aColRow: integer;
 begin
-
   if IsColumn and Columns.Enabled then begin
     Columns.ExchangeColumn( ColumnIndexFromGridColumn(Index),
       ColumnIndexFromGridColumn(WithIndex));
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58638)
+++ lcl/valedit.pas	(working copy)
@@ -150,6 +150,7 @@
     procedure ShowColumnTitles;
     procedure AdjustRowCount; virtual;
     procedure DefineCellsProperty(Filer: TFiler); override;
+    procedure DoOPExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     procedure InvalidateCachedRow;
     procedure GetAutoFillColumnInfo(const Index: Integer; var aMin,aMax,aPriority: Integer); override;
     function GetEditText(ACol, ARow: Integer): string; override;
@@ -162,6 +163,8 @@
     procedure SetEditText(ACol, ARow: Longint; const Value: string); override;
     procedure SetFixedRows(const AValue: Integer); override;
     procedure SetRowCount(AValue: Integer);
+    procedure Sort;
+    procedure Sort(Index, FromIndex, ToIndex: Integer);
     procedure TitlesChanged(Sender: TObject);
     function ValidateEntry(const ACol,ARow:Integer; const OldValue:string; var NewValue:string): boolean; override;
   public
@@ -178,7 +181,7 @@
     procedure InsertRowWithValues(Index: Integer; Values: array of String);
     procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
     function IsEmptyRow: Boolean; {Delphi compatible function}
-    function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
+    function IsEmptyRow(aRow: Integer): Boolean; {This makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
 
@@ -1104,6 +1107,12 @@
 begin
 end;
 
+procedure TValueListEditor.DoOPExchangeColRow(IsColumn: Boolean; index,
+  WithIndex: Integer);
+begin
+  ExchangeColRow(IsColumn, index, WithIndex);
+end;
+
 procedure TValueListEditor.InvalidateCachedRow;
 begin
   if (Strings.Count = 0) then
@@ -1336,6 +1345,16 @@
   end;
 end;
 
+procedure TValueListEditor.Sort;
+begin
+  SortColRow(True, 0);
+end;
+
+procedure TValueListEditor.Sort(Index, FromIndex, ToIndex: Integer);
+begin
+  inherited Sort(True, Index, FromIndex, ToIndex);
+end;
+
 procedure TValueListEditor.TitlesChanged(Sender: TObject);
 begin
   // Refresh the display.
valedit.sort.2.diff (3,160 bytes)   

Bart Broersma

2018-08-18 14:27

developer   ~0110123

valedit.sort.2.diff implements:
procedure Sort;
procedure Sort(Index, FromIndex, ToIndex: Integer);
They basically force IsColumn to be True in the inherited calls they make.

Calling SortColRow(False,..) already raises an exception as it is:

"The operation ExchangeColRow is not allowed on a TValueListEditor on columns."

wp

2018-08-18 16:15

developer   ~0110126

Your new patch is working fine.

-------------------

> TValueList.ExchangeColRow is public. So we can't simply rename that, it'll break backwards compatibility.

It is inherited from TCustomDrawGrid. So it will still be there even if we remove it and put the code into DoOPExchangeColRow.

The reason why I am objecting is a matter of taste. In my programming gut-feeling it is correct that a public procedure calls a protected one:

  procedure TCustomDrawGrid.ExchangeColRow(...);
  begin
    DoOPExchangeColRow(...);
  end;

But in your code, a protected procedure calls public code. This is kind of weird and feels wrong (of course it isn't). I don't insist on it.

--------------

As for above idea to inherit TValueListStrings from TStringGridStrings: I played with it and get the feeling that we will run into lots of issues. TStringGridStrings inherits from TStrings and thus has no Sort and no InsertItem methods which are needed by TValueListStrings. While I could work around the latter one the missing Sort will just bring in the QuickSort again which I wanted to remove. So, forget about this idea.

-------------

What is your opinon about the incorrect indexing of the ItemProps? This could be corrected by offseting the Index in TItemProps.GetItem and .SetItem by 1. But it will break user code...

On the other hand, I looked at this issue with Delphi 10.2CE, and it seems that they also have the same issue: in the first data row, the index of the Keys in the first row is 1 but the index of the ItemProps is 0.

So better leave it as it is.

Bart Broersma

2018-08-18 23:25

developer   ~0110139

Last edited: 2018-08-18 23:30

View 2 revisions

(This component should have been designed somewhat like ObjectInspector...)

>> TValueList.ExchangeColRow is public. So we can't simply rename that, it'll
>> break backwards compatibility.

> But in your code, a protected procedure calls public code. This is kind of weird
I agree.
The functionality of ExchangeColRow could be moved to a protected InternalExchangeColRow and DoOPExchangeColRow can then call InternalExchangeColRow.
This reduces the weirdness you mention, but it feels overcomplex to me.

> It is inherited from TCustomDrawGrid.

Yeah, but calling that will screw up a TValueListEditor (it won't exchange the associate objects and itemprops, nor will it hide the editor).

Jesus Reyes

2018-08-19 00:13

developer   ~0110140

A derived grid should override ColRowExchanged for this, this could be made by renaming ExchangeColRow to ColRowExchanged. In this case one might want to add a sort(vesSortByKey,vesSortByValue or ByValue:boolean) method to TValueListEditor which would call the inherited SortColRow with appropriated parameters. It should be used also for advanced use like sorting by specific ranges.

I tried this and it seems to work. I don't know the reason why it was mentioned that "DoOpExchangeColRow which does not fit to the way TValueListEditor handles its data". About "But of course, this method is a bit dangerous because the user could misuse it to sort by rows" I would say that I see no danger in that, we should not nanny the users. In any case a simply sort method that don't even mention "column" could be added as suggested.

Please consider this as a suggestion, feel free to implement the required functionality in any way you want or by modifying the grid like you have proposed in the last patch.

Bart Broersma

2018-08-19 00:44

developer   ~0110142

If I understand correctly then ColRowExchanged is called after the exchange has taken place (in TCustomGrid.DoOPExchangeColRow).
So, I don't really understand why TCustomDrawGrid.ColRowExchanged performs an ExchangeColRow?

And if I understand correctly then FCols.Exchange (called in TCustomGrid.DoOPExchangeColRow) messes up TValueListEditor, because it does not properly update the underlying data in FStrings, nor ItemProps (as the example shows).

Jesus Reyes

2018-08-19 10:34

developer   ~0110146

"If I understand correctly then ColRowExchanged is called after the exchange has taken place"....

Not completely at the 'after', because the last step is updating the editor bounds if needed, but remember that the exchange operation that has taken place at this moment is only the cols/rows sizes (FCols and FRows), for a TCustomGrid is the only data that is needed to be exchanged.

"why TCustomDrawGrid.ColRowExchanged performs an ExchangeColRow?" because TCustomDrawGrid descendants hold content and objects and they need to check for cells contained in the rows and cols exchanges too. Indeed, TBH I don't remember now why FGrid was declared in TCustomDrawGrid as it has not content or objects.

"if I understand correctly then FCols.Exchange (called in TCustomGrid.DoOPExchangeColRow) messes up TValueListEditor, because it does not properly update the underlying data in FStrings"

It should not because it doesn't know anything about the grid's content, don't forget that FCols (and FRows) hold only the internal column and row widths and heights, not the grid's content. In fact it is in the overridden ColRowExchanged method where a descendant should exchange its content, and that is precisely what TCustomDrawGrid does in its FGrid. This is why I'm suggesting to use this method in TValueListEditor for exchanging it's content.

"nor ItemProps (as the example shows)" I have no investigated if ItemProps is an object of the TValueListEditor strings or is an independent list, if it independent then it should be exchanged here too, if it's embedded in the strings then that is done automatically.

The exchange operation is supposed to follow this pattern: first the internals known to TCustomGrid are exchanged, then the internals of derived grids and finally finish the visual controls or appearance that all grids should do (so derived grids doesn't have to).

If ColRowExchanged is used don't forget to call the inherited one so the internal FGrid gets updated.

Bart Broersma

2018-08-19 12:11

developer   ~0110148

Last edited: 2018-08-19 12:13

View 2 revisions

Thanks for explaining that.
Let's see if I understand this correctly in the context of a TValueListEditor:

When I do not call FCols.Exchange(), then the sizes of the cells are not exchanged, but only the data (text).

I don't understand what Fgrid.ExchangeColRow(IsColumn, index, WithIndex) does (in a TStringGrid).

To be honest, even after spending lots of time on this component, I still don't really understand where the cells contents are stored and how the grid retrieves those vlaues when it draws itself.
Since we override GetCells in TValueListEditor to retrieve the value from Strings property, and since altering Strings alters the Grid (Strings.OnChange will invalidate the grid), I conclued that the data is stored in the place where SetCell and GetCell tell the grid where it is?

If that is the case, then at least there is no duplicate storage of the data in a TValueListEditor (as wp and me assumed)?


I will try to override ColRowExchanged and see if I get it working that way.

Bart Broersma

2018-08-19 12:34

developer   ~0110149

Attached valedit.sort.3.diff implements overriding ColRowExchanged as suggested by Jesus.
Can you test if it works OK (it runs fine in my test suite).

@Jesus: are the comments I placed in grids.pas correct?
(where it says "sizes" should it be "dimensions" (as in: width/height)?

Bart Broersma

2018-08-19 12:34

developer  

valedit.sort.3.diff (3,549 bytes)   
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 58638)
+++ lcl/grids.pas	(working copy)
@@ -948,7 +948,7 @@
     procedure CMMouseEnter(var Message: TLMessage); message CM_MOUSEENTER;
     procedure CMMouseLeave(var Message :TLMessage); message CM_MouseLeave;
     procedure ColRowDeleted(IsColumn: Boolean; index: Integer); virtual;
-    procedure ColRowExchanged(IsColumn: Boolean; index,WithIndex: Integer); virtual;
+    procedure ColRowExchanged(IsColumn: Boolean; index,WithIndex: Integer); virtual; //Exchanges the Data
     procedure ColRowInserted(IsColumn: boolean; index: integer); virtual;
     procedure ColRowMoved(IsColumn: Boolean; FromIndex,ToIndex: Integer); virtual;
     function  ColRowToOffset(IsCol, Relative: Boolean; Index:Integer;
@@ -6236,7 +6236,6 @@
 var
   aColRow: integer;
 begin
-
   if IsColumn and Columns.Enabled then begin
     Columns.ExchangeColumn( ColumnIndexFromGridColumn(Index),
       ColumnIndexFromGridColumn(WithIndex));
@@ -6244,10 +6243,10 @@
     exit;
   end;
   if IsColumn then
-    FCols.Exchange(index, WithIndex)
+    FCols.Exchange(index, WithIndex)  // exhchanges the sizes of the columns
   else
-    FRows.Exchange(index, WithIndex);
-  ColRowExchanged(IsColumn, index, WithIndex);
+    FRows.Exchange(index, WithIndex); //exchanges the sizes of the rows
+  ColRowExchanged(IsColumn, index, WithIndex);  //exchanges the actual data
   VisualChange;
 
   // adjust editor bounds
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58638)
+++ lcl/valedit.pas	(working copy)
@@ -150,6 +150,7 @@
     procedure ShowColumnTitles;
     procedure AdjustRowCount; virtual;
     procedure DefineCellsProperty(Filer: TFiler); override;
+    procedure ColRowExchanged(IsColumn: Boolean; index,WithIndex: Integer); override;
     procedure InvalidateCachedRow;
     procedure GetAutoFillColumnInfo(const Index: Integer; var aMin,aMax,aPriority: Integer); override;
     function GetEditText(ACol, ARow: Integer): string; override;
@@ -178,9 +179,11 @@
     procedure InsertRowWithValues(Index: Integer; Values: array of String);
     procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
     function IsEmptyRow: Boolean; {Delphi compatible function}
-    function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
+    function IsEmptyRow(aRow: Integer): Boolean; {This makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
+    procedure Sort;
+    procedure Sort(Index, FromIndex, ToIndex: Integer);
 
     property Modified;
     property Keys[Index: Integer]: string read GetKey write SetKey;
@@ -1104,6 +1107,15 @@
 begin
 end;
 
+procedure TValueListEditor.ColRowExchanged(IsColumn: Boolean; index,
+  WithIndex: Integer);
+begin
+  ExchangeColRow(IsColumn, Index, WithIndex);
+  if Assigned(OnColRowExchanged) then
+    OnColRowExchanged(Self, IsColumn, index, WithIndex);
+end;
+
+
 procedure TValueListEditor.InvalidateCachedRow;
 begin
   if (Strings.Count = 0) then
@@ -1336,6 +1348,16 @@
   end;
 end;
 
+procedure TValueListEditor.Sort;
+begin
+  SortColRow(True, 0);
+end;
+
+procedure TValueListEditor.Sort(Index, FromIndex, ToIndex: Integer);
+begin
+  inherited Sort(True, Index, FromIndex, ToIndex);
+end;
+
 procedure TValueListEditor.TitlesChanged(Sender: TObject);
 begin
   // Refresh the display.
valedit.sort.3.diff (3,549 bytes)   

wp

2018-08-19 21:32

developer   ~0110154

Yes, works fine in my test, too.


> (where it says "sizes" should it be "dimensions" (as in: width/height)?

I would talk of "column widths" and "row heights", instead of "sizes"

Bart Broersma

2018-08-19 22:01

developer   ~0110155

Well, I was not sure what Jesus meant...

Jesus Reyes

2018-08-20 02:23

developer   ~0110161

Last edited: 2018-08-20 02:45

View 2 revisions

Yes, by sizes I meant Column widths and row heights which should be used when referring to FCols and FRows, I may incorrectly use 'sizes' instead of 'widths and heights', sizes and dimensions in Spanish are synonymous at least to me :).

The comments should be at the declaration point so they will be picked up by the hint.

As proposed in your last patch, ExchangeColRow (the public interface) would be called multiple times when sorting, ColRowExchanged is the protected interface for derived grids and doesn't look right that it calls the public interface when it should be done the other way around.

As said, ExchangeColRow is the high level interface for exchanging functionality, because you want to check for incorrect usage, I guess it should be made virtual in the grid an overridden in TValueListEditor, there the inherited method should be called if the check passed. Then ColRowExchanged should be overridden and simply do Strings.Exchange(Index - FixedRows, WithIndex - FixedRows).

The content for a stringgrid is stored in the FGrid object. Later another interface for accessing this data was added for delphi compatibility but it uses on demand TStrings for Cols[] and Rows[] arrays functionality (maybe this creates some confusion with the internal FCols and FRows), but in the end they get or set the data from or to the FGrid storage.

Bart Broersma

2018-08-20 12:32

developer   ~0110165

Last edited: 2018-08-20 12:33

View 3 revisions

@Jesus: you confuse me:
- ExchangeColRow is not a method of TCustomXXXGrid, it was introduced in TValueListEditor.
- Whatever I do, in the end Strings.Exchange() will be called multipe times. I cannot see a way to avoid that. I could override Sort to do Strings.BeginUpdate/Strings.Endupdate, to suppress multiple Strings.OnChange events.

You suggested to override ColRowExchanged, since this was the place where the content will be exchanged.
I did so.

I can move all functionality of TValueListEditor.ExchangeColRow to TValueListEditor.ColwRowExchanged.
The public method TValueListEditor.ExchangeColRow will then simply call the protected ColRowEchanged.

In essence this will not change the functionality of the proposal in valedit.sort.3.diff.

If this is NOT what you mean, then I am at a loss, and I would ask you to post a patch here, impementing it the way you vision it.

Jesus Reyes

2018-08-20 20:37

developer   ~0110173

"- ExchangeColRow is not a method of TCustomXXXGrid, it was introduced in TValueListEditor."

ExchangeColRow was introduced by me in r7122 13 years, 3 months ago according to the svn log.

"Whatever I do, in the end Strings.Exchange() will be called multipe times. I cannot see a way to avoid that"

This is no problem at all, I have not suggested that it should be avoided.

"You suggested to override ColRowExchanged, since this was the place where the content will be exchanged."

Yes see the attached patch, it works here.

Jesus Reyes

2018-08-20 20:39

developer  

valedit.diff (3,217 bytes)   
Index: grids.pas
===================================================================
--- grids.pas	(revision 58711)
+++ grids.pas	(working copy)
@@ -1379,7 +1379,7 @@
     procedure DeleteColRow(IsColumn: Boolean; index: Integer);
     procedure DeleteCol(Index: Integer); virtual;
     procedure DeleteRow(Index: Integer); virtual;
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); virtual;
     procedure InsertColRow(IsColumn: boolean; index: integer);
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     procedure SortColRow(IsColumn: Boolean; index:Integer); overload;
Index: valedit.pas
===================================================================
--- valedit.pas	(revision 58689)
+++ valedit.pas	(working copy)
@@ -149,6 +149,7 @@
     procedure SetFixedCols(const AValue: Integer); override;
     procedure ShowColumnTitles;
     procedure AdjustRowCount; virtual;
+    procedure ColRowExchanged(IsColumn: Boolean; index, WithIndex: Integer); override;
     procedure DefineCellsProperty(Filer: TFiler); override;
     procedure InvalidateCachedRow;
     procedure GetAutoFillColumnInfo(const Index: Integer; var aMin,aMax,aPriority: Integer); override;
@@ -176,11 +177,13 @@
     procedure InsertColRow(IsColumn: boolean; index: integer);
     function InsertRow(const KeyName, Value: string; Append: Boolean): Integer;
     procedure InsertRowWithValues(Index: Integer; Values: array of String);
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     function IsEmptyRow: Boolean; {Delphi compatible function}
     function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
+    procedure Sort;
+    procedure Sort(Index, FromIndex, ToIndex: Integer);
 
     property Modified;
     property Keys[Index: Integer]: string read GetKey write SetKey;
@@ -848,7 +851,7 @@
 procedure TValueListEditor.ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
 begin
   if not IsColumn then
-    Strings.Exchange(Index - FixedRows, WithIndex - FixedRows)
+    inherited ExchangeColRow(IsColumn, index, WithIndex)
   else
     Raise EGridException.CreateFmt(rsVLEInvalidRowColOperation,['ExchangeColRow',' on columns']);
 end;
@@ -910,6 +913,16 @@
   end;
 end;
 
+procedure TValueListEditor.Sort;
+begin
+  SortColRow(True, 0);
+end;
+
+procedure TValueListEditor.Sort(Index, FromIndex, ToIndex: Integer);
+begin
+  SortColRow(True, Index, FromIndex, ToIndex);
+end;
+
 procedure TValueListEditor.StringsChange(Sender: TObject);
 begin
   //Since we never call inherited SetCell, this seems the logical place to do it
@@ -1100,6 +1113,12 @@
   end;
 end;
 
+procedure TValueListEditor.ColRowExchanged(IsColumn: Boolean; index,
+  WithIndex: Integer);
+begin
+  Strings.Exchange(Index - FixedRows, WithIndex - FixedRows);
+end;
+
 procedure TValueListEditor.DefineCellsProperty(Filer: TFiler);
 begin
 end;
valedit.diff (3,217 bytes)   

Bart Broersma

2018-08-20 22:32

developer   ~0110175

> ExchangeColRow was introduced by me in r7122 13 years, 3 months ago according to the svn log.

Yes, you are right.
When I wrote that I only saw the sources of valedit (no Lazarus at hand), and I saw the declaration there, without an override or reintroduce modifier.
The compiler does not seem to throw any hint/warning on that, so I missed that.

> This is no problem at all, I have not suggested that it should be avoided.
Well, I simply misundestood you.

Not tested yet, but shouldn't this be in the override TValueListEditor.ColRowExchanged?

+ if Assigned(OnColRowExchanged) then
+ OnColRowExchanged(Self, IsColumn, index, WithIndex);

Thanks for the patch.

Any thoughts on overriding Sort:

TValueListEditor.Sort(ColSorting: Boolean; index, IndxFrom, IndxTo: Integer);
begin
  Strings.BeginUpdate;
  try
    inherited Sort(ColSorting: Boolean; index, IndxFrom, IndxTo: Integer);
  finally
    Strings.EndUpdate;
  end;
end;

Bart Broersma

2018-08-20 23:00

developer  

valedit.sort.4.diff (3,715 bytes)   
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 58740)
+++ lcl/grids.pas	(working copy)
@@ -1379,7 +1379,7 @@
     procedure DeleteColRow(IsColumn: Boolean; index: Integer);
     procedure DeleteCol(Index: Integer); virtual;
     procedure DeleteRow(Index: Integer); virtual;
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); virtual;
     procedure InsertColRow(IsColumn: boolean; index: integer);
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     procedure SortColRow(IsColumn: Boolean; index:Integer); overload;
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58740)
+++ lcl/valedit.pas	(working copy)
@@ -149,6 +149,7 @@
     procedure SetFixedCols(const AValue: Integer); override;
     procedure ShowColumnTitles;
     procedure AdjustRowCount; virtual;
+    procedure ColRowExchanged(IsColumn: Boolean; index, WithIndex: Integer); override;
     procedure DefineCellsProperty(Filer: TFiler); override;
     procedure InvalidateCachedRow;
     procedure GetAutoFillColumnInfo(const Index: Integer; var aMin,aMax,aPriority: Integer); override;
@@ -176,11 +177,14 @@
     procedure InsertColRow(IsColumn: boolean; index: integer);
     function InsertRow(const KeyName, Value: string; Append: Boolean): Integer;
     procedure InsertRowWithValues(Index: Integer; Values: array of String);
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     function IsEmptyRow: Boolean; {Delphi compatible function}
     function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
+    procedure Sort;
+    procedure Sort(Index, FromIndex, ToIndex: Integer);
+    procedure Sort(ColSorting: Boolean; index,IndxFrom,IndxTo:Integer); override;
 
     property Modified;
     property Keys[Index: Integer]: string read GetKey write SetKey;
@@ -848,7 +852,7 @@
 procedure TValueListEditor.ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
 begin
   if not IsColumn then
-    Strings.Exchange(Index - FixedRows, WithIndex - FixedRows)
+    inherited ExchangeColRow(IsColumn, index, WithIndex)
   else
     Raise EGridException.CreateFmt(rsVLEInvalidRowColOperation,['ExchangeColRow',' on columns']);
 end;
@@ -910,9 +914,29 @@
   end;
 end;
 
+procedure TValueListEditor.Sort;
+begin
+  SortColRow(True, 0);
+end;
+
+procedure TValueListEditor.Sort(Index, FromIndex, ToIndex: Integer);
+begin
+  SortColRow(True, Index, FromIndex, ToIndex);
+end;
+
+procedure TValueListEditor.Sort(ColSorting: Boolean; index, IndxFrom,
+  IndxTo: Integer);
+begin
+  Strings.BeginUpdate;
+  try
+    inherited Sort(ColSorting, index, IndxFrom, IndxTo);
+  finally
+    Strings.EndUpdate;
+  end;
+end;
+
 procedure TValueListEditor.StringsChange(Sender: TObject);
 begin
-  //Since we never call inherited SetCell, this seems the logical place to do it
   Modified := True;
   AdjustRowCount;
   Invalidate;
@@ -1100,6 +1124,14 @@
   end;
 end;
 
+procedure TValueListEditor.ColRowExchanged(IsColumn: Boolean; index,
+  WithIndex: Integer);
+begin
+  Strings.Exchange(Index - FixedRows, WithIndex - FixedRows);
+  if Assigned(OnColRowExchanged) then
+    OnColRowExchanged(Self, IsColumn, index, WithIndex);
+end;
+
 procedure TValueListEditor.DefineCellsProperty(Filer: TFiler);
 begin
 end;
valedit.sort.4.diff (3,715 bytes)   

Bart Broersma

2018-08-20 23:08

developer   ~0110176

Last edited: 2018-08-20 23:26

View 4 revisions

valedit.sort.4.diff contains patch from Jesus, and an overridden TValueListEditor.Sort as suggested in the note above.

While now sorting is correct, and dimensions of the rows are copied as well, now we have a new problem:

In the attached sample project (ValEdit.zip) first click "PickList", then click "Sort".
This sorts correctly (although there may be a visual anomaly if the the cursor in the grid was on the row with "My plain RO").

Now click "Assign" and observe the exception.
("Assign" assigns the strings in the left memo to ValEd.Strings)

Close and re-open the app.
Doo the same as before, but instead of "Sort" click "Strings.Sort" and then"Assign".
The grid will be sorted correctly, but the dimensions are not copied.
Assign will not throw any error.

(Notice that ValEdit.zip is my current test-suite, so many of the buttons and code have no relation with this bug.)

As a workaround, if you put inside Strings.Assign (which is overridden) a call to Strings.Clear (also overridden) first, then the error goes away.
See: valedit.sort.5.diff

I have no idea why though.

Bart Broersma

2018-08-20 23:22

developer  

valedit.sort.5.diff (4,093 bytes)   
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 58740)
+++ lcl/grids.pas	(working copy)
@@ -1379,7 +1379,7 @@
     procedure DeleteColRow(IsColumn: Boolean; index: Integer);
     procedure DeleteCol(Index: Integer); virtual;
     procedure DeleteRow(Index: Integer); virtual;
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); virtual;
     procedure InsertColRow(IsColumn: boolean; index: integer);
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     procedure SortColRow(IsColumn: Boolean; index:Integer); overload;
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58740)
+++ lcl/valedit.pas	(working copy)
@@ -149,6 +149,7 @@
     procedure SetFixedCols(const AValue: Integer); override;
     procedure ShowColumnTitles;
     procedure AdjustRowCount; virtual;
+    procedure ColRowExchanged(IsColumn: Boolean; index, WithIndex: Integer); override;
     procedure DefineCellsProperty(Filer: TFiler); override;
     procedure InvalidateCachedRow;
     procedure GetAutoFillColumnInfo(const Index: Integer; var aMin,aMax,aPriority: Integer); override;
@@ -176,11 +177,14 @@
     procedure InsertColRow(IsColumn: boolean; index: integer);
     function InsertRow(const KeyName, Value: string; Append: Boolean): Integer;
     procedure InsertRowWithValues(Index: Integer; Values: array of String);
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     function IsEmptyRow: Boolean; {Delphi compatible function}
     function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
+    procedure Sort;
+    procedure Sort(Index, FromIndex, ToIndex: Integer);
+    procedure Sort(ColSorting: Boolean; index,IndxFrom,IndxTo:Integer); override;
 
     property Modified;
     property Keys[Index: Integer]: string read GetKey write SetKey;
@@ -536,6 +540,7 @@
 procedure TValueListStrings.Assign(Source: TPersistent);
 begin
   FGrid.InvalidateCachedRow;
+  Clear;  //if this is not done, and a TValueListEditor.Sort() is done and then later a Strings.Assign, an exception will occur.
   inherited Assign(Source);
   if (Source is TValueListStrings) then
     FItemProps.Assign(TValueListStrings(Source).FItemProps);
@@ -848,7 +853,7 @@
 procedure TValueListEditor.ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
 begin
   if not IsColumn then
-    Strings.Exchange(Index - FixedRows, WithIndex - FixedRows)
+    inherited ExchangeColRow(IsColumn, index, WithIndex)
   else
     Raise EGridException.CreateFmt(rsVLEInvalidRowColOperation,['ExchangeColRow',' on columns']);
 end;
@@ -910,9 +915,29 @@
   end;
 end;
 
+procedure TValueListEditor.Sort;
+begin
+  SortColRow(True, 0);
+end;
+
+procedure TValueListEditor.Sort(Index, FromIndex, ToIndex: Integer);
+begin
+  SortColRow(True, Index, FromIndex, ToIndex);
+end;
+
+procedure TValueListEditor.Sort(ColSorting: Boolean; index, IndxFrom,
+  IndxTo: Integer);
+begin
+  Strings.BeginUpdate;
+  try
+    inherited Sort(ColSorting, index, IndxFrom, IndxTo);
+  finally
+    Strings.EndUpdate;
+  end;
+end;
+
 procedure TValueListEditor.StringsChange(Sender: TObject);
 begin
-  //Since we never call inherited SetCell, this seems the logical place to do it
   Modified := True;
   AdjustRowCount;
   Invalidate;
@@ -1100,6 +1125,14 @@
   end;
 end;
 
+procedure TValueListEditor.ColRowExchanged(IsColumn: Boolean; index,
+  WithIndex: Integer);
+begin
+  Strings.Exchange(Index - FixedRows, WithIndex - FixedRows);
+  if Assigned(OnColRowExchanged) then
+    OnColRowExchanged(Self, IsColumn, index, WithIndex);
+end;
+
 procedure TValueListEditor.DefineCellsProperty(Filer: TFiler);
 begin
 end;
valedit.sort.5.diff (4,093 bytes)   

Bart Broersma

2018-08-20 23:23

developer  

ValEdit.zip (6,352 bytes)

Bart Broersma

2018-08-20 23:42

developer  

valedit.sort.6.diff (4,297 bytes)   
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 58740)
+++ lcl/grids.pas	(working copy)
@@ -1379,7 +1379,7 @@
     procedure DeleteColRow(IsColumn: Boolean; index: Integer);
     procedure DeleteCol(Index: Integer); virtual;
     procedure DeleteRow(Index: Integer); virtual;
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); virtual;
     procedure InsertColRow(IsColumn: boolean; index: integer);
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     procedure SortColRow(IsColumn: Boolean; index:Integer); overload;
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58740)
+++ lcl/valedit.pas	(working copy)
@@ -149,6 +149,7 @@
     procedure SetFixedCols(const AValue: Integer); override;
     procedure ShowColumnTitles;
     procedure AdjustRowCount; virtual;
+    procedure ColRowExchanged(IsColumn: Boolean; index, WithIndex: Integer); override;
     procedure DefineCellsProperty(Filer: TFiler); override;
     procedure InvalidateCachedRow;
     procedure GetAutoFillColumnInfo(const Index: Integer; var aMin,aMax,aPriority: Integer); override;
@@ -176,11 +177,14 @@
     procedure InsertColRow(IsColumn: boolean; index: integer);
     function InsertRow(const KeyName, Value: string; Append: Boolean): Integer;
     procedure InsertRowWithValues(Index: Integer; Values: array of String);
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     function IsEmptyRow: Boolean; {Delphi compatible function}
     function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
+    procedure Sort;
+    procedure Sort(Index, FromIndex, ToIndex: Integer);
+    procedure Sort(ColSorting: Boolean; index,IndxFrom,IndxTo:Integer); override;
 
     property Modified;
     property Keys[Index: Integer]: string read GetKey write SetKey;
@@ -536,6 +540,7 @@
 procedure TValueListStrings.Assign(Source: TPersistent);
 begin
   FGrid.InvalidateCachedRow;
+  Clear;  //if this is not done, and a TValueListEditor.Sort() is done and then later a Strings.Assign, an exception will occur.
   inherited Assign(Source);
   if (Source is TValueListStrings) then
     FItemProps.Assign(TValueListStrings(Source).FItemProps);
@@ -848,7 +853,7 @@
 procedure TValueListEditor.ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
 begin
   if not IsColumn then
-    Strings.Exchange(Index - FixedRows, WithIndex - FixedRows)
+    inherited ExchangeColRow(IsColumn, index, WithIndex)
   else
     Raise EGridException.CreateFmt(rsVLEInvalidRowColOperation,['ExchangeColRow',' on columns']);
 end;
@@ -910,9 +915,33 @@
   end;
 end;
 
+procedure TValueListEditor.Sort;
+begin
+  SortColRow(True, 0);
+end;
+
+procedure TValueListEditor.Sort(Index, FromIndex, ToIndex: Integer);
+begin
+  SortColRow(True, Index, FromIndex, ToIndex);
+end;
+
+procedure TValueListEditor.Sort(ColSorting: Boolean; index, IndxFrom, IndxTo: Integer);
+var
+  HideEditor: Boolean;
+begin
+  HideEditor := goAlwaysShowEditor in Options;
+  if HideEditor then Options := Options - [goAlwaysShowEditor];
+  Strings.BeginUpdate;
+  try
+    inherited Sort(ColSorting, index, IndxFrom, IndxTo);
+  finally
+    Strings.EndUpdate;
+  end;
+  if HideEditor then Options := Options + [goAlwaysShowEditor];
+end;
+
 procedure TValueListEditor.StringsChange(Sender: TObject);
 begin
-  //Since we never call inherited SetCell, this seems the logical place to do it
   Modified := True;
   AdjustRowCount;
   Invalidate;
@@ -1100,6 +1129,14 @@
   end;
 end;
 
+procedure TValueListEditor.ColRowExchanged(IsColumn: Boolean; index,
+  WithIndex: Integer);
+begin
+  Strings.Exchange(Index - FixedRows, WithIndex - FixedRows);
+  if Assigned(OnColRowExchanged) then
+    OnColRowExchanged(Self, IsColumn, index, WithIndex);
+end;
+
 procedure TValueListEditor.DefineCellsProperty(Filer: TFiler);
 begin
 end;
valedit.sort.6.diff (4,297 bytes)   

Bart Broersma

2018-08-20 23:42

developer   ~0110177

In valedit.sort.6.diff I try to fix the visual anomaly I mentioned above.

Jesus Reyes

2018-08-21 06:53

developer   ~0110179

Indeed, I forgot the inherited call in ColRowExchanged, that should take care of the OnColRowExchanged, and it's a must for a derived TCustomStringGrid that uses the internal storage, here it doesn't matter though.

I think beginupdate and endupdate are useful when there are a lot of changes that keeps the grid continuously redrawing, in this kind of grid I don't think you will see thousands of rows or even hundreds, so I think it should not be necessary, if one would still want to do it, I would use grid's own begin/end update because that locks the visual changes, I have not tried though.

I quickly checked the problem you reported about the exception, it is caused by changing the row before the rowcount, because that assumes that 'strings' storage and the grid are in sync which at this point are not. Anyway, I think one would not have to modify the row in AdjustRowCount because that is something the grid does automatically if needed (or it should) but I don't know the original motivation for doing so in TValueListEditor.

Bart Broersma

2018-08-21 10:07

developer   ~0110182

> but I don't know the original motivation for doing so in TValueListEditor

1083 procedure TValueListEditor.AdjustRowCount;
1084 // Change the number of rows based on the number of items in Strings collection.

If you directly modify Strings, and do not tell the grid the rowcount has changed, you will get exceptions...

Bart Broersma

2018-08-21 18:07

developer   ~0110207

Last edited: 2018-08-21 18:16

View 2 revisions

> I would use grid's own begin/end update because that locks the visual changes
inherited Sort already does this. So this should be fine.

> Indeed, I forgot the inherited call in ColRowExchanged,
I'll put it in, so as not to unnecessary duplicate code.

See: valedit.sort.7.diff

Bart Broersma

2018-08-21 18:16

developer  

valedit.sort.7.diff (4,861 bytes)   
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 58740)
+++ lcl/grids.pas	(working copy)
@@ -1379,7 +1379,7 @@
     procedure DeleteColRow(IsColumn: Boolean; index: Integer);
     procedure DeleteCol(Index: Integer); virtual;
     procedure DeleteRow(Index: Integer); virtual;
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); virtual;
     procedure InsertColRow(IsColumn: boolean; index: integer);
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     procedure SortColRow(IsColumn: Boolean; index:Integer); overload;
@@ -3691,6 +3691,7 @@
 begin
 end;
 
+{Use in derived grids to exchange the actual data}
 procedure TCustomGrid.ColRowExchanged(IsColumn: Boolean; index, WithIndex: Integer);
 begin
 end;
@@ -6282,9 +6283,9 @@
     exit;
   end;
   if IsColumn then
-    FCols.Exchange(index, WithIndex)
+    FCols.Exchange(index, WithIndex) //exchanges the dimensions (width/height) of the resp. columns
   else
-    FRows.Exchange(index, WithIndex);
+    FRows.Exchange(index, WithIndex); //exchanges the dimensions (width/height) of the resp. rows
   ColRowExchanged(IsColumn, index, WithIndex);
   VisualChange;
 
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58740)
+++ lcl/valedit.pas	(working copy)
@@ -149,6 +149,7 @@
     procedure SetFixedCols(const AValue: Integer); override;
     procedure ShowColumnTitles;
     procedure AdjustRowCount; virtual;
+    procedure ColRowExchanged(IsColumn: Boolean; index, WithIndex: Integer); override;
     procedure DefineCellsProperty(Filer: TFiler); override;
     procedure InvalidateCachedRow;
     procedure GetAutoFillColumnInfo(const Index: Integer; var aMin,aMax,aPriority: Integer); override;
@@ -176,11 +177,14 @@
     procedure InsertColRow(IsColumn: boolean; index: integer);
     function InsertRow(const KeyName, Value: string; Append: Boolean): Integer;
     procedure InsertRowWithValues(Index: Integer; Values: array of String);
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     function IsEmptyRow: Boolean; {Delphi compatible function}
     function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
+    procedure Sort;
+    procedure Sort(Index, FromIndex, ToIndex: Integer);
+    procedure Sort(ColSorting: Boolean; index,IndxFrom,IndxTo:Integer); override;
 
     property Modified;
     property Keys[Index: Integer]: string read GetKey write SetKey;
@@ -536,6 +540,7 @@
 procedure TValueListStrings.Assign(Source: TPersistent);
 begin
   FGrid.InvalidateCachedRow;
+  Clear;  //if this is not done, and a TValueListEditor.Sort() is done and then later a Strings.Assign, an exception will occur.
   inherited Assign(Source);
   if (Source is TValueListStrings) then
     FItemProps.Assign(TValueListStrings(Source).FItemProps);
@@ -848,7 +853,7 @@
 procedure TValueListEditor.ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
 begin
   if not IsColumn then
-    Strings.Exchange(Index - FixedRows, WithIndex - FixedRows)
+    inherited ExchangeColRow(IsColumn, index, WithIndex)
   else
     Raise EGridException.CreateFmt(rsVLEInvalidRowColOperation,['ExchangeColRow',' on columns']);
 end;
@@ -910,9 +915,33 @@
   end;
 end;
 
+procedure TValueListEditor.Sort;
+begin
+  SortColRow(True, 0);
+end;
+
+procedure TValueListEditor.Sort(Index, FromIndex, ToIndex: Integer);
+begin
+  SortColRow(True, Index, FromIndex, ToIndex);
+end;
+
+procedure TValueListEditor.Sort(ColSorting: Boolean; index, IndxFrom, IndxTo: Integer);
+var
+  HideEditor: Boolean;
+begin
+  HideEditor := goAlwaysShowEditor in Options;
+  if HideEditor then Options := Options - [goAlwaysShowEditor];
+  Strings.BeginUpdate;
+  try
+    inherited Sort(ColSorting, index, IndxFrom, IndxTo);
+  finally
+    Strings.EndUpdate;
+  end;
+  if HideEditor then Options := Options + [goAlwaysShowEditor];
+end;
+
 procedure TValueListEditor.StringsChange(Sender: TObject);
 begin
-  //Since we never call inherited SetCell, this seems the logical place to do it
   Modified := True;
   AdjustRowCount;
   Invalidate;
@@ -1100,6 +1129,13 @@
   end;
 end;
 
+procedure TValueListEditor.ColRowExchanged(IsColumn: Boolean; index,
+  WithIndex: Integer);
+begin
+  Strings.Exchange(Index - FixedRows, WithIndex - FixedRows);
+  inherited ColRowExchanged(IsColumn, index, WithIndex);
+end;
+
 procedure TValueListEditor.DefineCellsProperty(Filer: TFiler);
 begin
 end;
valedit.sort.7.diff (4,861 bytes)   

Jesus Reyes

2018-08-21 18:24

developer   ~0110208

"If you directly modify Strings, and do not tell the grid the rowcount has changed, you will get exceptions.."

Let me rephrase it: I don't know the original motivation for modifying the *row* property in this method, because is something already done by the grid when the rowcount is changed, the current row is adjusted automatically if needed.

Bart Broersma

2018-08-21 19:14

developer   ~0110212

What would happen if you would delete one string from Strings?
The grid would want to redraw, RowCount has not been altered.
When trying to draw Cells[ACol,RowCount-1], it will access Strings[RowCount-2] while at that point in time Strings.Count will be (old) RowCount-2, last string will be Strings[RowCount-3].

> because is something already done by the grid when the rowcount is changed
which is not the case in the above scenario.

Back to the sorting issue.
Is valedit.sort.7.diff OK now?

wp

2018-08-21 23:22

developer   ~0110221

Looks good as far as my original post is concerned.

---------------

There is still the issue posted by Euigon Suh in 0034160: When sorting occurs via Strings.Sort while the ValueListEditor is focused (e.g. by a context menu or a speedbutton), and when the cell editor is active then the cell editor remains at its old position.

This does not happen when sorting is done by ValueListEditor.Sort directly, when the cell editor is not active, or when the VLE is not focused.

There is a different behavior between VLE.Sort and VLE.Strings.Sort: The row height moves with the row in the former case, but stays at its old position in the latter case. But maybe this can even be considered to be a feature instead of a bug.

Bart Broersma

2018-08-21 23:41

developer   ~0110223

Last edited: 2018-08-21 23:46

View 2 revisions

Once implementes, sorting should NOT be performed using strings.sort.
This should be documented.

Edit:
Wrote a note in the wiki: http://wiki.lazarus.freepascal.org/Grids_Reference_Page#General_comments_on_the_use_of_TValueListEditor

Jesus Reyes

2018-08-22 05:22

developer   ~0110225

>What would happen if you would delete one string from Strings?
>The grid would want to redraw, RowCount has not been altered.
>When trying to draw Cells[ACol,RowCount-1], it will access Strings[RowCount-2] >while at that point in time Strings.Count will be (old) RowCount-2, last string >will be Strings[RowCount-3].
>
>> because is something already done by the grid when the rowcount is changed
>which is not the case in the above scenario.

Your comment makes me think you don't understand what is what I'm trying to explain to you. First you wondered why the exception occurred, I explained to you that the exception is caused by modifying the row property before modifying the rowcount. That's all, I'm not suggesting that rowcount should not be modified as your 'scenario' suggest.

Read again, row should not be changed before rowcount (or implement some kind of locking mechanism so out of sync strings and grid doesn't matter), as is currently done in in AdjustRowCount. There is more, I explained also that row should not be modified at all because this done automatically by the grid when you change the rowcount. On the why the implementor decided to modify the row property before rowcount I don't know his motivation. I really don't know how to explain this in a more clear way.

You can add the workaround of the 'clear' if you want but that doesn't fix the root of the problem, also I don't like the comment about the consequences of not using clear because you are using a workaround in despite of knowing how to fix the real problem, but that it's me.

The comments on grid.pas for FCols.Exchange -> "//exchanges the dimensions (width/height) of the resp. columns" doesn't look right, in any case it should simply be "//exchanges the column widths". But, but, as already said before, if you add the comments for FCols/FRows as for example:

  FCols: TIntegerList; // Holds the columns widths
  FRows: TIntegerList; // Holds the rows heights

No additional comments are needed because it is obvious what are do FCols/FRows Holds. Even more, as FCols and FRows are private I don't mind if they got renamed to FColsWidths and FRowsHeights, and in this case no comments are needed at all, although I would prefer just add a comment to them.

About using strings.sort, the comment on the wiki, etc. I think it should be not necessary as strings is TValueListStrings = class(TStringList) and sort is virtual in TStringList (at least in trunk), you just override it and call the ValueListEditor's one. This way the user is free to use whatever they want, again that is my opinion on it.

About

+procedure TValueListEditor.Sort(ColSorting: Boolean; index, IndxFrom, IndxTo: Integer);

I don't understand why it should be different than the other overloaded sort methods (BTW, why are there so many sort variations, IMO it should be only one or at most two). But anyway, I don't understand why hiding the editor and locking the strings, if the inherited sort doesn't hide and show the editor automatically, then the grid is wrong and it should be fixed and not adding workaround on derived grids (just because all derived grids would need to make the same workarounds)

wp

2018-08-22 09:38

developer   ~0110226

Last edited: 2018-08-22 17:23

View 3 revisions

procedure TValueListEditor.Sort(ColSorting: Boolean;Index, IndxFrom,IndxTo: Integer);

is introduced in TCustomGrid as being protected. Why is it public in TValueListEditor of valedit.sort.7.diff? This does not make sense at all because the user should not be motivated to sort the ValueListEditor by rows which destroys the key=value assignments.

Bart Broersma

2018-08-23 08:41

developer   ~0110249

I made it public so that people wont do Strings.Sorts, which is inferior to the new sort mechanisme.
I planned to remove the first paramter Colsorting (so basically overload the Sort function) (and dit so in valedit.sort.3.diff), but I based my new patch on the one from Jesus, where this was not done.

Thanks for spotting this.
I'll change it.

(B.t.w. stting ColSorting to False will trigger an exception somewhere down the line, and the message will state it is an illegal operation on a TValueListEditor, so we already catered for that in the past).

wp

2018-08-23 10:12

developer   ~0110251

Last edited: 2018-08-23 10:14

View 3 revisions

> setting ColSorting to False will trigger an exception somewhere down the line,
> and the message will state it is an illegal operation on a TValueListEditor, so
> we already catered for that in the past.

Searching for "illegal" in the source I find this one (even before the current changes, in Laz 1.8.4 and 1.2.0):

  procedure TValueListEditor.ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
  begin
    if not IsColumn then
      inherited ExchangeColRow(IsColumn, index, WithIndex)
    else
      Raise EGridException.CreateFmt(rsVLEInvalidRowColOperation,['ExchangeColRow',' on columns']);
  end;

It has been there already for a long time...

Bart Broersma

2018-08-23 16:19

developer   ~0110264

Last edited: 2018-08-23 16:19

View 2 revisions

> It has been there already for a long time...
Yeah, that's what I referred to.

Bart Broersma

2018-08-23 17:32

developer  

valedit.sort.8.diff (5,362 bytes)   
Index: lcl/grids.pas
===================================================================
--- lcl/grids.pas	(revision 58740)
+++ lcl/grids.pas	(working copy)
@@ -1379,7 +1379,7 @@
     procedure DeleteColRow(IsColumn: Boolean; index: Integer);
     procedure DeleteCol(Index: Integer); virtual;
     procedure DeleteRow(Index: Integer); virtual;
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); virtual;
     procedure InsertColRow(IsColumn: boolean; index: integer);
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     procedure SortColRow(IsColumn: Boolean; index:Integer); overload;
@@ -3691,6 +3691,7 @@
 begin
 end;
 
+{Use in derived grids to exchange the actual data}
 procedure TCustomGrid.ColRowExchanged(IsColumn: Boolean; index, WithIndex: Integer);
 begin
 end;
@@ -6282,9 +6283,9 @@
     exit;
   end;
   if IsColumn then
-    FCols.Exchange(index, WithIndex)
+    FCols.Exchange(index, WithIndex) //exchanges the dimensions (width/height) of the resp. columns
   else
-    FRows.Exchange(index, WithIndex);
+    FRows.Exchange(index, WithIndex); //exchanges the dimensions (width/height) of the resp. rows
   ColRowExchanged(IsColumn, index, WithIndex);
   VisualChange;
 
Index: lcl/valedit.pas
===================================================================
--- lcl/valedit.pas	(revision 58740)
+++ lcl/valedit.pas	(working copy)
@@ -149,6 +149,7 @@
     procedure SetFixedCols(const AValue: Integer); override;
     procedure ShowColumnTitles;
     procedure AdjustRowCount; virtual;
+    procedure ColRowExchanged(IsColumn: Boolean; index, WithIndex: Integer); override;
     procedure DefineCellsProperty(Filer: TFiler); override;
     procedure InvalidateCachedRow;
     procedure GetAutoFillColumnInfo(const Index: Integer; var aMin,aMax,aPriority: Integer); override;
@@ -162,6 +163,7 @@
     procedure SetEditText(ACol, ARow: Longint; const Value: string); override;
     procedure SetFixedRows(const AValue: Integer); override;
     procedure SetRowCount(AValue: Integer);
+    procedure Sort(ColSorting: Boolean; index,IndxFrom,IndxTo:Integer); override;
     procedure TitlesChanged(Sender: TObject);
     function ValidateEntry(const ACol,ARow:Integer; const OldValue:string; var NewValue:string): boolean; override;
   public
@@ -176,11 +178,13 @@
     procedure InsertColRow(IsColumn: boolean; index: integer);
     function InsertRow(const KeyName, Value: string; Append: Boolean): Integer;
     procedure InsertRowWithValues(Index: Integer; Values: array of String);
-    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
+    procedure ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer); override;
     function IsEmptyRow: Boolean; {Delphi compatible function}
     function IsEmptyRow(aRow: Integer): Boolean; {This for makes more sense to me}
     procedure MoveColRow(IsColumn: Boolean; FromIndex, ToIndex: Integer);
     function RestoreCurrentRow: Boolean;
+    procedure Sort;
+    procedure Sort(Index, IndxFrom, IndxTo: Integer);
 
     property Modified;
     property Keys[Index: Integer]: string read GetKey write SetKey;
@@ -536,6 +540,7 @@
 procedure TValueListStrings.Assign(Source: TPersistent);
 begin
   FGrid.InvalidateCachedRow;
+  Clear;  //if this is not done, and a TValueListEditor.Sort() is done and then later a Strings.Assign, an exception will occur.
   inherited Assign(Source);
   if (Source is TValueListStrings) then
     FItemProps.Assign(TValueListStrings(Source).FItemProps);
@@ -848,7 +853,7 @@
 procedure TValueListEditor.ExchangeColRow(IsColumn: Boolean; index, WithIndex: Integer);
 begin
   if not IsColumn then
-    Strings.Exchange(Index - FixedRows, WithIndex - FixedRows)
+    inherited ExchangeColRow(IsColumn, index, WithIndex)
   else
     Raise EGridException.CreateFmt(rsVLEInvalidRowColOperation,['ExchangeColRow',' on columns']);
 end;
@@ -910,9 +915,18 @@
   end;
 end;
 
+procedure TValueListEditor.Sort;
+begin
+  SortColRow(True, 0);
+end;
+
+procedure TValueListEditor.Sort(Index, IndxFrom, IndxTo: Integer);
+begin
+  Sort(True, Index, IndxFrom, IndxTo);
+end;
+
 procedure TValueListEditor.StringsChange(Sender: TObject);
 begin
-  //Since we never call inherited SetCell, this seems the logical place to do it
   Modified := True;
   AdjustRowCount;
   Invalidate;
@@ -1100,6 +1114,13 @@
   end;
 end;
 
+procedure TValueListEditor.ColRowExchanged(IsColumn: Boolean; index,
+  WithIndex: Integer);
+begin
+  Strings.Exchange(Index - FixedRows, WithIndex - FixedRows);
+  inherited ColRowExchanged(IsColumn, index, WithIndex);
+end;
+
 procedure TValueListEditor.DefineCellsProperty(Filer: TFiler);
 begin
 end;
@@ -1336,6 +1357,22 @@
   end;
 end;
 
+procedure TValueListEditor.Sort(ColSorting: Boolean; index, IndxFrom,
+  IndxTo: Integer);
+var
+  HideEditor: Boolean;
+begin
+  HideEditor := goAlwaysShowEditor in Options;
+  if HideEditor then Options := Options - [goAlwaysShowEditor];
+  Strings.BeginUpdate;
+  try
+    inherited Sort(True, index, IndxFrom, IndxTo);
+  finally
+    Strings.EndUpdate;
+  end;
+  if HideEditor then Options := Options + [goAlwaysShowEditor];
+end;
+
 procedure TValueListEditor.TitlesChanged(Sender: TObject);
 begin
   // Refresh the display.
valedit.sort.8.diff (5,362 bytes)   

Bart Broersma

2018-08-23 17:32

developer   ~0110265

Please see if you can spot any remaining problems/mistakes in valedit.sort.8.diff.

wp

2018-08-24 00:32

developer   ~0110277

Looks good.

I followed Jesus' suggestion and played with calling only FGrid.Sort in TValueListStrings.Sort. This resolves the issue noted in (0110221), but brings up the next problem: TValueListStrings.CustomSort still shows the same issue - this cannot be fixed so easily because the grid has no CustomSort compatible with TStringList.CustomSort.

Instead of squeezing grid properties into the strings I think it is better to keep your version and to deprecate both TValueListStrings.Sort and .CustomSort.

Bart Broersma

2018-08-24 16:20

developer   ~0110285

TValueListStrings.CustomSort is only there because in fpc < 3, sorting did not call Exchange, but InternalExchange, and therefore ItempProps weren't exchanged.
Since 3.0 TStringList.Sort will call Exchanged if it is overridden.
So this (and TValueListStrings.QuickSortStringsAndItemProps) can probably be removed (or ifdef-ed for fpc_fullversion < 30000).

Nothing however will prevent users from doing Strings.Sort or Strings.CustomSort, since these are public methods of the ancestor.

Bart Broersma

2018-08-24 18:09

developer   ~0110293

In the end I changed Sort; into Sort(ACol: TVleSortCol = colKey);, where TVleSortCol is an enum.

wp

2018-08-24 18:44

developer   ~0110294

Good. Thanks

Issue History

Date Modified Username Field Change
2018-08-17 17:07 wp New Issue
2018-08-17 17:07 wp Status new => assigned
2018-08-17 17:07 wp Assigned To => Bart Broersma
2018-08-17 17:09 wp File Added: valuelisteditor_sort.zip
2018-08-17 17:09 wp File Added: valedit.pas.patch
2018-08-17 17:10 wp Additional Information Updated View Revisions
2018-08-17 23:45 Bart Broersma Note Added: 0110095
2018-08-18 00:01 Bart Broersma Note Edited: 0110095 View Revisions
2018-08-18 00:35 wp File Added: valuelisteditor_sort_v2.zip
2018-08-18 00:36 wp File Added: valuelisteditor_sorting_v2.patch
2018-08-18 00:52 wp Note Added: 0110096
2018-08-18 00:54 wp Note Added: 0110097
2018-08-18 00:56 wp Note Edited: 0110097 View Revisions
2018-08-18 00:58 wp Note Edited: 0110096 View Revisions
2018-08-18 00:58 wp Note Edited: 0110096 View Revisions
2018-08-18 10:02 Bart Broersma Note Added: 0110107
2018-08-18 10:44 Bart Broersma Note Added: 0110113
2018-08-18 10:57 Bart Broersma File Added: valedit.sort.diff
2018-08-18 10:58 Bart Broersma Note Edited: 0110113 View Revisions
2018-08-18 11:01 Bart Broersma Note Edited: 0110113 View Revisions
2018-08-18 13:39 wp Note Added: 0110117
2018-08-18 13:39 wp File Added: valuelisteditor_sorting_v3.patch
2018-08-18 14:05 Bart Broersma Note Added: 0110118
2018-08-18 14:23 Bart Broersma File Added: valedit.sort.2.diff
2018-08-18 14:27 Bart Broersma Note Added: 0110123
2018-08-18 16:15 wp Note Added: 0110126
2018-08-18 23:25 Bart Broersma Note Added: 0110139
2018-08-18 23:30 Bart Broersma Note Edited: 0110139 View Revisions
2018-08-19 00:13 Jesus Reyes Note Added: 0110140
2018-08-19 00:44 Bart Broersma Note Added: 0110142
2018-08-19 10:34 Jesus Reyes Note Added: 0110146
2018-08-19 12:11 Bart Broersma Note Added: 0110148
2018-08-19 12:13 Bart Broersma Note Edited: 0110148 View Revisions
2018-08-19 12:26 Bart Broersma File Added: valedit.sort.3.diff
2018-08-19 12:34 Bart Broersma Note Added: 0110149
2018-08-19 12:34 Bart Broersma File Deleted: valedit.sort.3.diff
2018-08-19 12:34 Bart Broersma File Added: valedit.sort.3.diff
2018-08-19 21:32 wp Note Added: 0110154
2018-08-19 22:01 Bart Broersma Note Added: 0110155
2018-08-20 02:23 Jesus Reyes Note Added: 0110161
2018-08-20 02:45 Jesus Reyes Note Edited: 0110161 View Revisions
2018-08-20 12:32 Bart Broersma Note Added: 0110165
2018-08-20 12:33 Bart Broersma Note Edited: 0110165 View Revisions
2018-08-20 12:33 Bart Broersma Note Edited: 0110165 View Revisions
2018-08-20 20:37 Jesus Reyes Note Added: 0110173
2018-08-20 20:39 Jesus Reyes File Added: valedit.diff
2018-08-20 22:32 Bart Broersma Note Added: 0110175
2018-08-20 23:00 Bart Broersma File Added: valedit.sort.4.diff
2018-08-20 23:08 Bart Broersma Note Added: 0110176
2018-08-20 23:16 Bart Broersma Note Edited: 0110176 View Revisions
2018-08-20 23:22 Bart Broersma File Added: valedit.sort.5.diff
2018-08-20 23:23 Bart Broersma File Added: ValEdit.zip
2018-08-20 23:25 Bart Broersma Note Edited: 0110176 View Revisions
2018-08-20 23:26 Bart Broersma Note Edited: 0110176 View Revisions
2018-08-20 23:42 Bart Broersma File Added: valedit.sort.6.diff
2018-08-20 23:42 Bart Broersma Note Added: 0110177
2018-08-21 06:53 Jesus Reyes Note Added: 0110179
2018-08-21 10:07 Bart Broersma Note Added: 0110182
2018-08-21 16:03 Bart Broersma Relationship added has duplicate 0034160
2018-08-21 18:07 Bart Broersma Note Added: 0110207
2018-08-21 18:16 Bart Broersma File Added: valedit.sort.7.diff
2018-08-21 18:16 Bart Broersma Note Edited: 0110207 View Revisions
2018-08-21 18:24 Jesus Reyes Note Added: 0110208
2018-08-21 19:14 Bart Broersma Note Added: 0110212
2018-08-21 23:22 wp Note Added: 0110221
2018-08-21 23:41 Bart Broersma Note Added: 0110223
2018-08-21 23:46 Bart Broersma Note Edited: 0110223 View Revisions
2018-08-22 05:22 Jesus Reyes Note Added: 0110225
2018-08-22 09:38 wp Note Added: 0110226
2018-08-22 09:39 wp Note Edited: 0110226 View Revisions
2018-08-22 17:23 wp Note Edited: 0110226 View Revisions
2018-08-23 08:41 Bart Broersma Note Added: 0110249
2018-08-23 10:12 wp Note Added: 0110251
2018-08-23 10:14 wp Note Edited: 0110251 View Revisions
2018-08-23 10:14 wp Note Edited: 0110251 View Revisions
2018-08-23 16:19 Bart Broersma Note Added: 0110264
2018-08-23 16:19 Bart Broersma Note Edited: 0110264 View Revisions
2018-08-23 17:32 Bart Broersma File Added: valedit.sort.8.diff
2018-08-23 17:32 Bart Broersma Note Added: 0110265
2018-08-24 00:32 wp Note Added: 0110277
2018-08-24 16:20 Bart Broersma Note Added: 0110285
2018-08-24 18:09 Bart Broersma Fixed in Revision => r58773
2018-08-24 18:09 Bart Broersma LazTarget - => 1.10
2018-08-24 18:09 Bart Broersma Note Added: 0110293
2018-08-24 18:09 Bart Broersma Status assigned => resolved
2018-08-24 18:09 Bart Broersma Fixed in Version => 1.10
2018-08-24 18:09 Bart Broersma Resolution open => fixed
2018-08-24 18:09 Bart Broersma Target Version => 1.10
2018-08-24 18:44 wp Note Added: 0110294
2018-08-24 18:44 wp Status resolved => closed