From 4865588393e5292483575fb417b549d046ff496f Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 13 May 2026 10:07:18 +0200 Subject: [PATCH 1/6] instrument configs in ui get sensor choices --- src/virtualship/cli/_plan.py | 50 ++++++++++++++++++++++++++++++ src/virtualship/models/__init__.py | 2 ++ 2 files changed, 52 insertions(+) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index e397aad7..68619501 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -36,6 +36,7 @@ DrifterConfig, Expedition, Location, + SensorConfig, ShipConfig, ShipUnderwaterSTConfig, Waypoint, @@ -76,6 +77,14 @@ def log_exception_to_file( f.write("\n") +def _default_sensors(config_class) -> list: + """List of SensorConfig instances from the config class's sensors default_factory.""" + sensors_field = config_class.model_fields.get("sensors") + if sensors_field is None or sensors_field.default_factory is None: + return [] + return sensors_field.default_factory() + + DEFAULT_TS_CONFIG = {"period_minutes": 5.0} DEFAULT_ADCP_CONFIG = { @@ -83,6 +92,7 @@ def log_exception_to_file( "period_minutes": 5.0, } + INSTRUMENT_FIELDS = { "adcp_config": { "class": ADCPConfig, @@ -339,6 +349,34 @@ def compose(self) -> ComposeResult: id=f"validation-failure-label-{instrument_name}_{attr}", classes="-hidden validation-failure", ) + # sensor toggles, derived from the config class's sensors default_factory + default_sensor_configs = _default_sensors(config_class) + if default_sensor_configs: + yield Label("[b]Sensors:[/b]", markup=True) + # which sensors are currently active + if config_instance and hasattr( + config_instance, "sensors" + ): + active_sensor_types = { + sc.sensor_type + for sc in config_instance.sensors + if sc.enabled + } + else: + # if no config loaded yet, default all sensors on + active_sensor_types = { + sc.sensor_type for sc in default_sensor_configs + } + for sc in default_sensor_configs: + sensor_id = f"{instrument_name}_sensor_{sc.sensor_type.value}" + with Horizontal(classes="sensor-toggle-row"): + yield Label( + f" {sc.sensor_type.value.replace('_', ' ').title()}:" + ) + yield Switch( + value=sc.sensor_type in active_sensor_types, + id=sensor_id, + ) ## 2) SCHEDULE EDITOR @@ -449,6 +487,18 @@ def _update_instrument_configs(self): kwargs["max_depth_meter"] = -1000.0 else: kwargs["max_depth_meter"] = -150.0 + # collect sensor toggles + default_sensor_configs = _default_sensors(config_class) + if default_sensor_configs: + sensors = [ + SensorConfig(sensor_type=sc.sensor_type) + for sc in default_sensor_configs + if self.query_one( + f"#{instrument_name}_sensor_{sc.sensor_type.value}", Switch + ).value + ] + if sensors: + kwargs["sensors"] = sensors setattr( self.expedition.instruments_config, instrument_name, diff --git a/src/virtualship/models/__init__.py b/src/virtualship/models/__init__.py index 71f24211..dd4b2bf1 100644 --- a/src/virtualship/models/__init__.py +++ b/src/virtualship/models/__init__.py @@ -9,6 +9,7 @@ Expedition, InstrumentsConfig, Schedule, + SensorConfig, ShipConfig, ShipUnderwaterSTConfig, Waypoint, @@ -23,6 +24,7 @@ __all__ = [ # noqa: RUF022 "Location", "Schedule", + "SensorConfig", "ShipConfig", "Waypoint", "ArgoFloatConfig", From efc9ce2b805caeae59bdec38c78791e6bbed7315 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 13 May 2026 10:19:24 +0200 Subject: [PATCH 2/6] better handle non valid combinations of active instruments with no active sensors --- src/virtualship/cli/_plan.py | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 68619501..701a320e 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -101,6 +101,7 @@ def _default_sensors(config_class) -> list: {"name": "num_bins"}, {"name": "period", "minutes": True}, ], + # underway instrument, so active/inactive is controlled by the on/off toggle, not the schedule (therefore no instrument_type key) }, "ship_underwater_st_config": { "class": ShipUnderwaterSTConfig, @@ -108,10 +109,12 @@ def _default_sensors(config_class) -> list: "attributes": [ {"name": "period", "minutes": True}, ], + # underway instrument, so active/inactive is controlled by the on/off toggle, not the schedule (therefore no instrument_type key) }, "ctd_config": { "class": CTDConfig, "title": "CTD", + "instrument_type": InstrumentType.CTD, "attributes": [ {"name": "max_depth_meter"}, {"name": "min_depth_meter"}, @@ -121,6 +124,7 @@ def _default_sensors(config_class) -> list: "xbt_config": { "class": XBTConfig, "title": "XBT", + "instrument_type": InstrumentType.XBT, "attributes": [ {"name": "min_depth_meter"}, {"name": "max_depth_meter"}, @@ -131,6 +135,7 @@ def _default_sensors(config_class) -> list: "argo_float_config": { "class": ArgoFloatConfig, "title": "Argo Float", + "instrument_type": InstrumentType.ARGO_FLOAT, "attributes": [ {"name": "min_depth_meter"}, {"name": "max_depth_meter"}, @@ -145,6 +150,7 @@ def _default_sensors(config_class) -> list: "drifter_config": { "class": DrifterConfig, "title": "Drifter", + "instrument_type": InstrumentType.DRIFTER, "attributes": [ {"name": "depth_meter"}, {"name": "lifetime", "days": True}, @@ -431,6 +437,8 @@ def save_changes(self) -> bool: self._update_schedule() self.expedition.to_yaml(self.path.joinpath(EXPEDITION)) return True + except UserError: + raise except Exception as e: log_exception_to_file( e, @@ -497,8 +505,31 @@ def _update_instrument_configs(self): f"#{instrument_name}_sensor_{sc.sensor_type.value}", Switch ).value ] - if sensors: - kwargs["sensors"] = sensors + if not sensors: + # for schedule-based instruments, only raise if actually used in a waypoint + # for underway, this is handled by the on/off toggle + instrument_type = info.get("instrument_type") + is_active = instrument_type is None or any( + instrument_type + in ( + wp.instrument + if isinstance(wp.instrument, list) + else [wp.instrument] + ) + for wp in self.expedition.schedule.waypoints + if wp.instrument + ) + if is_active: + title = info.get( + "title", instrument_name.replace("_", " ").title() + ) + raise UserError( + f"'{title}' has no sensors selected. " + f"At least one sensor must be enabled for each active instrument." + ) + kwargs["sensors"] = ( + sensors if sensors else _default_sensors(config_class) + ) setattr( self.expedition.instruments_config, instrument_name, From 90ef8c17d186e2637b4ca2be353168e293e4a26c Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 13 May 2026 10:20:42 +0200 Subject: [PATCH 3/6] better horizontal align sensor toggles --- src/virtualship/cli/_plan.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 701a320e..ac992445 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -1337,6 +1337,18 @@ class PlanApp(App): margin: 0 1; } + .sensor-toggle-row { + height: auto; + margin: 0; + padding: 0; + } + + .sensor-toggle-row Label { + width: 24; + margin-top: 1; + color: $text-muted; + } + .year-select { width: 20; } From 33a84760c960005acebb306fdec2bb38787e274e Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 13 May 2026 12:03:26 +0200 Subject: [PATCH 4/6] add new and update existing tests --- tests/cli/test_plan.py | 262 +++++++++++++++++++++++++++++++++-------- 1 file changed, 210 insertions(+), 52 deletions(-) diff --git a/tests/cli/test_plan.py b/tests/cli/test_plan.py index f0522cde..078590b1 100644 --- a/tests/cli/test_plan.py +++ b/tests/cli/test_plan.py @@ -1,20 +1,20 @@ -import os -import shutil -import tempfile from datetime import datetime from pathlib import Path from unittest.mock import MagicMock import pytest import yaml -from textual.widgets import Button, Collapsible, Input +from textual.widgets import Button, Collapsible, Input, Switch -from virtualship.cli._plan import ExpeditionEditor, PlanApp +from virtualship.cli._plan import ExpeditionEditor, PlanApp, _default_sensors +from virtualship.instruments.sensors import SensorType from virtualship.models import ( + CTDConfig, Expedition, InstrumentsConfig, Location, Schedule, + SensorConfig, Waypoint, ) from virtualship.utils import EXPEDITION, get_example_expedition @@ -24,6 +24,24 @@ NEW_LON = "0.015" +def _make_expedition( + tmpdir: Path, + waypoints: list, + instruments_config=None, +) -> None: + """Write a minimal expedition YAML.""" + if instruments_config is None: + instruments_config = InstrumentsConfig.model_validate( + yaml.safe_load(get_example_expedition()).get("instruments_config") + ) + ship_config = yaml.safe_load(get_example_expedition()).get("ship_config") + Expedition( + schedule=Schedule(waypoints=waypoints), + instruments_config=instruments_config, + ship_config=ship_config, + ).to_yaml(tmpdir / EXPEDITION) + + async def simulate_input(pilot, box, new_value): """Simulate inputs to the UI.""" box.focus() @@ -36,14 +54,8 @@ async def simulate_input(pilot, box, new_value): @pytest.mark.asyncio -async def test_UI_changes(): +async def test_UI_changes(tmp_path): """Test making changes to UI inputs and saving to YAML (simulated botton presses and typing inputs).""" - tmpdir = Path(tempfile.mkdtemp()) - - instruments_config = InstrumentsConfig.model_validate( - yaml.safe_load(get_example_expedition()).get("instruments_config") - ) - ship_config = yaml.safe_load(get_example_expedition()).get("ship_config") waypoints = [ Waypoint( location=Location(0, 0), @@ -61,15 +73,9 @@ async def test_UI_changes(): instrument=["CTD"], ), ] - expedition = Expedition( - schedule=Schedule(waypoints=waypoints), - instruments_config=instruments_config, - ship_config=ship_config, - ) - - expedition.to_yaml(tmpdir / EXPEDITION) + _make_expedition(tmp_path, waypoints) - app = PlanApp(path=tmpdir) + app = PlanApp(path=tmp_path) async with app.run_test(size=(120, 100)) as pilot: await pilot.pause(0.5) @@ -133,8 +139,7 @@ async def test_UI_changes(): ) # verify changes to speed, lat, lon in saved YAML - expedition_path = os.path.join(tmpdir, EXPEDITION) - with open(expedition_path) as f: + with open(tmp_path / EXPEDITION) as f: saved_expedition = yaml.safe_load(f) assert saved_expedition["ship_config"]["ship_speed_knots"] == float(NEW_SPEED) @@ -148,45 +153,22 @@ async def test_UI_changes(): args, _ = plan_screen.notify.call_args assert "*** Error saving changes ***" in args[0] - # cleanup - shutil.rmtree(tmpdir) - @pytest.mark.asyncio -async def test_UI_opens_with_null_time_and_instrument(): +async def test_UI_opens_with_null_time_and_instrument(tmp_path): """Test that the UI opens correctly when waypoints have time: null and instrument: null.""" - tmpdir = Path(tempfile.mkdtemp()) - - instruments_config = InstrumentsConfig.model_validate( - yaml.safe_load(get_example_expedition()).get("instruments_config") - ) - ship_config = yaml.safe_load(get_example_expedition()).get("ship_config") waypoints = [ Waypoint( location=Location(0, 0), time=datetime(2022, 1, 1, 0, 0, 0), instrument=["CTD"], ), - Waypoint( - location=Location(0.01, 0.01), - time=None, - instrument=None, - ), - Waypoint( - location=Location(0.02, 0.02), - time=None, - instrument=None, - ), + Waypoint(location=Location(0.01, 0.01), time=None, instrument=None), + Waypoint(location=Location(0.02, 0.02), time=None, instrument=None), ] - expedition = Expedition( - schedule=Schedule(waypoints=waypoints), - instruments_config=instruments_config, - ship_config=ship_config, - ) + _make_expedition(tmp_path, waypoints) - expedition.to_yaml(tmpdir / EXPEDITION) - - app = PlanApp(path=tmpdir) + app = PlanApp(path=tmp_path) async with app.run_test(size=(120, 100)) as pilot: await pilot.pause(0.5) @@ -197,5 +179,181 @@ async def test_UI_opens_with_null_time_and_instrument(): # verify the app opened without errors by checking the editor loaded assert expedition_editor is not None - # cleanup - shutil.rmtree(tmpdir) + +def test_default_sensors_returns_declared_defaults(): + """_default_sensors() should mirror the config class's default sensor list.""" + ctd_sensors = _default_sensors(CTDConfig) + types = {sc.sensor_type for sc in ctd_sensors} + assert SensorType.TEMPERATURE in types + assert SensorType.SALINITY in types + + +@pytest.mark.asyncio +async def test_sensor_toggle_saved_to_yaml(tmp_path): + """Toggling a sensor switch off and saving should persist only the enabled sensors.""" + _make_expedition( + tmp_path, + [ + Waypoint( + location=Location(0, 0), + time=datetime(2022, 1, 1, 0, 0, 0), + instrument=["CTD"], + ), + Waypoint( + location=Location(0.01, 0.01), + time=datetime(2022, 1, 1, 1, 0, 0), + instrument=None, + ), + ], + ) + + app = PlanApp(path=tmp_path) + async with app.run_test(size=(160, 120)) as pilot: + await pilot.pause(0.5) + plan_screen = pilot.app.screen + plan_screen.notify = MagicMock() + expedition_editor = plan_screen.query_one(ExpeditionEditor) + + # turn off SALINITY on CTD + sal_switch = expedition_editor.query_one("#ctd_config_sensor_SALINITY", Switch) + await pilot.click(sal_switch) + await pilot.pause(0.2) + + await pilot.click(plan_screen.query_one("#save_button", Button)) + await pilot.pause(0.5) + + plan_screen.notify.assert_called_once_with( + "Changes saved successfully", severity="information", timeout=20 + ) + + with open(tmp_path / EXPEDITION) as f: + saved = yaml.safe_load(f) + saved_sensors = saved["instruments_config"]["ctd_config"]["sensors"] + assert "SALINITY" not in saved_sensors + assert "TEMPERATURE" in saved_sensors + + +@pytest.mark.asyncio +async def test_deselecting_all_sensors_on_active_instrument_blocks_save(tmp_path): + """Removing all sensors from an instrument used in the schedule should show an error.""" + _make_expedition( + tmp_path, + [ + Waypoint( + location=Location(0, 0), + time=datetime(2022, 1, 1, 0, 0, 0), + instrument=["XBT"], + ), + Waypoint( + location=Location(0.01, 0.01), + time=datetime(2022, 1, 1, 1, 0, 0), + instrument=None, + ), + ], + ) + + app = PlanApp(path=tmp_path) + async with app.run_test(size=(160, 120)) as pilot: + await pilot.pause(0.5) + plan_screen = pilot.app.screen + plan_screen.notify = MagicMock() + expedition_editor = plan_screen.query_one(ExpeditionEditor) + + # XBT only has TEMPERATURE, deselect it + temp_switch = expedition_editor.query_one( + "#xbt_config_sensor_TEMPERATURE", Switch + ) + await pilot.click(temp_switch) + await pilot.pause(0.2) + + await pilot.click(plan_screen.query_one("#save_button", Button)) + await pilot.pause(0.5) + + args, _ = plan_screen.notify.call_args + assert "*** Error saving changes ***" in args[0] + assert "XBT" in args[0] + + +@pytest.mark.asyncio +async def test_deselecting_all_sensors_on_inactive_instrument(tmp_path): + """Removing all sensors from an instrument NOT in the schedule should not block saving.""" + # only CTD waypoints, XBT is inactive + _make_expedition( + tmp_path, + [ + Waypoint( + location=Location(0, 0), + time=datetime(2022, 1, 1, 0, 0, 0), + instrument=["CTD"], + ), + Waypoint( + location=Location(0.01, 0.01), + time=datetime(2022, 1, 1, 1, 0, 0), + instrument=None, + ), + ], + ) + + app = PlanApp(path=tmp_path) + async with app.run_test(size=(160, 120)) as pilot: + await pilot.pause(0.5) + plan_screen = pilot.app.screen + plan_screen.notify = MagicMock() + expedition_editor = plan_screen.query_one(ExpeditionEditor) + + # deselect XBT's only sensor even though XBT is not in the schedule + temp_switch = expedition_editor.query_one( + "#xbt_config_sensor_TEMPERATURE", Switch + ) + await pilot.click(temp_switch) + await pilot.pause(0.2) + + await pilot.click(plan_screen.query_one("#save_button", Button)) + await pilot.pause(0.5) + + plan_screen.notify.assert_called_once_with( + "Changes saved successfully", severity="information", timeout=20 + ) + + +@pytest.mark.asyncio +async def test_sensor_initial_state_reflects_config(tmp_path): + """Sensor switches should reflect the enabled sensors already stored in the config.""" + ctd_config = CTDConfig( + stationkeeping_time_minutes=50, + min_depth_meter=-11.0, + max_depth_meter=-2000.0, + sensors=[SensorConfig(sensor_type=SensorType.TEMPERATURE)], + ) + instruments_config = InstrumentsConfig.model_validate( + yaml.safe_load(get_example_expedition()).get("instruments_config") + ) + instruments_config.ctd_config = ctd_config + _make_expedition( + tmp_path, + [ + Waypoint( + location=Location(0, 0), + time=datetime(2022, 1, 1, 0, 0, 0), + instrument=["CTD"], + ), + Waypoint( + location=Location(0.01, 0.01), + time=datetime(2022, 1, 1, 1, 0, 0), + instrument=None, + ), + ], + instruments_config, + ) + + app = PlanApp(path=tmp_path) + async with app.run_test(size=(160, 120)) as pilot: + await pilot.pause(0.5) + expedition_editor = pilot.app.screen.query_one(ExpeditionEditor) + + temp_switch = expedition_editor.query_one( + "#ctd_config_sensor_TEMPERATURE", Switch + ) + sal_switch = expedition_editor.query_one("#ctd_config_sensor_SALINITY", Switch) + assert temp_switch.value is True, "TEMPERATURE should be ON" + assert sal_switch.value is False, "SALINITY should be OFF (not in saved config)" From 16f56fbd5a9b6c1e2284b5b8a08ce7853d694602 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 11:04:39 +0200 Subject: [PATCH 5/6] handle nested collapsibles within the UI when testing --- tests/cli/test_plan.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/cli/test_plan.py b/tests/cli/test_plan.py index 078590b1..29459223 100644 --- a/tests/cli/test_plan.py +++ b/tests/cli/test_plan.py @@ -53,6 +53,23 @@ async def simulate_input(pilot, box, new_value): await pilot.pause(0.05) +async def _expand_instrument_configs( + expedition_editor, pilot, instrument_title: str | None = None +): + """Expand the outer 'Instrument Configurations' collapsible and the instrument-specific inner one.""" + for coll in expedition_editor.query(Collapsible): + if "Instrument Configurations" in (coll.title or ""): + coll.collapsed = False + await pilot.pause() + break + if instrument_title is not None: + for coll in expedition_editor.query(Collapsible): + if instrument_title in (coll.title or ""): + coll.collapsed = False + await pilot.pause() + break + + @pytest.mark.asyncio async def test_UI_changes(tmp_path): """Test making changes to UI inputs and saving to YAML (simulated botton presses and typing inputs).""" @@ -213,6 +230,7 @@ async def test_sensor_toggle_saved_to_yaml(tmp_path): plan_screen = pilot.app.screen plan_screen.notify = MagicMock() expedition_editor = plan_screen.query_one(ExpeditionEditor) + await _expand_instrument_configs(expedition_editor, pilot, "CTD") # turn off SALINITY on CTD sal_switch = expedition_editor.query_one("#ctd_config_sensor_SALINITY", Switch) @@ -258,6 +276,7 @@ async def test_deselecting_all_sensors_on_active_instrument_blocks_save(tmp_path plan_screen = pilot.app.screen plan_screen.notify = MagicMock() expedition_editor = plan_screen.query_one(ExpeditionEditor) + await _expand_instrument_configs(expedition_editor, pilot, "XBT") # XBT only has TEMPERATURE, deselect it temp_switch = expedition_editor.query_one( @@ -300,6 +319,7 @@ async def test_deselecting_all_sensors_on_inactive_instrument(tmp_path): plan_screen = pilot.app.screen plan_screen.notify = MagicMock() expedition_editor = plan_screen.query_one(ExpeditionEditor) + await _expand_instrument_configs(expedition_editor, pilot, "XBT") # deselect XBT's only sensor even though XBT is not in the schedule temp_switch = expedition_editor.query_one( From 3cdde84191f6ad9c0f80ea8f8ab9df2f78cbc348 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 19 May 2026 11:10:03 +0200 Subject: [PATCH 6/6] remove reference to "advanced users only" for instrument configs; not true anymore --- src/virtualship/cli/_plan.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index ac992445..f2e786c7 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -281,7 +281,7 @@ def compose(self) -> ComposeResult: ## SECTION: "Instrument Configurations"" with Collapsible( - title="[b]Instrument Configurations[/b] (advanced users only)", + title="[b]Instrument Configurations[/b]", collapsed=True, ): for instrument_name, info in INSTRUMENT_FIELDS.items():