Review — Py-PDF-Compare (paquete completo)

FIXES NEEDED
Alcance: pdf_compare/comparator.py, pdf_compare/cli.py, pdf_compare/gui.py, pdf_compare/config.py (823 lineas) · Foco: correctness · 2026-08-17
3 bloqueantes. Dos de ellos verificados ejecutando el codigo, no inferidos.

El diseno del nucleo es solido: alinear paginas por contenido y componer el informe con el contenido vectorial original es la decision correcta, y el patron hilo + cola de la GUI es el uso canonico de Tk. Los problemas no estan en la arquitectura sino en los bordes: coordenadas que no contemplan la rotacion de pagina, un comportamiento documentado que el codigo nunca cumple, y la gestion de recursos (ficheros temporales, documentos abiertos, memoria de la previsualizacion). El patron comun a los tres bloqueantes es que fallan en silencio: producen una salida plausible en lugar de un error.

3
Bloqueante
6
Mejora
2
Opcional

Bloqueante 3

BloqueantecorrectnessF1

Los resaltados se colocan mal en paginas rotadas

Ubicacion: pdf_compare/comparator.py:94-97
python
94
    def extract_words_with_bbox(self, page):
95
        """Extract words with their bounding boxes from a page."""
96
        words = page.get_text("words")  # Returns list of (x0, y0, x1, y1, "word", block_no, line_no, word_no)
97
        return [{'text': w[4], 'bbox': fitz.Rect(w[:4])} for w in words]
98
 
99
    def compare_visuals(self):
100
        """
Que pasa
get_text('words') devuelve las cajas en el sistema de coordenadas SIN rotar de la pagina, mientras que page.rect y show_pdf_page trabajan en el espacio ya rotado. El codigo usa la caja cruda y solo le suma los margenes, asi que en cualquier PDF con /Rotate distinto de 0 los rectangulos rojos y verdes se pintan sobre palabras que no son.
Impact
Verificado: en una pagina rotada 90 grados, una palabra cuya caja cruda es (100, 88) aparece realmente en (689, 100). Ambas caen dentro del rect de la pagina, por lo que no hay ni error ni aviso: el informe sale con aspecto correcto y senala texto equivocado. Un informe de diferencias que miente es peor que uno que falla.
Por que importa
Extraer contenido y componer una pagina son dos espacios de coordenadas distintos, y PyMuPDF no los unifica por ti: expone page.rotation_matrix precisamente porque espera que hagas la conversion. Siempre que mezcles coordenadas de dos APIs, comprueba que hablan del mismo sistema antes de sumarles un offset.
Sugerencia
Normalizar en el unico punto donde se leen las cajas, para que el resto del codigo no tenga que saber nada de rotaciones.
Recommended change
Current code
def extract_words_with_bbox(self, page):
    words = page.get_text("words")
    return [{'text': w[4], 'bbox': fitz.Rect(w[:4])} for w in words]
Suggested code
def extract_words_with_bbox(self, page):
    """Las cajas llegan sin rotar; se pasan al espacio visible de la pagina."""
    words = page.get_text("words")
    rot = page.rotation_matrix
    return [{'text': w[4], 'bbox': fitz.Rect(w[:4]) * rot} for w in words]
Tests
No hay ningun test. Hace falta uno que compare un PDF rotado 90 grados con una variante suya y afirme que la caja resaltada cae sobre la palabra cambiada; con la rotacion a 0 el bug no se manifiesta, asi que el caso rotado es imprescindible.
BloqueantecontratoF2

El caso 'no hay diferencias' esta documentado pero es inalcanzable

Ubicacion: pdf_compare/cli.py:38-44
python
37
        comparator = PDFComparator(args.file_a, args.file_b)
38
        pdf_bytes = comparator.compare_visuals()
39
 
40
        if not pdf_bytes:
41
            print("No differences found or error occurred.")
42
        else:
43
            print(f"Saving vector-based report to '{args.output}'...")
44
 
Que pasa
compare_visuals() siempre devuelve el PDF compuesto, tambien cuando los dos documentos son identicos. Por tanto la rama 'No differences found' de la CLI y las ramas equivalentes de la GUI (gui.py:196 y 214-215) no se ejecutan nunca. El README documenta que el metodo devuelve None si no hay diferencias.
Impact
Verificado: comparar sample-files/original.pdf consigo mismo devuelve 331.948 bytes de informe. El usuario recibe un PDF de tres paginas para descubrir que no ha cambiado nada, y quien integre la API siguiendo el README escribira un 'if result is None' que nunca sera cierto. Incumple RF-10 de docs/requisitos.md.
Por que importa
Codigo muerto que aparenta manejar un caso es peor que no manejarlo: lees el fuente, concluyes que el caso esta cubierto, y no lo esta. Ademas es una promesa de la API publica ya publicada en PyPI.
Sugerencia
Decidir donde vive la deteccion. Lo mas limpio es que compare_visuals acumule si hubo alguna diferencia real (ya se sabe por finding: la variable has_changes de F10 existe justo para eso, pero no se usa) y devuelva None cuando no la haya. Asi la CLI y la GUI quedan correctas sin tocarlas.
Recommended change
Current code
pdf_bytes = output_doc.tobytes()
...
return pdf_bytes
Suggested code
# propagar has_changes desde _add_comparison_page y contar
# tambien las paginas anadidas/eliminadas como diferencia
if not any_difference:
    output_doc.close()
    return None
return output_doc.tobytes()
Tests
Falta un test que compare un fichero consigo mismo y afirme que el resultado es None, y otro que compruebe que con una diferencia minima si devuelve bytes.
BloqueantesecurityF3

Fichero temporal con nombre fijo en un directorio compartido

Ubicacion: pdf_compare/gui.py:199-205
python
197
                file_size_mb = len(pdf_bytes) / (1024 * 1024)
198
 
199
                # Save to temporary file
200
                temp_dir = tempfile.gettempdir()
201
                self.output_path = os.path.join(temp_dir, "pdf_comparison_report.pdf")
202
 
203
                with open(self.output_path, 'wb') as f:
204
                    f.write(pdf_bytes)
Que pasa
El informe se escribe siempre en <tmp>/pdf_comparison_report.pdf, un nombre fijo y predecible en un directorio que en Linux y macOS es compartido por todos los usuarios de la maquina.
Impact
Tres consecuencias reales: cualquier otro usuario del sistema puede leer el informe, que contiene el contenido integro de dos documentos posiblemente confidenciales; un tercero puede dejar preparado ese nombre como enlace simbolico y hacer que la escritura vaya a parar a otro sitio; y dos instancias de la aplicacion abiertas a la vez se pisan el fichero. Choca de frente con el NFR-09, que promete tratamiento local y privado de los documentos.
Por que importa
La regla es no construir nunca rutas temporales concatenando un nombre conocido a gettempdir(). El modulo tempfile existe para esto: crea el fichero con permisos 0600 y un nombre imprevisible, en una sola operacion atomica que no se puede interceptar entre el 'compruebo' y el 'escribo'.
Sugerencia
Usar tempfile.mkstemp con sufijo .pdf y borrar el fichero al cerrar la aplicacion.
Recommended change
Current code
temp_dir = tempfile.gettempdir()
self.output_path = os.path.join(temp_dir, "pdf_comparison_report.pdf")

with open(self.output_path, 'wb') as f:
    f.write(pdf_bytes)
Suggested code
fd, self.output_path = tempfile.mkstemp(prefix="pdf_comparison_", suffix=".pdf")
with os.fdopen(fd, 'wb') as f:
    f.write(pdf_bytes)
Tests
Dificil de cubrir con test unitario. Basta con verificarlo a mano: lanzar dos comparaciones seguidas y comprobar que generan rutas distintas.

Mejora 6

Mejoraresource-leakF4

Los documentos abiertos no se cierran si algo falla

Ubicacion: pdf_compare/comparator.py:111-152
python
110
 
111
        # Open PDFs
112
        doc_a = fitz.open(self.file_path_a)
113
        doc_b = fitz.open(self.file_path_b)
114
 
115
        # Create output PDF
116
        output_doc = fitz.open()
117
 
118
        for tag, i1, i2, j1, j2 in opcodes:
119
            if tag == 'equal' or tag == 'replace':
120
                count = max(i2 - i1, j2 - j1)
121
 
122
                for k in range(count):
123
                    idx_a = i1 + k if i1 + k < i2 else None
124
                    idx_b = j1 + k if j1 + k < j2 else None
125
 
126
                    if idx_a is not None and idx_b is not None:
127
                        # Both pages exist - compare them
128
                        self._add_comparison_page(output_doc, doc_a, doc_b, idx_a, idx_b, tag)
129
                    elif idx_a is None and idx_b is not None:
130
                        # Page only in B (insertion)
131
                        self._add_single_page(output_doc, doc_b, idx_b, 'right', 'Added')
132
                    elif idx_b is None and idx_a is not None:
133
                        # Page only in A (deletion)
134
                        self._add_single_page(output_doc, doc_a, idx_a, 'left', 'Missing')
135
 
136
            elif tag == 'delete':
137
                # Pages in A but not in B
138
                for k in range(i1, i2):
139
                    self._add_single_page(output_doc, doc_a, k, 'left', 'Missing')
140
 
141
            elif tag == 'insert':
142
                # Pages in B but not in A
143
                for k in range(j1, j2):
144
                    self._add_single_page(output_doc, doc_b, k, 'right', 'Added')
145
 
146
        # Get PDF bytes
147
        pdf_bytes = output_doc.tobytes()
148
 
149
        # Close documents
150
        doc_a.close()
151
        doc_b.close()
152
        output_doc.close()
153
 
154
        return pdf_bytes
Que pasa
compare_visuals abre tres documentos y los cierra al final, sin try/finally. Cualquier excepcion en el bucle de composicion, que es donde esta el grueso de la logica, se lleva por delante los tres close(). Lo mismo ocurre en extract_text (lineas 16-20).
Impact
La CLI muere igualmente y el proceso libera todo, asi que ahi no se nota. Donde si duele es en la GUI, que es un proceso de larga vida: cada comparacion fallida deja tres documentos MuPDF sin liberar, y quien use PDFComparator como libreria dentro de un servicio acumula la fuga indefinidamente. Incumple el NFR-08.
Por que importa
Cuando un objeto posee un recurso del sistema operativo, su liberacion no puede depender de que el camino feliz llegue hasta el final. En Python eso se expresa con try/finally o, mejor, con el gestor de contexto que fitz.Document ya implementa.
Sugerencia
Envolver la apertura en un with. fitz.Document soporta el protocolo de contexto, asi que no hace falta try/finally explicito.
Recommended change
Current code
doc_a = fitz.open(self.file_path_a)
doc_b = fitz.open(self.file_path_b)
output_doc = fitz.open()
...
doc_a.close()
doc_b.close()
output_doc.close()
Suggested code
with fitz.open(self.file_path_a) as doc_a, \
     fitz.open(self.file_path_b) as doc_b, \
     fitz.open() as output_doc:
    ...
    return output_doc.tobytes()
Tests
Un test que fuerce una excepcion a mitad de la composicion y compruebe que los documentos quedan cerrados.
MejoramemoriaF5

La previsualizacion carga en memoria todas las paginas a la vez

Ubicacion: pdf_compare/gui.py:223-239
python
226
        doc = fitz.open(stream=pdf_bytes, filetype="pdf")
227
 
228
        for page_num in range(len(doc)):
229
            page = doc[page_num]
230
            # Render at lower DPI for preview (saves memory)
231
            mat = fitz.Matrix(dpi / 72, dpi / 72)
232
            pix = page.get_pixmap(matrix=mat)
233
 
234
            # Convert to PIL Image
235
            img = Image.frombytes("RGB", [pix.width, pix.height], pix.samples)
236
            images.append(img)
237
 
238
        doc.close()
Que pasa
pdf_to_images renderiza el informe entero a imagenes PIL y las devuelve en una lista, y despues update_ui_success crea ademas una copia redimensionada y un CTkImage por cada pagina. Las tres colecciones conviven en memoria y ninguna se libera.
Impact
El informe tiene una pagina por cada pareja comparada y mide el doble de ancho que el original. A 100 DPI, cada pagina ronda los 10 MB en RAM. Un documento de 100 paginas se va a mas de 1 GB solo en previsualizacion, con la aplicacion congelandose o muriendo. El NFR-07 afirma que la memoria esta acotada por renderizar a resolucion reducida; el DPI reducido acota el coste por pagina, pero nada acota el numero de paginas.
Por que importa
Bajar la resolucion controla una dimension del problema; la otra es cuantos elementos retienes a la vez. Una previsualizacion no necesita tener todas las paginas materializadas, solo las que el usuario esta mirando.
Sugerencia
Renderizar bajo demanda: generar la imagen de cada pagina cuando entra en el area visible del scroll y descartarla al salir. Como paso intermedio barato, limitar la previsualizacion a las primeras N paginas con un aviso de que el informe completo esta en el fichero; asi acotas el problema en pocas lineas.
Tests
No es cuestion de test unitario sino de medida: perfilar la memoria con un informe de 100 paginas antes y despues del cambio.
MejoraconcurrenciaF6

El hilo de comparacion impide cerrar la aplicacion

Ubicacion: pdf_compare/gui.py:181
python
178
        self.progress_bar.start()
179
 
180
        # Run in thread to not freeze GUI
181
        thread = threading.Thread(target=self.run_comparison, daemon=False)
182
        thread.start()
183
 
184
        # Start checking for results
Que pasa
El hilo trabajador se crea con daemon=False, de forma explicita. Python no termina el proceso mientras siga vivo un hilo no-daemon.
Impact
Si el usuario cierra la ventana durante una comparacion larga, la ventana desaparece pero el proceso sigue en memoria hasta que la comparacion acaba. Desde fuera parece que la aplicacion se ha colgado, y quien la haya lanzado desde el binario empaquetado no tiene forma facil de matarla.
Por que importa
daemon=True es lo que corresponde a un hilo de trabajo cuya unica salida es la interfaz: si no hay interfaz, su resultado ya no le interesa a nadie. El valor por defecto ya lo hereda del hilo principal, asi que ponerlo a False explicitamente sugiere que fue una decision, no un descuido: merece la pena revisar si habia una razon.
Sugerencia
Cambiar a daemon=True. Si la razon era garantizar que el fichero temporal se termina de escribir, eso se resuelve mejor interceptando el cierre de la ventana (protocol('WM_DELETE_WINDOW')) que manteniendo vivo el proceso.
Recommended change
Current code
thread = threading.Thread(target=self.run_comparison, daemon=False)
Suggested code
thread = threading.Thread(target=self.run_comparison, daemon=True)
Tests
Comprobacion manual: lanzar una comparacion larga y cerrar la ventana.
Mejoraoff-by-oneF7

La ventana de exploracion vale 2, no 3 como dice la constante

Ubicacion: pdf_compare/comparator.py:67-76
python
65
 
66
                # Look ahead for better alignment
67
                for skip_j in range(1, min(LOOKAHEAD_WINDOW, len_b - j)):
68
                    similarity = difflib.SequenceMatcher(None, text_a[i], text_b[j + skip_j]).ratio()
69
                    if similarity > best_match['similarity'] and similarity > SIMILARITY_THRESHOLD:
70
                        best_match = {'type': 'insert', 'i': i, 'j': j + skip_j, 'similarity': similarity, 'skip': skip_j}
71
 
72
                for skip_i in range(1, min(LOOKAHEAD_WINDOW, len_a - i)):
73
                    similarity = difflib.SequenceMatcher(None, text_a[i + skip_i], text_b[j]).ratio()
74
                    if similarity > best_match['similarity'] and similarity > SIMILARITY_THRESHOLD:
Que pasa
LOOKAHEAD_WINDOW vale 3, pero range(1, min(3, ...)) recorre solo los desplazamientos 1 y 2. La tercera posicion nunca se explora.
Impact
Insertar tres paginas seguidas es un caso perfectamente normal y el alineado no lo detecta: a partir de ahi el resto del documento se compara desalineado y el informe se llena de diferencias falsas. El sintoma aparece lejos de la causa, que es lo que lo hace caro de diagnosticar.
Por que importa
Con range(inicio, fin) el limite superior queda fuera. Cuando una constante da nombre a un tamano de ventana, el codigo debe recorrer ese tamano; si no, la constante documenta algo que es mentira y el proximo que lea el fuente creera que el caso de tres paginas esta cubierto.
Sugerencia
Corregir el limite a min(LOOKAHEAD_WINDOW + 1, ...) o, si la ventana de 2 era la intencion, cambiar el valor de la constante para que diga la verdad. Enlaza con P-04 de docs/requisitos.md: estos umbrales no tienen criterio documentado.
Recommended change
Current code
for skip_j in range(1, min(LOOKAHEAD_WINDOW, len_b - j)):
Suggested code
for skip_j in range(1, min(LOOKAHEAD_WINDOW + 1, len_b - j)):
Tests
Un test con tres paginas insertadas seguidas que afirme que se marcan como anadidas y que las siguientes siguen alineadas.
Mejoraedge-caseF8

Dos paginas sin texto se consideran identicas con similitud perfecta

Ubicacion: pdf_compare/comparator.py:63
python
60
                alignments.append(('delete', i, len_a, j, j))
61
                break
62
            else:
63
                current_similarity = difflib.SequenceMatcher(None, text_a[i], text_b[j]).ratio()
64
                best_match = {'type': 'equal', 'i': i, 'j': j, 'similarity': current_similarity}
65
 
66
                # Look ahead for better alignment
Que pasa
SequenceMatcher devuelve 1.0 al comparar dos cadenas vacias (verificado). Como todas las paginas de un PDF escaneado extraen texto vacio, cualquier par de paginas escaneadas alcanza la similitud maxima.
Impact
Un documento escaneado no produce un error ni un aviso: produce un informe con todas las paginas emparejadas una a una y cero resaltados, es decir, un informe que afirma que no hay cambios. Es el mecanismo concreto detras de la limitacion que docs/requisitos.md declara fuera de alcance, y la razon por la que conviene detectarlo aunque no se implemente OCR.
Por que importa
Los valores por defecto de las funciones de similitud rara vez son los que quieres en los casos degenerados. Comparar vacio con vacio no es 'igual', es 'no comparable', y son dos cosas distintas que el codigo debe poder distinguir.
Sugerencia
Detectar antes de alinear que ambos documentos tienen capa de texto util y, si no la tienen, avisar explicitamente al usuario de que la comparacion no es fiable. Es codigo barato y convierte un fallo silencioso en un mensaje claro.
Tests
Un test con un PDF sin capa de texto que afirme que el sistema avisa en vez de devolver un informe vacio.
MejoravalidacionF9

La CLI valida que el fichero existe, no que sea un PDF legible

Ubicacion: pdf_compare/cli.py:25-31
python
23
    args = parser.parse_args()
24
 
25
    if not os.path.exists(args.file_a):
26
        print(f"Error: File '{args.file_a}' not found.")
27
        sys.exit(1)
28
 
29
    if not os.path.exists(args.file_b):
30
        print(f"Error: File '{args.file_b}' not found.")
31
        sys.exit(1)
32
 
33
    print(f"Comparing '{args.file_a}' and '{args.file_b}'...")
Que pasa
La validacion previa se limita a os.path.exists. Un fichero que no es un PDF, uno corrupto o uno protegido con contrasena pasan el control y revientan mas adelante dentro de PyMuPDF.
Impact
El usuario recibe el mensaje interno de la libreria mas una traza completa por stdout, en lugar de un error accionable. Para el caso protegido con contrasena, ademas, el fallo puede llegar tarde: fitz.open abre el documento y es al extraer texto cuando falla.
Por que importa
La validacion de entrada debe comprobar lo que el programa necesita de verdad (un PDF que se pueda leer), no una condicion cercana pero mas debil (que exista una ruta). El traceback es informacion para ti, no para quien usa la herramienta.
Sugerencia
Intentar abrir ambos documentos en la validacion previa y traducir el fallo a un mensaje claro, distinguiendo al menos 'no es un PDF valido' de 'esta protegido con contrasena' (fitz expone doc.needs_pass).
Tests
Tests de CLI con un fichero de texto renombrado a .pdf y con un PDF cifrado, afirmando el codigo de salida y el mensaje.

Opcional 2

Opcionaldead-codeF10

has_changes se calcula y se tira

Ubicacion: pdf_compare/comparator.py:206-212
python
204
 
205
        # Highlight differences
206
        has_changes = False
207
        for inner_tag, ii1, ii2, jj1, jj2 in matcher.get_opcodes():
208
            if inner_tag == 'equal':
209
                continue
210
 
211
            has_changes = True
212
 
213
            # Highlight deletions (red on left page)
Que pasa
La variable has_changes se inicializa y se pone a True cuando aparece una diferencia, pero no se lee en ningun momento ni se devuelve.
Impact
Ninguno en ejecucion. Es interesante por otra razon: es exactamente el dato que le falta a F2 para poder devolver None cuando no hay diferencias. Alguien empezo a resolverlo y se quedo a medias.
Por que importa
El codigo muerto no es solo ruido: registra una intencion que no se completo, y al proximo lector le cuesta distinguir si sobra o si falta lo que la usaba.
Sugerencia
Aprovecharla al arreglar F2 haciendo que _add_comparison_page la devuelva, o eliminarla.
Tests
Queda cubierto por el test de F2.
OpcionalduplicacionF11

Constantes de maquetacion duplicadas y contradictorias

Ubicacion: pdf_compare/comparator.py:297-306
python
296
 
297
    def _add_label(self, page, text, x, y, width, bg_color=(1, 1, 1)):
298
        """Add a label box at the top of the page area."""
299
        label_height = 30
300
 
301
        # Draw background
302
        page.draw_rect(fitz.Rect(x, y, x + width, y + label_height), color=(0, 0, 0), fill=bg_color, width=1)
Que pasa
margin, gap y label_height se repiten literalmente en _add_comparison_page (166-168) y en _add_single_page (250-252). Ademas label_height vale 40 en ambos y 30 dentro de _add_label, que es quien pinta de verdad la etiqueta.
Impact
Los 10 puntos de diferencia son la razon del hueco entre la etiqueta y la pagina. Puede ser deliberado, pero nada lo dice: el que toque uno de los tres valores para ajustar el diseno descubrira que el resultado no es el que esperaba.
Por que importa
Cuando el mismo numero vive en tres sitios, deja de haber una fuente de verdad y los cambios se aplican a medias. Si los 10 puntos son intencionados, son un margen y merecen su propio nombre.
Sugerencia
Subir margin, gap, label_height y el alto real de la etiqueta a constantes de modulo, con el hueco expresado explicitamente.
Tests
No requiere test.

Lo que esta bien resuelto