From 3c1f99b95a24d15c6e3d9524e384470540c77f4f Mon Sep 17 00:00:00 2001 From: Dusten Hubbard Date: Fri, 31 Jul 2026 08:13:03 -0500 Subject: [PATCH] fix(gui): edit or delete part of a tag without losing the rest Correcting a tag meant deleting it and typing it again. Reproducing that turned up a separate reason on each surface that edits tags: - `MultiInput.remove` popped the last row wherever the caret was, so fixing the first of several tags deleted every row below it. It now removes the row being edited, and keeps one row alive by clearing it rather than leaving a field with nothing to type into. - The tag rows and the palette's Tags cell were built straight from a set, so a tag moved between openings and a part-finished edit resumed on the wrong row. Both are sorted now. - The palette's Tags cell was parsed with `text.split(", ")`, which turns the "axon, " left behind by deleting "spine" into two tags, the second empty, and turns an untagged trace's empty cell into a single empty tag. `parseTags` splits on the comma, strips, and drops empties. - `Series.editObjectAttributes` passed `add_tags=True` to `Section.editTraceAttributes`, which iterates the incoming set and adds each element, so an edited set could add a tag but never drop one. It takes `add_tags` now, defaulting to True so no other caller changes, and the object list passes False when the dialog was actually showing that object's tags. Adds a tests/ directory, which the repo did not have. Closes #119 --- PyReconstruct/modules/datatypes/series.py | 12 +- PyReconstruct/modules/gui/dialog/helper.py | 60 +- PyReconstruct/modules/gui/dialog/trace.py | 5 +- .../modules/gui/dialog/trace_palette.py | 25 +- .../modules/gui/main/field_widget_3_object.py | 13 +- pyproject.toml | 13 + tests/conftest.py | 90 +++ tests/test_settings_isolation.py | 34 + tests/test_tag_editing.py | 608 ++++++++++++++++++ 9 files changed, 849 insertions(+), 11 deletions(-) create mode 100644 tests/conftest.py create mode 100644 tests/test_settings_isolation.py create mode 100644 tests/test_tag_editing.py diff --git a/PyReconstruct/modules/datatypes/series.py b/PyReconstruct/modules/datatypes/series.py index 0985b4d7..e5a6bd7b 100644 --- a/PyReconstruct/modules/datatypes/series.py +++ b/PyReconstruct/modules/datatypes/series.py @@ -1209,6 +1209,7 @@ def editObjectAttributes( mode : tuple = None, sections : list = None, series_states=None, + add_tags : bool = True, log_event=True): """Edit the attributes of objects. @@ -1216,10 +1217,17 @@ def editObjectAttributes( obj_names (list): the names of the objects to rename name (str): the new name for the objects color (tuple): the new color for the objects - tags (set): the tags to ADD to the traces of the objects + tags (set): the tags for the traces of the objects mode (tuple): the display mode to set for the traces section (list): the section numbers to modify the object on (default: all) series_states: the series states as store in the GUI + add_tags (bool): True if tags should be added to each trace's + existing tags, False if they should REPLACE them. Only a + replacement can remove a tag, so a caller that shows the user + the current tags and takes an edited set back must pass False. + Additive is the default because a caller working on a + selection whose tags it never displayed cannot ask for a + replacement without discarding tags the user never saw. log_event (bool): True if event should be logged """ ## Preemptively create log @@ -1267,7 +1275,7 @@ def editObjectAttributes( if traces: section.editTraceAttributes( - traces, name, color, tags, mode, add_tags=True, log_event=False + traces, name, color, tags, mode, add_tags=add_tags, log_event=False ) ## Gather new traces diff --git a/PyReconstruct/modules/gui/dialog/helper.py b/PyReconstruct/modules/gui/dialog/helper.py index b4cc02f1..9a65aaaa 100644 --- a/PyReconstruct/modules/gui/dialog/helper.py +++ b/PyReconstruct/modules/gui/dialog/helper.py @@ -14,6 +14,7 @@ QPainter, QPalette, ) +from PySide6.QtCore import Qt from .file_dialog import FileDialog @@ -97,12 +98,18 @@ def __init__(self, parent : QWidget, entries : list = None, combo=False, combo_i vbl.addLayout(self.input_layout) # create the add/remove buttons + # + # Neither button takes focus: "-" removes the row being edited, and it + # can only know which row that is if pressing the button leaves the + # caret where the user put it. ar_row = QHBoxLayout() ar_row.addStretch(10) remove = QPushButton(self, text="-") + remove.setFocusPolicy(Qt.NoFocus) remove.clicked.connect(self.remove) ar_row.addWidget(remove) add = QPushButton(self, text="+") + add.setFocusPolicy(Qt.NoFocus) add.clicked.connect(self.add) ar_row.addWidget(add) vbl.addLayout(ar_row) @@ -118,12 +125,55 @@ def add(self): self.input_layout.addWidget(w) self.inputs.append(w) + def currentIndex(self): + """Index of the row being edited, or the last row if none has focus. + + Returns: + (int) an index into self.inputs, -1 if the field has no rows + """ + # an editable combobox is focused through its own internal line edit, + # so walk up from the focused widget until the row itself is found + w = QApplication.focusWidget() + while w is not None: + if w in self.inputs: + return self.inputs.index(w) + w = w.parentWidget() + return len(self.inputs) - 1 + def remove(self): - """Remove a line edit row from the field.""" - if self.inputs: - self.inputs.pop().deleteLater() - self.container.adjustSize() - + """Remove the row being edited (the last row if none has focus). + + "-" popped the final row wherever the caret was, so correcting the first + of several entries meant deleting every row below it and typing them + again. The field also keeps one row alive: removing the only row clears + its text instead, since a field with no line edit in it cannot be typed + into at all. + """ + if not self.inputs: + return + + if len(self.inputs) == 1: + if self.is_combo: + self.inputs[0].setCurrentText("") + else: + self.inputs[0].setText("") + return + + w = self.inputs.pop(self.currentIndex()) + self.input_layout.removeWidget(w) + w.deleteLater() + + # removeWidget() only marks the layouts dirty and posts a layout + # request, which is delivered after this slot returns, so both size + # hints still describe the layout with the removed row in it. Activating + # them makes the hints current, otherwise adjustSize() resizes to the + # previous row count and leaves a band of empty space in the dialog. + self.layout().activate() + container_layout = self.container.layout() + if container_layout is not None: + container_layout.activate() + self.container.adjustSize() + def getEntries(self): """Get the strings input by the user.""" l = [] diff --git a/PyReconstruct/modules/gui/dialog/trace.py b/PyReconstruct/modules/gui/dialog/trace.py index e87f95a9..b5158236 100644 --- a/PyReconstruct/modules/gui/dialog/trace.py +++ b/PyReconstruct/modules/gui/dialog/trace.py @@ -110,7 +110,10 @@ def __init__( shape_row.addStretch() tags_text = QLabel(self, text="Tags:") - self.tags_input = MultiInput(self, tags) + # sorted because trace.tags is a set: unsorted, a tag lands on a + # different row every time the dialog opens, so the row a user is part + # way through editing is not the row they left off on + self.tags_input = MultiInput(self, sorted(tags)) self.selected_input = QCheckBox("Fill when selected") if fill_condition in ("selected", "always"): diff --git a/PyReconstruct/modules/gui/dialog/trace_palette.py b/PyReconstruct/modules/gui/dialog/trace_palette.py index 1badf2f2..5f742514 100644 --- a/PyReconstruct/modules/gui/dialog/trace_palette.py +++ b/PyReconstruct/modules/gui/dialog/trace_palette.py @@ -12,6 +12,25 @@ from .quick_dialog import QuickTabDialog, getLayout + +def parseTags(text : str): + """Split the text of a Tags cell into a list of tags. + + The cell is edited in place, so deleting part of it routinely leaves a stray + separator behind: taking "spine" out of "axon, spine" leaves "axon, ", and an + untagged trace starts out as "". Splitting on ", " alone turned both of those + into an empty tag, because "".split(", ") is [""], and that empty tag was + then written into the palette and offered in the tag filters. Split on the + comma itself, strip each piece, and drop whatever is left empty. + + Params: + text (str): the contents of the Tags cell + Returns: + (list) the tags, in the order they were entered + """ + return [tag.strip() for tag in text.split(",") if tag.strip()] + + class TracePaletteDialog(QuickTabDialog): def __init__(self, parent, series : Series): @@ -62,7 +81,9 @@ def getStructure(self, trace_list : list): (True, "text", t.name), (True, "color", t.color), (True, "shape", shape), - ("text", ", ".join(t.tags)), + # sorted because t.tags is a set: the cell would otherwise list + # the same tags in a different order every time it is opened + ("text", ", ".join(sorted(t.tags))), (True, "combo", ["none", "transparent", "solid"], t.fill_mode[0]), (True, "combo", ["none", "selected", "unselected", "always"], t.fill_mode[1]), (True, "float", round(t.getRadius(), 7)) @@ -179,7 +200,7 @@ def exec(self): x = [p[0] for p in shape] y = [p[1] for p in shape] - tags = tags.split(", ") + tags = parseTags(tags) t = Trace.fromList([ name, x, y, color, True, False, False, diff --git a/PyReconstruct/modules/gui/main/field_widget_3_object.py b/PyReconstruct/modules/gui/main/field_widget_3_object.py index 8b672c97..a34c9e0e 100644 --- a/PyReconstruct/modules/gui/main/field_widget_3_object.py +++ b/PyReconstruct/modules/gui/main/field_widget_3_object.py @@ -123,12 +123,22 @@ def editAttributes(self, obj_names : list): """Edit the name of object(s) in the entire series.""" ## Query user for new object name + ## + ## tags_displayed records whether the dialog is showing this selection's + ## real tags. It decides, further down, whether the set coming back is a + ## replacement or an addition: a set that was displayed and edited is the + ## user's intended final list, and is the only way a tag can be removed, + ## while a field that started blank for lack of a single value to show + ## cannot express a replacement without discarding tags the user never + ## saw. if len(obj_names) == 1: displayed_name = obj_names[0] tags = self.series.data.getTags(obj_names[0]) + tags_displayed = True else: displayed_name = None tags=None + tags_displayed = False response, confirmed = TraceDialog( self, @@ -156,7 +166,8 @@ def editAttributes(self, obj_names : list): tags, mode, sections, - series_states=self.series_states + series_states=self.series_states, + add_tags=not tags_displayed, ) ## Decorator will not know to update new name and host trees if name is changed diff --git a/pyproject.toml b/pyproject.toml index 53a40f8c..630538fa 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -43,12 +43,25 @@ dependencies = [ "packaging", ] +# Test-only, not installed for end users. Qt is already a runtime dependency, +# so the widget tests need nothing beyond pytest itself. +[project.optional-dependencies] +test = [ + "pytest>=8", +] + [project.urls] Homepage = "https://github.com/SynapseWeb/PyReconstruct" [project.scripts] PyReconstruct = "PyReconstruct.cli:main" +# testpaths keeps collection inside tests/. Without it pytest also picks up +# packaging/smoke_test.py, which is a launcher check for a frozen build rather +# than a unit test and crashes when imported into a test session. +[tool.pytest.ini_options] +testpaths = ["tests"] + [tool.setuptools.packages.find] include = ["PyReconstruct*"] diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 00000000..2d5a224b --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,90 @@ +"""Shared setup for the test suite. + +Two things have to happen before any test module imports the application: + +* Qt has to run without a display, so the widget tests work over ssh and in CI. + +* QSettings has to be redirected. The application reads and writes real user + settings through ``QSettings("KHLab", "PyReconstruct")``, and + ``Series.getOption`` writes a default back whenever a key is missing, so a + test that touched an option would edit the settings of whoever ran it. Every + call site constructs ``QSettings(organization, application)``, which resolves + to the native backend: a plist through ``cfprefsd`` on macOS, the registry on + Windows. Neither ``setPath`` nor ``setDefaultFormat`` moves that (``setPath`` + documents no effect on the native backends, and a native default is what the + two-argument constructor resolves to regardless), and redirecting ``$HOME`` + does not either, because ``cfprefsd`` resolves the real user's domain. What + does work is binding the name the application imports to a subclass that + hands every instance an explicit INI file under a temporary directory. +""" + +import os +import tempfile + +os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") + +import pytest + +import PySide6.QtCore + +_RealQSettings = PySide6.QtCore.QSettings +_SETTINGS_DIR = tempfile.mkdtemp(prefix="pyrecon-test-settings-") + + +class _TempQSettings(_RealQSettings): + """QSettings backed by a throwaway INI file, keyed by organization/app.""" + + def __init__(self, *args, **kwargs): + organization = args[0] if args else "test" + application = args[1] if len(args) > 1 else "test" + super().__init__( + os.path.join(_SETTINGS_DIR, f"{organization}.{application}.ini"), + _RealQSettings.Format.IniFormat, + ) + + +PySide6.QtCore.QSettings = _TempQSettings + +# for the test that guards the redirect +SETTINGS_DIR = _SETTINGS_DIR +REAL_QSETTINGS = _RealQSettings + + +@pytest.fixture(scope="session") +def qapp(): + """The one QApplication the widget tests share.""" + from PySide6.QtWidgets import QApplication + + app = QApplication.instance() or QApplication(["pytest"]) + yield app + + +@pytest.fixture +def real_series(qapp, tmp_path): + """A real series opened from the checker fixture, on a copy. + + Nothing is stubbed: the palette, the sections and the object data are the + ones the application would load. + """ + import shutil + + from PyReconstruct.modules.datatypes.series import Series + from PyReconstruct.modules.datatypes.series_data import SeriesData + + src = os.path.join( + os.path.dirname(__file__), "..", "PyReconstruct", "assets", + "checker", "files", "shapes1.jser", + ) + assert os.path.exists(src), f"missing test fixture: {src}" + + fp = str(tmp_path / "shapes1.jser") + shutil.copyfile(src, fp) + + series = Series.openJser(fp) + data = SeriesData(series) + data.refresh() + series.data = data + + yield series + + series.close() diff --git a/tests/test_settings_isolation.py b/tests/test_settings_isolation.py new file mode 100644 index 00000000..947ff786 --- /dev/null +++ b/tests/test_settings_isolation.py @@ -0,0 +1,34 @@ +"""Guards the QSettings redirect that conftest.py installs. + +A settings write that escaped the redirect would land in the real user domain +and stay there, and nothing else in a passing run would say so. This asserts the +seam rather than trusting it. +""" + +from PySide6.QtCore import QSettings + +import conftest + + +def test_application_settings_go_to_a_temporary_file(): + settings = QSettings("KHLab", "PyReconstruct") + + assert settings.format() == conftest.REAL_QSETTINGS.Format.IniFormat + assert settings.fileName().startswith(conftest.SETTINGS_DIR) + + +def test_per_series_settings_go_to_a_temporary_file(): + """Series options use their own ``PyReconstruct-`` organization.""" + settings = QSettings("KHLab", "PyReconstruct-ABCD") + + assert settings.fileName().startswith(conftest.SETTINGS_DIR) + + +def test_the_modules_that_read_settings_see_the_redirect(): + """The application binds the name at import time, so the patch has to be in + place before the first import, not just before the first call.""" + from PyReconstruct.modules.constants import getdatetime + from PyReconstruct.modules.datatypes import series + + assert getdatetime.QSettings is conftest._TempQSettings + assert series.QSettings is conftest._TempQSettings diff --git a/tests/test_tag_editing.py b/tests/test_tag_editing.py new file mode 100644 index 00000000..39fbafb4 --- /dev/null +++ b/tests/test_tag_editing.py @@ -0,0 +1,608 @@ +"""Tests for editing part of an existing tag. + +Tags are edited on three surfaces and each had its own reason a partial edit did +not survive: + +* ``Trace ▸ Edit trace attributes...`` gives every tag its own line edit + (``MultiInput``), but "-" popped the last row wherever the caret was, so + correcting the first of several tags meant deleting the rows after it and + typing them again. The rows also came straight out of a set, so a tag moved + between openings. +* The trace palette's Tags column is one comma-separated cell. It was parsed + with ``text.split(", ")``, which turns the "axon, " left behind by deleting + "spine" into two tags, the second of them empty, and turns an untagged trace's + empty cell into a single empty tag. +* The object list's ``Edit attributes...`` prefills the Tags field and then adds + the set it gets back instead of assigning it, so an edited set can add a tag + but never drop one. + +None of this covers the click itself. What it covers is what the widgets and the +series do when driven through their real slots and methods. +""" + +import pytest + +from PyReconstruct.modules.datatypes import Trace +from PyReconstruct.modules.gui.dialog import trace as trace_dialog_module +from PyReconstruct.modules.gui.dialog.helper import MultiInput +from PyReconstruct.modules.gui.dialog.quick_dialog import QuickTabDialog +from PyReconstruct.modules.gui.dialog.trace import TraceDialog +from PyReconstruct.modules.gui.dialog.trace_palette import ( + TracePaletteDialog, + parseTags, +) + + +def _trace(tags): + """A minimal closed trace carrying the given tags.""" + t = Trace("axon", (255, 0, 0)) + t.points = [(0, 0), (1, 0), (1, 1)] + t.tags = set(tags) + return t + + +def _shown(widget): + """Show and activate a widget so that setFocus() actually takes effect. + + Offscreen Qt hands out no focus at all until a window is active, so a test + that drives focus has to ask for it explicitly. + """ + from PySide6.QtWidgets import QApplication + + widget.show() + widget.activateWindow() + QApplication.instance().processEvents() + return widget + + +# --- parseTags (pure) ------------------------------------------------------- + + +@pytest.mark.parametrize("text, expected", [ + ("", []), # an untagged trace's cell + (" ", []), + ("axon", ["axon"]), + ("axon, spine", ["axon", "spine"]), + ("axon, ", ["axon"]), # "spine" deleted off the end + (", spine", ["spine"]), # "axon" deleted off the front + ("axon,,spine", ["axon", "spine"]), # a tag deleted from the middle + ("axon,spine", ["axon", "spine"]), # separator typed without a space + (" axon , spine ", ["axon", "spine"]), +]) +def test_parse_tags(text, expected): + assert parseTags(text) == expected + + +# --- the palette's Tags column --------------------------------------------- + + +@pytest.fixture +def palette_dialog(qapp, real_series, monkeypatch): + """A real TracePaletteDialog whose exec() does not block. + + ``TracePaletteDialog.exec`` calls up to ``QuickDialog.exec``, which spins a + modal loop that offscreen Qt has nobody to dismiss. Replacing the inherited + exec with the responses the widgets already produced leaves the part under + test, the palette write-back, running for real. + """ + from PySide6.QtWidgets import QWidget + + monkeypatch.setattr( + QuickTabDialog, "exec", lambda self: (self.responses, True) + ) + parent = QWidget() + dialog = TracePaletteDialog(parent, real_series) + yield dialog + dialog.deleteLater() + parent.deleteLater() + + +def _tags_field(dialog, palette_name, row): + """The Tags line edit for one palette row (7 fields per trace).""" + return dialog.inputs[palette_name][row * 7 + 3].widget + + +def _all_palette_tags(series): + return [ + t.tags + for traces in series.palette_traces.values() + for t in traces + ] + + +def test_palette_untagged_trace_stays_untagged(palette_dialog, real_series): + # every palette trace in the fixture series starts untagged, so accepting + # the dialog untouched used to give every one of them a single empty tag + assert all(tags == set() for tags in _all_palette_tags(real_series)) + + palette_dialog.accept() + palette_dialog.exec() + + assert all(tags == set() for tags in _all_palette_tags(real_series)) + + +def test_palette_partial_delete_leaves_the_remaining_tag( + palette_dialog, real_series +): + name = next(iter(palette_dialog.inputs)) + _tags_field(palette_dialog, name, 0).setText("axon, spine") + _tags_field(palette_dialog, name, 1).setText("axon, ") + + palette_dialog.accept() + palette_dialog.exec() + + traces = real_series.palette_traces[name] + assert traces[0].tags == {"axon", "spine"} + assert traces[1].tags == {"axon"} + assert not any("" in tags for tags in _all_palette_tags(real_series)) + + +def test_palette_tags_cell_is_ordered(): + """The cell text is sorted, so a tag does not move between openings.""" + structure = TracePaletteDialog.getStructure( + None, [_trace({"zeta", "alpha", "mu"})] + ) + assert structure[1][3] == ("text", "alpha, mu, zeta") + + +# --- MultiInput, the trace attributes dialog's tag rows -------------------- + + +def test_remove_takes_the_row_being_edited(qapp): + from PySide6.QtWidgets import QVBoxLayout, QWidget + + host = QWidget() + field = MultiInput(host, ["axon", "spine", "dendrite"]) + layout = QVBoxLayout() + layout.addWidget(field) + host.setLayout(layout) + _shown(host) + + field.inputs[0].setFocus() + qapp.processEvents() + assert field.currentIndex() == 0 + + field.remove() + assert field.getEntries() == ["spine", "dendrite"] + + host.deleteLater() + + +def test_remove_falls_back_to_the_last_row(qapp): + """With the caret nowhere in the field, "-" keeps its old meaning.""" + from PySide6.QtWidgets import QWidget + + host = QWidget() + field = MultiInput(host, ["axon", "spine", "dendrite"]) + + field.remove() + assert field.getEntries() == ["axon", "spine"] + + host.deleteLater() + + +def test_removing_the_only_row_clears_it_instead(qapp): + from PySide6.QtWidgets import QWidget + + host = QWidget() + field = MultiInput(host, ["axon"]) + + field.remove() + assert field.getEntries() == [] + assert len(field.inputs) == 1 # still a line edit to type into + + field.remove() + assert len(field.inputs) == 1 + + host.deleteLater() + + +def test_remove_takes_the_focused_row_of_a_combo_field(qapp): + """A combobox row is focused through its own internal line edit. + + ``MultiInput`` also backs the group and host fields, which are comboboxes, so + the row lookup has to walk up from the focused widget rather than expect it + to be the row itself. + """ + from PySide6.QtWidgets import QVBoxLayout, QWidget + + host = QWidget() + field = MultiInput( + host, ["axon", "spine"], combo=True, combo_items=["axon", "spine"] + ) + layout = QVBoxLayout() + layout.addWidget(field) + host.setLayout(layout) + _shown(host) + + field.inputs[0].lineEdit().setFocus() + qapp.processEvents() + assert field.currentIndex() == 0 + + field.remove() + assert field.getEntries() == ["spine"] + + host.deleteLater() + + +def test_removing_the_only_combo_row_clears_it_instead(qapp): + from PySide6.QtWidgets import QWidget + + host = QWidget() + field = MultiInput(host, ["axon"], combo=True, combo_items=["axon"]) + + field.remove() + assert field.getEntries() == [] + assert len(field.inputs) == 1 + + host.deleteLater() + + +def test_add_remove_buttons_do_not_steal_focus(qapp): + """The premise of the fix: "-" can only know which row the caret is in if + pressing it leaves the caret alone.""" + from PySide6.QtCore import Qt + from PySide6.QtWidgets import QPushButton, QWidget + + host = QWidget() + field = MultiInput(host, ["axon"]) + buttons = field.findChildren(QPushButton) + + assert [b.text() for b in buttons] == ["-", "+"] + assert all(b.focusPolicy() == Qt.NoFocus for b in buttons) + + host.deleteLater() + + +# --- the trace attributes dialog ------------------------------------------ + + +def test_trace_dialog_lists_tags_in_a_stable_order(qapp, monkeypatch): + """The dialog hands MultiInput an ordered sequence, not the raw set.""" + captured = [] + real_multi_input = trace_dialog_module.MultiInput + + def recording(parent, entries=None, *args, **kwargs): + captured.append(entries) + return real_multi_input(parent, entries, *args, **kwargs) + + monkeypatch.setattr(trace_dialog_module, "MultiInput", recording) + + dialog = TraceDialog(None, traces=[_trace({"zeta", "alpha", "mu"})]) + + assert captured == [["alpha", "mu", "zeta"]] + assert [w.text() for w in dialog.tags_input.inputs] == [ + "alpha", "mu", "zeta" + ] + + dialog.deleteLater() + + +def test_trace_dialog_drops_a_tag_cleared_in_place(qapp): + """Clearing a tag's text is how a single tag is deleted; the dialog must not + return it as an empty tag.""" + dialog = TraceDialog(None, traces=[_trace({"axon", "spine"})]) + + rows = {w.text(): w for w in dialog.tags_input.inputs} + rows["axon"].setText("") + assert dialog.tags_input.getEntries() == ["spine"] + + dialog.deleteLater() + + +# --- the object list's Edit attributes... ---------------------------------- + + +def _all_sections(series): + return list(series.sections.keys()) + + +def _tags_on_disk(series, obj_name): + """Every stored trace's tags for one object, read back per section.""" + out = [] + for snum, section in series.enumerateSections(show_progress=False): + if obj_name in section.contours: + for trace in section.contours[obj_name].getTraces(): + out.append(set(trace.tags)) + assert out, f"object {obj_name} had no traces to check" + return out + + +def _set_tags(series, obj_name, tags): + """Put a known set of tags on an object, using the replacement path.""" + series.editObjectAttributes( + [obj_name], + tags=set(tags), + sections=_all_sections(series), + add_tags=False, + log_event=False, + ) + + +def _two_objects(series): + names = sorted(series.data["objects"].keys()) + assert len(names) >= 2, "fixture has fewer than two objects" + return names[0], names[1] + + +def test_object_edit_can_drop_one_tag(real_series): + """The reported symptom at the layer that caused it: one tag of two goes.""" + obj, _ = _two_objects(real_series) + _set_tags(real_series, obj, {"alpha", "beta"}) + assert all(t == {"alpha", "beta"} for t in _tags_on_disk(real_series, obj)) + + real_series.editObjectAttributes( + [obj], + tags={"alpha"}, + sections=_all_sections(real_series), + add_tags=False, + log_event=False, + ) + + assert all(t == {"alpha"} for t in _tags_on_disk(real_series, obj)), ( + "an edited tag set must be able to drop a tag; when the set is added " + "rather than assigned, the missing tag simply survives" + ) + + +def test_object_edit_can_clear_every_tag(real_series): + """Emptying the field. An empty set has to mean "no tags".""" + obj, _ = _two_objects(real_series) + _set_tags(real_series, obj, {"alpha", "beta"}) + + real_series.editObjectAttributes( + [obj], + tags=set(), + sections=_all_sections(real_series), + add_tags=False, + log_event=False, + ) + + assert all(t == set() for t in _tags_on_disk(real_series, obj)), ( + "an empty set is the only way to say 'clear'; added rather than " + "assigned it is an empty loop and nothing happens" + ) + + +def test_add_tags_defaults_to_the_existing_behavior(real_series): + """A caller that does not pass ``add_tags`` still adds. + + ``Object.name``'s setter calls ``editObjectAttributes`` without it. + """ + obj, _ = _two_objects(real_series) + _set_tags(real_series, obj, {"alpha"}) + + real_series.editObjectAttributes( + [obj], + tags={"beta"}, + sections=_all_sections(real_series), + log_event=False, + ) + + assert all(t == {"alpha", "beta"} for t in _tags_on_disk(real_series, obj)) + + +def test_additive_preserves_divergent_tags_across_objects(real_series): + """The property the multi-object path depends on. + + Two objects with different tags, one shared tag added: neither loses what it + had. This is what makes it safe to leave a multi-object edit additive. + """ + obj_a, obj_b = _two_objects(real_series) + _set_tags(real_series, obj_a, {"only_a"}) + _set_tags(real_series, obj_b, {"only_b"}) + + real_series.editObjectAttributes( + [obj_a, obj_b], + tags={"shared"}, + sections=_all_sections(real_series), + add_tags=True, + log_event=False, + ) + + assert all( + t == {"only_a", "shared"} for t in _tags_on_disk(real_series, obj_a) + ) + assert all( + t == {"only_b", "shared"} for t in _tags_on_disk(real_series, obj_b) + ) + + +def test_none_leaves_tags_alone_under_either_flag(real_series): + """None means "no value chosen" for tags exactly as it does for name/color. + + The trace dialog reports no single tag set for a selection whose tags + disagree, so this has to hold on the replacement path too. + """ + obj, _ = _two_objects(real_series) + _set_tags(real_series, obj, {"alpha"}) + + real_series.editObjectAttributes( + [obj], + color=(9, 9, 9), + tags=None, + sections=_all_sections(real_series), + add_tags=False, + log_event=False, + ) + assert all(t == {"alpha"} for t in _tags_on_disk(real_series, obj)) + + real_series.editObjectAttributes( + [obj], + color=(8, 8, 8), + tags=None, + sections=_all_sections(real_series), + add_tags=True, + log_event=False, + ) + assert all(t == {"alpha"} for t in _tags_on_disk(real_series, obj)) + + +class _FakeTraceDialog: + """Stands in for ``TraceDialog``, recording what it was shown. + + The real dialog needs a modal event loop and a QWidget parent. What matters + here is the contract either side of it: which tags the object list hands the + dialog, and what it does with the set the dialog hands back. + """ + + seen = None # kwargs the object list constructed it with + returns = None # (tags, sections) to report as the user's input + + def __init__(self, parent, **kwargs): + type(self).seen = kwargs + + def exec(self): + tags, sections = type(self).returns + trace = Trace(None, None) + trace.color = None + trace.tags = tags + trace.fill_mode = (None, None) + return (trace, sections), True + + +class _FakeTable: + def hasFocus(self): + # Not an ObjectTableWidget, so object_function falls back to the + # selected traces in the field for the selection. + return None + + def updateObjects(self, names): + pass + + +class _FakeMainWindow: + def saveAllData(self): + pass + + def seriesModified(self, modified): + pass + + +class _FakeSection: + def __init__(self, obj_names): + self.selected_traces = [Trace(n, (0, 0, 0)) for n in obj_names] + + +class _FieldStub: + """The minimum ``FieldWidgetObject.editAttributes`` and its decorator touch.""" + + def __init__(self, series, obj_names): + self.series = series + self.series_states = None + self.section = _FakeSection(obj_names) + self.table_manager = _FakeTable() + self.mainwindow = _FakeMainWindow() + + def reload(self): + pass + + +def _run_edit_attributes(monkeypatch, series, obj_names, returns): + """Drive the real ``editAttributes`` with the dialog faked out.""" + from PyReconstruct.modules.gui.main import field_widget_3_object as mod + + _FakeTraceDialog.seen = None + _FakeTraceDialog.returns = returns + monkeypatch.setattr(mod, "TraceDialog", _FakeTraceDialog) + + stub = _FieldStub(series, obj_names) + mod.FieldWidgetObject.editAttributes(stub) + return _FakeTraceDialog.seen + + +def test_single_object_edit_can_clear_tags(real_series, monkeypatch): + """The reported symptom, driven through the real command. + + One object selected, its tags shown, the user empties the field. + """ + obj, _ = _two_objects(real_series) + _set_tags(real_series, obj, {"alpha", "beta"}) + + seen = _run_edit_attributes( + monkeypatch, + real_series, + [obj], + returns=(set(), _all_sections(real_series)), + ) + + assert seen["tags"] == {"alpha", "beta"}, ( + "the dialog has to be prefilled with the object's tags; that is why the " + "set it returns can be read as a replacement" + ) + assert all(t == set() for t in _tags_on_disk(real_series, obj)), ( + "emptying the Tags field on a single-object selection has to remove the " + "tags" + ) + + +def test_single_object_edit_can_drop_one_tag(real_series, monkeypatch): + """The everyday case: delete one row of the Tags field, keep the rest.""" + obj, _ = _two_objects(real_series) + _set_tags(real_series, obj, {"alpha", "beta"}) + + _run_edit_attributes( + monkeypatch, + real_series, + [obj], + returns=({"alpha"}, _all_sections(real_series)), + ) + + assert all(t == {"alpha"} for t in _tags_on_disk(real_series, obj)) + + +def test_multi_object_edit_adds_without_erasing(real_series, monkeypatch): + """The other half of the fix, and the reason it is not a blanket flip. + + Two objects with different tags. The dialog shows a blank Tags field because + there is no single value to show, the user types one tag, and both objects + keep what they had. + """ + obj_a, obj_b = _two_objects(real_series) + _set_tags(real_series, obj_a, {"only_a"}) + _set_tags(real_series, obj_b, {"only_b"}) + + seen = _run_edit_attributes( + monkeypatch, + real_series, + [obj_a, obj_b], + returns=({"shared"}, _all_sections(real_series)), + ) + + assert seen["tags"] is None, ( + "a multi-object selection has no single tag set to display" + ) + assert all( + t == {"only_a", "shared"} for t in _tags_on_disk(real_series, obj_a) + ) + assert all( + t == {"only_b", "shared"} for t in _tags_on_disk(real_series, obj_b) + ) + + +def test_multi_object_edit_with_a_blank_field_changes_nothing( + real_series, monkeypatch +): + """Confirming a multi-object edit without touching the Tags field. + + The dialog reports either an empty set or nothing at all here, depending on + how it resolves an untouched blank field. Neither may remove a tag, since + the user was never shown one. + """ + obj_a, obj_b = _two_objects(real_series) + _set_tags(real_series, obj_a, {"only_a"}) + _set_tags(real_series, obj_b, {"only_b"}) + + for blank in (set(), None): + _run_edit_attributes( + monkeypatch, + real_series, + [obj_a, obj_b], + returns=(blank, _all_sections(real_series)), + ) + assert all( + t == {"only_a"} for t in _tags_on_disk(real_series, obj_a) + ), blank + assert all( + t == {"only_b"} for t in _tags_on_disk(real_series, obj_b) + ), blank