Skip to content
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

Merged
merged 7 commits into from
Jul 21, 2024

Conversation

mjh65
Copy link
Collaborator

@mjh65 mjh65 commented Jun 25, 2024

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!

mjh65 added 7 commits June 25, 2024 16:01
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
Copy link
Owner

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.

Copy link
Collaborator Author

@mjh65 mjh65 Jul 6, 2024

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the check!

@dave6502 dave6502 mentioned this pull request Jul 2, 2024
@fpw fpw merged commit 91901e2 into fpw:master Jul 21, 2024
@mjh65 mjh65 deleted the pr_support_png_charts branch July 23, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants