-
Notifications
You must be signed in to change notification settings - Fork 40
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
Revert magicbytes (-M) check for print conversion #2184
Conversation
… filetypes (.pptx).
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.
In theory, wmf files should be supported, but the mimetype that's listed as supported in libreoffice is image/x-wmf
and the mimetype of the sample file is image/wmf
even though it was created in libreoffice. 🙃 Libreoffice can in fact convert the file, but we'd have to hardcode in the support, which I don't love.
My suggestion would be to file a followup issue for these cases and figure out what to do, rather than trying to work around the behaviour of the tools we're using. But it's annoying. The same is true for the image/emf
type.
8868dcf
to
c7e02a2
Compare
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, this looks good. I like that as we increase file type support we can just move files from the unsupported folder to supported :)
@@ -20,7 +20,8 @@ | |||
SAMPLE_OUTPUT_UNSUPPORTED_PRINTER = b"network beh\nnetwork https\nnetwork ipp\nnetwork ipps\nnetwork http\nnetwork\nnetwork ipp14\ndirect usb://Canon/QL-700%?serial=A00000A000000\nnetwork lpd" # noqa | |||
|
|||
SUPPORTED_MIMETYPE_COUNT = 107 # Mimetypes in the sample LibreOffice .desktop files | |||
SAMPLE_ODT_FILENAME = "Sample_Print.odt" # see export/tests/files | |||
SAMPLE_FILES_SUPPORTED = Path.cwd() / "tests" / "files" / "samples_supported" |
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.
I was going to flag this but I realized it was already used in the code and you just moved it.
For the future let's not depend on what the current directory is since people can run pytest from export
or export/tests
, instead something like Path(__file__).parent.parent / "files" / "samples_supported"
...
Status
Ready for review
Description
Fixes #2172 (comment) and adds additional sample files of varying formats to our libreoffice integration tests.
Test Plan
Checklist
If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:
If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:
If these changes modify the database schema, you should include a database migration. Please check as applicable:
main
and confirmed that the migration is self-contained and applies cleanlymain
and would like the reviewer to do so