From 0421a4cf1038aafc6b87c9f28cc6e3af493004be Mon Sep 17 00:00:00 2001 From: Tristan Brennan Date: Sun, 16 Aug 2026 09:21:00 +0900 Subject: [PATCH] Fix change signaling when tool session path changes Two defects in the sessionPath setters: - Tool.sessionPath: a missing comma merged 'trackerStlFilepath' and 'sessionPath' into one bogus attrib name via implicit string concatenation, so those attribs were never announced as changed. - Tools.sessionPath: compared the resolved filepathsRelTo path against the literal token '' (never true), so session-relative tools were always excluded from the emitted changingKeys. Use filepathsRelToKey instead, and pass the changed attrib names in the emit as done everywhere else in GenericCollection. The attrib list is hoisted to a single module-level constant so the two setters can't drift apart again. Side effects of the collection-level change worth noting: the old emit (empty keys, implicit attribs=None) made CollectionTableModel classify every session move as a 'full' layout change with a spurious selection-cleared signal, and made ToolsPanel spawn a throwaway ToolPositionsClient; the new emit is classified as 'modifyExisting' and neither side effect occurs. --- NaviNIBS/Navigator/Model/Tools.py | 25 +++++++---- tests/test_Model/test_toolsSessionPath.py | 51 +++++++++++++++++++++++ 2 files changed, 67 insertions(+), 9 deletions(-) create mode 100644 tests/test_Model/test_toolsSessionPath.py diff --git a/NaviNIBS/Navigator/Model/Tools.py b/NaviNIBS/Navigator/Model/Tools.py index ac74161..78a9149 100644 --- a/NaviNIBS/Navigator/Model/Tools.py +++ b/NaviNIBS/Navigator/Model/Tools.py @@ -28,6 +28,17 @@ SurfMesh = pv.PolyData +_sessionPathDependentAttribs = [ + 'romFilepath', + 'toolStlFilepath', + 'trackerStlFilepath', + 'sessionPath', +] +""" +Attribs signaled as changed when a tool's session path changes, since session-relative +filepaths resolve differently afterward. +""" + @attrs.define class Tool(GenericCollectionDictItem[str]): @@ -303,12 +314,7 @@ def sessionPath(self, newPath: tp.Optional[str]): if self._sessionPath == newPath: return - filepathAttribs = [ - 'romFilepath', - 'toolStlFilepath', - 'trackerStlFilepath' - 'sessionPath' - ] + filepathAttribs = _sessionPathDependentAttribs if self._filepathsRelTo == '': self.sigItemAboutToChange.emit(self.key, filepathAttribs) @@ -656,13 +662,14 @@ def sessionPath(self, newPath: tp.Optional[str]): if self._sessionPath == newPath: return - changingKeys = [tool.key for tool in self.values() if tool.filepathsRelTo == ''] - self.sigItemsAboutToChange.emit(changingKeys) + changingKeys = [tool.key for tool in self.values() if tool.filepathsRelToKey == ''] + filepathAttribs = _sessionPathDependentAttribs + self.sigItemsAboutToChange.emit(changingKeys, filepathAttribs) self._sessionPath = newPath with self.sigItemsAboutToChange.blocked(), self.sigItemsChanged.blocked(): for tool in self.values(): tool.sessionPath = self._sessionPath - self.sigItemsChanged.emit(changingKeys) + self.sigItemsChanged.emit(changingKeys, filepathAttribs) def asList(self) -> tp.List[tp.Dict[str, tp.Any]]: toolList = super().asList() diff --git a/tests/test_Model/test_toolsSessionPath.py b/tests/test_Model/test_toolsSessionPath.py new file mode 100644 index 0000000..c7d2f8a --- /dev/null +++ b/tests/test_Model/test_toolsSessionPath.py @@ -0,0 +1,51 @@ +""" +Regression tests for change signaling when a tool or tool collection's +session path changes (e.g. after save-as / session move). +""" + +from NaviNIBS.Navigator.Model.Tools import Tool, Tools + + +def test_toolSessionPathChangeSignalsFilepathAttribs(): + """ + A missing comma previously merged 'trackerStlFilepath' and 'sessionPath' + into one bogus attrib name in the signaled attrib list. + """ + tool = Tool(key='t1', usedFor='visualization', + filepathsRelTo='', sessionPath='/tmp/sesA') + received = [] + tool.sigItemAboutToChange.connect(lambda key, attribs=None: received.append(list(attribs))) + tool.sessionPath = '/tmp/sesB' + + assert received, 'sessionPath change did not signal' + attribs = received[0] + assert 'trackerStlFilepathsessionPath' not in attribs + assert 'trackerStlFilepath' in attribs + assert 'sessionPath' in attribs + + +def test_toolsCollectionSessionPathChangeSignalsSessionRelativeTools(): + """ + The collection-level setter previously compared the resolved + ``filepathsRelTo`` path against the literal token '', so + session-relative tools were never included in the signaled keys. + """ + tools = Tools(sessionPath='/tmp/sesA') + tools.addItem(Tool(key='rel', usedFor='visualization', + filepathsRelTo='', sessionPath='/tmp/sesA')) + tools.addItem(Tool(key='abs', usedFor='visualization', + filepathsRelTo='', sessionPath='/tmp/sesA')) + + receivedAboutTo = [] + receivedChanged = [] + tools.sigItemsAboutToChange.connect( + lambda keys, attribs=None: receivedAboutTo.append((list(keys), attribs))) + tools.sigItemsChanged.connect( + lambda keys, attribs=None: receivedChanged.append((list(keys), attribs))) + + tools.sessionPath = '/tmp/sesB' + + assert receivedAboutTo and receivedChanged + assert receivedAboutTo[0][0] == ['rel'] + assert receivedChanged[0][0] == ['rel'] + assert 'sessionPath' in receivedChanged[0][1]