Fix crash when deleting Points layer immediately after loading#226
Open
C-Achard wants to merge 20 commits into
Open
Fix crash when deleting Points layer immediately after loading#226C-Achard wants to merge 20 commits into
C-Achard wants to merge 20 commits into
Conversation
Add tools/launch_napari_debug.py: a CLI utility to launch napari with aggressive crash and event logging for debugging napari/napari-deeplabcut issues. It sets diagnostic environment variables before importing Qt/vispy, tees stdout/stderr to a timestamped log file, enables faulthandler with periodic stack dumps, and configures structured logging and a Qt message handler. The script can optionally trace viewer layer/selection events, open provided paths, and supports flags for log directory, software GL, fatal Qt warnings, and disabling event tracing.
Introduce active_plottable_traj_layer to return the currently active plottable trajectory Points layer (or None) without falling back to another managed layer. This avoids accidentally resurrecting a layer during deletion when napari temporarily sets the active layer to None. The method checks viewer.layers.selection.active and validates it with is_plottable_traj_layer.
Add robust synchronization of the currently active layer after a points layer is added. Introduces _sync_current_active_layer_state to call on_active_layer_change using types.SimpleNamespace when loading a layer doesn't trigger a selection change; errors are caught and logged. Also call this sync after completing points layer UI setup. Minor fixes: pass allow_fallback=True to traj_canvas.refresh_from_viewer_layers to avoid refresh errors, and use a local `layer` variable in on_active_layer_change for clarity.
Add OwnedTimersMixin and use scheduled owned timers instead of QTimer.singleShot; track the current plotted Points layer. Introduce allow_fallback flags to refresh/load routines and _get_plot_points_layer to avoid falling back to other layers during viewer layer removal/selection events. Tighten selection sync so it only reads selected_data from the active plottable layer that matches the rendered plot. Also remove unused QTimer import.
Assign the local points_layer to self._plot_layer when building the trajectory plot. This preserves a reference to the layer alongside _plot_state so the canvas can access the points layer for subsequent updates or queries; the assignment is added just before the existing debug log.
Adjust tests to match updated plotting API: pass allow_fallback=True to canvas._load_dataframe, adapt _get_plot_points_layer lambdas to accept the allow_fallback kwarg, and set canvas._plot_layer in UI tests to ensure the test layer is used. Also add a file header comment and minor formatting tweaks.
Introduce unit tests for TrajectoryMatplotlibCanvas behavior. Adds src/napari_deeplabcut/_tests/ui/test_traj_plot.py with a DummyTrajectoryLayerManager and tests covering _get_plot_points_layer strict vs fallback behavior, validation of suggested layers not present in the viewer, refresh_from_viewer_layers behavior (ensuring no fallback in strict mode and allowing fallback for explicit initialization), and selected line key resolution. Ensures deterministic tests by preventing the canvas' initial deferred refresh.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a hard crash that occurs when deleting a managed Points layer immediately after loading by tightening how the trajectory plot resolves its source layer during event-driven refreshes, and by making selection-sync logic more deletion-safe.
Changes:
- Split “strict” vs “fallback” trajectory-layer resolution to avoid rebuilding the plot from a layer mid-deletion.
- Replaced
QTimer.singleShot(...)usage in the trajectory widget withOwnedTimersMixin-managed timers and improved selection-sync correctness. - Added/updated tests to cover strict vs fallback behavior and selection-sync edge cases, plus a new debug launcher tool for crash diagnostics.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/launch_napari_debug.py | Adds a standalone diagnostic launcher that enables aggressive logging, faulthandler output, and napari layer/selection event tracing. |
| src/napari_deeplabcut/ui/plots/trajectory.py | Makes trajectory refresh logic deletion-safe (strict layer resolution for event-driven refreshes), adds OwnedTimersMixin, and prevents selection sync from reading the wrong layer. |
| src/napari_deeplabcut/ui/base_widget/init.py | Exposes OwnedTimersMixin via the base_widget package. |
| src/napari_deeplabcut/core/layer_lifecycle/manager.py | Adds active_plottable_traj_layer() helper for strict active-layer resolution (no fallback). |
| src/napari_deeplabcut/_widgets.py | Syncs UI state after points-layer setup and hardens active-layer handling. |
| src/napari_deeplabcut/_tests/ui/test_traj_select.py | Updates tests to align with strict layer resolution and new _plot_layer behavior. |
| src/napari_deeplabcut/_tests/ui/test_traj_plot.py | Adds new tests covering strict vs fallback resolution and refresh behavior. |
| src/napari_deeplabcut/_tests/test_widgets.py | Updates trajectory canvas test to call _load_dataframe with explicit fallback semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Add an extra membership check so the manager only returns the active trajectory-capable layer if it is actually present in viewer.layers. This prevents returning a layer object that was removed from the viewer, avoiding potential downstream errors when operating on stale layers.
Add configurable trajectory window defaults and robust windowing logic for the trajectory plot. - Add _MIN_TRAJ_PLOT_WINDOW and _DEFAULT_TRAJ_PLOT_WINDOW settings and use them to initialize the range slider. - Implement helpers (_clamp, _small_span_padding, _effective_window) to compute a safe, readable display window and padding for very small frame spans. - Replace previous ad-hoc start/end calculations with logic that handles inverted/zero spans, clamps the window inside available frames, and avoids visually collapsed axes for tiny videos. - Update the vertical marker to be plotted as a two-point line and refresh canvas when slider bounds change. - Improve _update_slider_max to prefer plot state frame bounds (falling back to the video layer), compute a sensible max window, block slider signals while updating, adjust tick interval, and refresh the canvas as needed. These changes prevent collapsed limits and keep the slider and plotted window consistent for short or irregular frame ranges.
Keep the trajectory window slider stable and backward-compatible by no longer shrinking its maximum to the plotted/video span. _update_slider_max now sets a large fixed maximum (10000), enforces the minimum window, preserves the current slider value (clamped to the minimum if needed), and uses a fixed tick interval (50). The test was adjusted to pick a target value inside the slider's range (or a fallback inside the range) before setting the slider, avoiding brittle behavior when the initial value is already at the maximum. This preserves the requested window semantics while actual displayed ranges remain clamped elsewhere (_refresh_canvas()).
This reverts commit 7ebfeae.
Replace a brittle hard-coded slider increment with range-aware checks in the matplotlib canvas test. The test now reads slider.minimum/maximum/value, verifies the current value is within the range, picks a valid target (min or max) and sets the slider to that target, then asserts slider.value(), canvas._window and slider_value text match. Removed the previous out-of-range increment and the separate hidden-plot/update_plot_range/x-limits assertions to avoid flaky failures and ensure the slider update behavior is reliably tested.
deruyter92
approved these changes
Jun 24, 2026
deruyter92
left a comment
deruyter92
left a comment
Collaborator
There was a problem hiding this comment.
Good fix! No major comments.
Pass allow_fallback=True to _traj_mpl_canvas.refresh_from_viewer_layers when docking the trajectory Matplotlib canvas in KeypointControls. This allows the refresh routine to use fallback behavior if viewer layers aren't ready, reducing refresh failures and improving robustness when the canvas is docked.
Replace direct access to viewer.layers.selection.active and manual is_plottable_traj_layer check with layer_manager.active_plottable_traj_layer(). This centralizes the logic for obtaining the currently active plottable trajectory layer and avoids duplicating validation code while preserving the fallback behavior.
When no points layer is available, _selected_line_keys_from_points_layer() now returns None instead of an empty set. The caller now checks for None and returns early, preventing accidental fallback behavior (like showing all keypoints) when there is no points layer to inspect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Crash description
This produced a hard crash but did not occur if any other layer was selected prior to deletion.
Cause
During layer deletion, selection can transiently become empty while the layer being removed is still present in viewer.layers:
The trajectory plot reacted to selection/removal events by using a fallback layer selection intended for better UX (by selecting another available layer if any).
In that transient state, fallback could select the managed Points layer currently being deleted, causing the plot to rebuild from a layer mid-deletion. This triggered a hard crash downstream while the remaining Image layer was redrawn.
Fix
Misc
Added tools/launch_napari_debug.py, a diagnostic launcher that enables faulthandler, Qt/VisPy logging, and layer/selection event tracing for future native crash debugging. This actually shows all errors when hard crashing instead of only a minimal report of the very last failure.