Skip to content

Fix AttributeError on iconlist when GalleryTab parent dialog closes (bug 13326)#2330

Merged
Nick-Hall merged 3 commits into
gramps-project:maintenance/gramps61from
eduralph:fix/bug-13326-gallerytab-iconlist-cleanup
Jun 16, 2026
Merged

Fix AttributeError on iconlist when GalleryTab parent dialog closes (bug 13326)#2330
Nick-Hall merged 3 commits into
gramps-project:maintenance/gramps61from
eduralph:fix/bug-13326-gallerytab-iconlist-cleanup

Conversation

@eduralph

@eduralph eduralph commented May 24, 2026

Copy link
Copy Markdown

Root cause

GalleryTab.clean_up() calls super().clean_up(), which removes
self.iconlist via track_ref_for_deletion("iconlist"), but never
disconnects the 'selection-changed' signal handler on the iconlist
widget. When the parent dialog tears down on Cancel — the Forms
addon's GalleryTab-in-a-dialog path exercises this reliably — GTK
dispatches a final selection-changed to the still-live widget; the
handler calls self.get_selected()self.iconlist.get_selected_items()
and raises AttributeError: 'GalleryTab' object has no attribute 'iconlist'.

Fix

Capture the 'selection-changed' handler id in build_interface,
then disconnect it at the head of clean_up() — before
super().clean_up() removes the attribute. Guard the disconnect
with GObject.signal_handler_is_connected so the already-disposed
case (the reason the original disconnect was removed in 37395da)
stays silent.

Verified against

  • gramps/gui/editors/displaytabs/gallerytab.py:232,659-660 — the
    unguarded connect and the no-op clean_up that the reported
    trace fingers.
  • gramps/gui/editors/displaytabs/buttontab.py:319 — the call site
    in the inherited _selection_changed callback that reaches into
    self.get_selected().
  • The 2012 disconnect (svn r20849 / 51a53ccebd) — same defect
    class, same fix shape; removed in 37395da262 to silence a GTK
    warning on the normal close path. The new
    signal_handler_is_connected guard keeps that path silent while
    restoring teardown-race safety.

Test

gramps/gui/editors/displaytabs/test/gallerytab_test.py covers both
the bug and the warning the original disconnect was removed to
silence:

  • test_clean_up_disconnects_selection_changed constructs a real
    GalleryTab, records the 'selection-changed' signal id, and
    asserts via GObject.signal_handler_find that no handler remains
    after clean_up(). Pre-fix 42 != 0; post-fix passes.
  • test_clean_up_after_iconlist_destroyed_emits_no_warning destroys
    the iconlist widget before clean_up() — the normal close path
    where commit 37395da was seeing the GTK critical — and asserts
    via warnings.catch_warnings that no instance '...' has no handler with id '...' warning is recorded. Mutating the fix to
    drop the signal_handler_is_connected guard surfaces exactly that
    warning, so future removals of the guard regress here.

Both need a display (Gtk.IconView), so run under xvfb-run:

xvfb-run -a python3 -m unittest \
    gramps.gui.editors.displaytabs.test.gallerytab_test -v

Fixes #13326.

@eduralph eduralph changed the title Fix AttributeError on iconlist when GalleryTab parent dialog closes Fix AttributeError on iconlist when GalleryTab parent dialog closes (bug 13326) May 24, 2026
@eduralph eduralph marked this pull request as ready for review May 24, 2026 12:40
@romjeromealt

Copy link
Copy Markdown
Contributor

Is it also related to bug report #12289?

@eduralph

Copy link
Copy Markdown
Author

Thanks for the pointer to 0012289 — it's relevant to understand the story behind this one.

The key distinction is severity and path. 12289 was a cosmetic GObject warning (instance ... has no handler with id ...) on the normal Person Editor close. 13326 is different in kind: an unhandled AttributeError crash on a specific sequence — add an image to the gallery, then Cancel the dialog — where a selection-changed callback fires after iconlist has been torn down.

This resolves the crash without re-touching the part of the cleanup behavior that the earlier reports were wary about. Happy to adjust the approach if there's a side effect I've missed — the guard is specifically there to respect the original concern,

@romjeromealt

romjeromealt commented May 26, 2026

Copy link
Copy Markdown
Contributor

Do not need to worry too much, I often make relations, this does not mean something should be improved. I am fine with this PR. As far as I remember the root of 12289 was pointed during a tracking down session around memory leaks. So, even cosmetic GalleryTab or some gramplets, generated some minor memory issues. Now, with the help of AI, we can just provide something more "professional[1]", quick and maybe more efficient. That's all.
[1] my workflow was to run Gramps 5.1.3 with an OS using less than 512Mo of ram... and I closed 12289 as "wont't fix" because today, such config would not run Gramps 6 and later.

@Nick-Hall Nick-Hall added Status: Pending A pull request that is pending review. and removed Status: New labels Jun 8, 2026
@Nick-Hall Nick-Hall added Status: Approved A pull request that has been approved, but may need further testing. and removed Status: Pending A pull request that is pending review. labels Jun 15, 2026
@Nick-Hall Nick-Hall force-pushed the fix/bug-13326-gallerytab-iconlist-cleanup branch from 06add58 to 2f48559 Compare June 16, 2026 21:29
eduralph added 3 commits June 16, 2026 22:32
GalleryTab.clean_up() did not disconnect the iconlist
'selection-changed' signal handler before super().clean_up() deleted
the iconlist attribute via track_ref_for_deletion. A late
selection-changed emission - which fires when the parent dialog tears
down on Cancel, e.g. when the Forms addon hosts a GalleryTab - then
re-entered _selection_changed -> get_selected() ->
self.iconlist.get_selected_items() and crashed with AttributeError:
'GalleryTab' object has no attribute 'iconlist'.

The original 2012 disconnect (r20849) was removed in 2023 to silence a
GTK warning when the iconlist had already been disposed. Restore the
disconnect, guarded by GObject.signal_handler_is_connected so it stays
silent in the dispose-first case.

Fixes #13326.
The disconnect restored for bug 13326 is gated by
GObject.signal_handler_is_connected so that the case commit
37395da was working around (iconlist already disposed by GTK
before clean_up) stays silent. Add a second test case that destroys
the iconlist widget before clean_up and asserts no Python warning of
the shape "instance '...' has no handler with id '...'" is recorded.
Pre-fix the unguarded disconnect emits exactly that warning;
post-fix the test passes.

Issue #13326.
gramps-ci.yml runs the unit-test suite without Xvfb and exports
GDK_BACKEND=-. Constructing a Gtk.IconView against a NULL screen
"succeeds" but segfaults when those orphan widgets are garbage
collected at process exit, taking the whole suite down with exit
code 139.

Detect the no-display case via DISPLAY / GDK_BACKEND / Gtk.init_check
at module import and skip the entire TestCase class cleanly. The
two regression tests still run under xvfb-run or on a real desktop.

Issue #13326.
@Nick-Hall Nick-Hall force-pushed the fix/bug-13326-gallerytab-iconlist-cleanup branch from 2f48559 to cba8c69 Compare June 16, 2026 21:34
@Nick-Hall

Copy link
Copy Markdown
Member

Added 2 files to POTFILES.skip.

We probably should run our CI unit test suite using Xvfb, but that can be done in a separate PR.

@Nick-Hall Nick-Hall merged commit bc41df6 into gramps-project:maintenance/gramps61 Jun 16, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Approved A pull request that has been approved, but may need further testing. Type: Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants