CODEWITHSUNDEEP
delphidelphi-xe7
Steve F
Steve F

Reputation: 1567

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

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?

Upvotes: 0

Views: 3497

Answers (2)

Mick
Mick

Reputation: 161

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!

Upvotes: 2

David Heffernan
David Heffernan

Reputation: 612794

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.

Upvotes: 2

Related Questions