Create a booklet from multiple pages

[nicos68]

Merge and rearrange pages for booklet printing.

Issue: #438

The action is available through a new entry in the edit dialog. The generated pages are sized using the source pages (adding widths and using max of height of the two pages) and cannot be sized manually (as this would involve rescaling the source pages).

Note that

• I didn't implement tests. If you want I can have a look to it.
• The behavior of the "cancel" action is not atomic, as I use some other actions (add, delete, insert)
• Rotated pages are not properly handled, I can also have a look to it if you wan.

This PR might be a bit premature, however there already is some working code and it doesn't break anything else, so I created it anyway, also because I would be happy to have feedback on my code. It's my very first time sending some (more or less) serious amount of code to an open source project, so sorry if things are a bit clumsy.

[codecov-commenter]

Codecov Report

Merging #500 (7a206e5) into main (65b651e) will decrease coverage by 1.52%.
The diff coverage is 19.67%.

@@            Coverage Diff             @@
##             main     #500      +/-   ##
==========================================
- Coverage   73.58%   72.06%   -1.53%     
==========================================
  Files          10       10              
  Lines        2828     2935     +107     
==========================================
+ Hits         2081     2115      +34     
- Misses        747      820      +73     

Impacted Files	Coverage Δ
pdfarranger/exporter.py	60.92% <6.89%> (-11.08%)	⬇️
pdfarranger/pdfarranger.py	66.37% <31.25%> (-0.98%)	⬇️
pdfarranger/splitter.py	63.44% <0.00%> (-7.29%)	⬇️
pdfarranger/core.py	87.74% <0.00%> (-0.19%)	⬇️
pdfarranger/metadata.py	83.60% <0.00%> (+0.13%)	⬆️
pdfarranger/croputils.py	88.09% <0.00%> (+0.24%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65b651e...7a206e5. Read the comment docs.

[jeromerobert]

Thank you !

I didn't implement tests. If you want I can have a look to it.

Not top priority IMHO, this can be done in an other PR.

The behavior of the "cancel" action is not atomic, as I use some other actions (add, delete, insert)

"cancel" ? Do you mean "undo" ? If yes this deserve to be fixed in this PR. Your action should start with undomanager.commit("Create booklet") and do not modify the undomanager later on. For the PageAdder you can use the add_to_undomanager argument.

As clear_all is not a end user action it should not update the undomanager. I'll need to look at it closer but I'm not even sure you need a clear_all function. model.clear() should be enough.

Rotated pages are not properly handled, I can also have a look to it if you wan.

Not top priority IMHO. This could be fixed in an other PR.

This PR might be a bit premature, however there already is some working code

Yes and yes. Could you rebase your branch on origin/main and squash/reorganize your commits so your PR show what you modify in the end and not your whole development history (It's a bit difficult to read as is). May be you could just have:

• commits which add utilities functions like make_tmp_file or _insert_page and modify existing code
• one big commit for the feature you add

sorry if things are a bit clumsy

They are not. You are almost there.

[jeromerobert]

Some qpdf/pikepdf version do not accept pikepdf.Dictionary as pages. See #427 and #435 for workaround.

[jeromerobert]

Could you rebase your branch on origin/main and squash/reorganize your commits so your PR show what you modify in the end and not your whole development history (It's a bit difficult to read as is).

[nicos68]

Could you rebase your branch on origin/main and squash/reorganize your commits so your PR show what you modify in the end and not your whole development history (It's a bit difficult to read as is).

Sorry I wasn't finished, I didn't think you'd get notified by my push. I still had to handle the pikepdf version issue.

About my commits, I tend to commit very often and I'm used to Gitlab where we usually squash all the commits into one when the code is merged. Can you do that ? About the readability, the "files changed" tab looks like it should allow you to review my code easily. If it really doesn't work for you then that's ok, I'll squash all my commits.

[jeromerobert]

Ok that sounds good.

[nicos68]

Any idea when you'll merge this ?

[angsch]

@nicos68 Sorry that nobody has responded to your PR for so long. I think I also speak on behalf of the others when I say that it was not clear that you consider the feature complete. Unless someone else is faster, I will have a look at the code tomorrow.

[nicos68]

I do consider it complete.

[angsch]

Hello nicos68,

Thank you for your PR. I feel really sorry that you had to wait for so long.

Could you please check one more time that the result is correct when you have 6 pages? I have never used booklets and don't know what is correct. However, I expected that the content is consecutive when folded. In that case, when you fold the 6 pages, don't you need two blank pages in the booklet? [b | 1] [2 | b] [6 | 3] [4 | 5]

I ask you to change two things:

• Please add the functionality to the right-click menu. It is really hidden in the hamburger menu and unless you search for it, you will likely not find it. Most actions are duplicated and this one should be too.
• Please ensure that at least one page is selected (I guess 2 pages makes most sense, but this is up to you) to enable your feature. This is for two reasons. 1) Selecting all pages on the iconview as a default is not consistent with the behaviour of the other actions. 2) You can create a booklet without having a single page on the iconview, which triggers a crash.

If this is included, I would merge the PR in.

[nicos68]

You are absolutely right, my algorithm was flawed, I fixed it. I also made the changes you requested.

[angsch]

@nicos68 Great work! Thank you very much.

[mara004]

I've just tested this interesting new feature (checked out from master branch) and noticed a few issues:

• the created booklet pages are appended to the end of the thumbnail list, which is confusing - I would have expected them to show up at the same position as the input
• sometimes (that is, very often) the "generate booklet" command doesn't work - in the command-line, I get the following Exception:

Traceback (most recent call last):
  File "/home/manuel/Downloads/builds/pdfarranger/pdfarranger/pdfarranger.py", line 402, in generate_booklet
    with self.model_lock:
AttributeError: __enter__

From looking at the code, this is because model_lock is a method and not a context manager, so the false with block should be replaced by a pair of self.model_lock() self.model_unlock().

self.model_lock()
for _ in range(blank_page_count):
    self._insert_pages(model, file, selection)
    added_page_index = selection[-1].get_indices()[-1] + 1
    # Fetch the additional blank pages and remove them from the model.
    added_page = model.get_value(model.get_iter(added_page_index), 0)
    pages.append(added_page)
    model.remove(model.get_iter(added_page_index))
self.model_unlock()

However, with the code modified as shown above, the UI sometimes freezes and never unlocks again when triggering "generate booklet". Maybe a model lock/unlock is inappropriate in this place after all? It seems to work when I just comment it out.

[nicos68]

I've just tested this interesting new feature (checked out from master branch) and noticed a few issues:

* the created booklet pages are appended to the end of the thumbnail list, which is confusing - I would have expected them to show up at the same position as the input

* sometimes the "generate booklet" command doesn't work - in the command-line, I get the following Exception:

Traceback (most recent call last):
  File "/home/manuel/Downloads/builds/pdfarranger/pdfarranger/pdfarranger.py", line 402, in generate_booklet
    with self.model_lock:
AttributeError: __enter__

From looking at the code, this is because model_lock is a method and not a context manager, so the false with block should be replaced by a pair of self.model_lock() self.model_unlock().

self.model_lock()
for _ in range(blank_page_count):
    self._insert_pages(model, file, selection)
    added_page_index = selection[-1].get_indices()[-1] + 1
    # Fetch the additional blank pages and remove them from the model.
    added_page = model.get_value(model.get_iter(added_page_index), 0)
    pages.append(added_page)
    model.remove(model.get_iter(added_page_index))
self.model_unlock()

However, with the code modified as shown above, the UI sometimes freezes and never unlocks again when triggering "generate booklet". Maybe a model lock/unlock is inappropriate in this place after all? It seems to work when I just comment it out.

Same for me. I've never had any problems before adding the lock. I'm okay to remove it but if more experienced maintainers of the project could give their feedback on this I'd feel more comfortable doing it.

Also, I used the context manager because

• it is used in many other places in the code, sometimes with the method syntax, sometimes directly using model_lock, so a bit of consistency throughout the app would be welcome here.
• it is generally good practice to use context manager, as if something crashes between lock and unlock then the app stays locked.

[mara004]

Also, I used the context manager because [...]

It is correct that a context manager would be possible/appropriate. However, you certainly cannot use a method like a context manager. You would have to create a class that implements __init__(self, lock), __enter__(self) and __exit__(self, exc_type, exc_value, exc_traceback).

as if something crashes between lock and unlock then the app stays locked.

Yes, good point. Introcucing a context manager for lock/unlock would make sense, in my opinion.

[angsch]

Thank you for pointing out the problem with the rendering. That as well as the missing cases with cropped pages and rotations should be taken up. An updated TODO-list can be found in #438 - if something is missing, please add it.

[mara004]

Improve rendering

Primary points:

• fix the with blunder
• investigate possible exceptions or infinite waits inside the for loop (It is really suspicious that wrapping the loop with model lock/unlock causes pdfarranger to freeze. The unlock is never called, so the loop code either crashes before finishing or takes forever.)

After this visuals can be dealt with, like positioning the new booklet pages correctly.

[kbengs]

The idea with the model lock is that if python interpreter decides to switch to the other thread between "path" and "ref" lines in core.py, (see below, at read arrow) and then a page is removed from the model in main thread, then path and ref will not point to the same page. I am not sure if that could lead to a problem. If you think it can not cause a problem, feel free to remove the lock.

[mara004]

I am not experienced in threading, but would vote for keeping the lock to be safe.
However, we need to investigate what is wrong with the for loop that causes pdfarranger to freeze forever.

[jeromerobert]

Note that there are 2 model_lock:

• PdfArranger.model_lock which is a method and must not be as a with statement.
• PDFRenderer.model_lock which is a threading.Lock (can be used as a with statement)

I agree with mara004, that PdfArranger.model_lock should be replaced by a context manager and all PdfArranger.model_lock/PdfArranger.model_unlock pair replaced by a with statements. I also think one of those 2 model_lock should be renamed to avoid further confusions.

[jeromerobert]

I'm on it.