From 992f13d53c4189e61e8de6c31b9b0aff0ca490e5 Mon Sep 17 00:00:00 2001 From: sepehr Date: Wed, 15 Jul 2026 22:21:53 +0200 Subject: [PATCH] fix(translate): apply Excel chart text setters (were silently dropped) Chart elements (title, axis labels, series names) were collected and sent to the LLM, but the apply loop never invoked their setters. Reason: the apply loop iterated only text_elements[:sheet_name_offset], which excluded the chart-text positions (sheet names were inserted in the middle, pushing chart texts past sheet_name_offset). The LLM correctly returned French for "Revenue by Product" / "Quantity Trend" / "Order #" but the result was thrown away. Fix: include the chart-text slice [sheet_name_offset + N .. total_texts] in the apply loop. Setters for sheet names are still None, so they are naturally skipped. Add test_excel_chart_text_applied.py (2 end-to-end tests) using a fixed provider that pre-translates every known chart text; the test asserts all chart values in the output .xlsx come from the FR table, not the original English source. --- tests/test_excel_chart_text_applied.py | 111 +++++++++++++++++++++++++ translators/excel_translator.py | 15 +++- 2 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 tests/test_excel_chart_text_applied.py diff --git a/tests/test_excel_chart_text_applied.py b/tests/test_excel_chart_text_applied.py new file mode 100644 index 0000000..36a2022 --- /dev/null +++ b/tests/test_excel_chart_text_applied.py @@ -0,0 +1,111 @@ +"""Regression test for the bug where chart text was never written back +to the .xlsx after translation, even though the LLM had returned the +correct French translation. + +Root cause: the apply loop only iterated text_elements[:sheet_name_offset] +which excluded the chart-text positions (sheet names were added in the +middle, pushing chart texts past sheet_name_offset). +""" +import sys +import shutil +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from lxml import etree +from openpyxl import load_workbook +import zipfile + +from translators.excel_translator import ExcelTranslator + +NS_A = "http://schemas.openxmlformats.org/drawingml/2006/main" +NS_C = "http://schemas.openxmlformats.org/drawingml/2006/chart" + +# Force a known translation for every chart-relevant string. +FR = { + "Monthly Sales Volume (Units)": "Volume de ventes mensuel (unites)", + "Month": "Mois", + "Quantity": "Quantite", + "Revenue by Product": "Chiffre d'affaires par produit", + "Quantity Trend": "Tendance des quantites", + "Order #": "Numero de commande", +} + + +class FixedProvider: + def translate_batch(self, *args, **kwargs): + if args and hasattr(args[0], 'text'): + return [type("R", (), {"translated_text": FR.get(req.text, req.text)})() for req in args[0]] + texts = args[0] if args else kwargs.get('texts', []) + return [FR.get(t, t) for t in texts] + + +def _read_chart_a_t(xlsx_path: Path): + out = {} + with zipfile.ZipFile(xlsx_path, 'r') as zf: + for name in zf.namelist(): + if name.startswith('xl/charts/') and name.endswith('.xml'): + xml = etree.fromstring(zf.read(name)) + out[name] = [t.text for t in xml.iter(f'{{{NS_A}}}t') if t.text] + return out + + +def test_chart_texts_get_applied_to_output(tmp_path): + src = Path(r'D:\dev1405\office_translator\sample_files\test_corpus\test_excel.xlsx') + dst = tmp_path / "out.xlsx" + shutil.copy(src, dst) + + tr = ExcelTranslator() + tr.set_provider(FixedProvider()) + tr.translate_file( + input_path=src, + output_path=dst, + target_language="fr", + source_language="en", + translate_images=False, + ) + + chart_texts = _read_chart_a_t(dst) + # All chart a:t elements should be translated, not still English. + for cf, texts in chart_texts.items(): + for t in texts: + assert t in FR.values(), ( + f"{cf} still has untranslated text: {t!r}. " + f"Expected one of: {list(FR.values())}" + ) + + +def test_chart_refs_also_rewritten(tmp_path): + """Sheet refs are rewritten as part of the same fix flow.""" + src = Path(r'D:\dev1405\office_translator\sample_files\test_corpus\test_excel.xlsx') + dst = tmp_path / "out2.xlsx" + shutil.copy(src, dst) + + tr = ExcelTranslator() + tr.set_provider(FixedProvider()) + tr.translate_file( + input_path=src, + output_path=dst, + target_language="fr", + source_language="en", + translate_images=False, + ) + + # Sheet name 'Sales 2024' is not in FR so it should not be renamed. + # The chart refs should still point at 'Sales 2024' (no rename happened). + with zipfile.ZipFile(dst, 'r') as zf: + for name in zf.namelist(): + if name.startswith('xl/charts/') and name.endswith('.xml'): + xml = etree.fromstring(zf.read(name)) + refs = [f.text for f in xml.iter(f'{{{NS_C}}}f') if f.text] + # The chart refs should still be valid (no broken refs) + for r in refs: + assert '!' in r, f"Invalid chart ref in {name}: {r!r}" + + +if __name__ == "__main__": + import tempfile + with tempfile.TemporaryDirectory() as d: + test_chart_texts_get_applied_to_output(Path(d)) + test_chart_refs_also_rewritten(Path(d)) + print("chart text applied test passed.") diff --git a/translators/excel_translator.py b/translators/excel_translator.py index 9f8de44..dee144d 100644 --- a/translators/excel_translator.py +++ b/translators/excel_translator.py @@ -264,11 +264,20 @@ class ExcelTranslator: } ) - # Apply cell translations + # Apply cell + chart translations. The structure of + # text_elements is: + # [0 .. sheet_name_offset) -> cell/header/footer setters + # [sheet_name_offset .. +N) -> sheet names (setter=None) + # [sheet_name_offset + N .. total_texts) -> chart setters + # The old code only iterated the first slice, silently skipping + # every chart setter. We now concatenate the two non-None slices. + _n_sheet_names = len(sheet_names_to_translate) + _chart_slice = text_elements[sheet_name_offset + _n_sheet_names:] + _chart_trans_slice = translated_texts[sheet_name_offset + _n_sheet_names:] for i, ((original_text, setter), translated) in enumerate( zip( - text_elements[:sheet_name_offset], - translated_texts[:sheet_name_offset], + text_elements[:sheet_name_offset] + _chart_slice, + translated_texts[:sheet_name_offset] + _chart_trans_slice, ) ): if translated is not None and setter is not None: