Skip to content
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

Revise toc calculation #2047

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Revise toc calculation #2047

wants to merge 12 commits into from

Conversation

peterschaer
Copy link
Collaborator

adresses #2038

I completely reworked the calculation:

  • all the pixel values where verified in the templates and updated when needed
  • unused variables were deleted
  • qrcode (if activated) is now included
  • landregister disclaimer is now included
  • the ConcernedTheme-Heading cannot be calculated at runtime. The label is defined in the templates which are not accessible from the python code.

@svamaa @michmuel @voisardf @lopo977 could you test with your data?

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 97.08738% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.62%. Comparing base (86fe195) to head (36f687d).
Report is 19 commits behind head on master.

Files with missing lines Patch % Lines
...reb/contrib/print_proxy/mapfish_print/toc_pages.py 98.00% 2 Missing ⚠️
...contrib/print_proxy/mapfish_print/mapfish_print.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2047      +/-   ##
==========================================
+ Coverage   85.51%   85.62%   +0.10%     
==========================================
  Files         120      120              
  Lines        5276     5320      +44     
==========================================
+ Hits         4512     4555      +43     
- Misses        764      765       +1     
Flag Coverage Δ
unittests 85.62% <97.08%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@lopo977
Copy link
Collaborator

lopo977 commented Aug 29, 2024

adresses #2038

I completely reworked the calculation:

* all the pixel values where verified in the templates and updated when needed

* unused variables were deleted

* qrcode (if activated) is now included

* landregister disclaimer is now included

* the ConcernedTheme-Heading cannot be calculated at runtime. The label is defined in the templates which are not accessible from the python code.

@svamaa @michmuel @voisardf @lopo977 could you test with your data?

It looks good to me.

CH657302880741-1.pdf

@michmuel
Copy link
Collaborator

extract_as_dict['nbTocPages'] = TocPages(extract_as_dict, display_qrcode).getNbPages()

Are the law status already taken into account here? If a theme has plrs "in force" and plrs "changes" the topic is listed twice in the toc of the pdf.

@peterschaer
Copy link
Collaborator Author

extract_as_dict['nbTocPages'] = TocPages(extract_as_dict, display_qrcode).getNbPages()

Are the law status already taken into account here? If a theme has plrs "in force" and plrs "changes" the topic is listed twice in the toc of the pdf.

Yes, the law status is taken into account here:

total_size += len(self.extract['ConcernedTheme']) * toc_item_height

If a theme has plrs of type inForce and plrs of type changeWithoutPreEffect there are two entries in the extract['ConcernedTheme'] list and therefore are included in the calculation.

@peterschaer
Copy link
Collaborator Author

extract_as_dict['nbTocPages'] = TocPages(extract_as_dict, display_qrcode).getNbPages()

Are the law status already taken into account here? If a theme has plrs "in force" and plrs "changes" the topic is listed twice in the toc of the pdf.

Yes, the law status is taken into account here:

total_size += len(self.extract['ConcernedTheme']) * toc_item_height

If a theme has plrs of type inForce and plrs of type changeWithoutPreEffect there are two entries in the extract['ConcernedTheme'] list and therefore are included in the calculation.

@michmuel your comment on the daily was correct. the toc calculation is executed before the json is transformed for mapfish_print. Therefore it does not take into account different law_status. I will try to fix this behaviour this afternoon.

@peterschaer
Copy link
Collaborator Author

The calculation now takes into account the law_status. The PR can now be tested...

@svamaa
Copy link
Collaborator

svamaa commented Sep 2, 2024

I tested this branch within our oereb-server. I could not find any parcel, that calculates a wrong toc, thus.
Works for us ;) However, we should find out what's different in BL.

@voisardf
Copy link
Collaborator

Same here, the calculation works for us too. 👍

total_size = 0
page_bottom_margin = 20 # toc.jrxml
footer_height = 10 # toc.jrxml
log.debug(f"header total_size: {total_size}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total_size is always zero.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted, thanks! I just commited a bugfix.

@michmuel
Copy link
Collaborator

michmuel commented Sep 24, 2024

@peterschaer
Copy link
Collaborator Author

@voisardf, @peterschaer I have a question concerning https://github.com/openoereb/pyramid_oereb_mfp/blob/8d9da5ab3a8d1a8469fd693e4f86c5870ee9f6dc/print-apps/oereb/toc.jrxml#L225-L243 What is the use of this?

This detail contains the NoData heading ("Öffentlich-rechtliche Eigentumsbeschränkungen, zu denen noch keine Daten vorhanden sind").

@michmuel
Copy link
Collaborator

michmuel commented Oct 2, 2024

@voisardf, @peterschaer I have a question concerning https://github.com/openoereb/pyramid_oereb_mfp/blob/8d9da5ab3a8d1a8469fd693e4f86c5870ee9f6dc/print-apps/oereb/toc.jrxml#L225-L243 What is the use of this?

This detail contains the NoData heading ("Öffentlich-rechtliche Eigentumsbeschränkungen, zu denen noch keine Daten vorhanden sind").

I selected the wrong lines. I meant these lines:
https://github.com/openoereb/pyramid_oereb_mfp/blob/8d9da5ab3a8d1a8469fd693e4f86c5870ee9f6dc/print-apps/oereb/toc.jrxml#L244-L255

@peterschaer
Copy link
Collaborator Author

@voisardf, @peterschaer I have a question concerning https://github.com/openoereb/pyramid_oereb_mfp/blob/8d9da5ab3a8d1a8469fd693e4f86c5870ee9f6dc/print-apps/oereb/toc.jrxml#L225-L243 What is the use of this?

This detail contains the NoData heading ("Öffentlich-rechtliche Eigentumsbeschränkungen, zu denen noch keine Daten vorhanden sind").

I selected the wrong lines. I meant these lines: https://github.com/openoereb/pyramid_oereb_mfp/blob/8d9da5ab3a8d1a8469fd693e4f86c5870ee9f6dc/print-apps/oereb/toc.jrxml#L244-L255

This detail contains the names of the topics having no data. It can be empty.

return x
total_size = 0
theme_without_data_item_height = 15 # toc.jrxml (12 in themelist.jrxml)
total_size += len(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total_size should be calculated like this: len(self.extract["ThemeWithoutData"]) * theme_without_data_item_height
Also, the theme_without_data_item_height is 15 only for one entry. 12 is the line height of an individual entry, plus padding. (@voisardf )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed a bugfix adressing this. Did I understand it correctly?

@voisardf
Copy link
Collaborator

voisardf commented Oct 25, 2024

@peterschaer @michmuel I checked the templates and found, that some spacings and alignments were (in my opinion) over time not defined as they should have or could have. I adapted them slightly and pushed a draft version to openoereb/pyramid_oereb_mfp#147

@voisardf
Copy link
Collaborator

TOC_adapted_spacing_line_heights

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants