View Issue Details

IDProjectCategoryView StatusLast Update
0036284FPCRTLpublic2019-11-12 14:04
ReporterAlexey Tor.Assigned ToMichael Van Canneyt 
PrioritynormalSeverityminorReproducibilityN/A
Status resolvedResolutionfixed 
Product Version3.3.1Product Build 
Target VersionFixed in Version3.3.1 
Summary0036284: TStringList.SetCommaText needs refactor
DescriptionIt's obvious this should use SetDelimitedText with other params (2nd 3rd):

Procedure TStrings.SetCommaText(const Value: string);
begin
  CheckSpecialChars;
  C1:=Delimiter;
  C2:=QuoteChar;
  Delimiter:=',';
  QuoteChar:='"';
  Try
    SetDelimitedText(Value);
  Finally
    Delimiter:=C1;
    QuoteChar:=C2;
  end;
end;

it's obvious this needs to be changed too:

procedure TStrings.AddCommaText(const S: String);
var
  L: TStringList;
begin
  L := TStringList.Create;
  try
    L.CommaText := S;
    AddStrings(L);
  finally
    L.Free;
  end;
end;
TagsNo tags attached.
Fixed in Revision43454
FPCOldBugId
FPCTarget3.2.0
Attached Files

Activities

jamie philbrook

2019-11-09 21:41

reporter   ~0119177

Does this cause some issue "Crash for example" that wasn't there before the addition of these methods ?
 
Does the new methods somehow interfere or change the original behavior of the TStrings with other controls currently using it?

There was never an issue with the TStrings before this addition and as far as I know nothing has changed with the prior operation of the class so what's the issue ?

  I believe I know what the issue is. Need I say anymore ?

wp

2019-11-09 22:16

reporter   ~0119178

What is the issue with these procedures? Not "obvious" at least for me...

Alexey Tor.

2019-11-10 06:04

reporter   ~0119181

To make it obvious- such method was added in last week--

Procedure TStrings.DoSetDelimitedText(const AValue: string; DoClear,aStrictDelimiter : Boolean; aQuoteChar,aDelimiter : Char);

1) so .AddCommaText can just call this method, w/o creating temp StringList (with DoClear=false)
2) so .SetCommaText can call this method, w/o saving/restoring values Delimiter, QuoteChar (with DoClear=true)

Thaddy de Koning

2019-11-10 14:09

reporter   ~0119187

Last edited: 2019-11-10 17:49

View 5 revisions

Yes, because the Tstrings can differ in their handling, the delimited text settings have to be synchronized which is what the code does.
At least for this report it is obvious you made the mistake to assume the delimiter settings would be the same between the two Tstrings, which is obviously not always the case.

The Receiving TStrings can only accept the text if it cleaned of the separators used in the added Tstrings and that can be different from the receiving Tstrings, so the applied patch is correctl You can not take any obvious shortcut.
Commatext is more restricted, but imho does not deserve a different code path from delimitedtext for this feature.

Michael Van Canneyt

2019-11-12 14:04

administrator   ~0119239

Refactored. Thanks for reporting.

Issue History

Date Modified Username Field Change
2019-11-09 18:22 Alexey Tor. New Issue
2019-11-09 21:41 jamie philbrook Note Added: 0119177
2019-11-09 22:16 wp Note Added: 0119178
2019-11-10 06:04 Alexey Tor. Note Added: 0119181
2019-11-10 14:09 Thaddy de Koning Note Added: 0119187
2019-11-10 17:37 Thaddy de Koning Note Edited: 0119187 View Revisions
2019-11-10 17:39 Thaddy de Koning Note Edited: 0119187 View Revisions
2019-11-10 17:49 Thaddy de Koning Note Edited: 0119187 View Revisions
2019-11-10 17:49 Thaddy de Koning Note Edited: 0119187 View Revisions
2019-11-12 13:40 Michael Van Canneyt Assigned To => Michael Van Canneyt
2019-11-12 13:40 Michael Van Canneyt Status new => assigned
2019-11-12 14:04 Michael Van Canneyt Status assigned => resolved
2019-11-12 14:04 Michael Van Canneyt Resolution open => fixed
2019-11-12 14:04 Michael Van Canneyt Fixed in Version => 3.3.1
2019-11-12 14:04 Michael Van Canneyt Fixed in Revision => 43454
2019-11-12 14:04 Michael Van Canneyt FPCTarget => 3.2.0
2019-11-12 14:04 Michael Van Canneyt Note Added: 0119239