TStrings.GetTextStr patch
lazarus / 64-FPC development

----------------------------------------------------------------------
ASerge · 2019-03-18 18:04

Before posting to bugtracker, I decided to show up on the forum first,
in case I missed something.

A simple project to show the bug in TStrings.GetTextStr:
Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i program
TestStringWithDiffCodePages; {$APPTYPE CONSOLE}{$MODE
OBJFPC}{$LONGSTRINGS ON}{$CODEPAGE UTF8}  // Assuming the file is
saved in UTF 8 encoding uses Windows, Classes;    procedure
MsgBox(const S: string);begin  MessageBoxW(0,
PWideChar(UnicodeString(S)), 'Info',
MB_ICONINFORMATION);end; procedure ShowByStringList(const A: array of
string);var  L: TStringList;begin  L := TStringList.Create;  try   
L.AddStrings(A);    //L.LineBreak := 'à';    MsgBox(L.Text);  finally 
  L.Free;  end;end; var  S: RawByteString;  UserInput: string;begin 
//SetMultiByteFileSystemCodePage(CP_UTF8); // Imitation of the
behavior of LCL  //ShowByStringList(['', '', '']);  S := 'It was about
5 hours';  //SetCodePage(S, 1250);  Write('Enter any string: '); 
Readln(UserInput);  ShowByStringList([UserInput, S, 'Il était à peu
près 5 heures', '大約5個小時']);end.The problem is that GetTextStr simply
ignores the code pages of the list strings.
The proposed solution works as fast as the old code, if the code pages
of all lines are the same (more frequent case), slower, but correct
when different.
It also takes into account the case of a non-standard line break in
particular that its code page does not match those of the strings.
Add this to the beginning of the project above to check the fix:
Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i type 
TStringList = class(Classes.TStringList)  protected    function
GetTextStr: string; override;  end; function TStringList.GetTextStr:
string;   procedure ConvertToUtf8(var S: string);  begin    // Fans of
inlining so complicated the AnsiToUtf8, that one this call    //
generates a lot of code, so it is made as a separate procedure    S :=
AnsiToUtf8(S);  end; var  LLineBreak: string;  CodePage:
TSystemCodePage;   // In most cases, the line break is an ASCII string
and does not depend  // on the code page, but we must to check 
function LineBreakDependsOnCodePage: Boolean;  var   
ChangedLineBreak: RawByteString;  begin    ChangedLineBreak :=
LLineBreak;    SetCodePage(ChangedLineBreak, CodePage, True); // With
conversion    Result := (LLineBreak <> ChangedLineBreak);  end; const 
CLineBreaks: array[TTextLineBreakStyle] of string = (    #10, //
tlbsLF    #13#10, // tlbsCRLF    #13); // tlbsCRvar  i, LCount:
SizeInt;  DifferentCodePages, CodePageValid: Boolean; 
CurrentStringLength, ResultLength, LineBreakLength: SizeInt; 
CurrentString: string;  InsertPoint: PAnsiChar;begin  LCount :=
Self.Count;  if LCount <= 0 then    Exit('');  CheckSpecialChars; 
LLineBreak := Self.LineBreak;  // When the LineBreak is default, the
TextLineBreakStyle is used  if LLineBreak = sLineBreak then   
LLineBreak := CLineBreaks[TextLineBreakStyle];  // Calculate the
required place for the result  ResultLength := 0;  LineBreakLength :=
Length(LLineBreak);  // When scanning, check that the code pages of
all strings are the same  // We do not use the default code page to
avoid conversions when  // all code pages are the same but do not
match the default  DifferentCodePages := False;  CodePageValid :=
False;  CodePage := 0; // Only to suppress the compiler warning  i :=
0;  repeat    CurrentString := Strings[i];    if CurrentString <> ''
then    begin      if CodePageValid then        DifferentCodePages :=
(StringCodePage(CurrentString) <> CodePage)      else      begin     
 CodePage := StringCodePage(CurrentString);        CodePageValid :=
True;        DifferentCodePages := LineBreakDependsOnCodePage;     
end;      Inc(ResultLength, Length(CurrentString));    end;   
Inc(ResultLength, LineBreakLength);    Inc(i);  until (i >= LCount) or
DifferentCodePages;  if not CodePageValid then // All strings are
empty, so we use the LineBreak code page  begin    if LLineBreak = ''
then // Empty strings with an empty line break      Exit('');   
CodePage := StringCodePage(LLineBreak);  end;  if DifferentCodePages
then // Convert all strings to UTF8  begin    CodePage := CP_UTF8;   
ConvertToUtf8(LLineBreak);    LineBreakLength := Length(LLineBreak); 
 ResultLength := 0;    i := 0;    repeat      CurrentString :=
Strings[i];      if CurrentString <> '' then      begin       
ConvertToUtf8(CurrentString);        Inc(ResultLength,
Length(CurrentString));      end;      Inc(ResultLength,
LineBreakLength);      Inc(i);    until i >= LCount;  end;  // OK, new
length defined, allocate and copy to it  SetLength(Result,
ResultLength);  SetCodePage(RawByteString(Result), CodePage, False);
// without conversion!  InsertPoint := Pointer(Result);  i := 0; 
repeat    CurrentString := Strings[i];    if CurrentString <> '' then 
  begin      if DifferentCodePages then       
ConvertToUtf8(CurrentString);      CurrentStringLength :=
Length(CurrentString);      System.Move(Pointer(CurrentString)^,
InsertPoint^, CurrentStringLength);      Inc(InsertPoint,
CurrentStringLength);    end;    System.Move(Pointer(LLineBreak)^,
InsertPoint^, LineBreakLength);    Inc(InsertPoint, LineBreakLength); 
  Inc(i);  until i >= LCount;  if SkipLastLineBreak then   
SetLength(Result, Length(Result) - LineBreakLength);end;

----------------------------------------------------------------------
marcov · 2019-03-18 18:21

The simplest test is to test it with an encoding other than utf8, and
a test set with strings that use the full range (0..255) of that
encoding.

Anyway,  a conversion to utf8 that is not guarded by a check for a
codepage (e.g. stringcodepage) is enormously doubtful.

----------------------------------------------------------------------
engkin · 2019-03-18 19:01

Two notes:
1-AnsiToUtf8 converts to UnicodeString before it converts to UTF8.
2-When DifferentCodePages is true, the conversion to UTF8 occurs
twice. Which makes it four coversions: codepage->Unicode->UTF8 twice.

Might be faster, when different code pages, to initially have 4x the
total length of strings (to account for the worst case, and this total
length should work for both UTF16 and UTF8), then convert to
UnicodeString before converting to UTF8 at the end.

----------------------------------------------------------------------
Thaddy · 2019-03-18 20:06

Quote from: engkin on March 18, 2019, 08:01:56 pmTwo notes:
1-AnsiToUtf8 converts to UnicodeString before it converts to UTF8.
2-When DifferentCodePages is true, the conversion to UTF8 occurs
twice. Which makes it four coversions: codepage->Unicode->UTF8 twice.

Might be faster, when different code pages, to initially have 4x the
total length of strings (to account for the worst case, and this total
length should work for both UTF16 and UTF8), then convert to
UnicodeString before converting to UTF8 at the end.
3. Also note Tstrings is a FPC class and not part of Lazarus. The FPC
compiler supports Ansi, UTF16 and ShortString. Be careful with UTF8
other than its codepage support for patches that are geared towards
UTF8: doesn't belong in FPC..
4. The conversion to UTF8 from UTF16 is already transparent and
assignment compatible.
5. The UTF16 lengths can indeed differ from UTF8: 2-4 and 1-2-3-4
bytes

----------------------------------------------------------------------
ASerge · 2019-03-18 20:28

Quote from: marcov on March 18, 2019, 07:21:14 pmThe simplest test is
to test it with an encoding other than utf8, and a test set with
strings that use the full range (0..255) of that encoding.
Readln(UserInput);

----------------------------------------------------------------------
ASerge · 2019-03-18 20:32

Quote from: engkin on March 18, 2019, 08:01:56 pmTwo notes:
1-AnsiToUtf8 converts to UnicodeString before it converts to UTF8.
Of course, this is required for the correct conversion of different
code pages.

Quote2-When DifferentCodePages is true, the conversion to UTF8 occurs
twice. Which makes it four coversions: codepage->Unicode->UTF8 twice.
As I said, this rarely happens, but if you offer a faster, but correct
option, it will be great.

----------------------------------------------------------------------
ASerge · 2019-03-18 20:36

Quote from: Thaddy on March 18, 2019, 09:06:56 pm3. Also note Tstrings
is a FPC class and not part of Lazarus. The FPC compiler supports
Ansi, UTF16 and ShortString. Be careful with UTF8 other than its
codepage support for patches that are geared towards UTF8: doesn't
belong in FPC..
Classes compiled with {$LONGSTRINGS ON}, so string is AnsiString.

----------------------------------------------------------------------
ASerge · 2019-03-31 12:38

Quote from: ASerge on March 18, 2019, 09:32:06 pmQuote from: engkin on
March 18, 2019, 08:01:56 pm2-When DifferentCodePages is true, the
conversion to UTF8 occurs twice. Which makes it four coversions:
codepage->Unicode->UTF8 twice.
As I said, this rarely happens, but if you offer a faster, but correct
option, it will be great.
Next version. With speed test.