After submitting the issue reported in How not to do exception classes in a Delphi library to Embarcadero (QC102796) they asked me for a test case.  Didn’t make much sense to me but I duly wrote one and am about to submit it.  However, in the process I collected a lot more information about work that should have been done on the Vcl.Imaging.pngimage unit when it was integrated into the Delphi source. Here’s the full list of the exception classes and what should be done with them.

• EPNGError should inherit from EInvalidGraphicOperation or be eliminated as it raises inappropriate exceptions
• EPngUnexpectedEnd should inherit from EInvalidGraphic
• EPngInvalidCRC should inherit from EInvalidGraphic
• EPngInvalidIHDR should inherit from EInvalidGraphic
• EPNGMissingMultipleIDAT should inherit from EInvalidGraphic
• EPNGZLIBError should inherit from EInvalidGraphic
• EPNGInvalidPalette should inherit from EInvalidGraphic
• EPNGInvalidFileHeader should inherit from EInvalidGraphic
• EPNGInvalidFileHeader should inherit from EInvalidGraphic
• EPNGSizeExceeds should inherit from EInvalidGraphic
• EPNGMissingPalette is unused and should be deleted
• EPNGUnknownCriticalChunk should inherit from EInvalidGraphic
• EPNGUnknownCompression should inherit from EInvalidGraphic
• EPNGUnknownInterlace should inherit from EInvalidGraphic
• EPNGNoImageData should inherit from EInvalidGraphic
• EPNGHeaderNotPresent should inherit from EInvalidGraphic

The following three exceptions are really reimplementations of existing exception classes and should be abolished entirely:

• EPNGOutMemory should just really be replaced with RaiseLastOSError.  Or, if backward compatibility for this exception is really needed then inherit from EOutOfMemory
• EPNGNotExists is not used unless USEDELPHI is not defined, which it is…
• EPNGCouldNotLoadResource masks a number of other exceptions and should if possible be removed entirely.  The code in question re-raises an exception and buries the original error with an entirely unhelpful secondary exception (and what is that exit; doing in there?).

  try 
    ResStream := TResourceStream.Create(Instance, Name, RT_RCDATA); 
  except
    RaiseError(EPNGCouldNotLoadResource, EPNGCouldNotLoadResourceText);   
    exit; 
  end;

Finally, these three need to inherit from EInvalidGraphicOperation:

• EPNGCannotChangeTransparent should inherit from EInvalidGraphicOperation
• EPNGInvalidNewSize should inherit from EInvalidGraphicOperation
• EPNGInvalidSpec should inherit from EInvalidGraphicOperation

In a number of locations EPNGError and Exception are also raised (albeit sometimes in code that is $ifdef‘d out of use), often with blank error messages!  For example:

    case BitDepth of 
      {Only supported by COLOR_PALETTE}
      1: DetectPixelFormat := pf1bit; 
      2, 4: DetectPixelFormat := pf4bit; 
      {8 may be palette or r, g, b values} 
      8, 16: 
        case ColorType of 
          COLOR_RGB, COLOR_GRAYSCALE: DetectPixelFormat := pf24bit; 
          COLOR_PALETTE: DetectPixelFormat := pf8bit; 
          else raise Exception.Create('');
        end {case ColorFormat of} 
        else raise Exception.Create('');
    end {case BitDepth of}

Finally, the unit reimplements a bunch of standard Delphi VCL functionality that is inappropriate to keep within the Delphi VCL codebase, including core classes such as TStream.  While these are not compiled in because they surrounded by $ifdef statements, all this code should really be stripped out for maintenance and code cleanliness/readability reasons.  Thus, all code surrounded by $ifndef UseDelphi should be deleted. This is not a systematic review of the issues in the Vcl.Imaging.pngimage unit; it’s more just a rant and giving a heads up about the quality of this unit.  Embarcadero, please clean this unit up!