-
Notifications
You must be signed in to change notification settings - Fork 24
fix: corrige decisão de layout de figuras no gerador de PDF (unidade cm/EMU e DPI de figuras irmãs) #1294
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
base: master
Are you sure you want to change the base?
fix: corrige decisão de layout de figuras no gerador de PDF (unidade cm/EMU e DPI de figuras irmãs) #1294
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -423,11 +423,14 @@ def _get_href_from_node(node): | |
| caption_texts.append(txt) | ||
| caption = ' '.join([c for c in caption_texts if c]) | ||
|
|
||
| # graphic may be direct child or inside <alternatives> | ||
| # graphic may be a direct child, or offered as several representations | ||
| # inside <alternatives> - only match the direct child here so the latter | ||
| # case falls through to the ranking logic below instead of grabbing the | ||
| # first <graphic> in document order. | ||
| href = None | ||
| alt_text = None | ||
|
|
||
| graphic = fig_node.find('.//graphic') | ||
| graphic = fig_node.find('graphic') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comento aqui porque não consigo comentar em código não participante do PR. O comentário seguinte serve para o arquivo Agora que chega a esta ordenação, specific-use continua não participando da prioridade: a chave considera apenas thumbnail, área e extensão. Isso contradiz a intenção descrita no commit de preferir specific-use="scielo-web". Sugiro registrar is_scielo_web em cada candidato e ordenar, por exemplo, por (is_thumbnail, not is_scielo_web, -dims_area, ext_rank), preservando a rejeição de thumbnails antes de priorizar a variante web. |
||
| if graphic is not None: | ||
| href = _get_href_from_node(graphic) | ||
| alt_text = graphic.get('alt') or graphic.get('alt-text') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,7 @@ def decide_figure_layout(docx, figure_data, page_attributes=pdf_enum.PAGE_ATTRIB | |
| Decide whether a figure should occupy the full page width (double-column-layout) or a single column (single-column-layout). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A documentação está invertendo os labels usados pela implementação. SINGLE_COLUMN_PAGE_LABEL cria uma seção de uma coluna e permite largura total; DOUBLE_COLUMN_PAGE_LABEL mantém a figura dentro de uma das duas colunas. Além disso, a assinatura usa threshold=1.1, mas a seção Args informa default 0.9. Sugiro corrigir a docstring para evitar que os próximos ajustes e testes sejam escritos com a interpretação oposta. |
||
|
|
||
| Heuristic: | ||
| - Compute the natural width of the image in centimeters using Pillow and its DPI metadata (tries dpi, jfif_density; defaults to 150 DPI when missing). | ||
| - Compute the natural width of the image in centimeters using Pillow and its DPI metadata (tries dpi, jfif_density; defaults to 300 DPI when missing). | ||
| - Compute the available content width (page width minus margins) and the single-column width ((content - column_spacing)/2). | ||
| - If natural image width >= threshold * single-column width, return 'double-column-layout'; otherwise 'single-column-layout'. | ||
|
|
||
|
|
@@ -82,40 +82,69 @@ def decide_figure_layout(docx, figure_data, page_attributes=pdf_enum.PAGE_ATTRIB | |
| column_spacing_cm = Cm(column_spacing_twips / 567.0) | ||
| single_col_width = (content_width - column_spacing_cm) / 2 | ||
|
|
||
| # Resolve image path (local or download) | ||
| probe = probe_image_dpi(docx, figure_data) | ||
| if probe is None: | ||
| return pdf_enum.SINGLE_COLUMN_PAGE_LABEL | ||
| px_w, dpi = probe | ||
|
|
||
| # A caller may have compared this image's DPI against its siblings and | ||
| # supplied a more trustworthy value here - see 'layout_dpi_override'. | ||
| if isinstance(figure_data, dict) and figure_data.get('layout_dpi_override'): | ||
| try: | ||
| dpi = float(figure_data['layout_dpi_override']) | ||
| except (TypeError, ValueError): | ||
| pass | ||
|
|
||
| # width in inches then to Cm | ||
| width_in_cm = (px_w / max(1.0, dpi)) * 2.54 | ||
|
|
||
| # single_col_width degraded from a Cm object to a raw EMU number in the | ||
| # subtraction/division above (python-docx's Length has no operator | ||
| # overloads that preserve units) - convert back to cm so this compares | ||
| # against width_in_cm in the same unit, instead of cm against EMU. | ||
| single_col_width_cm = single_col_width / Cm(1) | ||
|
|
||
| # If the image is wider than a single column by the threshold factor, prefer a single-column-layout (full width). Otherwise, keep double-column-layout. | ||
| return pdf_enum.SINGLE_COLUMN_PAGE_LABEL if width_in_cm >= float(threshold) * single_col_width_cm else pdf_enum.DOUBLE_COLUMN_PAGE_LABEL | ||
|
|
||
|
|
||
| def probe_image_dpi(docx, figure_data): | ||
| """ | ||
| Resolve a figure's image and return its (pixel_width, dpi), or None if | ||
| the image can't be opened. | ||
| """ | ||
| try: | ||
| from PIL import Image | ||
| except Exception: | ||
| return None | ||
|
|
||
| context = _get_docx_context(docx) | ||
| href = figure_data.get('href') if isinstance(figure_data, dict) else None | ||
| img_path = _resolve_image_path(href, context) | ||
| if not img_path: | ||
| return pdf_enum.SINGLE_COLUMN_PAGE_LABEL | ||
| if not img_path or not os.path.exists(img_path): | ||
| return None | ||
|
|
||
| # Open image and compute its natural width in Cm using DPI (default 72 DPI) | ||
| if not os.path.exists(img_path): | ||
| return pdf_enum.SINGLE_COLUMN_PAGE_LABEL | ||
| try: | ||
| with Image.open(img_path) as im: | ||
| px_w = im.width | ||
| dpi = _infer_image_dpi(im) | ||
| # width in inches then to Cm | ||
| width_in_cm = (px_w / max(1.0, dpi)) * 2.54 | ||
| return im.width, _infer_image_dpi(im) | ||
| except Exception: | ||
| return pdf_enum.SINGLE_COLUMN_PAGE_LABEL | ||
|
|
||
| # If the image is wider than a single column by the threshold factor, prefer a single-column-layout (full width). Otherwise, keep double-column-layout. | ||
| return pdf_enum.SINGLE_COLUMN_PAGE_LABEL if width_in_cm >= float(threshold) * float(single_col_width) else pdf_enum.DOUBLE_COLUMN_PAGE_LABEL | ||
| return None | ||
|
|
||
|
|
||
| # ----------------- | ||
| # Private helpers | ||
| # ----------------- | ||
|
|
||
| _NO_METADATA_DPI_FALLBACK = 300.0 | ||
|
|
||
|
|
||
| def _infer_image_dpi(im) -> float: | ||
| """Infer the horizontal DPI from a PIL Image, considering multiple metadata sources. | ||
|
|
||
| Priority: | ||
| - im.info['dpi']: tuple or number | ||
| - im.info['jfif_unit'] and im.info['jfif_density'] (unit 1=inches, 2=cm) | ||
| Fallback: 96 dpi | ||
| Fallback: _NO_METADATA_DPI_FALLBACK. | ||
| """ | ||
| try: | ||
| info = getattr(im, 'info', {}) or {} | ||
|
|
@@ -138,10 +167,10 @@ def _infer_image_dpi(im) -> float: | |
|
|
||
| if unit == 2: # per cm | ||
| return float(density[0]) * 2.54 | ||
| return 96.0 | ||
| return _NO_METADATA_DPI_FALLBACK | ||
|
|
||
| except Exception: | ||
| return 96.0 | ||
| return _NO_METADATA_DPI_FALLBACK | ||
|
|
||
| def _add_paragraph_with_formatting(docx, text, style_name='SCL Paragraph'): | ||
| """Minimal helper to add a paragraph with an optional style, avoiding circular imports.""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1238,7 +1238,48 @@ def test_extract_trans_abstract_data_custom_namespace(self): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| result = xml_pipe.extract_trans_abstract_data( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| xml, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| xml, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| namespaces={'xml': 'http://custom.namespace'} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.assertEqual(result, expected) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class TestExtractFigureData(unittest.TestCase): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Tests for extract_figure_data's graphic href resolution, including | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| the ranking logic used when a figure only offers <alternatives>.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_direct_graphic_child_is_used_as_is(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| xml = etree.fromstring( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<fig xmlns:xlink="http://www.w3.org/1999/xlink" id="F1">' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<label>Figure 1</label>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<graphic xlink:href="figure1.jpg"/>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '</fig>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| result = xml_pipe.extract_figure_data(xml) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.assertEqual(result['href'], 'figure1.jpg') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_alternatives_prefers_scielo_web_over_raw_tif(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| xml = etree.fromstring( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<fig xmlns:xlink="http://www.w3.org/1999/xlink" id="F1">' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<label>Graph 1</label>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<alternatives>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<graphic xlink:href="raw.tif"/>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<graphic xlink:href="web.png" specific-use="scielo-web"/>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<graphic xlink:href="thumb.jpg" specific-use="scielo-web" content-type="scielo-267x140"/>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '</alternatives>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '</fig>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| result = xml_pipe.extract_figure_data(xml) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.assertEqual(result['href'], 'web.png') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1261
to
+1273
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Este teste não comprova que specific-use="scielo-web" tem prioridade. web.png vence raw.tif apenas porque o ranking atual prefere PNG a TIFF e nem considera specific-use. Ao trocar as entradas para raw.png e web.jpg com specific-use="scielo-web", o resultado é raw.png. Sugiro tornar o teste discriminante e, se a regra desejada for realmente priorizar SciELO Web, incluir specific-use explicitamente na chave de ordenação.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_alternatives_falls_back_to_tif_when_no_better_option(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| xml = etree.fromstring( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<fig xmlns:xlink="http://www.w3.org/1999/xlink" id="F1">' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<label>Graph 1</label>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<alternatives>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '<graphic xlink:href="raw.tif"/>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '</alternatives>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '</fig>' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| result = xml_pipe.extract_figure_data(xml) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.assertEqual(result['href'], 'raw.tif') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import os | ||
| import tempfile | ||
| import unittest | ||
|
|
||
| from docx import Document | ||
| from PIL import Image | ||
|
|
||
| from packtools.sps.formats.pdf.renderer.docx.figure import decide_figure_layout | ||
| from packtools.sps.formats.pdf import enum as pdf_enum | ||
|
|
||
|
|
||
| class TestDecideFigureLayoutUnits(unittest.TestCase): | ||
| """ | ||
| Regression test for a units bug: single_col_width degrades from a Cm | ||
| object to a raw EMU number under python-docx's Length arithmetic (it has | ||
| no operator overloads that preserve units), so comparing it directly | ||
| against width_in_cm (a real centimeter float) compared cm against EMU - | ||
| a ~360000x scale mismatch that made the full-width branch unreachable | ||
| for any real image, regardless of size. | ||
| """ | ||
|
|
||
| def setUp(self): | ||
| self.tmpdir = tempfile.TemporaryDirectory() | ||
| self.addCleanup(self.tmpdir.cleanup) | ||
|
|
||
| def _make_png(self, px_width, px_height=200, dpi=96): | ||
| path = os.path.join(self.tmpdir.name, f"img_{px_width}.png") | ||
| Image.new("RGB", (px_width, px_height), color="white").save( | ||
| path, format="PNG", dpi=(dpi, dpi) | ||
| ) | ||
| return path | ||
|
|
||
| def test_wide_image_gets_full_width_layout(self): | ||
| # ~18.5cm at 96 DPI - wider than a single column (~8.2cm) by far more | ||
| # than the 1.1 threshold, on an A4/2-column default page. | ||
| img_path = self._make_png(px_width=698) | ||
| docx = Document() | ||
| fig = {"href": img_path, "label": "Figure 1"} | ||
| self.assertEqual( | ||
| decide_figure_layout(docx, fig), pdf_enum.SINGLE_COLUMN_PAGE_LABEL | ||
| ) | ||
|
|
||
| def test_narrow_image_stays_within_column(self): | ||
| # ~2.6cm at 96 DPI - well under the single-column width. | ||
| img_path = self._make_png(px_width=100) | ||
| docx = Document() | ||
| fig = {"href": img_path, "label": "Figure 1"} | ||
| self.assertEqual( | ||
| decide_figure_layout(docx, fig), pdf_enum.DOUBLE_COLUMN_PAGE_LABEL | ||
| ) | ||
|
|
||
| def test_untagged_image_uses_print_resolution_fallback(self): | ||
| # An image with no DPI tag at all must fall back to print | ||
| # resolution, not screen resolution. | ||
| img_path = self._make_png(px_width=700) | ||
| with Image.open(img_path) as im: | ||
| im.info.pop("dpi", None) | ||
| im.save(img_path) # re-save without the dpi tag | ||
| docx = Document() | ||
| fig = {"href": img_path, "label": "Graph 1"} | ||
| self.assertEqual( | ||
| decide_figure_layout(docx, fig), pdf_enum.DOUBLE_COLUMN_PAGE_LABEL | ||
| ) | ||
|
|
||
| def test_layout_dpi_override_replaces_embedded_dpi(self): | ||
| # 'layout_dpi_override' must take precedence over the image's own | ||
| # embedded DPI. | ||
| img_path = self._make_png(px_width=612, dpi=72) | ||
| docx = Document() | ||
| fig = {"href": img_path, "label": "Graph 1", "layout_dpi_override": 300.0} | ||
| self.assertEqual( | ||
| decide_figure_layout(docx, fig), pdf_enum.DOUBLE_COLUMN_PAGE_LABEL | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
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.
Sugiro remover a heurística de mediana deste PR. O caso a4 não a exercita nem depende dela: o resultado consistente já é obtido pela seleção das variantes PNG e pelo fallback de 300 DPI. Além disso, pertencer à mesma seção não implica compartilhar DPI ou escala física. No conjunto de XMLs/PDFs ampliado (26 arquivos), a mediana atua em apenas 3 de 116 figuras e prejudica um dos casos. Parece mais seguro usar o DPI próprio quando presente e aplicar 300 DPI somente quando os metadados estiverem ausentes.