Probleme mit Objektkopie und eigenem MessageDialog

[haentschman]

Hallöle...

Ich stehe wieder mal vor einem Problem das ich nicht verstehe...

Gegeben:
* Klasse:

Delphi-Quellcode:

TDocumentProperties = class(TBaseObject)
  strict private
  {$REGION 'Fields'}
    FStoreLocation: string;
    FStoreStreet: string;
    FReceiptReciverName: string;
    FDocumentTypeString: string;
    FDocumentChoiceString: string;
    FDocumentLocation: TSEAMDocumentLocation;
    FStoreName: string;
    FCreateDate: TDateTime;
    FCreateName: string;
    FCreateYear: Integer;
    FModifiedName: string;
    FModifiedDate: TDateTime;
    FDocumentChoice: TSEAMDocumentChoice;
    FServiceDate: TDateTime;
    FNote: string;
    FStoreCountry: string;
    FReceiptDate: TDateTime;
    FStorePostCode: string;
    FReceiptReciverNumber: string;
    FServicePartnerNumber: string;
    FLocationNumber: string;
    FReceiptNumber: string;
    FStoreName_1: string;
    FDocumentCaption: string;
    FStoreNumber: string;
    FFileName: string;
    FFilePath: string;
    FExtension: string;
    FSendType: string;
    FFileNameContent: TStringList;
    procedure SetExtension(const Value: string);
    procedure SetFileName(const Value: string);
  {$ENDREGION}
  public
    constructor Create;
    destructor Destroy; override;
    property StoreNumber: string read FStoreNumber write FStoreNumber;
    property StoreName: string read FStoreName write FStoreName;
    property StoreName_1: string read FStoreName_1 write FStoreName_1;
    property StoreStreet: string read FStoreStreet write FStoreStreet;
    property StoreCountry: string read FStoreCountry write FStoreCountry;
    property StorePostCode: string read FStorePostCode write FStorePostCode;
    property StoreLocation: string read FStoreLocation write FStoreLocation;
    property Note: string read FNote write FNote;
    property CreateDate: TDateTime read FCreateDate write FCreateDate;
    property CreateName: string read FCreateName write FCreateName;
    property CreateYear: Integer read FCreateYear write FCreateYear;
    property ModifiedDate: TDateTime read FModifiedDate write FModifiedDate;
    property ModifiedName: string read FModifiedName write FModifiedName;
    property ReceiptDate: TDateTime read FReceiptDate write FReceiptDate;
    property ServiceDate: TDateTime read FServiceDate write FServiceDate;
    property DocumentChoice: TSEAMDocumentChoice read FDocumentChoice write FDocumentChoice;
    property DocumentLocation: TSEAMDocumentLocation read FDocumentLocation write FDocumentLocation;
    property DocumentChoiceString: string read FDocumentChoiceString write FDocumentChoiceString;
    property DocumentTypeString: string read FDocumentTypeString write FDocumentTypeString;
    property DocumentCaption: string read FDocumentCaption write FDocumentCaption;
    property SendType: string read FSendType write FSendType;
    property ReceiptNumber: string read FReceiptNumber write FReceiptNumber;
    property ReceiptReceiverNumber: string read FReceiptReciverNumber write FReceiptReciverNumber;
    property ReceiverReceiverName: string read FReceiptReciverName write FReceiptReciverName;
    property ServicePartnerNumber: string read FServicePartnerNumber write FServicePartnerNumber;
    property LocationNumber: string read FLocationNumber write FLocationNumber;
    property Extension: string read FExtension write SetExtension;
    property FileName: string read FFileName write SetFileName;
    property FilePath: string read FFilePath write FFilePath;
    procedure Assign(Source: TObject);
    function EncodeFileName: string;
    procedure DecodeFileName(FileName: string);
  end;

* GUI

Delphi-Quellcode:

procedure TfoDocuments.DoScrollDocuments(FocusedRecord: TcxCustomGridRecord);
var
  MessageList: TStringList;
  FileName: string;

  Test1, Test2: Pointer;
begin
  if FLogic.DocumentDatasource.Changed then
  begin
    MessageList := TStringList.Create;
    try
      FileName := FLogic.ModifiedDocument.FileName;
      MessageList.Add(Format(conTextConfirmationChangedDocument, [QuotedStr(FileName)]));
      MessageList.Add('');
      MessageList.Add(conTextConfirmationChangedDocument_1);
      if TMessageDialog.MessageDlg(MessageList, conTextSaveHeader, vmtQuestion, [buYes, buNo], clBlack, Self) = mrYes then
      begin
        FLogic.SaveDocument;
      end;
    finally
      MessageList.Free;
    end;
  end;

  FLogic.AfterScrollDocuments(FocusedRecord.RecordIndex); // Wechsel des Objekte in der Logic

  Test1 := Pointer(FLogic.OriginalDocument); // Kontrolle
  Test2 := Pointer(FLogic.ModifiedDocument); // Kontrolle

  if FLogic.ModifiedDocument.FileName.EndsWith('.pdf', True) then
  begin
    if FileExists(FLogic.ModifiedDocument.FileName) then
    begin
      WPViewPDF1.LoadFromFile(FLogic.ModifiedDocument.FileName);
    end
    else
    begin
      WPViewPDF1.Clear;
      TMessageDialog.MessageDlg(Format(conErrorFileNotPresent, [QuotedStr(FLogic.ModifiedDocument.FileName)]), conTextCheckHeader, vmtError, [buOk], clBlack, Self); // --> Zugriffsverletzung aus dem TReader im OnCreate der Dialog Form (siehe Unit)
    end;
  end;

  ShowDocument(FLogic.ModifiedDocument);
end;

* Logic

Delphi-Quellcode:

constructor TFormDocumentsLogic.Create(Preferences: TSEAMPreferences);
begin
  inherited Create(Preferences);
  FDocumentDatasource := TSEAMDocumentsDatasource.Create;
  FDocumentDatasource.OnChange := DoOnChange;
  FOriginalDocument := nil; // aus Liste
  FModifiedDocument := nil; // über Objektkopie
  FLastStoreName := '';
end;
...
procedure TFormDocumentsLogic.AfterScrollDocuments(FocusedRecordIndex: Integer);
begin
  FOriginalDocument := FDocumentDatasource.GetObject(FocusedRecordIndex); // Pointer aus Liste bleibt unangetastet
  if Assigned(FModifiedDocument) then // mit jedem Scroll die "Kopie" austauschen
  begin
    FModifiedDocument.Free;
  end;
  FModifiedDocument := TSEAMDocumentProperties(TSEAMToolsJson.ObjectCopy(FOriginalDocument)); // Kopie des Originals aus der Liste 
  //wenn ich diese Zeile auskommentiere wird der Dialog angezeigt. 
end;

* Objektkopie (funktioniert eigentlich immer)

Delphi-Quellcode:

class function TToolsJson.ObjectCopy(aValue: TObject): TObject;
var
  MarshalObj: TJSONMarshal;
  UnMarshalObj: TJSONUnMarshal;
  JSONValue: TJSONValue;
begin
  Result := nil;
  MarshalObj := TJSONMarshal.Create;
  try
    UnMarshalObj := TJSONUnMarshal.Create;
    try
      JSONValue := MarshalObj.Marshal(aValue);
      try
        if Assigned(JSONValue) then
          Result := UnMarshalObj.Unmarshal(JSONValue);
      finally
        JSONValue.Free;
      end;
    finally
      UnMarshalObj.Free;
    end;
  finally
    MarshalObj.Free;
  end;
end;

Problem:
1. Wenn ich ausschließlich das Originalobjekt oder ein leeres Objekt benutze (ohne FModifiedDocument Kopie), dann wird der Dialog angezeigt.

Delphi-Quellcode:

FModifiedDocument := TDocumentProperties.Create;  //(TToolsJson.ObjectCopy(FOriginalDocument));
.

2. Wenn ich das Originalobjekt kopiere, auch bei der Benutzung von FOriginalDocument, kommt eine Zugriffsverletzung im OnCreate der Dialog Form.


Frage:
Was hat der Reader der Form damit zu tun? Der Code des MessageDialogs ist in Ordnung...


Danke...

[CCRDude]

Wie schön dass Du beurteilst dass der Code des MessageDialogs in Ordnung ist, aber offensichtlich spielt er ja doch mit rein, insofern wäre der constructor dort vermutlich hilfreich bei der Problemsuche
(oder anders ausgedrückt: mit solchen Ausschlüssen steht man sich selber gerne im Weg, schließlich ist man als Autor des Codes ja noch am ehesten betriebsblind)

[freimatz]

Mein Vorschläge:
1. reduziere den Code so dass der Fehler gerade noch auftritt und dann weiter bis er nicht mehr auftritt. Dazwischen muss die Ursache liegen.
2. Baue den Code nach Clean Code Regeln um. Dann funktioniert er auch

[haentschman]

Guten Morgen...

Danke für eure Anteilnahme.

Zitat:

Wie schön dass Du beurteilst dass der Code des MessageDialogs in Ordnung ist, aber offensichtlich spielt er ja doch mit rein, insofern wäre der constructor dort vermutlich hilfreich bei der Problemsuche

...das dumme ist nur, daß der Fehler im TReader beim Lesen des Formulars auftritt...in den constructor kommt er nicht!

Zitat:

reduziere den Code so dass der Fehler gerade noch auftritt und dann weiter bis er nicht mehr auftritt. Dazwischen muss die Ursache liegen.

Ich konnte die Ursache herausfinden. Nur verstehe ich es nicht! Ich mache nur eine Objektkopie (privat) aus einem Objekt in einer Objektliste. Ohne Kopie wird der Dialog angezeigt, mit Kopie wird der Dialog nicht angezeigt!

Zitat:

Baue den Code nach Clean Code Regeln um. Dann funktioniert er auch

Verstehe ich nicht. Was hast daran auszusetzen?

[Uwe Raabe]

Der Fehler tauch beim Lesen der DFM-Resource der Dialog-Forms aus. Wenn das plötzlich schief geht, ist das ein Zeichen für einen korrupten Speicher (die Resource stimmt offenbar nicht mehr).

Das deutet auf einen unerlaubten Speicher-Zugriff hin und hat vermutlich gar nichts mit deinem Code selbst zu tun. Eventuell passiert in ObjectCopy irgendwas Unanständiges.

Lass das Programm doch mal mit Range-Checking laufen. Wenn das keine Erkenntnisse bringt, würde ich den Einsatz von FastMM im FullDebugMode empfehlen.

[haentschman]

Moin...

Zitat:

Das deutet auf einen unerlaubten Speicher-Zugriff hin und hat vermutlich gar nichts mit deinem Code selbst zu tun. Eventuell passiert in ObjectCopy irgendwas Unanständiges.

...das vermute ich auch. Die Objektkopiefunktion habe ich ständig in Gebrauch. Die macht nix anderes als TObject in JSON zu wandeln und das JSON wieder in ein TObject zu erzeugen.

Zitat:

Lass das Programm doch mal mit Range-Checking laufen. Wenn das keine Erkenntnisse bringt, würde ich den Einsatz von FastMM im FullDebugMode empfehlen

...mach ich.

Danke...

[freimatz]

Ich habe da gar nichts auszusetzen. Meine Erfahrung ist oft wenn ich in altem Code Fehler suche, dass es mir hilft ihn erst mal nach diesen Regelnum zubauen.

Was mir konkret auffällt:

1. Warum hat TDocumentProperties so viele properties die nichts machen? Warum nicht gleich ein record? (KISS)
2. Warum hat die Klasse auch Methoden? (Auch ein record kann Methoden haben.) Vielleicht wäre es besser die Datenhaltung und die Verarbeitung zu trennen und ein DTO daraus zu machen (SoC)
3. "TMessageDialog.MessageDlg(Format(conErrorFileNotP resent, ...);" aufteilen. Die Zeile ist sehr lang. Einfach wäre es erst mal den string in einer lokalen Variablen zwischenzuspeichern dessen Namen auch sagt um was es geht.
4. "FModifiedDocument := TSEAMDocumentProperties(TSEAMToolsJson.ObjectCopy( FOriginalDocument));" - harte Typecasts vermeiden. Wenn dann erst prüfen ob das auch der gewünschte Typ ist.

[haentschman]

Soooo...

Letztendlich war das Object Copy verantwortlich. Warum auch immer. Egal wie ich es umgestellt habe, die Zugriffsverletzung kam dann an anderer Stelle. Sollte man das Bug betrachten?

Ich habe eine ObjectCopy hier gefunden...https://delphihaven.wordpress.com/20...ng-using-rtti/ und für mich angepaßt. Damit wird das Object, ohne Speicherfehler, kopiert. Mein MessageDialog war nicht die Ursache.

Diese basiert auf der RTTI und nicht auf Marshall/Unmarshall. Wobei ich davon ausgehe, daß REST/DBX JSON auch die RTTI benutzt. Oder?

Delphi-Quellcode:

unit ObjectClone;

interface

type
  TObjectClone = record
    class function From<T: class>(Source: T): T; static;
  end;

implementation

uses
  SysUtils, Classes, TypInfo, RTTI, Controls;

class function TObjectClone.From<T>(Source: T): T;
var
  Context: TRttiContext;
  IsComponent, LookOutForNameProp: Boolean;
  RttiType: TRttiType;
  Method: TRttiMethod;
  MinVisibility: TMemberVisibility;
  Params: TArray<TRttiParameter>;
  Prop: TRttiProperty;
  SourceAsPointer, ResultAsPointer: Pointer;
begin
  RttiType := Context.GetType(Source.ClassType);
  //find a suitable constructor, though treat components specially
  IsComponent := (Source is TComponent);
  for Method in RttiType.GetMethods do
    if Method.IsConstructor then
    begin
      Params := Method.GetParameters;
      if Params = nil then Break;
      if (Length(Params) = 1) and IsComponent and
         (Params[0].ParamType is TRttiInstanceType) and
         SameText(Method.Name, 'Create') then Break;
    end;
  if Params = nil then
    Result := Method.Invoke(Source.ClassType, []).AsType<T>
  else
    Result := Method.Invoke(Source.ClassType, [TComponent(Source).Owner]).AsType<T>;
  try
    //many VCL control properties require the Parent property to be set first
    if Source is TControl then TControl(Result).Parent := TControl(Source).Parent;
    //loop through the props, copying values across for ones that are read/write
    Move(Source, SourceAsPointer, SizeOf(Pointer));
    Move(Result, ResultAsPointer, SizeOf(Pointer));
    LookOutForNameProp := IsComponent and (TComponent(Source).Owner <> nil);
    if IsComponent then
      MinVisibility := mvPublished //an alternative is to build an exception list
    else
      MinVisibility := mvPublic;
    for Prop in RttiType.GetProperties do
      if (Prop.Visibility >= MinVisibility) and Prop.IsReadable and Prop.IsWritable then
        if LookOutForNameProp and (Prop.Name = 'Name') and
          (Prop.PropertyType is TRttiStringType) then
          LookOutForNameProp := False
        else
          Prop.SetValue(ResultAsPointer, Prop.GetValue(SourceAsPointer));
  except
    Result.Free;
    raise;
  end;
end;

end.

@freimatz:

Zitat:

Warum hat TDocumentProperties so viele properties die nichts machen? Warum nicht gleich ein record? (KISS)

...weil Records nicht vererbbar sind und nicht in TObjectList<> liegen können.

Zitat:

Warum hat die Klasse auch Methoden?

...wie so nicht?

Zitat:

TMessageDialog.MessageDlg(Format(conErrorFileNotP resent, ...);" aufteilen. Die Zeile ist sehr lang.

...das ist kein langer String sondern mehrere Parameter.

Zitat:

harte Typecasts vermeiden

...in diesem Falle unnötig. Das was reingeht, kommt wieder raus. Mit der o.g. Version ist auch der Cast Geschichte.

[CCRDude]

Entschuldigung dass ich den Pfeil auf den Dialog-Konstruktor im Screenshot dann falsch verstanden habe

Re: Objekt-Kopie: dafür gibt es doch TPersistent. Ich kann mir gerade schwer vorstellen, dass ein generischer Kopiermechanismus immer alles richtig macht. Beispiel Feld vom Typ TStringList. Kopiert das generische ObjectCopy dabei dann den Inhalt, oder nur den Zeiger?
1. Wenn das kopierte Objekt sein Objekt vom Typ TStringList selber initialisiert und finalisiert, würde ein Kopieren des Zeigers zu Problemen führen, weil dann zwei Objekte im destructor Free auf das gleiche TStringList-Feld ausführen, das zweite führt dann zwangsweise zu einem Problem.
2. Alternativ kann das ObjectCopy Inhalte kopieren, aber was ist mit einem property vom Typ TStringList (oder komplexerer Klasse), das von außen dem Objekt zugewiesen wird? Dort würde das kopierte Objekt dann nur mit einer Kopie arbeiten, wo evtl. die Arbeit am Original gewünscht ist.

In beiden Fällen kann ich mir Szenarios mit Zugriffsverletzungen auf Adresse 00000000 vorstellen. Daher halte ich ein Erben von TPersistent mit selbstdefiniertem Assign für deutlich zuverlässigere Lösung. Ist mehr Aufwand, dafür hat man wirklich in der Hand, dass die Kopie so wird, wie man will.

(evlt. noch übertriebener ausgedrückt, weil ich nicht weiß ob ich mich anschaulich ausgedrückt habe: wenn das Objekt ein Property vom Typ TCustomForm hat, das auf die MainForm verweist - soll dann beim Klonen eine zweite Kopie vom Hauptformular angelegt werden? Aber wenn nur der Zeiger kopiert wird, woher weiß das ObjectClone, dass es sich nicht um eine Form handelt, die im destructor aufgelöst wird, weil sie auch im constructor erstellt wurde?)
)

[haentschman]

Moin...

Zitat:

Entschuldigung dass ich den Pfeil auf den Dialog-Konstruktor im Screenshot dann falsch verstanden habe

...wir haben alle unsere Leiden.

Zu deinen Fragen:
Die Objekte werden neu erzeugt. Auch die enthaltenen TStringsList werden mit Inhalt neu erzeugt. Bei der alten Funktion wurden auch komplexe, mit enthaltenen generischen Objekt Listen, Objekte kopiert. Mit der neuen Funktion habe das noch nicht ausprobiert. Vieleicht kann das mir jemand abnehmen...

Es wird immer Szenarien geben die nicht in das "Muster" passen. Für meine "Datenobjekte" ist das ausreichend.