convert(): add option to get engine document (e.g. pikepdf.Pdf) #203

mara0004 · 2024-08-26T12:54:36Z

mara0004 commented

2024-08-26 12:54:36 +00:00

This allows the caller to get the engine document handle (e.g. pikepdf.Pdf) instead of saving to a file or memory buffer, which avoids overhead if you intend to continue processing the document with that engine anyway (e.g. add pages from other PDFs, or do N-up composition with the image pages).

This is achieved by adding a new option return_engine_doc to the convert() API.
Beside the document handle, this also returns the minimum output version, which may be needed for saving once post-processing has been done.

Related to #97

This allows the caller to get the engine document handle (e.g. `pikepdf.Pdf`) instead of saving to a file or memory buffer, which avoids overhead if you intend to continue processing the document with that engine anyway (e.g. add pages from other PDFs, or do N-up composition with the image pages). ~~This is achieved by adding a new option `return_engine_doc` to the `convert()` API. Beside the document handle, this also returns the minimum output version, which may be needed for saving once post-processing has been done.~~ Related to #97

mara0004 added 3 commits 2024-08-26 12:54:38 +00:00

convert(): add option to return the engine document handle be313aa793

Add simple test 8d931f9570

Minor cleanup d4b49c510a

mara0004 added 1 commit 2024-08-26 13:05:33 +00:00

Write to tmp path rather than output/ 316629fa40

output seems to be part of the git repository and used for comparisons

josch commented

2024-08-27 08:10:49 +00:00

I'm not happy with the design of adding another function argument which then, if set, changes the output type of the function. I think a better pattern would be to factor out the relevant code into a new function which can then be called by convert or by the 3rd party utility which needs the underlying engine object.

I'm not happy with the design of adding another function argument which then, if set, changes the output type of the function. I think a better pattern would be to factor out the relevant code into a new function which can then be called by `convert` or by the 3rd party utility which needs the underlying engine object.

👍 1

mara0004 commented

2024-08-27 12:07:31 +00:00

Fair enough, that might be cleaner.

To confirm, you only want me to factor out a new function from pdfdoc.tostream() but keep the option + if-check in convert(), right?
Or do you also want me to factor out a convert_to_enginedoc() from convert()?

Fair enough, that might be cleaner. To confirm, you only want me to factor out a new function from `pdfdoc.tostream()` but keep the option + if-check in `convert()`, right? Or do you also want me to factor out a `convert_to_enginedoc()` from `convert()`?

mara0004 added 1 commit 2024-08-27 20:43:39 +00:00

Extract a separate function from tostream() 8aca925ee3

mara0004 force-pushed return_engine_doc from 8aca925ee3 to c1c77a7453

2024-08-27 20:44:47 +00:00

Compare

mara0004 commented

2024-08-27 20:49:57 +00:00

I think a better pattern would be to factor out the relevant code into a new function which can then be called by convert or by the 3rd party utility which needs the underlying engine object.

I think I updated the code accordingly. Kindly let me know if you are OK with it now, or if there's anything else to change.

> I think a better pattern would be to factor out the relevant code into a new function which can then be called by convert or by the 3rd party utility which needs the underlying engine object. I think I updated the code accordingly. Kindly let me know if you are OK with it now, or if there's anything else to change.

mara0004 commented

2024-08-27 20:56:21 +00:00

(In case you decide to merge any of my 4 PRs, please --squash the commits into one. Thanks!)

josch commented

2024-08-27 21:57:23 +00:00

Kindly let me know if you are OK with it now, or if there's anything else to change.

What i had in mind was: take the convert function and put all the code that generates the pdf variable and put it in its own function. Let the convert function call that function. Now your code can call this new function and can do with the pdf variable whatever it wants.

> Kindly let me know if you are OK with it now, or if there's anything else to change. What i had in mind was: take the convert function and put all the code that generates the `pdf` variable and put it in its own function. Let the convert function call that function. Now your code can call this new function and can do with the `pdf` variable whatever it wants.

mara0004 commented

2024-08-27 22:06:34 +00:00

Aah, that's what I was wondering in Comment 2, whether you want the factoring out only for pdfdoc.tostream() or also for convert() itself.
I had presumed you meant only pdfdoc.tostream(), but this clarifies you actually want it for convert(). So I'll change that.

Aah, that's what I was wondering in [Comment 2](https://gitlab.mister-muffin.de/josch/img2pdf/pulls/203#issuecomment-2661), whether you want the factoring out only for `pdfdoc.tostream()` or also for `convert()` itself. I had presumed you meant only `pdfdoc.tostream()`, but this clarifies you actually want it for `convert()`. So I'll change that.

mara0004 added 1 commit 2024-08-27 22:32:41 +00:00

Factor out from convert() too, adapt pdfdoc 7afbdd8925

mara0004 commented

2024-08-27 22:34:45 +00:00

OK, updated. Please take a look at the new patch set. Thanks :)

mara0004 added 1 commit 2024-08-27 22:39:38 +00:00

Fix typo in test fd27129b10

mara0004 referenced this pull request

2024-09-13 21:42:16 +00:00

Add support for JBIG2 (generic coding) #184

josch requested changes 2024-09-15 06:03:03 +00:00

josch left a comment

Thank you for your work and for your patience with me. I'm going to be on a business trip next week (I'm switching jobs end of september/beginning of october) so my next feedback might take a while but please feel free to ping me if you like.

👍 1

src/img2pdf.py

					
				@ -1076,3 +1076,3 @@

				        return stream.getvalue()

				    def tostream(self, outputstream):

				    def finalize(self):

josch commented

2024-09-15 06:01:12 +00:00

You split tostream() into finalize() and tostream() but then why does the new tostream() not call finalize()?

You split `tostream()` into `finalize()` and `tostream()` but then why does the new `tostream()` not call `finalize()`?

mara0004 commented

2024-09-15 14:14:13 +00:00

Because I though the embedder of convert_to_docobject() should not have to invoke finalize().
Instead, finalize() technically belongs into convert_to_docobject() itself, after all image pages have been added. So tostream() cannot also finalize() as that would result in a double call.

Because I though the embedder of `convert_to_docobject()` should not have to invoke `finalize()`. Instead, `finalize()` technically belongs into `convert_to_docobject()` itself, after all image pages have been added. So `tostream()` cannot also `finalize()` as that would result in a double call.

src/img2pdf.py

					
				@ -2803,0 +2806,4 @@

				# Input images can be given as file like objects (they must implement read()),

				# as a binary string representing the image content or as filenames to the

				# images.

				def convert(*images, outputstream=None, **kwargs):

josch commented

2024-09-15 05:59:41 +00:00

Please do not change the signature of the convert() function. This is necessary to preserve API stability. You have to extract "outputstream" from kwargs.

mara0004 commented

2024-09-15 15:29:38 +00:00

It's not clear to me why this is supposed to break the API -- can you explain?
AFAICS, kwargs is internal, and no callee expects kwargs["outputstream"], right?

It's not clear to me why this is supposed to break the API -- can you explain? AFAICS, `kwargs` is internal, and no callee expects `kwargs["outputstream"]`, right?

mara0004 commented

2024-09-15 15:40:35 +00:00

Relatedly, it looks like the _default_kwargs strategy silently ignores nonexistent parameters, which is problematic. (And of course it breaks IDE completion.)
~~I see you're using kwargs to unify access of {crop,bleed,trim,art}border, which is fine, but why can't any others params be in the signature directly?~~

Relatedly, it looks like the `_default_kwargs` strategy silently ignores nonexistent parameters, which is problematic. (And of course it breaks IDE completion.) ~~I see you're using kwargs to unify access of `{crop,bleed,trim,art}border`, which is fine, but why can't any others params be in the signature directly?~~

mara0004 commented

2024-09-16 21:21:24 +00:00

It's not clear to me why this is supposed to break the API -- can you explain?

Okay, self-answering the question: a SO search yielded that specifying optional params after a *capture raises a SyntaxError on Python 2. Never having written code for Python 2, I did not know that. I'll change to extract from kwargs as you said, then.

The issue with invalid kwargs assumably being ignored without error still stands, though.

> It's not clear to me why this is supposed to break the API -- can you explain? Okay, self-answering the question: a SO search yielded that specifying optional params after a `*capture` raises a `SyntaxError` on Python 2. Never having written code for Python 2, I did not know that. I'll change to extract from kwargs as you said, then. The issue with invalid kwargs assumably being ignored without error still stands, though.

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions