Skip to content

Root cause: guarded the symptom (has_display) instead of removing the import-time cause (PR 2357) #15

@eduralph

Description

@eduralph

PR gramps-project/gramps#2357 (the results/issue_headless-ut-segfault bundle's fix) drew a maintainer objection from Nick-Hall on grampletpane.py:228:

"Using the function has_display in GUI code that is designed to run with a display just looks wrong."

Technical root cause (the bug)

LinkTag in gramps/gui/widgets/grampletpane.py computes the theme link colour in its class bodyGtk.Label(...) + get_style_context() run at import time. Headless, that aborts the interpreter (Gtk-ERROR: Can't create a GtkStyleContext without a display connection → SIGTRAP), taking down the whole core unit-test discovery.

Process root cause (why the cycle shipped the weaker fix)

The brief scoped the fix as "minimal, reuse the existing has_display()," and the build-notes explicitly considered and rejected the lazy-compute alternative as "heavier." That self-assessment was wrong: the structural fix — compute linkcolor lazily on first LinkTag() construction (which only ever happens under a display) — is comparably small and removes the import-time cause entirely, needing no display probe at all. The cycle optimized for "fewest lines using an existing helper" over "remove the cause," and so guarded the symptom rather than eliminating it.

Lesson (Act candidate)

  • Prefer removing the import-time cause over guarding the symptom. A has_display() (or any capability probe) sitting inside code meant to run with that capability is a smell — a maintainer reads it as papering over the real issue.
  • Don't let a brief pre-commit to a guard approach when a structural fix is comparably small. The Plan/Do "minimal" heuristic must weigh "removes the cause" above "fewest lines / reuse a helper."
  • A draft PR catching this at maintainer review is the feedback loop working — but it costs a rework round-trip; the brief-quality bar should weigh "symptom-guard vs cause-removal" before Do, not at upstream review.

Resolution

Rework to lazy-compute linkcolor in __init__ (cached on the class) and drop the has_display import; the headless-import regression test stays valid (and stronger — nothing display-dependent runs at import). PR 2357 updated accordingly.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions