PChar to String changes in Dec 3, 2023 commit 7023af0 broke AdFaxCvt.pas for me

[koenick]

The PChar to String changes in Dec 3, 2023's commit 7023af0 are giving me errors when trying to build "\packages\11AndAbove\Delphi\AsyncProDelphi.groupproj". I'm getting numerous "E2010 Incompatible types: 'PWideChar' and 'string'" throughout TApdCustomFaxConverter.ChangeDefPrinter().

The errors all involve calls to procedures in Vcl.Printers that expect PChar, such as:
procedure TPrinter.GetPrinter(ADevice, ADriver, APort: PChar; var ADeviceMode: THandle);
procedure TPrinter.SetPrinter(ADevice, ADriver, APort: PChar; ADeviceMode: THandle);

I've been away from Delphi and RAD Studio for several years now. Am I expected to have some sort of automatic string/pchar type casting enabled? It unclear to me how romankassebaum didn't get the same errors I am.

[romankassebaum]

I'm using Delphi 12 that has new functions for GetPrinter and SetPrinter, that use string instead of PChar. The old functions are deprecated.

If you like, you can add the Delphi 11 code and surround it with defines and I will merge it.

[koenick]

Great. Let's do that. I've made changes based on

{$IF CompilerVersion < 36.0}
      using pchar
{$ELSE}
      using strings
{$ENDIF}

I'm sorry, but I'm new to git. I don't have access to create a branch in this repo, so I think I'm supposed to create my own fork so I can make a pull request from my fork to your base. Correct?

[romankassebaum]

You can simply add the code here and I will merge it.

It would be nice if you could write a class helper like this

{$IF CompilerVersion < 36.0}  
  TPrinterHelper = class helper for TPrinter
  public
    procedure GetPrinter(var ADevice, ADriver, APort: string; var ADeviceMode: THandle); overload;
    procedure SetPrinter(ADevice, ADriver, APort: string; ADeviceMode: THandle); overload;
  end;
{$IFEND}

Then we can use the Athens code in Alexandria.

[koenick]

Thank you for your quick attention to this.

If I understand you correctly, you're asking me to implement a helper class so we won't have to modify TApdCustomFaxConverter.ChangeDefPrinter() at all from what you recently changed it to. The idea is to segregate anything compiler-specific into a helper class that would only exist for old compilers. That might looks something like this:

type
{$IF CompilerVersion < 36.0}  
  TPrinterHelper = class helper for TPrinter
  public
    procedure GetPrinter(var ADevice, ADriver, APort: string; var ADeviceMode: THandle); overload;
    procedure SetPrinter(ADevice, ADriver, APort: string; ADeviceMode: THandle); overload;
  end;
{$IFEND}


implementation

{$IF CompilerVersion < 36.0}
procedure TPrinterHelper.GetPrinter(var ADevice, ADriver, APort: string; var ADeviceMode: THandle);
begin
 { Simple type casting for older versions of TPrinter }
  GetPrinter(PChar(ADevice), PChar(ADriver), PChar(APort), ADeviceMode);
end;

procedure TPrinterHelper.SetPrinter(ADevice, ADriver, APort: string; ADeviceMode: THandle);
begin
 { Simple type casting for older versions of TPrinter }
  SetPrinter(PChar(ADevice), PChar(ADriver), PChar(APort), ADeviceMode);
end;
{$IFEND}

I'm concerned that simple type casting won't work because TPrinter.GetPrinter() in Alexandria's Vcl.Printers uses a series of StrCopy()'s.

procedure TPrinter.GetPrinter(ADevice, ADriver, APort: PChar; var ADeviceMode: THandle);
begin
  with TPrinterDevice(Printers.Objects[PrinterIndex]) do
  begin
    StrCopy(ADevice, PChar(Device)); 
    StrCopy(ADriver, PChar(Driver));
    StrCopy(APort, PChar(Port));
  end;
  ADeviceMode := FDeviceMode;
end;

"StrCpy does not perform any length check. The destination buffer must have room for at least StrLen(Source)+1 characters." That means we'll need to set each string length to 255 before calling TPrinter.GetPrinter(). To accomplish that with the helper class, I think I'd have to do SetLength()'s right in the overloaded methods.

implementation

{$IF CompilerVersion < 36.0}
procedure TPrinterHelper.GetPrinter(var ADevice, ADriver, APort: string; var ADeviceMode: THandle);
begin
  SetLength(ADevice, 255);
  SetLength(ADriver, 255);
  SetLength(APort, 255);
 { Simple type casting for older versions of TPrinter }
  GetPrinter(PChar(ADevice), PChar(ADriver), PChar(APort), ADeviceMode);
end;

procedure TPrinterHelper.SetPrinter(ADevice, ADriver, APort: string; ADeviceMode: THandle);
begin
  SetLength(ADevice, 255);
  SetLength(ADriver, 255);
  SetLength(APort, 255);
 { Simple type casting for older versions of TPrinter }
  SetPrinter(PChar(ADevice), PChar(ADriver), PChar(APort), ADeviceMode);
end;
{$IFEND}

GetPrinter and SetPrinter are called repeatedly, and it seems inefficient to repeatedly set the length of the strings to 255 when I really just need to do it once when the original array is declared. Also, I don't have a good way of fully testing this conversion helper class with it's parent. (Can I skip SetLength in TPrinterHelper.SetPrinter because the strings should already have values? What happens if the supplied string is empty?) I'm more confident in a solution that makes the exact same calls AsyncPro has always made. It's not as elegant as a helper class, but I think it may be most direct to just use #ifdefs in three places in the one function that needs them.

{ Change the default printer if printto don't work, but}
{      print does work to convert to APF }
procedure TApdCustomFaxConverter.ChangeDefPrinter(UseFax: Boolean);
const
  DefPrn : string = '';
var
{$IF CompilerVersion < 36.0}
  Device, Name, Port : array[0..255] of char;
{$ELSE}
  Device, Name, Port : string;
{$IFEND}
  DevMode : THandle;
  N, Last : integer;
begin
  { Check to make sure default printer is not already changed }
  with Printer do begin
    if UseFax then begin
    { find one of our printers }
     DefPrn := Printer.Printers[Printer.PrinterIndex];
     Last := Printer.Printers.Count - 1;
     for N := 0 to Last do begin
       Printer.PrinterIndex := N;
       Printer.GetPrinter(Device, Name, Port, Devmode);
       Printer.SetPrinter(Device, Name, Port, Devmode);
       if Device = 'APF Fax Printer' then begin
         { get the required info }
         Printer.GetPrinter(Device, Name, Port, DevMode);
{$IF CompilerVersion < 36.0}
         { concatenate the strings }
         StrCat(Device, ',');
         StrCat(Device, Name);
         StrCat(Device, ',');
         StrCat(Device, Port);
         { write the string to the ini/registry }
         WriteProfileString( 'Windows', 'Device', Device );
         StrCopy(Device, 'Windows' );
{$ELSE}
         { concatenate the strings }
         Device := Device + ';' + Name + ',' + Port;
         { write the string to the ini/registry }
         WriteProfileString('Windows', 'Device', PChar(Device));
         Device := 'Windows';
{$ENDIF}
         { tell everyone that we've changed the default }
         SendMessage(HWND_BROADCAST, WM_SETTINGCHANGE, 0, LPARAM(@Device));
         { make the TPrinter use the device capabilities of the new default}
         SetPrinter(Device, Name, Port, 0);
       end;
     end;
    end else begin
      { revert back to the original }
      N := Printer.Printers.IndexOf(DefPrn);
      Printer.PrinterIndex := N;
      Printer.GetPrinter(Device, Name, Port, DevMode);
{$IF CompilerVersion < 36.0}
      { concatenate the strings }
      StrCat(Device, ',');
      StrCat(Device, Name);
      StrCat(Device, ',');
      StrCat(Device, Port);
      { write the string to the ini/registry }
      WriteProfileString( 'Windows', 'Device', Device );
      StrCopy(Device, 'Windows' );
{$ELSE}
      { concatenate the strings }
      Device := Device + ','+ Name + ',' + Port;
      { write the string to the ini/registry }
      WriteProfileString('Windows', 'Device', PChar(Device));
      Device := 'Windows';
{$ENDIF}
      { tell everyone that we've changed the default }
      SendMessage(HWND_BROADCAST, WM_SETTINGCHANGE, 0, LPARAM(@Device));
    end;
  end;
end;

I'm happy to submit a pull request from a fork, if that makes this easier or cleaner.

[romankassebaum]

I checked in a first approach. SetPrinter does not seem to be critical. In GetPrinter it is difficult to get the DeviceMode, that's why I called the old method with a buffer.