From b1fa1b321b876ef16532b4b8d23110bde4d15153 Mon Sep 17 00:00:00 2001 From: AnniekStok Date: Mon, 10 Aug 2026 18:12:18 +0200 Subject: [PATCH 1/3] subtract on a deepcopy, to make sure the action can be fully undone --- src/funtracks/actions/update_segmentation.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/funtracks/actions/update_segmentation.py b/src/funtracks/actions/update_segmentation.py index 4d83a158..6d99e202 100644 --- a/src/funtracks/actions/update_segmentation.py +++ b/src/funtracks/actions/update_segmentation.py @@ -1,5 +1,6 @@ from __future__ import annotations +import copy from typing import TYPE_CHECKING import tracksdata as td @@ -60,8 +61,14 @@ def _apply(self) -> None: mask_new = self.mask if value == 0: - # val=0 means deleting (part of) the mask - mask_old = self.tracks.graph.nodes[self.node][self.mask_key] + # val=0 means deleting (part of) the mask. + # Copy first: Mask.__isub__ mutates in place and returns self, which would + # store the same object identity back on the node. Downstream consumers + # (e.g. the GraphArrayView render cache) only invalidate when a *new* mask + # object is written, so subtracting in place would leave stale pixels in the + # rendered segmentation (visible when undoing a grow). Subtract on a copy so a + # distinct object is stored. + mask_old = copy.deepcopy(self.tracks.graph.nodes[self.node][self.mask_key]) mask_subtracted = mask_old.__isub__(mask_new) self.tracks.update_mask(self.node, mask_subtracted, mask_key=self.mask_key) From b558da93dc4cf4afdbec81c198d93b11d4350837 Mon Sep 17 00:00:00 2001 From: AnniekStok Date: Mon, 10 Aug 2026 18:13:07 +0200 Subject: [PATCH 2/3] add _top_level to UserUpdateSegmentation and UserDeleteNodes --- .../user_actions/user_delete_nodes.py | 9 ++- .../user_actions/user_update_segmentation.py | 11 +++- .../test_user_update_segmentation.py | 57 ++++++++++++++++++- 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/src/funtracks/user_actions/user_delete_nodes.py b/src/funtracks/user_actions/user_delete_nodes.py index 05f30bb2..df478ff7 100644 --- a/src/funtracks/user_actions/user_delete_nodes.py +++ b/src/funtracks/user_actions/user_delete_nodes.py @@ -22,6 +22,9 @@ class UserDeleteNodes(ActionGroup): nodes: The node ids to delete. pixels: Optional list of pixel masks for each node, matching the order of nodes. Defaults to None. + _top_level: If True, add this action to the history and emit the refresh + signal. Set to False when this action is part of a bigger action group, + so that the whole group is undone in one step. Defaults to True. """ def __init__( @@ -29,6 +32,7 @@ def __init__( tracks: SolutionTracks, nodes: list[int], pixels: None | list[tuple[np.ndarray, ...]] = None, + _top_level: bool = True, ): super().__init__(tracks, actions=[]) self.tracks: SolutionTracks # Narrow type from base class @@ -42,5 +46,6 @@ def __init__( ) ) - self.tracks.action_history.add_new_action(self) - self.tracks.refresh.emit() + if _top_level: + self.tracks.action_history.add_new_action(self) + self.tracks.refresh.emit() diff --git a/src/funtracks/user_actions/user_update_segmentation.py b/src/funtracks/user_actions/user_update_segmentation.py index 6a312265..ec398cee 100644 --- a/src/funtracks/user_actions/user_update_segmentation.py +++ b/src/funtracks/user_actions/user_update_segmentation.py @@ -23,6 +23,7 @@ def __init__( updated_pixels: list[tuple[tuple[np.ndarray, ...], int]], current_track_id: int, force: bool = False, + _top_level: bool = True, ): """Assumes that the pixels have already been updated in the project.segmentation NOTE: Re discussion with Kasia: we should have a basic action that updates the @@ -40,9 +41,13 @@ def __init__( the currently selected track id in the viewer. force (bool): Whether to force the operation by removing conflicting edges. Defaults to False. + _top_level (bool): If True, add this action to the history and emit the + refresh signal. Set to False when this action is part of a bigger action + group, so that the whole group is undone in one step. Defaults to True. """ super().__init__(tracks, actions=[]) self.tracks: SolutionTracks # Narrow type from base class + self.node_to_select: int | None = None node_to_select = None if self.tracks.segmentation is None: raise ValueError("Cannot update non-existing segmentation.") @@ -106,5 +111,7 @@ def __init__( self.actions.append( UpdateNodeSeg(tracks, old_value, mask_pixels, added=False) ) - self.tracks.action_history.add_new_action(self) - self.tracks.refresh.emit(node_to_select) + self.node_to_select = node_to_select + if _top_level: + self.tracks.action_history.add_new_action(self) + self.tracks.refresh.emit(node_to_select) diff --git a/tests/user_actions/test_user_update_segmentation.py b/tests/user_actions/test_user_update_segmentation.py index d2d67a12..d2fdef5e 100644 --- a/tests/user_actions/test_user_update_segmentation.py +++ b/tests/user_actions/test_user_update_segmentation.py @@ -3,8 +3,9 @@ import numpy as np import pytest +from funtracks.actions import ActionGroup from funtracks.exceptions import InvalidActionError -from funtracks.user_actions import UserUpdateSegmentation +from funtracks.user_actions import UserDeleteNodes, UserUpdateSegmentation from funtracks.utils.tracksdata_utils import td_mask_to_pixels iou_key = "iou" @@ -289,3 +290,57 @@ def test_missing_seg(get_tracks): tracks = get_tracks(ndim=3, with_seg=False, is_solution=True) with pytest.raises(ValueError, match="Cannot update non-existing segmentation"): UserUpdateSegmentation(tracks, 0, [], 1) + + +@pytest.mark.parametrize("ndim", [3]) +def test_not_top_level_actions_group_into_one_undo(get_tracks, ndim): + """With ``_top_level=False`` the action is applied but not recorded, so a caller can + group several actions into a single, jointly undoable step.""" + + tracks = get_tracks(ndim=ndim, with_seg=True, is_solution=True) + node_id = 3 + orig_pixels = td_mask_to_pixels( + tracks.get_mask(node_id), tracks.get_time(node_id), ndim=tracks.ndim + ) + orig_area = tracks.get_node_attr(node_id, area_key) + n_actions = len(tracks.action_history.undo_stack) + + # remove the pixels in two steps, neither of which lands in the history + first = tuple(orig_pixels[d][1:2] for d in range(len(orig_pixels))) + second = tuple(orig_pixels[d][2:] for d in range(len(orig_pixels))) + actions = [ + UserUpdateSegmentation( + tracks, + new_value=0, + updated_pixels=[(pixels, node_id)], + current_track_id=1, + _top_level=False, + ) + for pixels in (first, second) + ] + + assert tracks.get_node_attr(node_id, area_key) == orig_area - ( + len(first[0]) + len(second[0]) + ) + assert len(tracks.action_history.undo_stack) == n_actions + + # grouped, the two updates are undone together + group = ActionGroup(tracks, actions=actions) + tracks.action_history.add_new_action(group) + assert len(tracks.action_history.undo_stack) == n_actions + 1 + + tracks.undo() + assert tracks.get_node_attr(node_id, area_key) == orig_area + + +@pytest.mark.parametrize("ndim", [3]) +def test_delete_nodes_not_top_level(get_tracks, ndim): + """UserDeleteNodes with ``_top_level=False`` deletes without recording history.""" + + tracks = get_tracks(ndim=ndim, with_seg=True, is_solution=True) + n_actions = len(tracks.action_history.undo_stack) + + UserDeleteNodes(tracks, nodes=[3], _top_level=False) + + assert not tracks.graph.has_node(3) + assert len(tracks.action_history.undo_stack) == n_actions From 3ea1d2fa90b28bcd9cd76ca6cde23e8809b9731f Mon Sep 17 00:00:00 2001 From: AnniekStok Date: Mon, 10 Aug 2026 21:57:02 +0200 Subject: [PATCH 3/3] fix test --- tests/user_actions/test_user_update_segmentation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/user_actions/test_user_update_segmentation.py b/tests/user_actions/test_user_update_segmentation.py index 163cbf56..644b9f44 100644 --- a/tests/user_actions/test_user_update_segmentation.py +++ b/tests/user_actions/test_user_update_segmentation.py @@ -297,7 +297,7 @@ def test_not_top_level_actions_group_into_one_undo(get_tracks, ndim): """With ``_top_level=False`` the action is applied but not recorded, so a caller can group several actions into a single, jointly undoable step.""" - tracks = get_tracks(ndim=ndim, with_seg=True, is_solution=True) + tracks = get_tracks(ndim=ndim, with_seg=True, prefill_track_ids=True) node_id = 3 orig_pixels = td_mask_to_pixels( tracks.get_mask(node_id), tracks.get_time(node_id), ndim=tracks.ndim @@ -337,7 +337,7 @@ def test_not_top_level_actions_group_into_one_undo(get_tracks, ndim): def test_delete_nodes_not_top_level(get_tracks, ndim): """UserDeleteNodes with ``_top_level=False`` deletes without recording history.""" - tracks = get_tracks(ndim=ndim, with_seg=True, is_solution=True) + tracks = get_tracks(ndim=ndim, with_seg=True, prefill_track_ids=True) n_actions = len(tracks.action_history.undo_stack) UserDeleteNodes(tracks, nodes=[3], _top_level=False)