-
-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add suport for non-PDF files served by ChartFox #209
Conversation
curl-8.7.1 is the most recent version that does not require an update to mbedtls. Added bcrypt to the list of libraries required by the MinGW build system. Other platforms appear to handle this automatically. Downgraded the required version of CMake to 3.16 since this is the version currently supported by Ubuntu/WSL and the Avitab CMake scripts do not require any features that are not available in 3.16.
This renaming reflects the more generic capabilities of the underlying MuPDF (fitz) rendering library, and is preparation for enhancements to the ChartFox provider module to support downloading of charts in formats other than PDF.
The PDFViewer class isn't actually used in Avitab. It could be renamed to DocumentViewer in keeping with the other refactoring in this set of updates, but it seems cleaner to simply remove it.
Various symbols including the word PDF are changed to document to better reflect the use/purpose of the named entity.
Preference groups named pdfreading are changed to docreading to reflect the more general capabilities of the related apps. The preference file version number is incremented, and code to upgrade the preference file from version 1 to version 2 is implemented.
Define LocalFileSource and DownloadedSource as specializations of DocumentSource, and move LocalFile-specific methods and data into LocalFileSource class. This is in preparation for downloaded content type, which will be specific to the DownloadedSource class.
Obtain the content type header from the curl get request and use this string as the parameter to the MuPDF fitz rasterizer.
@@ -52,6 +52,7 @@ target_link_libraries(avitab_common | |||
if(WIN32) | |||
target_link_libraries(avitab_common | |||
ws2_32 | |||
bcrypt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this library ship with Windows or is it a new runtime dependency? In the latter case, that won't work unfortunately: Every dependency must be linked statically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated curl library introduces the dependency on bcrypt. The inclusion of this library seems to be implicit on Linux and MacOS. On Windows bcrypt needs to be listed explicitly as a dependency.
I took a look at XP12 using ProcessExplorer, and it shows the bcrypt library being loaded from the Windows distribution (C:\Windows\System32\bcrypt.dll) The library is loaded even when Avitab is not installed as a plugin. When Avitab is added as a plugin then it still works as expected, even though bcrypt is a DLL and not a statically linked library.
I think the dependency is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the check!
This is a revival of #207, since I was unable to reopen that request after I closed it (temporarily I had hoped!)
While using the ChartFox interface for CYOW I found that the charts were not displayed (Cannot open document: ...) and traced this issue to the rasterizer assuming that charts are in PDF format.
This fix retrieves the content type header from the http transaction and passes that to the rasterizer (MuPDF/fitz) so that it is able to handle the data blob correctly (previously the blob type was hard-coded to application/pdf).
A newer version of the curl submodule was required in order to acess the header fields to get the content type. Since there is a dependency on the mbedtls module, and updating this turned out to be non-trivial, I have selected the latest version of the curl module that does not require any changes to the mbedtls module.
Since the purpose of this PR is to support downloaded charts that are not PDF formatted, I have renamed classes and files that had 'PDF' in their names to the more general 'Document', and similarly variables are renamed to reduce the possibility of confusion. I have also removed 2 files that are not linked into the build and appear to be redundant.
One of the settings in the json preferences file was called pdfreading. Since this applies to all documents, it has been renamed to docreading, and code is added to support upgrading of the preferences file on first run of Avitab after this PR has been included in the build.
The original PDFSource class, now renamed as DocumentSource overloads downloaded documents and local file documents, with some parts of the class only relevant to local files. The final refactoring update was to create subclasses of DocumentSource to represent LocalFileSource and DownloadedSource documents.
The actual functional change that obtains the content type of the downloaded document and passes this to the rasterizer is in the final commit and is relatively trivial!