Strings getting corrupted in ComboBox.AddObject. How to add them the proper way?

Question

I'm adding strings with objects (also strings) to a TComboBox, but getting corrupted strings when trying to retrieve them later.

This is how I'm adding them:

var
  i: Integer;
  sl: TStringList;
  c: Integer;
  s: PChar;
begin

  for i := 1 to tblCalls.FieldCount do
    if tblCalls.Fields[i - 1].Tag = 1 then
      ListBox1.Items.Append(tblCalls.Fields[i - 1].FieldName);

  sl := TStringList.Create;
  try
    LoadStyles(TStrings(sl));

    for c := 0 to sl.Count - 1 do
    begin

      s := PChar(sl.Values[sl.Names[c]]);
      ComboBox1.Items.AddObject(sl.Names[c], TObject(s));
    end;

  finally
    sl.Free;
  end;

end;

procedure LoadStyles(var AStylesList: TStrings);
var
  f, n: String;
  filelist: TStringDynArray;
begin
  f := ExtractFilePath(ParamStr(0)) + 'Styles';
  if (not DirectoryExists(f)) then
    Exit;

  filelist := TDirectory.GetFiles(f);

  for f in filelist do
  begin
    n := ChangeFileExt(ExtractFileName(f), EmptyStr);
    AStylesList.Add(n + '=' + f);
  end;
end;

..and this is where I'm trying to retrieve a string object:

procedure TfrmOptions.ComboBox1Change(Sender: TObject);
var
  si: TStyleInfo;
  i: Integer;
  s: String;
begin
  i := TComboBox(Sender).ItemIndex;
  s := PChar(TComboBox(Sender).Items.Objects[i]);

  Showmessage(s); // --> Mostly shows a corrupted string (gibberish chars)

  if (TStyleManager.IsValidStyle(s, si)) then
  begin
    if (not MatchStr(s, TStyleManager.StyleNames)) then
      TStyleManager.LoadFromFile(s);
    TStyleManager.TrySetStyle(si.Name);
  end;
end;

I suspect that its something with the way I'm adding them. Perhaps I need to allocate memory at:

s := PChar(sl.Values[sl.Names[c]]);

Not sure. Looking at the help on StrNew, NewStr and StrAlloc, it says that those functions are deprecated. Can you help point out whats wrong?

Why not just keep the name values pairs as you get them from the `LoadStyles` procedure and do a custom drawing in the combo box (where you would render only names) ? — TLama, Jun 07 '15 at 13:27
@TLama Custom drawing just ain't my thing. Too much extra code to write. ;) — Steve F, Jun 07 '15 at 13:43
Some observations: You should declare s1 not as TStringlist, but TStrings var s1 : TStrings; .... s1:=TStringlist.create(self); Then you don't need the cast when you call "Loadstyles" When you use Sender in ComboBoxchange you should check if it is valid and has the expected type. This could be done by if Sender is TComboBox then begin ... you can be sure it is not nil and it is from type TCombobox end; — Christine Ross, Jun 07 '15 at 16:54
@ChristineRoss That's the wrong solution. Making `LoadStyles` accept a value param rather than a var param is the correct solution. — David Heffernan, Jun 08 '15 at 12:31
@DavidHeffernan I refer to a discussion like that [link](http://stackoverflow.com/questions/9379607/why-variables-are-declared-as-tstrings-and-created-as-tstringlist) — Christine Ross, Jun 08 '15 at 16:02
@ChristineRoss Actually, if you read that carefully I am saying that the local variable may as well be the more specialized class. The only reason the user had to cast here was because of the `var` parameter. And the `var` parameter is categorically a mistake. — David Heffernan, Jun 08 '15 at 16:05
@DavidHeffernan, sorry the problem is not reading, but understanding. I think I understand the basic difference between var and value parameter, but I found no examples with lists, Please help me, and maybe also other — Christine Ross, Jun 08 '15 at 16:28

score 2 · Accepted Answer · answered Jun 07 '15 at 13:33

There's nothing to keep the string alive. When you write:

s := PChar(sl.Values[sl.Names[c]]);

an implicit local variable of type string is created to hold whatever sl.Values[sl.Names[c]] evaluates to. That local variable goes out of scope, as far as the compiler is aware, nothing references it, and the string object is destroyed.

In fact, it's even worse than that. Because the assignment above happens in a loop, there is only one implicit local variable. Each time round the loop, the string that you asked the combo box to remember is destroyed.

You need to find a way to extend the lifetime of the string. You could do it like this:

var
  StrPtr: ^string;
....
for c := 0 to sl.Count - 1 do
begin
  New(StrPtr);
  StrPtr^ := sl.Values[sl.Names[c]];
  ComboBox1.Items.AddObject(sl.Names[c], TObject(StrPtr));
end;

Then when you need to access the string you can do so like this:

var
  StrPtr: ^string;
....
TObject(StrPtr) := TComboBox(Sender).Items.Objects[i];
// do something with StrPtr^

When you clear the combo box you must also run through each item and call Dispose on the pointer.

Having said that, it's going to be much easier not to do it that way. Stop trying to force strings into the TObject data associated with each item. Instead keep a parallel string list containing these strings. When you need to look up a name look it up in that list rather than in the combo box.

there is another option, to make a string-holder objects and store them as .Objects - it might be easier than a separate TStrings object, if there would be another module getting those stings out of the standard non-extended TComboBox. For - how would you pass the link to the said stringslist to that another module ? via TCombobox.Tag ? via global var ? — Arioch 'The, Jun 08 '15 at 12:26

Mick · Answer 2 · 2020-11-16T17:21:05.333

I know this is an old question but I came across this problem again and rather than use the separate string list I used an object with a string value (I think someone suggested it in a comment) as follows:

Declare a type as TObject with a string value:

  TStringObject = class(TObject)
    StringValue : string;
  end;

Then when adding your items declare a local var of TStringObject and create a new instance for each item:

var
  strObj : TStringObject
begin

    ...

    for c := 0 to sl.Count - 1 do
    begin
      strObj := TStringObject.Create;
      strObj.StringValue := sl.Values[sl.Names[c]];
      ComboBox1.Items.AddObject(sl.Names[c], strObj);
    end;

And when retrieving the values:

s := TStringObject(TComboBox(Sender).Items.Objects[i]).StringValue;

As @Dejan Dozet mentions in the comments - you should always free the data objects before freeing the TStringList!

nice Mike, do we need to worry about destroying that object too? — Dejan Dozet, Nov 16 '20 at 13:33

Strings getting corrupted in ComboBox.AddObject. How to add them the proper way?

2 Answers2