Funktion die TStrings zurückgibt

[RWarnecke]

Delphi-Quellcode:

{...}
  FPaperTrays : TStrings;
{...}

function TPrinterCustom.GetPaperTrays: TStrings;
var
  pDevMode : PDeviceMode;
  bin      : array[0..255,0..23] of Char;
  i        : DWORD;
  Res      : DWORD;
  PPrinter : PPrinterInfo2;
begin
  if (FPaperTrays = nil) then
  begin
    try
      FPaperTrays := TStringList.Create;
    except
      FPaperTrays.Free;
      FPaperTrays := nil;
    raise;
    end;
  end;
  FPaperTrays.Clear;
  if FAllInfos.Count <> 0 then
  begin
    PPrinter := FAllInfos.Items[FPrinterIndex];
    pDevMode := nil;
    Res := DeviceCapabilities(PPrinter^.pPrinterName,
                              PPrinter^.pPortName,
                              DC_BINNAMES,
                              PChar(@(bin[0][0])),
                              pDevMode);
    for i := 0 to Res-1 do
      FPaperTrays.Append(bin[i]);
  end;
  Result := FPaperTrays;
end;

Ich habe beim zweiten oder dritten Mal aufrufen das Problem, das ich einen AccessViolation-Fehlermeldung bekomme in der FOR-Schleife. Desweiteren wird das Ergebnis auch nicht wie gewollt mit den neu ermittelten Werten überschrieben.

Was mache ich falsch und was könnte man besser machen ? Das Ergebnis soll eine Liste mit allen Papierschächten des Druckers sein.

[jfheins]

Eine Sache machst du schon mal falsch (weiß aber nicht ob's daran liegt)

Und zwar: wenn im Konstruktor ne Exception geworfen wird, dann darf der Destruktor nicht aufgerufen werden. In diesem Fall sorgt Delphi schon dafür dass das nichts überigbleibt, ein .Free könnte also in einer AV münden.

Wird eventuell irgendwo die Liste, die du zurückgibts mit .Free zerstört? Das könnte sehr gut die Fehlerursache sein. Ich glaube nämlich der Aufruf von TStringlist.Create dürfte fast nie fehlschlagen.
Falls FPaperTrays ein privates feld deiner Klasse ist, würde ich das auch nicht "herausgeben" - besser wäre hier ein Parameter

Delphi-Quellcode:

const x:TStrings

in den das hineingeschrieben wird. Dann brauchst du dich auch nicht mehr um den Konstruktor kümmern.

[Luckie]

Der Code:

Delphi-Quellcode:

if (FPaperTrays = nil) then
  begin
    try
      FPaperTrays := TStringList.Create;
    except
      FPaperTrays.Free;
      FPaperTrays := nil;
    raise;
    end;
  end;

ist Blödsinn. Wenn nichts erzeugt werden kann, kann auch nichts freigegeben werden.

Das Reicht:

Delphi-Quellcode:

if (FPaperTrays = nil) then
  FPaperTrays := TStringList.Create;
else
  ...;
  ...;

Wie erzeugst du die StringListe und wie gibst du sie wieder frei? das sind nicht ganz irrelevante Codeteile.

[DeddyH]

Ich würde aus der Funktion eine Prozedur machen, die ein TStrings-Objekt als Parameter annimmt. Somit ist die aufrufende Routine für die Instanzierung verantwortlich.

[RWarnecke]

Zitat:

Zitat von Luckie (Beitrag 1026052)

Wie erzeugst du die StringListe und wie gibst du sie wieder frei? das sind nicht ganz irrelevante Codeteile.

Diese Funktion ist eine protected-Funktion in einer Klasse. Ich benutze die TStringListe nur zur Übergabe an das Result. Für mehr nicht.

Zitat:

Zitat von DeddyH (Beitrag 1026053)

Ich würde aus der Funktion eine Prozedur machen, die ein TStrings-Objekt als Parameter annimmt. Somit ist die aufrufende Routine für die Instanzierung verantwortlich.

Das heißt, Du würdest bei Listen in einer Klasse, das ganze besser in eine Procedure in den Public-Teil packen ?

[Luckie]

Wenn das Methoden einer Klasse sind, warum kennen sie dann nicht die StringListe? Da braucht man doch gar nichts zu übergeben und andauert wieder neu zu erstellen, nur bei Bedarf leeren.

[jfheins]

Achso, die ist im protected teil, ja dann aist das was anderes :mrgreen:

Ich tippe mal, du hast noch irgendwo ein

Delphi-Quellcode:

FPaperTrays.Free;

stehen - das wird aufgerufen, und das Objekt ferigegeben. Sobald dann die Methode nochmal aufgerufen wird, knallts.
Denk' dran: ein ".Free" setzt den Zeiger nicht zurück auf nil, d.h. deine Abfrage ist nur ganz am Anfang beim ersten Mal von Bedeutung. Und dann könnte man das erstellen der Stringliste eigentlich auch in den Konstruktor packen und nur noch leeren und wiederbefüllen.

[Christian Seehase]

Moin Rolf,

die Umsetzung von Detlefs Idee würde, z.B., so aussehen:

Delphi-Quellcode:

procedure TPrinterCustom.GetPaperTrays(const APaperTrays : TStrings);
var
  pDevMode : PDeviceMode;
  bin : array[0..255,0..23] of Char;
  i : DWORD;
  Res : DWORD;
  PPrinter : PPrinterInfo2;
begin
  APaperTrays.Clear;
  if FAllInfos.Count <> 0 then
  begin
    PPrinter := FAllInfos.Items[FPrinterIndex];
    pDevMode := nil;
    Res := DeviceCapabilities(PPrinter^.pPrinterName,
                              PPrinter^.pPortName,
                              DC_BINNAMES,
                              PChar(@(bin[0][0])),
                              pDevMode);
    for i := 0 to Res-1 do
      APaperTrays.Append(bin[i]);
  end;
end;

Vorteil:
Man erhält eine klarere Struktur für das Erzeugen und Freigeben der Stringliste.

[DeddyH]

Ganz genau. Ich würde nur noch in der ersten Zeile auf Assigned prüfen.

Delphi-Quellcode:

if not Assigned(APaperTrays) then
  raise Exception.Create('Was soll ich denn hier füllen?');

[RWarnecke]

Danke Christian, Danke Detlef. Ich glaube ich werde in der Klasse alle TStrings-Rückgaben so umschreiben wie Christian es gemacht hat. Dann habe ich doch eine saubere Struktur drin. Oder gibt es dagegen etwas einzuwenden ?

[DeddyH]

Ich wüsste jetzt spontan nichts, was dagegen spricht.

[RWarnecke]

Ich habe das ganze jetzt in der Zwischenzeit mal ausprobiert mit der Procedure vom Christian, nur leider bekomme ich immer noch eine AccessViolation in der Zeile APapeTrays.Append(bin[i]);

Da die Unit eh OpenSource ist, hänge ich es einfach an. Das ist die Unit aus meiner Komponente TPrinterInfo. Die angehängte Unit beinhaltet schon die Procedure von Christian.

Aufrufen tue ich das ganze so :

Delphi-Quellcode:

procedure TForm23.LB_InstPrnClick(Sender: TObject);
begin
  PrnInfo_1.PrinterIndex := LB_InstPrn.ItemIndex;
  LB_Papertray.Items.Clear;
  PrnInfo_1.GetPaperTrays(LB_Papertray.Items);
end;

[jbg]

Zitat:

Delphi-Quellcode:

var Res: DWORD;
for i := 0 to Res-1 do

Was ist wohl "DWORD(0)-1" wenn es implizit (und ohne Compiler-Warnung) in ein DWORD umgewandelt wird? Das $FFFFFFFF den 0..255 Bereich bei weitem sprengt sollte klar sein.

[RWarnecke]

Hallo Andreas,

danke erstmal für Deine Antwort. Nur leider bekomme ich auch dann den Fehler nicht weg, wenn ich Res auf Integer setze und vorher noch eine IF-Abfrage einbaue ob Res > 0 ist. Irgednwie sehe ich den Wald vor lauter Bäumen nicht.

Dieser Beitrag war ja die Grundlage für die Procedure. Nur in dem Demo Programm funktioniert das Ganze wunderbar. Habe ich da vielleicht etwas übersehen ?

[jbg]

Zitat:

Zitat von RWarnecke (Beitrag 1026118)

Irgednwie sehe ich den Wald vor lauter Bäumen nicht.

Da sind noch mehr stellen die ein "if Res > 0 then" vertragen könnten. Bei mir funktionierte es so lange, bis ich als Drucker den FreePDF eingestellt hatte. Dann kam nämlich Res = 0 raus, was zur Zugriffsverletzung führte.
Vielleicht solltest du auch noch ein "Res := Max(Res, 255)" einbauen, damit du nicht über die Array-Grenze drüberläufst. Besser ist es, wenn du DeviceCapabilities() zuvor mit NIL als lpOutput Parameter aufrufst, damit du die genaue Anzahl an "BinNames" Einträgen bekommst und es keine Speicherüberschreibung gibt.