-
Notifications
You must be signed in to change notification settings - Fork 36
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
Use master header and footer when composing docs with sections. #82
Conversation
Because the secPr of the master template is usually in the body it only applies to the last section of the composed document. This is fine as long as we only add documents themselves having a single section, but breaks down as soon as we have multiple sections. We then need to make sure to add the header and footer to the first section of the document, so that it gets inherited to the other sections of the document.
c377f52
to
f19353c
Compare
We also need to move the page number type tag to that section properties and remove it from the section properties from the body.
if len(self.doc.sections) == 1 or len(doc.sections) == 1: | ||
return | ||
|
||
first_new_section_idx = len(self.doc.sections) - len(doc.sections) |
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.
is this safe? Isn't it possible to get a negative number here?
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 think so, it's already used exactly like this on line 647 above
|
||
for footer_name in ('footer', 'even_page_footer', 'first_page_footer'): | ||
footer_main = getattr(self.doc.sections[-1], footer_name) | ||
if not footer_main._has_definition: |
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.
do we always have a footer_main
? Or could it also be None
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.
see below
|
||
for header_name in ('header', 'even_page_header', 'first_page_header'): | ||
header_main = getattr(self.doc.sections[-1], header_name) | ||
if not header_main._has_definition: |
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.
do we always have a header_main
? Or could it also be None
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.
Yes this is safe. I checked the code, the first_section
is a Section
object for which the attributes used here always return a Header
or Footer
object, which always have a _has_definition
@@ -5,7 +5,8 @@ Changelog | |||
1.3.5 (unreleased) | |||
------------------ | |||
|
|||
- Nothing changed yet. | |||
- Support missing style elements. [BryceStevenWilley] | |||
- Correctly handle headers and footers when merging documents with sections. [njohner] |
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.
merge issue
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.
No sorry, I should have specified. I merged a PR from BryceStevenWilley and did not see he had not added a changelog, so I sneaked it in here 🐍
206c6d9
to
0cab8b4
Compare
Because the
secPr
of the master template is usually in the body it only applies to the last section of the composed document. This is fine as long as we only add documents themselves having a single section (as we ignore theirsecPr
tags), but breaks down as soon as we have multiple sections. In these cases we keep thesecPr
tags, but this has two side effects:secPr
of the first section added applies to everything that came before, and thesecPr
of the master template now only applies to the last sectionThe first problem is hard to fix and raises a lot of questions on what is the behaviour we want. The second point is fixed with this PR. We already had decided we always want to use the header and footer from the master template (that is why we already delete the headers and footers from all
secPr
tags that get added from other documents). We therefore need to make sure to add the header and footer of the master template to the first section of the document, so that it gets inherited to the other sections of the document.For CA-4386