Recent

Author Topic: sort bug for cells with empty ('') string  (Read 1415 times)

iteh

  • New Member
  • *
  • Posts: 33
sort bug for cells with empty ('') string
« on: May 20, 2019, 10:31:12 am »
problem with sorting columns with cells in which text was added by function WriteText, but added an empty line ('').

Example: form with sWorksheetGrid1: TsWorksheetGrid.

FormCreate:
  sWorksheetGrid1.Worksheet.WriteText(0,1,'B1');
  sWorksheetGrid1.Worksheet.WriteText(1,1,'B2');
  sWorksheetGrid1.Worksheet.WriteText(3,1,'B4');
  sWorksheetGrid1.Worksheet.WriteText(0,0,'A1');
  sWorksheetGrid1.Worksheet.WriteText(1,0,'A2');
  sWorksheetGrid1.Worksheet.WriteText(2,0,'');
  sWorksheetGrid1.Worksheet.WriteText(3,0,'A4');

sWorksheetGrid1HeaderClick:
  ShowMessage('begin');
  if (IsColumn = true) and (Index > 0) then
    sWorksheetGrid1.SortColRow(true,Index);
  ShowMessage('end');

(with sWorksheetGrid1.Worksheet.Sort - the same result)

if start the program and click on column B - everything is fine, and if restart the program and click on column A - the program freezes.

wp

  • Hero Member
  • *****
  • Posts: 6454
Re: sort bug for cells with empty ('') string
« Reply #1 on: May 21, 2019, 04:27:43 pm »
Please try the new revision in svn.
Lazarus trunk / fpc 3.0.4 / all 32-bit on Win-10

iteh

  • New Member
  • *
  • Posts: 33
Re: sort bug for cells with empty ('') string
« Reply #2 on: May 21, 2019, 09:53:11 pm »
in r6932 sorting works correctly and with empty ('') cells, thank you !

(maybe other users will write their opinion) Now empty cells are always placed at the end of the list (both with Asc and Desc sorting).

Maybe in one case (for example, Asc) to place them at the top of the list, and in the other (for example, Desc sorting) at the end of the list ?
« Last Edit: May 21, 2019, 10:01:58 pm by iteh »

iteh

  • New Member
  • *
  • Posts: 33
Re: sort bug for cells with empty ('') string
« Reply #3 on: May 22, 2019, 09:57:05 am »
another example of "freezing" and strange sorting. Check, please:
1) form with
    Button1: TButton;
    Button2: TButton;
    sWorksheetGrid1: TsWorksheetGrid;

2) FormCreate:
  sWorksheetGrid1.Worksheet.WriteText(0,1,'B1');
  sWorksheetGrid1.Worksheet.WriteText(1,1,'B2');
  sWorksheetGrid1.Worksheet.WriteText(2,1,'');
  sWorksheetGrid1.Worksheet.WriteText(3,1,'B4');
  sWorksheetGrid1.Worksheet.WriteText(0,0,'A1');
  sWorksheetGrid1.Worksheet.WriteText(1,0,'A2');
  sWorksheetGrid1.Worksheet.WriteText(2,0,'');
  sWorksheetGrid1.Worksheet.WriteText(3,0,'A4');

3) Button1Click:
var
  sp: TsSortParams;
begin
  sp := InitSortParams(true,1);
  sp.Keys[0].ColRowIndex := 0;
  sWorksheetGrid1.Worksheet.Sort(sp,'A1:A4');

4) Button2Click:
var
  sp: TsSortParams;
begin
  sp := InitSortParams(true,1);
  sp.Keys[0].ColRowIndex := 0;
  sWorksheetGrid1.Worksheet.Sort(sp,'B1:B4');

The first click on button1 - the column "A" is sorted as: A1, A2, A4, (empty)
And more sorting does not work (another button click).

If click on button 2 - each press changes the sorting from B1, B2, B4, (empty) to B1, B2, (empty), B4 - but initialization with the same parameters.

column index in "ColRowIndex" should be relative within the sort range or absolute inside the entire grid ?

wp

  • Hero Member
  • *****
  • Posts: 6454
Re: sort bug for cells with empty ('') string
« Reply #4 on: May 22, 2019, 10:02:23 am »
(maybe other users will write their opinion) Now empty cells are always placed at the end of the list (both with Asc and Desc sorting).

Maybe in one case (for example, Asc) to place them at the top of the list, and in the other (for example, Desc sorting) at the end of the list ?
You can use the OnCompareCells event of the Worksheet to define custom sorting criteria. At first handle the special cases (e.g. move empty cells to the top), then call the default sorting procedure which is accessible now as DefaultCompareCells.
Lazarus trunk / fpc 3.0.4 / all 32-bit on Win-10

iteh

  • New Member
  • *
  • Posts: 33
Re: sort bug for cells with empty ('') string
« Reply #5 on: May 22, 2019, 10:28:51 am »
You can use the OnCompareCells event of the Worksheet to define custom sorting criteria. At first handle the special cases (e.g. move empty cells to the top), then call the default sorting procedure which is accessible now as DefaultCompareCells.
I cannot find OnCompareCells in the description on http://wiki.freepascal.org/FPSpreadsheet and visually in the object inspector.

Can this event be added to the object inspector with cell1, cell2 and sort params or are these Worksheet event and cannot be added to the object inspector for TsWorksheetGrid ?

I found:

  {@@ This event can be used to override the built-in comparing function which
    is called when cells are sorted. }
  TsCellCompareEvent = procedure (Sender: TObject; ACell1, ACell2: PCell; var AResult: Integer) of object;

how I can check which sorting parameters (to return the result of the comparison depending on the parameters) are applied to the current compared cells - what index is currently used inside the keys array, for the current call of custom comparison procedure ?
« Last Edit: May 22, 2019, 10:39:27 am by iteh »

wp

  • Hero Member
  • *****
  • Posts: 6454
Re: sort bug for cells with empty ('') string
« Reply #6 on: June 04, 2019, 10:46:55 pm »
Sorry, I missed that there are still open questions.

another example of "freezing" and strange sorting. Check, please:
1) form with
    Button1: TButton;
    Button2: TButton;
    sWorksheetGrid1: TsWorksheetGrid;

2) FormCreate:
  sWorksheetGrid1.Worksheet.WriteText(0,1,'B1');
  sWorksheetGrid1.Worksheet.WriteText(1,1,'B2');
  sWorksheetGrid1.Worksheet.WriteText(2,1,'');
  sWorksheetGrid1.Worksheet.WriteText(3,1,'B4');
  sWorksheetGrid1.Worksheet.WriteText(0,0,'A1');
  sWorksheetGrid1.Worksheet.WriteText(1,0,'A2');
  sWorksheetGrid1.Worksheet.WriteText(2,0,'');
  sWorksheetGrid1.Worksheet.WriteText(3,0,'A4');

3) Button1Click:
var
  sp: TsSortParams;
begin
  sp := InitSortParams(true,1);
  sp.Keys[0].ColRowIndex := 0;
  sWorksheetGrid1.Worksheet.Sort(sp,'A1:A4');

4) Button2Click:
var
  sp: TsSortParams;
begin
  sp := InitSortParams(true,1);
  sp.Keys[0].ColRowIndex := 0;
  sWorksheetGrid1.Worksheet.Sort(sp,'B1:B4');

The first click on button1 - the column "A" is sorted as: A1, A2, A4, (empty)
And more sorting does not work (another button click).

If click on button 2 - each press changes the sorting from B1, B2, B4, (empty) to B1, B2, (empty), B4 - but initialization with the same parameters.

I think you misunderstand the meaning of the parameters. The Keys[0].ColRowIndex identifies the column (in your case, but it can also be a row) which provides the cells used as sorting criteria. The cell range given as second parameter of the Sort method identifies the cells which will be reordered according to the sorting criteria.

Normally the ColRowIndex column is included in the Sort range parameter, i.e. I would normally call Sort(sp, 'A1:B4'). Under these circumstances the cells in column A are compared, reordered and the cells in column B follow. Once the sort is finished the sheet does not change any more when you sort with the same parameters a second time because the criteria column is already sorted.

The click code of Button1 is exactly this type, it sorts only column A and also column A is rearranged.

The click code of Button 2 is strange: Column A provides the sorting criteria, but the cells in column B are exchanged for the sort. So, after the "sort" has finished column A is still the same but column B has been rearranged. When you repeat the sort by a second click the column B will be reordered again because the sort criteria column had not changed. In total, this is an invalid sorting procedure. Probably the method should raise an error when the criteria column is not part of the sorted range.

I cannot find OnCompareCells in the description on http://wiki.freepascal.org/FPSpreadsheet and visually in the object inspector.

Can this event be added to the object inspector with cell1, cell2 and sort params or are these Worksheet event and cannot be added to the object inspector for TsWorksheetGrid ?
The OnCompareCells is a way to provide custom sorting criteria for the worksheet. It is not compatible with grid events. Since the grid merely displays the contents of the worksheet it was implemented this way.

I found:

  {@@ This event can be used to override the built-in comparing function which
    is called when cells are sorted. }
  TsCellCompareEvent = procedure (Sender: TObject; ACell1, ACell2: PCell; var AResult: Integer) of object;

how I can check which sorting parameters (to return the result of the comparison depending on the parameters) are applied to the current compared cells - what index is currently used inside the keys array, for the current call of custom comparison procedure ?
Since the compare procedure is active only during the Sort procedure you should immediately see which sorting criteria are used.

If you ask how you can tell which compare procedure is used by default the answer is: Open unit "fpspreadsheet" and study the method TsWorksheet.DefaultCompareCells: it compare text cells by their text content, number and date/time cells by the numerical value. In the mixed case, i.e. comparison of a text cell with a number cell, the order depends on the Priority element of the SortParams record (spNumAlpha = numbers first, or spAlphaNum = text first).
Lazarus trunk / fpc 3.0.4 / all 32-bit on Win-10

iteh

  • New Member
  • *
  • Posts: 33
Re: sort bug for cells with empty ('') string
« Reply #7 on: June 05, 2019, 04:02:26 am »

I think you misunderstand the meaning of the parameters. The Keys[0].ColRowIndex identifies the column (in your case, but it can also be a row) which provides the cells used as sorting criteria. The cell range given as second parameter of the Sort method identifies the cells which will be reordered according to the sorting criteria.

I probably incorrectly formulated a question.

what defines ColRowIndex (for "columns sort" example) ?
1) the global index of the column inside the grid: if the sorting range is "A1:D10" and we sort only by D - then Keys[0].ColRowIndex = 3 (A = 0, B = 1, ..., D = 3)
2) the sequential number (starting from zero) of the column inside the range for sorting: if we have a range with the data "A1:D10" in the grid, but we need to sort only (I understand that with such sorting I “break” the rows in columns A and B, but I give an example for clarity) the range "C1:D10" and sort by column "D" - then specify Keys[0].ColRowIndex as 1 (column "C" = 0, column "D" = 1 inside the "C:D" range if we count from zero).

  TsCellCompareEvent = procedure (Sender: TObject; ACell1, ACell2: PCell; var AResult: Integer) of object;
Since the compare procedure is active only during the Sort procedure you should immediately see which sorting criteria are used.

I will clarify the question (it is difficult to formulate through Google a translator from another language, sorry):

I wrote my handler as TsCellCompareEvent, inside I get from caller only the Sender, ACell1 and ACell2.

How did I (when my custom sorting procedure was called), inside this procedure, to understand which of the columns (index inside Keys array) is now sorted and which ASortOptions should be used for the current compared column (so that I write the correct code for comparing ACell1 and ACell2 for current ASortOptions because these options may be different for different columns) ?

Here is the source code:

Code: Pascal  [Select]
  1.     if Assigned(OnCompareCells) then
  2.       OnCompareCells(Self, cell1, cell2, Result)
  3.     else
  4.       Result := DefaultCompareCells(cell1, cell2, FSortParams.Keys[key].Options);

here we see that the default function receives information about options (FSortParams.Keys[key].Options), but the custom procedure does not receive such information ("OnCompareCells(Self, cell1, cell2, Result)" and the "FSortParams.Keys[key].Options" is not passed anywhere).

:(
« Last Edit: June 05, 2019, 04:04:31 am by iteh »

wp

  • Hero Member
  • *****
  • Posts: 6454
Re: sort bug for cells with empty ('') string
« Reply #8 on: June 15, 2019, 11:54:35 pm »
I think that the sorting support by fpspreadsheet should be rewritten in a more practical way. Since you seem to be more involved in sorting than myself, I ask you for your opinion on the following proposal:

I would like to provide the full TsSortKey parameter in the OnCompareCells event (and in DefaultCompareCells, as well) instead of the TsSortOptions alone. The TsSortPriority parameter should be moved to the TsSortKey record.
Code: Pascal  [Select]
  1. type
  2.   TsSortOption = (ssoDescending, ssoCaseInsensitive);
  3.   TsSortOptions = set of TsSortOption;
  4.  
  5.   TsSortPriority = (spNumAlpha, spAlphaNum);   // spNumAlpha: Number < Text
  6.  
  7.   TsSortKey = record
  8.     ColRowIndex: Integer;
  9.     Options: TsSortOptions;
  10.     Priority: TsSortPriority;
  11.   end;
  12.  
  13.   TsSortKeys = array of TsSortKey;
  14.  
  15.   TsSortParams = record
  16.     SortByCols: Boolean;
  17.     Keys: TsSortKeys;
  18.   end;  
  19.  
  20.   TsCellCompareEvent = procedure (Sender: TObject; ACell1, ACell2: PCell; const AKey: TsSortKey;
  21.     var AResult: Integer) of object;
  22.  

Would this modification meet your requirements?
I fear, however, that it will break existing code...
Lazarus trunk / fpc 3.0.4 / all 32-bit on Win-10

iteh

  • New Member
  • *
  • Posts: 33
Re: sort bug for cells with empty ('') string
« Reply #9 on: June 16, 2019, 07:59:13 am »
I think that the sorting support by fpspreadsheet should be rewritten in a more practical way. Since you seem to be more involved in sorting than myself, I ask you for your opinion on the following proposal:

I would like to provide the full TsSortKey parameter in the OnCompareCells event (and in DefaultCompareCells, as well) instead of the TsSortOptions alone. The TsSortPriority parameter should be moved to the TsSortKey record.

Would this modification meet your requirements?
I fear, however, that it will break existing code...
(rewrote the message to make it shorter)
Can we not provide the sorting parameters to the custom procedure, but add a global index variable (as a sheet / book class properties like "CurrentSortKeyIndex") and write to it the current index (from 0 to High(FSortParams.Keys)) of the element inside the sorting array ?

And, from the custom sorting procedure, using this index to get the necessary parameters.
And this will allow us (from another thread or by timer) to analyze this variable and understand how many have already sorted if the sorting process takes a long time.
If there is no sorting now, write to this variable, for example, "-1".

« Last Edit: June 16, 2019, 08:44:22 am by iteh »

Zoran

  • Hero Member
  • *****
  • Posts: 1468
    • http://wiki.lazarus.freepascal.org/User:Zoran
Re: sort bug for cells with empty ('') string
« Reply #10 on: June 16, 2019, 06:42:50 pm »
I have a problem with sort, as implemented currently.

First, let us see that, as currently implemented, Sort procedure of tsWorkSheet calls DoCompareCells, which then iterates through SortParams.Keys and for each Key:
  -- if OnCompareCells event is assigned, calls it for each key
  -- or (if the event is not assigned) calls DefaultCompareCells with SortOptions of the current key.

DefaultCompareCells method, according to SortOptions decides whether numbers go before text, whether direction should be ascending or not, etc.
But, when it falls to comparing two text cells, it uses UTF8CompareString (or UTF8CompareText, if ssoCaseInsensitive is in options) from LazUTF8 unit.
I don't think it is the best decision.

What other FCL and LCL controls use as default is AnsiCompareText (or AnsiCompareString) from sysutils. That is what TStringList uses for sorting, that is what TBufDataset uses, etc.
Contrary to their "Ansi" name, these functions actually use currently installed WideStringManager for comparing text and they are expected to sort utf8 text well.
My problem is that I want text sorting somewhat different from LazUtf8.Utf8CompareText, and I have WideStringManager adjusted to what I need (I always do it on application start), but unlike other components, fpSpreedSheet ignores WideStringManager.

Then, I wanted to use OnCompareCells. I wanted to copy DefaultCompareCelss, and just replace the text comparing functions, as mentioned above. But then, this is a problem:

Unlike DefaultCompareCells function, OnCompareCells does not have SortOptions parameter, which is a problem. I want to use SortOptions first, just like DefaultCompareCells uses them, but only to change text comparing. This is not possible, as SortOptions are not passed to the event.

Of course, as a workaround, I can replace these function calls directly inside the source or DefaultCompareCells implementation, but I really don't like such "solutions".

Perhaps, if you don't like to change the default behaviour, you could add another event - OnCompareText, so that DefaultCompareCells takes care of sorting options (numbers before text, positioning of empty cells, the direction - asc. or desc.), but only when it comes to comparing text cells, then, if this new event is assigned, use it instead currently default functions from LazUTF8.

wp

  • Hero Member
  • *****
  • Posts: 6454
Re: sort bug for cells with empty ('') string
« Reply #11 on: June 17, 2019, 12:52:43 am »
Wouldn't it be better to have the complete TsSortKey as an argument in the OnCompareCells event instead of the SortOptions alone? This way it could be determined which column is currently sorted which may be useful when the individual columns have different sorting criteria. At least this is what I understood as iteh's request.

And shouldn't the SortPriority be part of the TsSortKey record? Again for the same reason: maybe one columns wants the numbers at the top, the other one at the end.
Lazarus trunk / fpc 3.0.4 / all 32-bit on Win-10

iteh

  • New Member
  • *
  • Posts: 33
Re: sort bug for cells with empty ('') string
« Reply #12 on: June 17, 2019, 09:11:10 am »
Wouldn't it be better to have the complete TsSortKey as an argument in the OnCompareCells event instead of the SortOptions alone? This way it could be determined which column is currently sorted which may be useful when the individual columns have different sorting criteria. At least this is what I understood as iteh's request.

And shouldn't the SortPriority be part of the TsSortKey record? Again for the same reason: maybe one columns wants the numbers at the top, the other one at the end.
Yes, I would like to see the full set of parameters inside OnCompareCells, but I would also like to see a global variable (outside OnCompareCells) where I could see which index (not the column index, but the current index then iterates through SortParams.Keys array) is now processed to display the progressbar if we sort many columns and big table.

maybe add a class (?) Sort in TsWorksheet and put in it:
- SortParams (type: TsSortParams) in which to write the sorting parameters
- CurrSortIndex (integer type) in which to record the current index inside the array SortParams.Keys, which is currently being sorted
- the "DoSort" method which is called instead of the existing TsWorksheet.Sort (but no longer pass TsSortParams separately to this function)

and then inside the existing OnCompareCells we can always get all the sorting parameters by referring to
SortParams.Keys[CurrSortIndex] or something like that

Zoran

  • Hero Member
  • *****
  • Posts: 1468
    • http://wiki.lazarus.freepascal.org/User:Zoran
Re: sort bug for cells with empty ('') string
« Reply #13 on: June 17, 2019, 11:24:17 am »
Wouldn't it be better to have the complete TsSortKey as an argument in the OnCompareCells event instead of the SortOptions alone? This way it could be determined which column is currently sorted which may be useful when the individual columns have different sorting criteria. At least this is what I understood as iteh's request.

And shouldn't the SortPriority be part of the TsSortKey record? Again for the same reason: maybe one columns wants the numbers at the top, the other one at the end.

Yes, that would be more flexible, of course.

And according to this, instead of more talking, I'm attaching the patch.
I wanted to avoid breaking existing code, so this is what I did:

-- added ssoAlphaBeforeNum to TsSortOptions

However, Priority remains in TsSortParams, so that backward compatibility is not broken and it is used inside InitSortParams, so that each key's priority is initialized according to TsSortParams.Priority by default.

-- added new event OnFullCompareCells, which has SortKey added to parameters. Notice that, as ssoAlphaBeforeNum is now included in Options of each key, the user has full control of sorting parameters.

The old OnCompareCells event is marked deprecated, but it is still working, so that backward compatibility is not broken.

DefaultCompareCells is of course also updated to use priority from each key.

Please test the patch, if you think that something should be implemented differently, feel free to change my code.
I just wanted to help, hope it is useful.

Zoran

Zoran

  • Hero Member
  • *****
  • Posts: 1468
    • http://wiki.lazarus.freepascal.org/User:Zoran
Re: sort bug for cells with empty ('') string
« Reply #14 on: June 17, 2019, 12:01:33 pm »
I noticed a bug in the patch, here corrected, sorry.

It was one line, but still the corrected patch is made against the remote repo, so the new patch replaces the previous.

Or if you applied the previos patch, just change line 4358 in fpspreadsheet.pas
Should be
Code: Pascal  [Select]
  1. if (ACell2 <> nil) and (ACell2^.ContentType <> cctEmpty) then
insead of
Code: Pascal  [Select]
  1. if (ACell2 = nil) or (ACell2^.ContentType = cctEmpty) then