ENH: Automatically preserve links in added pages

[larsga]

Here is a draft implementation of the first stage of the issue #3290 implementation. It handles links in pages added via add_page and insert_page, but it doesn't handle pages merged into those pages before adding.

Does this look OK?

I'm wondering if some users may already have written their own link patching code -- will this code break theirs? If so, should we make it possible to turn this behaviour off somehow?

At the moment I'm resolving everything by searching lists to find corresponding indirect references. It would be much faster with a hash, but I haven't been able to make that work. Thoughts?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.73%. Comparing base (e8e2ab6) to head (c4486dd).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3298      +/-   ##
==========================================
+ Coverage   96.71%   96.73%   +0.02%     
==========================================
  Files          53       54       +1     
  Lines        9034     9095      +61     
  Branches     1674     1685      +11     
==========================================
+ Hits         8737     8798      +61     
  Misses        177      177              
  Partials      120      120              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[stefan6419846]

Thanks for the PR.

Does this look OK?

At first sight it looks okay, but unless there are specific aspects to talk about, I tend to prefer a proper review once the automated checks were successful.

It seems like CI is still failing due to coverage, typing and code style issues - this is something to consider in a second step. Nevertheless, regarding the code style, I would prefer to move the new classes into a submodule of pypdf.generic to not bloat the pypdf._writer module where possible.

I'm wondering if some users may already have written their own link patching code -- will this code break theirs? If so, should we make it possible to turn this behaviour off somehow?

In theory, each release could break the code of some user when doing special stuff. As this fixes shortcomings of the current implementation without removing anything, I do not see the need to introduce a deprecation period for this at the moment.

At the moment I'm resolving everything by searching lists to find corresponding indirect references. It would be much faster with a hash, but I haven't been able to make that work. Thoughts?

What exactly have you tried and what has been the result?

[larsga]

I tend to prefer a proper review once the automated checks were successful.

Yeah, sorry. I had to cook dinner, and now I have a meeting. I didn't intend to leave the build broken like this. The trouble is I can't get the right version of ruff on my laptop right now, so all the checks end up being done in CI, which is slow. Anyway, I will sort all this out.

I would prefer to move the new classes into a submodule of pypdf.generic to not bloat the pypdf._writer module where possible.

Will do.

I do not see the need to introduce a deprecation period for this at the moment.

Ack! 👍

What exactly have you tried and what has been the result?

Mainly that I couldn't get the reference lookups to work, but I take this to mean that they should work. I'll work on this a bit more.

[larsga]

Now it should finally be ready for review.

Note that I changed the type of the Destination.page property. As far as I can tell it's been wrong all the time. I certainly don't get an int back when I reference it. I get an IndirectObject, and that's also what the docs say should be passed in as the value. I was forced to do this to get type checking to accept my code -- let me know if you want this change separated out.

Once this PR is merged I'll look at handling links in merged-in pages, but because of upcoming holiday that will take a while.

[stefan6419846]

Thanks. I will try to have a look at this as soon as possible - this might take some time as well.

Regarding the broken type hints, there is a corresponding issue as well: #3233.

[stefan6419846]

Thanks for the PR. I just had a first look at the changes and added some small comments. As general notes:

• Using abbreviations in the names and docstrings should be avoided. It is completely fine to use "reference" instead of "ref" for example to improve clarity and avoid having to deprecate stuff later on.
• In the type hints, please use type1, type2 instead of type1,type2. I have marked some cases, but not all.
• Instead of nesting functions and bloating the already large modules, consider moving the corresponding functionality to the new submodule.

[larsga]

I think I have addressed all review comments now. Please review again.