Didstopia / PDFSharp

A .NET Standard 2.0 library for reading, writing and editing PDF files.
MIT License
52 stars 18 forks source link

Ximage.ExistFile Erroneously returning false #19

Closed ronnyek closed 6 years ago

ronnyek commented 6 years ago

I'm not sure I understand what PDFRreader.TestPdfFile is doing... but its whats being run inside XImage.ExistsFile. This is somewhat problematic because file does indeed exist (even though its not proper pdf file).

I'd just go ahead and fix this one, but I figure as I don't understand what TestPdfFile check is supposed to be doing... I'm gonna leave this one here. In XImage.FileExists, there is old platform specific code that just returns File.Exists(path) for netfx, and with that there, the code operates as expected.

public static bool ExistsFile(string path)
        {
            // Support for "base64:" pseudo protocol is a MigraDoc feature, currently completely implemented in MigraDoc files. TODO: Does support for "base64:" make sense for PDFsharp? Probably not as PDFsharp can handle images from streams.
            //if (path.StartsWith("base64:")) // The Image is stored in the string here, so the file exists.
            //    return true;

            //if (PdfReader.TestPdfFile(path) > 0)
            //    return true;
            return File.Exists(path);
#if !NETFX_CORE && !UWP && !PORTABLE
            return File.Exists(path);
#else
            return false;
#endif
        }

Actually with this little patch, I'm rendering a massive 100page report, and the only artifacts I can see now, are oddities with the color pallete stuff (honestly probably a bug in System.Drawing compat libs)

Dids commented 6 years ago

So I glanced over the code, and it's essentially just checking if the file is a valid PDF. It checks the file header (initial X bytes) for a specific string, and if not found, it return false.

Naturally that's fine for validating PDFs before trying to open them, but I don't think ExistsFile should have anything to do with validating a file, as it's just supposed to determine if the file exists or not, right?

Dids commented 6 years ago

We might need more tests for writing/generating PDF files and validating them, but I'll go ahead and make this change, as well as comment the code so it's more apparent what's changed.