From f021258738902f42cb927b2b02b8a69dcfe5c4d6 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Fri, 6 Jun 2025 12:04:06 +0200 Subject: [PATCH 01/30] add plan command to click --- src/virtualship/cli/commands.py | 14 +++++++++++++- src/virtualship/cli/main.py | 1 + 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/virtualship/cli/commands.py b/src/virtualship/cli/commands.py index 706dc79a..7fb4b529 100644 --- a/src/virtualship/cli/commands.py +++ b/src/virtualship/cli/commands.py @@ -4,6 +4,7 @@ from virtualship import utils from virtualship.cli._fetch import _fetch +from virtualship.cli._plan import _plan from virtualship.expedition.do_expedition import do_expedition from virtualship.utils import ( SCHEDULE, @@ -55,7 +56,8 @@ def init(path, from_mfp): mfp_to_yaml(mfp_file, schedule) click.echo( "\n⚠️ The generated schedule does not contain time values. " - "\nPlease edit 'schedule.yaml' and manually add the necessary time values." + "\nPlease either use the 'virtualship plan` app to complete the schedule configuration, " + "\nOR edit 'schedule.yaml' and manually add the necessary time values." "\n🕒 Expected time format: 'YYYY-MM-DD HH:MM:SS' (e.g., '2023-10-20 01:00:00').\n" ) else: @@ -66,6 +68,16 @@ def init(path, from_mfp): click.echo(f"Created '{config.name}' and '{schedule.name}' at {path}.") +@click.command() +@click.argument( + "path", + type=click.Path(exists=False, file_okay=False, dir_okay=True), +) +def plan(path): + """Launch UI to help build schedule and ship config files.""" + _plan(Path(path)) + + @click.command() @click.argument( "path", diff --git a/src/virtualship/cli/main.py b/src/virtualship/cli/main.py index 6d0aa258..6ee12eff 100644 --- a/src/virtualship/cli/main.py +++ b/src/virtualship/cli/main.py @@ -10,6 +10,7 @@ def cli(): cli.add_command(commands.init) +cli.add_command(commands.plan) cli.add_command(commands.fetch) cli.add_command(commands.run) From b27698af724dbdeec456eb483f6dcf67455d619c Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Fri, 6 Jun 2025 12:04:44 +0200 Subject: [PATCH 02/30] first draft of UI app --- src/virtualship/cli/_plan.py | 961 +++++++++++++++++++++++++++++++++++ 1 file changed, 961 insertions(+) create mode 100644 src/virtualship/cli/_plan.py diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py new file mode 100644 index 00000000..94c58e01 --- /dev/null +++ b/src/virtualship/cli/_plan.py @@ -0,0 +1,961 @@ +import datetime + +from textual import on +from textual.app import App, ComposeResult +from textual.containers import Container, Horizontal, VerticalScroll +from textual.screen import Screen +from textual.widgets import ( + Button, + Collapsible, + Input, + Label, + Rule, + Select, + Static, + Switch, +) + +from virtualship.errors import ConfigError +from virtualship.models.location import Location +from virtualship.models.schedule import Schedule, Waypoint +from virtualship.models.ship_config import InstrumentType, ShipConfig +from virtualship.models.space_time_region import ( + SpaceTimeRegion, + SpatialRange, + TimeRange, +) + +# TODO: I wonder whether it would be a good idea to be able to recognise all the subcomponents of a pydantic model +# TODO: such as the instrument configuration, and be able to loop (?) rather than explicitly type each one out +# TODO: maybe in some kind of form where it's stored externally so as to facilitate it being in one place and helps there be more ease of adding instruments +# TODO: down the line without having to make changes in so many places...? +# TODO: but is this possible given the different requirements/units etc. that they all require? + +# TODO: NEED TO MAKE SURE THE CHANGES OVERWRITTEN TO THE YAML FILES DO IT IN THE RIGHT ORDER, IT DOESN'T SAVE PROPERLY AT CURRENT +# TODO: implement action for 'Save' button +# TODO: probably with a second pop up that says "are you sure you want to save, this will overwrite schedule. and ship_config.yaml" + + +# TODO: error handling needs to be better! E.g. if elements of the pydantic model are missing then they should be specifically flagged +# TODO: e.g. if you remove ship_speed_knots from the ship_config.yaml it fails but the error message is misleading (starts going on about adcp_config) + + +# TODO: Can the whole lot be tidied up by moving some classes/methods to a new directory/files?! + + +class WaypointWidget(Static): + def __init__(self, waypoint: Waypoint, index: int): + super().__init__() + self.waypoint = waypoint + self.index = index + + def compose(self) -> ComposeResult: + if self.index > 0: + yield Button( + "Copy from Previous", id=f"wp{self.index}_copy", variant="warning" + ) + yield Label(f"[b]Waypoint {self.index + 1}[/b]") + yield Label("Location:") + yield Label(" Latitude:") + yield Input( + id=f"wp{self.index}_lat", + value=str(self.waypoint.location.lat), + ) + yield Label(" Longitude:") + yield Input( + id=f"wp{self.index}_lon", + value=str(self.waypoint.location.lon), + ) + yield Label("Time:") + with Horizontal(): + yield Label("Year:") + yield Select( + [ + (str(year), year) + for year in range( + datetime.datetime.now().year - 3, + datetime.datetime.now().year + 1, + ) + ], + id=f"wp{self.index}_year", + value=int(self.waypoint.time.year) + if self.waypoint.time + else Select.BLANK, + prompt="YYYY", + classes="year-select", + ) + yield Label("Month:") + yield Select( + [(f"{m:02d}", m) for m in range(1, 13)], + id=f"wp{self.index}_month", + value=int(self.waypoint.time.month) + if self.waypoint.time + else Select.BLANK, + prompt="MM", + classes="month-select", + ) + yield Label("Day:") + yield Select( + [(f"{d:02d}", d) for d in range(1, 32)], + id=f"wp{self.index}_day", + value=int(self.waypoint.time.day) + if self.waypoint.time + else Select.BLANK, + prompt="DD", + classes="day-select", + ) + yield Label("Hour:") + yield Select( + [(f"{h:02d}", h) for h in range(24)], + id=f"wp{self.index}_hour", + value=int(self.waypoint.time.hour) + if self.waypoint.time + else Select.BLANK, + prompt="hh", + classes="hour-select", + ) + yield Label("Min:") + yield Select( + [(f"{m:02d}", m) for m in range(0, 60, 5)], + id=f"wp{self.index}_minute", + value=int(self.waypoint.time.minute) + if self.waypoint.time + else Select.BLANK, + prompt="mm", + classes="minute-select", + ) + + yield Label("Instruments:") + for instrument in InstrumentType: + is_selected = instrument in (self.waypoint.instrument or []) + with Horizontal(): + yield Label(instrument.value) + yield Switch(value=is_selected, id=f"wp{self.index}_{instrument.value}") + + def copy_from_previous(self) -> None: + if self.index > 0: + schedule_editor = self.parent + if schedule_editor: + prev_lat = schedule_editor.query_one(f"#wp{self.index - 1}_lat") + prev_lon = schedule_editor.query_one(f"#wp{self.index - 1}_lon") + curr_lat = self.query_one(f"#wp{self.index}_lat") + curr_lon = self.query_one(f"#wp{self.index}_lon") + + if prev_lat and prev_lon and curr_lat and curr_lon: + curr_lat.value = prev_lat.value + curr_lon.value = prev_lon.value + + time_components = ["year", "month", "day", "hour", "minute"] + for comp in time_components: + prev = schedule_editor.query_one(f"#wp{self.index - 1}_{comp}") + curr = self.query_one(f"#wp{self.index}_{comp}") + if prev and curr: + curr.value = prev.value + + for instrument in InstrumentType: + prev_switch = schedule_editor.query_one( + f"#wp{self.index - 1}_{instrument.value}" + ) + curr_switch = self.query_one(f"#wp{self.index}_{instrument.value}") + if prev_switch and curr_switch: + curr_switch.value = prev_switch.value + + @on(Button.Pressed, "Button") + def button_pressed(self, event: Button.Pressed) -> None: + if event.button.id == f"wp{self.index}_copy": + self.copy_from_previous() + + +class ScheduleEditor(Static): + def __init__(self, path: str): + super().__init__() + self.path = path + self.schedule = None + + def compose(self) -> ComposeResult: + try: + self.schedule = Schedule.from_yaml(f"{self.path}/schedule.yaml") + yield Label("[b]Schedule Editor[/b]", id="title", markup=True) + yield Rule(line_style="heavy") + + with Collapsible( + title="[b]Waypoints[/b]", + collapsed=True, + ): + for i, waypoint in enumerate(self.schedule.waypoints): + yield WaypointWidget(waypoint, i) + + # Space-Time Region Section + # TODO: MAY NEED TO ADD A FEATURE ON SAVE CHANGES WHICH AUTOMATICALLY DETECTS MAX AND MIN TIME + # TODO: FOR THE SCENARIO WHERE YAML LOADED IN IS NULL AND USER DOES NOT EDIT THEMSELVES + with Collapsible( + title="[b]Space-Time Region[/b] (advanced users only)", + collapsed=True, + ): + if self.schedule.space_time_region: + str_data = self.schedule.space_time_region + yield Label("Minimum Latitude:") + yield Input( + id="min_lat", + value=str(str_data.spatial_range.minimum_latitude), + ) + yield Label("Maximum Latitude:") + yield Input( + id="max_lat", + value=str(str_data.spatial_range.maximum_latitude), + ) + yield Label("Minimum Longitude:") + yield Input( + id="min_lon", + value=str(str_data.spatial_range.minimum_longitude), + ) + yield Label("Maximum Longitude:") + yield Input( + id="max_lon", + value=str(str_data.spatial_range.maximum_longitude), + ) + yield Label("Minimum Depth (meters):") + yield Input( + id="min_depth", + value=str(str_data.spatial_range.minimum_depth), + ) + yield Label("Maximum Depth (meters):") + yield Input( + id="max_depth", + value=str(str_data.spatial_range.maximum_depth), + ) + yield Label("Start Time:") + yield Input( + id="start_time", + placeholder="YYYY-MM-DD hh:mm:ss", + value=( + str(str_data.time_range.start_time) + if str_data.time_range and str_data.time_range.start_time + else "" + ), + ) + yield Label("End Time:") + yield Input( + id="end_time", + placeholder="YYYY-MM-DD hh:mm:ss", + value=( + str(str_data.time_range.end_time) + if str_data.time_range and str_data.time_range.end_time + else "" + ), + ) + + except Exception as e: + yield Label(f"Error loading schedule: {e!s}") + + def save_changes(self) -> None: + """Save changes to schedule.yaml.""" + try: + # spacetime region + spatial_range = SpatialRange( + minimum_longitude=self.query_one("#min_lon").value, + maximum_longitude=self.query_one("#max_lon").value, + minimum_latitude=self.query_one("#min_lat").value, + maximum_latitude=self.query_one("#max_lat").value, + minimum_depth=self.query_one("#min_depth").value, + maximum_depth=self.query_one("#max_depth").value, + ) + + time_range = TimeRange( + start_time=self.query_one("#start_time").value, + end_time=self.query_one("#end_time").value, + ) + + space_time_region = SpaceTimeRegion( + spatial_range=spatial_range, time_range=time_range + ) + + self.schedule.space_time_region = space_time_region + + # waypoints + waypoints = [] + for i in range(len(self.schedule.waypoints)): + location = Location( + latitude=float(self.query_one(f"#wp{i}_lat").value), + longitude=float(self.query_one(f"#wp{i}_lon").value), + ) + time = datetime.datetime( + int(self.query_one(f"#wp{i}_year").value), + int(self.query_one(f"#wp{i}_month").value), + int(self.query_one(f"#wp{i}_day").value), + int(self.query_one(f"#wp{i}_hour").value), + int(self.query_one(f"#wp{i}_minute").value), + 0, + ) + instruments = [ + instrument + for instrument in InstrumentType + if self.query_one(f"#wp{i}_{instrument.value}").value + ] + + waypoints.append( + Waypoint(location=location, time=time, instrument=instruments) + ) + + self.schedule.waypoints = waypoints + + self.schedule.to_yaml(f"{self.path}/schedule.yaml") + + # TODO: one error type for User Errors (e.g. new UserError class?) which gives information on where went wrong (e.g. as above "Inputs for all latitude, longitude and time parameters are required for waypoint {i + 1}") + except Exception as e: + self.notify(f"Error saving schedule: {e!r}", severity="error", timeout=60) + raise + # TODO: then also another type of generic error (e.g. the existing except Exception as e) where there is a bug in the code but it's unknown where (i.e. it has got past all the other tests before being committed) + # TODO: and add a message saying "please raise an issue on the VirtualShip GitHub issue tracker" + # TODO: also provide a full traceback which the user can copy to their issue (may need to quit the application to provide this option) + + +class ConfigEditor(Container): + # TODO: Also incorporate verify methods! + + def __init__(self, path: str): + super().__init__() + self.path = path + self.config = None + + def compose(self) -> ComposeResult: + try: + self.config = ShipConfig.from_yaml(f"{self.path}/ship_config.yaml") + yield Label("[b]Ship Config Editor[/b]", id="title", markup=True) + yield Rule(line_style="heavy") + + # ship speed and onboard measurement selection + with Collapsible(title="[b]Ship Speed and Onboard Measurements[/b]"): + with Horizontal(classes="ship_speed"): + yield Label("[b]Ship Speed (knots):[/b]") + yield Input( + id="speed", + classes="ship_speed_input", + placeholder="knots", + value=str( + self.config.ship_speed_knots + if self.config.ship_speed_knots + else "" + ), + ) + + with Horizontal(classes="ts-section"): + yield Label("[b]Onboard Temperature/Salinity:[/b]") + yield Switch( + value=bool(self.config.ship_underwater_st_config), + id="has_onboard_ts", + ) + + with Horizontal(classes="adcp-section"): + yield Label("[b]Onboard ADCP:[/b]") + yield Switch(value=bool(self.config.adcp_config), id="has_adcp") + + # adcp type selection + with Horizontal(id="adcp_type_container", classes="-hidden"): + is_deep = ( + self.config.adcp_config + and self.config.adcp_config.max_depth_meter == -1000.0 + ) + yield Label(" OceanObserver:") + yield Switch(value=is_deep, id="adcp_deep") + yield Label(" SeaSeven:") + yield Switch(value=not is_deep, id="adcp_shallow") + + # specific instrument configurations + with Collapsible( + title="[b]Instrument Configurations[/b] (advanced users only)", + collapsed=True, + ): + # TODO: could all of the below be a loop instead?! Extracting each of the sub parameters for each instrument config? + + with Collapsible( + title="[b]Onboard ADCP Configuration[/b]", collapsed=True + ): + with Container(classes="instrument-config"): + yield Label("Number of Bins:") + yield Input( + id="adcp_num_bins", + value=str( + self.config.adcp_config.num_bins + if self.config.adcp_config + else "" + ), + ) + yield Label("Period (minutes):") + yield Input( + id="adcp_period", + value=str( + self.config.adcp_config.period.total_seconds() / 60.0 + if self.config.adcp_config + else "" + ), + ) + + with Collapsible( + title="[b]Onboard Temperature/Salinity Configuration[/b]", + collapsed=True, + ): + with Container(classes="instrument-config"): + yield Label("Period (minutes):") + yield Input( + id="ts_period", + value=str( + self.config.ship_underwater_st_config.period.total_seconds() + / 60.0 + if self.config.ship_underwater_st_config + else "" + ), + ) + + with Collapsible(title="[b]CTD Configuration[/b]", collapsed=True): + with Container(classes="instrument-config"): + yield Label("Maximum Depth (meters):") + yield Input( + id="ctd_max_depth", + value=str( + self.config.ctd_config.max_depth_meter + if self.config.ctd_config + else "" + ), + ) + yield Label("Minimum Depth (meters):") + yield Input( + id="ctd_min_depth", + value=str( + self.config.ctd_config.min_depth_meter + if self.config.ctd_config + else "" + ), + ) + yield Label("Stationkeeping Time (minutes):") + yield Input( + id="ctd_stationkeeping_time", + value=str( + self.config.ctd_config.stationkeeping_time.total_seconds() + / 60.0 + if self.config.ctd_config + else "" + ), + ) + + with Collapsible(title="[b]CTD-BGC Configuration[/b]", collapsed=True): + with Container(classes="instrument-config"): + yield Label("Maximum Depth (meters):") + yield Input( + id="ctd_bgc_max_depth", + value=str( + self.config.ctd_bgc_config.max_depth_meter + if self.config.ctd_bgc_config + else "" + ), + ) + yield Label("Minimum Depth (meters):") + yield Input( + id="ctd_bgc_min_depth", + value=str( + self.config.ctd_bgc_config.min_depth_meter + if self.config.ctd_bgc_config + else "" + ), + ) + yield Label("Stationkeeping Time (minutes):") + yield Input( + id="ctd_bgc_stationkeeping_time", + value=str( + self.config.ctd_bgc_config.stationkeeping_time.total_seconds() + / 60.0 + if self.config.ctd_bgc_config + else "" + ), + ) + + with Collapsible(title="[b]XBT Configuration[/b]", collapsed=True): + with Container(classes="instrument-config"): + yield Label("Maximum Depth (meters):") + yield Input( + id="xbt_max_depth", + value=str( + self.config.xbt_config.max_depth_meter + if self.config.xbt_config + else "" + ), + ) + yield Label("Minimum Depth (meters):") + yield Input( + id="xbt_min_depth", + value=str( + self.config.xbt_config.min_depth_meter + if self.config.xbt_config + else "" + ), + ) + yield Label("Fall Speed (meters/second):") + yield Input( + id="xbt_fall_speed", + value=str( + self.config.xbt_config.fall_speed_meter_per_second + if self.config.xbt_config + else "" + ), + ) + yield Label("Deceleration Coefficient:") + yield Input( + id="xbt_decel_coeff", + value=str( + self.config.xbt_config.deceleration_coefficient + if self.config.xbt_config + else "" + ), + ) + + with Collapsible( + title="[b]Argo Float Configuration[/b]", collapsed=True + ): + with Container(classes="instrument-config"): + yield Label("Cycle Days:") + yield Input( + id="argo_cycle_days", + value=str( + self.config.argo_float_config.cycle_days + if self.config.argo_float_config + else "" + ), + ) + yield Label("Drift Days:") + yield Input( + id="argo_drift_days", + value=str( + self.config.argo_float_config.drift_days + if self.config.argo_float_config + else "" + ), + ) + yield Label("Drift Depth (meters):") + yield Input( + id="argo_drift_depth", + value=str( + self.config.argo_float_config.drift_depth_meter + if self.config.argo_float_config + else "" + ), + ) + yield Label("Maximum Depth (meters):") + yield Input( + id="argo_max_depth", + value=str( + self.config.argo_float_config.max_depth_meter + if self.config.argo_float_config + else "" + ), + ) + yield Label("Minimum Depth (meters):") + yield Input( + id="argo_min_depth", + value=str( + self.config.argo_float_config.min_depth_meter + if self.config.argo_float_config + else "" + ), + ) + yield Label("Vertical Speed (meters/second):") + yield Input( + id="argo_vertical_speed", + value=str( + self.config.argo_float_config.vertical_speed_meter_per_second + if self.config.argo_float_config + else "" + ), + ) + + with Collapsible(title="[b]Drifter Configuration[/b]", collapsed=True): + with Container(classes="instrument-config"): + yield Label("Depth (meters):") + yield Input( + id="drifter_depth", + value=str( + self.config.drifter_config.depth_meter + if self.config.drifter_config + else "" + ), + ) + yield Label("Lifetime (minutes):") + yield Input( + id="drifter_lifetime", + value=str( + self.config.drifter_config.lifetime.total_seconds() + / 60.0 + if self.config.drifter_config + else "" + ), + ) + + except Exception as e: + yield Label(f"Error loading ship config: {e!s}") + + def on_mount(self) -> None: + self.show_hide_adcp_type(bool(self.config.adcp_config)) + + def show_hide_adcp_type(self, show: bool) -> None: + container = self.query_one("#adcp_type_container") + if show: + container.remove_class("-hidden") + else: + container.add_class("-hidden") + + @on(Switch.Changed, "#has_adcp") + def on_adcp_toggle(self, event: Switch.Changed) -> None: + self.show_hide_adcp_type(event.value) + + @on(Switch.Changed, "#adcp_deep") + def deep_changed(self, event: Switch.Changed) -> None: + if event.value: + shallow = self.query_one("#adcp_shallow", Switch) + shallow.value = False + + @on(Switch.Changed, "#adcp_shallow") + def shallow_changed(self, event: Switch.Changed) -> None: + if event.value: + deep = self.query_one("#adcp_deep", Switch) + deep.value = False + + def save_changes(self) -> None: + """Save changes to ship_config.yaml.""" + try: + # ship speed + speed_input = self.query_one("#speed") + if not speed_input.value: + raise ConfigError("Ship speed is required") + self.config.ship_speed_knots = float(speed_input.value) + + # TODO: could all of the below be a loop instead?! Extracting each of the sub parameters for each instrument config? + + # adcp config + has_adcp = self.query_one("#has_adcp", Switch).value + if has_adcp: + num_bins = self.query_one("#adcp_num_bins").value + period = self.query_one("#adcp_period").value + if not all([num_bins, period]): + raise ConfigError( + "Inputs for all ADCP configuration parameters are required when ADCP is enabled" + ) + + self.config.adcp_config = { + "max_depth_meter": -1000.0 + if self.query_one("#adcp_deep", Switch).value + else -150.0, + "num_bins": int(num_bins), + "period_minutes": float(period), + } + else: + self.config.adcp_config = None + + # T/S config + has_ts = self.query_one("#has_onboard_ts", Switch).value + if has_ts: + period = self.query_one("#ts_period").value + if not period: + raise ConfigError( + "Input for `period` parameter is required when onboard T/S is enabled" + ) + self.config.ship_underwater_st_config = { + "period_minutes": float(period) + } + else: + self.config.ship_underwater_st_config = None + + # ctd config + ctd_max = self.query_one("#ctd_max_depth").value + ctd_min = self.query_one("#ctd_min_depth").value + ctd_time = self.query_one("#ctd_stationkeeping_time").value + if not all([ctd_max, ctd_min, ctd_time]): + raise ConfigError( + "Inputs for all CTD configuration parameters must be provided" + ) + self.config.ctd_config = { + "max_depth_meter": float(ctd_max), + "min_depth_meter": float(ctd_min), + "stationkeeping_time_minutes": float(ctd_time), + } + + # CTD-BGC config + ctd_bgc_max = self.query_one("#ctd_bgc_max_depth").value + ctd_bgc_min = self.query_one("#ctd_bgc_min_depth").value + ctd_bgc_time = self.query_one("#ctd_bgc_stationkeeping_time").value + if not all([ctd_bgc_max, ctd_bgc_min, ctd_bgc_time]): + raise ConfigError( + "Inputs for all CTD-BGC configuration parameters must be provided" + ) + self.config.ctd_bgc_config = { + "max_depth_meter": float(ctd_bgc_max), + "min_depth_meter": float(ctd_bgc_min), + "stationkeeping_time_minutes": float(ctd_bgc_time), + } + + # xbt config + xbt_max = self.query_one("#xbt_max_depth").value + xbt_min = self.query_one("#xbt_min_depth").value + xbt_fall = self.query_one("#xbt_fall_speed").value + xbt_decel = self.query_one("#xbt_decel_coeff").value + if not all([xbt_max, xbt_min, xbt_fall, xbt_decel]): + raise ConfigError( + "Inputs for all XBT configuration parameters must be provided" + ) + self.config.xbt_config = { + "max_depth_meter": float(xbt_max), + "min_depth_meter": float(xbt_min), + "fall_speed_meter_per_second": float(xbt_fall), + "deceleration_coefficient": float(xbt_decel), + } + + # argo config + argo_fields = [ + "cycle_days", + "drift_days", + "drift_depth", + "max_depth", + "min_depth", + "vertical_speed", + ] + argo_values = { + field: self.query_one(f"#argo_{field}").value for field in argo_fields + } + + if not all(argo_values.values()): + raise ConfigError( + "Inputs for all Argo Float configuration parameters must be provided" + ) + self.config.argo_float_config = { + "cycle_days": float(argo_values["cycle_days"]), + "drift_days": float(argo_values["drift_days"]), + "drift_depth_meter": float(argo_values["drift_depth"]), + "max_depth_meter": float(argo_values["max_depth"]), + "min_depth_meter": float(argo_values["min_depth"]), + "vertical_speed_meter_per_second": float(argo_values["vertical_speed"]), + } + + # drifter config + drifter_depth = self.query_one("#drifter_depth").value + drifter_lifetime = self.query_one("#drifter_lifetime").value + if not all([drifter_depth, drifter_lifetime]): + raise ConfigError("All Drifter configuration fields must be provided") + self.config.drifter_config = { + "depth_meter": float(drifter_depth), + "lifetime_minutes": float(drifter_lifetime), + } + + self.config.to_yaml(f"{self.path}/ship_config.yaml") + + except Exception as e: + self.notify(f"Error saving config: {e!s}", severity="error", timeout=60) + raise + + +class ScheduleScreen(Screen): + def __init__(self, path: str): + super().__init__() + self.path = path + + def compose(self) -> ComposeResult: + with VerticalScroll(): + yield ConfigEditor(self.path) + yield ScheduleEditor(self.path) + # Move buttons to screen level + with Horizontal(): + yield Button("Save Changes", id="save_button", variant="success") + yield Button("Exit", id="exit_button", variant="error") + + @on(Button.Pressed, "#exit_button") + def exit_pressed(self) -> None: + self.app.exit() + + @on(Button.Pressed, "#save_button") + def save_pressed(self) -> None: + """Handle save button press.""" + config_editor = self.query_one(ConfigEditor) + schedule_editor = self.query_one(ScheduleEditor) + + try: + config_editor.save_changes() + schedule_editor.save_changes() + self.notify( + "Changes saved successfully", severity="information", timeout=20 + ) + except Exception as e: + self.notify(f"Error saving changes: {e!s}", severity="error", timeout=60) + + +class ScheduleApp(App): + CSS = """ + Screen { + align: center middle; + } + + ConfigEditor { + background: $panel; + padding: 1; + margin-bottom: 1; + height: auto; + } + + VerticalScroll { + width: 100%; + height: 100%; + background: $surface; + color: $text; + padding: 1; + } + + WaypointWidget { + border: solid $primary; + margin: 1; + padding: 1; + } + + Input { + margin: 1; + } + + Label { + margin-top: 1; + } + + Button { + min-width: 16; + margin: 1; + color: $text; + } + + Button.-primary { + background: $primary; + width: 100%; + } + + Button.-default { + background: $boost; + } + + Button.-success { + background: $success; + } + + Button.-error { + background: $error; + } + + Button#exit_button { + margin-left: 1; + } + + Horizontal { + height: auto; + align: left middle; + } + + Vertical { + height: auto; + } + + Switch { + margin: 0 1; + } + + #title { + text-style: bold; + padding: 1; + } + + .path { + color: $text-muted; + text-style: italic; + } + + Collapsible { + background: $boost; + margin: 1; + } + + Collapsible > .collapsible--content { + padding: 1; + } + + Collapsible > .collapsible--title { + padding: 1; + } + + Collapsible > .collapsible--content > Collapsible { + margin: 0 1; + background: $surface; + } + + .-hidden { + display: none; + } + + .ts-section { + margin-bottom: 1; + } + + .adcp-section { + margin-bottom: 1; + } + + .ship_speed { + align: left middle; + margin-bottom: 1; + } + + .ship_speed_input { + width: 20; + margin: 0 4; + } + + .instrument-config { + margin: 1; + padding: 0 2; + height: auto; + } + + .instrument-config Label { + margin-top: 1; + color: $text-muted; + } + + .instrument-config Input { + width: 30; + margin: 0 1; + } + + .year-select { + width: 20; + } + + .month-select, .day-select { + width: 18; + } + + .hour-select, .minute-select { + width: 15; + } + """ + + def __init__(self, path: str): + super().__init__() + self.path = path + + def on_mount(self) -> None: + self.push_screen(ScheduleScreen(self.path)) + self.theme = "textual-light" + + +def _plan(path: str) -> None: + app = ScheduleApp(path) + app.run() + + +# FOR DEV?! + +# if __name__ == "__main__": +# _plan(".") + +# # FOR RUNNING IN DEV MODE: +# # PYTHONPATH="/Users/Atkin004/Documents/virtualship/src" textual run --dev virtualship.cli._plan From ed245c9f2681417cf221ac7934b66c371637276d Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Fri, 6 Jun 2025 14:25:18 +0200 Subject: [PATCH 03/30] updates before valid entry checking implemented --- src/virtualship/cli/_plan.py | 333 ++++++++++++++++------------------- 1 file changed, 155 insertions(+), 178 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 94c58e01..2fd56d54 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -15,10 +15,19 @@ Switch, ) -from virtualship.errors import ConfigError from virtualship.models.location import Location from virtualship.models.schedule import Schedule, Waypoint -from virtualship.models.ship_config import InstrumentType, ShipConfig +from virtualship.models.ship_config import ( + ADCPConfig, + ArgoFloatConfig, + CTD_BGCConfig, + CTDConfig, + DrifterConfig, + InstrumentType, + ShipConfig, + ShipUnderwaterSTConfig, + XBTConfig, +) from virtualship.models.space_time_region import ( SpaceTimeRegion, SpatialRange, @@ -50,87 +59,89 @@ def __init__(self, waypoint: Waypoint, index: int): self.index = index def compose(self) -> ComposeResult: - if self.index > 0: - yield Button( - "Copy from Previous", id=f"wp{self.index}_copy", variant="warning" - ) - yield Label(f"[b]Waypoint {self.index + 1}[/b]") - yield Label("Location:") - yield Label(" Latitude:") - yield Input( - id=f"wp{self.index}_lat", - value=str(self.waypoint.location.lat), - ) - yield Label(" Longitude:") - yield Input( - id=f"wp{self.index}_lon", - value=str(self.waypoint.location.lon), - ) - yield Label("Time:") - with Horizontal(): - yield Label("Year:") - yield Select( - [ - (str(year), year) - for year in range( - datetime.datetime.now().year - 3, - datetime.datetime.now().year + 1, - ) - ], - id=f"wp{self.index}_year", - value=int(self.waypoint.time.year) - if self.waypoint.time - else Select.BLANK, - prompt="YYYY", - classes="year-select", - ) - yield Label("Month:") - yield Select( - [(f"{m:02d}", m) for m in range(1, 13)], - id=f"wp{self.index}_month", - value=int(self.waypoint.time.month) - if self.waypoint.time - else Select.BLANK, - prompt="MM", - classes="month-select", - ) - yield Label("Day:") - yield Select( - [(f"{d:02d}", d) for d in range(1, 32)], - id=f"wp{self.index}_day", - value=int(self.waypoint.time.day) - if self.waypoint.time - else Select.BLANK, - prompt="DD", - classes="day-select", - ) - yield Label("Hour:") - yield Select( - [(f"{h:02d}", h) for h in range(24)], - id=f"wp{self.index}_hour", - value=int(self.waypoint.time.hour) - if self.waypoint.time - else Select.BLANK, - prompt="hh", - classes="hour-select", + with Collapsible(title=f"[b]Waypoint {self.index + 1}[/b]", collapsed=False): + if self.index > 0: + yield Button( + "Copy from Previous", id=f"wp{self.index}_copy", variant="warning" + ) + yield Label("Location:") + yield Label(" Latitude:") + yield Input( + id=f"wp{self.index}_lat", + value=str(self.waypoint.location.lat), ) - yield Label("Min:") - yield Select( - [(f"{m:02d}", m) for m in range(0, 60, 5)], - id=f"wp{self.index}_minute", - value=int(self.waypoint.time.minute) - if self.waypoint.time - else Select.BLANK, - prompt="mm", - classes="minute-select", + yield Label(" Longitude:") + yield Input( + id=f"wp{self.index}_lon", + value=str(self.waypoint.location.lon), ) - - yield Label("Instruments:") - for instrument in InstrumentType: - is_selected = instrument in (self.waypoint.instrument or []) + yield Label("Time:") with Horizontal(): - yield Label(instrument.value) - yield Switch(value=is_selected, id=f"wp{self.index}_{instrument.value}") + yield Label("Year:") + yield Select( + [ + (str(year), year) + for year in range( + datetime.datetime.now().year - 3, + datetime.datetime.now().year + 1, + ) + ], + id=f"wp{self.index}_year", + value=int(self.waypoint.time.year) + if self.waypoint.time + else Select.BLANK, + prompt="YYYY", + classes="year-select", + ) + yield Label("Month:") + yield Select( + [(f"{m:02d}", m) for m in range(1, 13)], + id=f"wp{self.index}_month", + value=int(self.waypoint.time.month) + if self.waypoint.time + else Select.BLANK, + prompt="MM", + classes="month-select", + ) + yield Label("Day:") + yield Select( + [(f"{d:02d}", d) for d in range(1, 32)], + id=f"wp{self.index}_day", + value=int(self.waypoint.time.day) + if self.waypoint.time + else Select.BLANK, + prompt="DD", + classes="day-select", + ) + yield Label("Hour:") + yield Select( + [(f"{h:02d}", h) for h in range(24)], + id=f"wp{self.index}_hour", + value=int(self.waypoint.time.hour) + if self.waypoint.time + else Select.BLANK, + prompt="hh", + classes="hour-select", + ) + yield Label("Min:") + yield Select( + [(f"{m:02d}", m) for m in range(0, 60, 5)], + id=f"wp{self.index}_minute", + value=int(self.waypoint.time.minute) + if self.waypoint.time + else Select.BLANK, + prompt="mm", + classes="minute-select", + ) + + yield Label("Instruments:") + for instrument in InstrumentType: + is_selected = instrument in (self.waypoint.instrument or []) + with Horizontal(): + yield Label(instrument.value) + yield Switch( + value=is_selected, id=f"wp{self.index}_{instrument.value}" + ) def copy_from_previous(self) -> None: if self.index > 0: @@ -297,8 +308,8 @@ def save_changes(self) -> None: Waypoint(location=location, time=time, instrument=instruments) ) + # save self.schedule.waypoints = waypoints - self.schedule.to_yaml(f"{self.path}/schedule.yaml") # TODO: one error type for User Errors (e.g. new UserError class?) which gives information on where went wrong (e.g. as above "Inputs for all latitude, longitude and time parameters are required for waypoint {i + 1}") @@ -622,132 +633,88 @@ def save_changes(self) -> None: """Save changes to ship_config.yaml.""" try: # ship speed - speed_input = self.query_one("#speed") - if not speed_input.value: - raise ConfigError("Ship speed is required") - self.config.ship_speed_knots = float(speed_input.value) - - # TODO: could all of the below be a loop instead?! Extracting each of the sub parameters for each instrument config? + self.config.ship_speed_knots = float(self.query_one("#speed").value) # adcp config has_adcp = self.query_one("#has_adcp", Switch).value if has_adcp: - num_bins = self.query_one("#adcp_num_bins").value - period = self.query_one("#adcp_period").value - if not all([num_bins, period]): - raise ConfigError( - "Inputs for all ADCP configuration parameters are required when ADCP is enabled" - ) - - self.config.adcp_config = { - "max_depth_meter": -1000.0 + self.config.adcp_config = ADCPConfig( + max_depth_meter=-1000.0 if self.query_one("#adcp_deep", Switch).value else -150.0, - "num_bins": int(num_bins), - "period_minutes": float(period), - } + num_bins=int(self.query_one("#adcp_num_bins").value), + period=float(self.query_one("#adcp_period").value), + ) else: self.config.adcp_config = None # T/S config has_ts = self.query_one("#has_onboard_ts", Switch).value if has_ts: - period = self.query_one("#ts_period").value - if not period: - raise ConfigError( - "Input for `period` parameter is required when onboard T/S is enabled" - ) - self.config.ship_underwater_st_config = { - "period_minutes": float(period) - } + self.config.ship_underwater_st_config = ShipUnderwaterSTConfig( + period=float(self.query_one("#ts_period").value) + ) else: self.config.ship_underwater_st_config = None # ctd config - ctd_max = self.query_one("#ctd_max_depth").value - ctd_min = self.query_one("#ctd_min_depth").value - ctd_time = self.query_one("#ctd_stationkeeping_time").value - if not all([ctd_max, ctd_min, ctd_time]): - raise ConfigError( - "Inputs for all CTD configuration parameters must be provided" - ) - self.config.ctd_config = { - "max_depth_meter": float(ctd_max), - "min_depth_meter": float(ctd_min), - "stationkeeping_time_minutes": float(ctd_time), - } + self.config.ctd_config = CTDConfig( + max_depth_meter=float(self.query_one("#ctd_max_depth").value), + min_depth_meter=float(self.query_one("#ctd_min_depth").value), + stationkeeping_time=float( + self.query_one("#ctd_stationkeeping_time").value + ), + ) # CTD-BGC config - ctd_bgc_max = self.query_one("#ctd_bgc_max_depth").value - ctd_bgc_min = self.query_one("#ctd_bgc_min_depth").value - ctd_bgc_time = self.query_one("#ctd_bgc_stationkeeping_time").value - if not all([ctd_bgc_max, ctd_bgc_min, ctd_bgc_time]): - raise ConfigError( - "Inputs for all CTD-BGC configuration parameters must be provided" - ) - self.config.ctd_bgc_config = { - "max_depth_meter": float(ctd_bgc_max), - "min_depth_meter": float(ctd_bgc_min), - "stationkeeping_time_minutes": float(ctd_bgc_time), - } + self.config.ctd_bgc_config = CTD_BGCConfig( + max_depth_meter=float(self.query_one("#ctd_bgc_max_depth").value), + min_depth_meter=float(self.query_one("#ctd_bgc_min_depth").value), + stationkeeping_time=float( + self.query_one("#ctd_bgc_stationkeeping_time").value + ), + ) # xbt config - xbt_max = self.query_one("#xbt_max_depth").value - xbt_min = self.query_one("#xbt_min_depth").value - xbt_fall = self.query_one("#xbt_fall_speed").value - xbt_decel = self.query_one("#xbt_decel_coeff").value - if not all([xbt_max, xbt_min, xbt_fall, xbt_decel]): - raise ConfigError( - "Inputs for all XBT configuration parameters must be provided" - ) - self.config.xbt_config = { - "max_depth_meter": float(xbt_max), - "min_depth_meter": float(xbt_min), - "fall_speed_meter_per_second": float(xbt_fall), - "deceleration_coefficient": float(xbt_decel), - } + self.config.xbt_config = XBTConfig( + min_depth_meter=float(self.query_one("#xbt_min_depth").value), + max_depth_meter=float(self.query_one("#xbt_max_depth").value), + fall_speed_meter_per_second=float( + self.query_one("#xbt_fall_speed").value + ), + deceleration_coefficient=float( + self.query_one("#xbt_decel_coeff").value + ), + ) # argo config - argo_fields = [ - "cycle_days", - "drift_days", - "drift_depth", - "max_depth", - "min_depth", - "vertical_speed", - ] - argo_values = { - field: self.query_one(f"#argo_{field}").value for field in argo_fields - } - - if not all(argo_values.values()): - raise ConfigError( - "Inputs for all Argo Float configuration parameters must be provided" - ) - self.config.argo_float_config = { - "cycle_days": float(argo_values["cycle_days"]), - "drift_days": float(argo_values["drift_days"]), - "drift_depth_meter": float(argo_values["drift_depth"]), - "max_depth_meter": float(argo_values["max_depth"]), - "min_depth_meter": float(argo_values["min_depth"]), - "vertical_speed_meter_per_second": float(argo_values["vertical_speed"]), - } + self.config.argo_float_config = ArgoFloatConfig( + min_depth_meter=float(self.query_one("#argo_min_depth").value), + max_depth_meter=float(self.query_one("#argo_max_depth").value), + drift_depth_meter=float(self.query_one("#argo_drift_depth").value), + vertical_speed_meter_per_second=float( + self.query_one("#argo_vertical_speed").value + ), + cycle_days=float(self.query_one("#argo_cycle_days").value), + drift_days=float(self.query_one("#argo_drift_days").value), + ) # drifter config - drifter_depth = self.query_one("#drifter_depth").value - drifter_lifetime = self.query_one("#drifter_lifetime").value - if not all([drifter_depth, drifter_lifetime]): - raise ConfigError("All Drifter configuration fields must be provided") - self.config.drifter_config = { - "depth_meter": float(drifter_depth), - "lifetime_minutes": float(drifter_lifetime), - } + self.config.drifter_config = DrifterConfig( + depth_meter=float(self.query_one("#drifter_depth").value), + lifetime=float(self.query_one("#drifter_lifetime").value), + ) + # save self.config.to_yaml(f"{self.path}/ship_config.yaml") + # TODO: one error type for User Errors (e.g. new UserError class?) which gives information on where went wrong (e.g. as above "Inputs for all latitude, longitude and time parameters are required for waypoint {i + 1}") except Exception as e: self.notify(f"Error saving config: {e!s}", severity="error", timeout=60) raise + # TODO: then also another type of generic error (e.g. the existing except Exception as e) where there is a bug in the code but it's unknown where (i.e. it has got past all the other tests before being committed) + # TODO: and add a message saying "please raise an issue on the VirtualShip GitHub issue tracker" + # TODO: also provide a full traceback which the user can copy to their issue (may need to quit the application to provide this option) class ScheduleScreen(Screen): @@ -806,8 +773,18 @@ class ScheduleApp(App): } WaypointWidget { - border: solid $primary; + padding: 0; + margin: 0; + border: none; + } + + WaypointWidget > Collapsible { margin: 1; + background: $boost; + border: solid $primary; + } + + WaypointWidget > Collapsible > .collapsible--content { padding: 1; } From a863ae76256116fc66b3a45e56d7f15515e2aca7 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Fri, 6 Jun 2025 15:10:00 +0200 Subject: [PATCH 04/30] starting to introduce more precise error messages --- src/virtualship/cli/_plan.py | 124 +++++++++++++++++++++++++++-------- src/virtualship/errors.py | 6 ++ 2 files changed, 101 insertions(+), 29 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 2fd56d54..ca71ce14 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -1,4 +1,5 @@ import datetime +from typing import ClassVar from textual import on from textual.app import App, ComposeResult @@ -15,6 +16,7 @@ Switch, ) +from virtualship.errors import UserError from virtualship.models.location import Location from virtualship.models.schedule import Schedule, Waypoint from virtualship.models.ship_config import ( @@ -52,6 +54,10 @@ # TODO: Can the whole lot be tidied up by moving some classes/methods to a new directory/files?! +# ------ +# TODO: the valid entry + User errors etc. need to be added to the Schedule editor (currently on the Config Editor) + + class WaypointWidget(Static): def __init__(self, waypoint: Waypoint, index: int): super().__init__() @@ -259,7 +265,7 @@ def compose(self) -> ComposeResult: except Exception as e: yield Label(f"Error loading schedule: {e!s}") - def save_changes(self) -> None: + def save_changes(self) -> bool: """Save changes to schedule.yaml.""" try: # spacetime region @@ -311,17 +317,21 @@ def save_changes(self) -> None: # save self.schedule.waypoints = waypoints self.schedule.to_yaml(f"{self.path}/schedule.yaml") + return True - # TODO: one error type for User Errors (e.g. new UserError class?) which gives information on where went wrong (e.g. as above "Inputs for all latitude, longitude and time parameters are required for waypoint {i + 1}") except Exception as e: self.notify(f"Error saving schedule: {e!r}", severity="error", timeout=60) - raise - # TODO: then also another type of generic error (e.g. the existing except Exception as e) where there is a bug in the code but it's unknown where (i.e. it has got past all the other tests before being committed) - # TODO: and add a message saying "please raise an issue on the VirtualShip GitHub issue tracker" - # TODO: also provide a full traceback which the user can copy to their issue (may need to quit the application to provide this option) + return False class ConfigEditor(Container): + DEFAULT_ADCP_CONFIG: ClassVar[dict[str, float]] = { + "num_bins": 40, + "period_minutes": 5.0, + } + + DEFAULT_TS_CONFIG: ClassVar[dict[str, float]] = {"period_minutes": 5.0} + # TODO: Also incorporate verify methods! def __init__(self, path: str): @@ -613,9 +623,33 @@ def show_hide_adcp_type(self, show: bool) -> None: else: container.add_class("-hidden") + def _set_adcp_default_values(self): + self.query_one("#adcp_num_bins").value = str( + self.DEFAULT_ADCP_CONFIG["num_bins"] + ) + self.query_one("#adcp_period").value = str( + self.DEFAULT_ADCP_CONFIG["period_minutes"] + ) + self.query_one("#adcp_shallow").value = True + self.query_one("#adcp_deep").value = False + + def _set_ts_default_values(self): + self.query_one("#ts_period").value = str( + self.DEFAULT_TS_CONFIG["period_minutes"] + ) + @on(Switch.Changed, "#has_adcp") def on_adcp_toggle(self, event: Switch.Changed) -> None: self.show_hide_adcp_type(event.value) + if event.value and not self.config.adcp_config: + # ADCP was turned on and was previously null + self._set_adcp_default_values() + + @on(Switch.Changed, "#has_onboard_ts") + def on_ts_toggle(self, event: Switch.Changed) -> None: + if event.value and not self.config.ship_underwater_st_config: + # T/S was turned on and was previously null + self._set_ts_default_values() @on(Switch.Changed, "#adcp_deep") def deep_changed(self, event: Switch.Changed) -> None: @@ -629,21 +663,44 @@ def shallow_changed(self, event: Switch.Changed) -> None: deep = self.query_one("#adcp_deep", Switch) deep.value = False - def save_changes(self) -> None: + def _try_create_config(self, config_class, input_values, config_name): + """Helper to create config with error handling.""" + try: + return config_class(**input_values) + except ValueError as e: + field = ( + str(e).split()[0] if str(e).split() else "unknown field" + ) # extract field name from Pydantic error + raise UserError( + f"Invalid {config_name} configuration: {field} - {e!s}" + ) from e + + def save_changes(self) -> bool: """Save changes to ship_config.yaml.""" try: # ship speed - self.config.ship_speed_knots = float(self.query_one("#speed").value) - + try: + speed = float(self.query_one("#speed").value) + if speed <= 0: + raise UserError("Ship speed must be greater than 0") + self.config.ship_speed_knots = speed + except ValueError: + raise UserError("Ship speed must be a valid number") from None + + # TODO: more precise user errors etc need to be added for the below as well! # adcp config has_adcp = self.query_one("#has_adcp", Switch).value if has_adcp: - self.config.adcp_config = ADCPConfig( - max_depth_meter=-1000.0 - if self.query_one("#adcp_deep", Switch).value - else -150.0, - num_bins=int(self.query_one("#adcp_num_bins").value), - period=float(self.query_one("#adcp_period").value), + self.config.adcp_config = self._try_create_config( + ADCPConfig, + { + "max_depth_meter": -1000.0 + if self.query_one("#adcp_deep", Switch).value + else -150.0, + "num_bins": int(self.query_one("#adcp_num_bins").value), + "period": float(self.query_one("#adcp_period").value), + }, + "ADCP", ) else: self.config.adcp_config = None @@ -651,8 +708,10 @@ def save_changes(self) -> None: # T/S config has_ts = self.query_one("#has_onboard_ts", Switch).value if has_ts: - self.config.ship_underwater_st_config = ShipUnderwaterSTConfig( - period=float(self.query_one("#ts_period").value) + self.config.ship_underwater_st_config = self._try_create_config( + ShipUnderwaterSTConfig, + {"period": float(self.query_one("#ts_period").value)}, + "Temperature/Salinity", ) else: self.config.ship_underwater_st_config = None @@ -707,14 +766,19 @@ def save_changes(self) -> None: # save self.config.to_yaml(f"{self.path}/ship_config.yaml") + return True - # TODO: one error type for User Errors (e.g. new UserError class?) which gives information on where went wrong (e.g. as above "Inputs for all latitude, longitude and time parameters are required for waypoint {i + 1}") - except Exception as e: - self.notify(f"Error saving config: {e!s}", severity="error", timeout=60) - raise - # TODO: then also another type of generic error (e.g. the existing except Exception as e) where there is a bug in the code but it's unknown where (i.e. it has got past all the other tests before being committed) - # TODO: and add a message saying "please raise an issue on the VirtualShip GitHub issue tracker" - # TODO: also provide a full traceback which the user can copy to their issue (may need to quit the application to provide this option) + except UserError as e: + self.notify(f"Input error: {e!s}", severity="error", timeout=60) + return False + except Exception: + # TODO: and need to make the error also print the traceback to paste into the GitHub issue!!! + self.notify( + "An unexpected error occurred. Please report this issue on GitHub.", + severity="error", + timeout=60, + ) + return False class ScheduleScreen(Screen): @@ -742,11 +806,13 @@ def save_pressed(self) -> None: schedule_editor = self.query_one(ScheduleEditor) try: - config_editor.save_changes() - schedule_editor.save_changes() - self.notify( - "Changes saved successfully", severity="information", timeout=20 - ) + config_saved = config_editor.save_changes() + schedule_saved = schedule_editor.save_changes() + + if config_saved and schedule_saved: + self.notify( + "Changes saved successfully", severity="information", timeout=20 + ) except Exception as e: self.notify(f"Error saving changes: {e!s}", severity="error", timeout=60) diff --git a/src/virtualship/errors.py b/src/virtualship/errors.py index 838a62a8..882091fa 100644 --- a/src/virtualship/errors.py +++ b/src/virtualship/errors.py @@ -26,3 +26,9 @@ class ConfigError(RuntimeError): """An error in the config.""" pass + + +class UserError(Exception): + """Error raised when user input is invalid.""" + + pass From 0b8f8f108d824929e4687c57f3f69a9b70db62f8 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Fri, 6 Jun 2025 15:14:34 +0200 Subject: [PATCH 05/30] add textual as dependency --- environment.yml | 1 + pyproject.toml | 1 + 2 files changed, 2 insertions(+) diff --git a/environment.yml b/environment.yml index c5317c60..bce0d80f 100644 --- a/environment.yml +++ b/environment.yml @@ -14,6 +14,7 @@ dependencies: - copernicusmarine >= 2 - openpyxl - yaspin + - textual # linting - pre-commit diff --git a/pyproject.toml b/pyproject.toml index 1932d563..e06eaf44 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -35,6 +35,7 @@ dependencies = [ "PyYAML", "copernicusmarine >= 2", "yaspin", + "textual", ] [project.urls] From 74f69dcf33a794d8628ea88aa8fb74147f3088a5 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Mon, 16 Jun 2025 17:10:00 +0200 Subject: [PATCH 06/30] working towards better validation of inputs --- src/virtualship/cli/_plan.py | 162 +++++++++++++++++++++-------------- 1 file changed, 98 insertions(+), 64 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index ca71ce14..86e865dd 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -1,6 +1,7 @@ import datetime from typing import ClassVar +from pydantic import ValidationError from textual import on from textual.app import App, ComposeResult from textual.containers import Container, Horizontal, VerticalScroll @@ -320,7 +321,12 @@ def save_changes(self) -> bool: return True except Exception as e: - self.notify(f"Error saving schedule: {e!r}", severity="error", timeout=60) + self.notify( + f"Error saving schedule: {e!r}", + severity="error", + timeout=60, + markup=False, + ) return False @@ -687,92 +693,115 @@ def save_changes(self) -> bool: except ValueError: raise UserError("Ship speed must be a valid number") from None - # TODO: more precise user errors etc need to be added for the below as well! - # adcp config + # ADCP config has_adcp = self.query_one("#has_adcp", Switch).value if has_adcp: - self.config.adcp_config = self._try_create_config( - ADCPConfig, - { - "max_depth_meter": -1000.0 - if self.query_one("#adcp_deep", Switch).value - else -150.0, - "num_bins": int(self.query_one("#adcp_num_bins").value), - "period": float(self.query_one("#adcp_period").value), - }, - "ADCP", - ) + try: + self.config.adcp_config = self._try_create_config( + ADCPConfig, + { + "max_depth_meter": -1000.0 + if self.query_one("#adcp_deep", Switch).value + else -150.0, + "num_bins": int(self.query_one("#adcp_num_bins").value), + "period": float(self.query_one("#adcp_period").value), + }, + "ADCP", + ) + except (ValueError, ValidationError) as e: + raise UserError(f"Invalid ADCP configuration: {e!s}") from e else: self.config.adcp_config = None # T/S config has_ts = self.query_one("#has_onboard_ts", Switch).value if has_ts: - self.config.ship_underwater_st_config = self._try_create_config( - ShipUnderwaterSTConfig, - {"period": float(self.query_one("#ts_period").value)}, - "Temperature/Salinity", - ) + try: + self.config.ship_underwater_st_config = self._try_create_config( + ShipUnderwaterSTConfig, + {"period": float(self.query_one("#ts_period").value)}, + "Temperature/Salinity", + ) + except (ValueError, ValidationError) as e: + raise UserError( + f"Invalid Temperature/Salinity configuration: {e!s}" + ) from e else: self.config.ship_underwater_st_config = None - # ctd config - self.config.ctd_config = CTDConfig( - max_depth_meter=float(self.query_one("#ctd_max_depth").value), - min_depth_meter=float(self.query_one("#ctd_min_depth").value), - stationkeeping_time=float( - self.query_one("#ctd_stationkeeping_time").value - ), - ) + # CTD config + try: + self.config.ctd_config = CTDConfig( + max_depth_meter=float(self.query_one("#ctd_max_depth").value), + min_depth_meter=float(self.query_one("#ctd_min_depth").value), + stationkeeping_time=float( + self.query_one("#ctd_stationkeeping_time").value + ), + ) + except (ValueError, ValidationError) as e: + raise UserError(f"Invalid CTD configuration: {e!s}") from e # CTD-BGC config - self.config.ctd_bgc_config = CTD_BGCConfig( - max_depth_meter=float(self.query_one("#ctd_bgc_max_depth").value), - min_depth_meter=float(self.query_one("#ctd_bgc_min_depth").value), - stationkeeping_time=float( - self.query_one("#ctd_bgc_stationkeeping_time").value - ), - ) + try: + self.config.ctd_bgc_config = CTD_BGCConfig( + max_depth_meter=float(self.query_one("#ctd_bgc_max_depth").value), + min_depth_meter=float(self.query_one("#ctd_bgc_min_depth").value), + stationkeeping_time=float( + self.query_one("#ctd_bgc_stationkeeping_time").value + ), + ) + except (ValueError, ValidationError) as e: + raise UserError(f"Invalid CTD-BGC configuration: {e!s}") from e - # xbt config - self.config.xbt_config = XBTConfig( - min_depth_meter=float(self.query_one("#xbt_min_depth").value), - max_depth_meter=float(self.query_one("#xbt_max_depth").value), - fall_speed_meter_per_second=float( - self.query_one("#xbt_fall_speed").value - ), - deceleration_coefficient=float( - self.query_one("#xbt_decel_coeff").value - ), - ) + # XBT config + try: + self.config.xbt_config = XBTConfig( + min_depth_meter=float(self.query_one("#xbt_min_depth").value), + max_depth_meter=float(self.query_one("#xbt_max_depth").value), + fall_speed_meter_per_second=float( + self.query_one("#xbt_fall_speed").value + ), + deceleration_coefficient=float( + self.query_one("#xbt_decel_coeff").value + ), + ) + except (ValueError, ValidationError) as e: + raise UserError(f"Invalid XBT configuration: {e!s}") from e - # argo config - self.config.argo_float_config = ArgoFloatConfig( - min_depth_meter=float(self.query_one("#argo_min_depth").value), - max_depth_meter=float(self.query_one("#argo_max_depth").value), - drift_depth_meter=float(self.query_one("#argo_drift_depth").value), - vertical_speed_meter_per_second=float( - self.query_one("#argo_vertical_speed").value - ), - cycle_days=float(self.query_one("#argo_cycle_days").value), - drift_days=float(self.query_one("#argo_drift_days").value), - ) + # Argo config + try: + self.config.argo_float_config = ArgoFloatConfig( + min_depth_meter=float(self.query_one("#argo_min_depth").value), + max_depth_meter=float(self.query_one("#argo_max_depth").value), + drift_depth_meter=float(self.query_one("#argo_drift_depth").value), + vertical_speed_meter_per_second=float( + self.query_one("#argo_vertical_speed").value + ), + cycle_days=float(self.query_one("#argo_cycle_days").value), + drift_days=float(self.query_one("#argo_drift_days").value), + ) + except (ValueError, ValidationError) as e: + raise UserError(f"Invalid Argo Float configuration: {e!s}") from e - # drifter config - self.config.drifter_config = DrifterConfig( - depth_meter=float(self.query_one("#drifter_depth").value), - lifetime=float(self.query_one("#drifter_lifetime").value), - ) + # Drifter config + try: + self.config.drifter_config = DrifterConfig( + depth_meter=float(self.query_one("#drifter_depth").value), + lifetime=float(self.query_one("#drifter_lifetime").value), + ) + except (ValueError, ValidationError) as e: + raise UserError(f"Invalid Drifter configuration: {e!s}") from e # save self.config.to_yaml(f"{self.path}/ship_config.yaml") return True except UserError as e: - self.notify(f"Input error: {e!s}", severity="error", timeout=60) + self.notify( + f"Input error: {e!s}", severity="error", timeout=60, markup=False + ) return False except Exception: - # TODO: and need to make the error also print the traceback to paste into the GitHub issue!!! self.notify( "An unexpected error occurred. Please report this issue on GitHub.", severity="error", @@ -814,7 +843,12 @@ def save_pressed(self) -> None: "Changes saved successfully", severity="information", timeout=20 ) except Exception as e: - self.notify(f"Error saving changes: {e!s}", severity="error", timeout=60) + self.notify( + f"Error saving changes: {e!s}", + severity="error", + timeout=60, + markup=False, + ) class ScheduleApp(App): From e17166702253c429429a2dd60137cbfc88bebe3b Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 17 Jun 2025 14:09:17 +0200 Subject: [PATCH 07/30] launch TUI in the browser --- src/virtualship/cli/_plan.py | 22 ++++++++++++---------- src/virtualship/cli/commands.py | 5 +++-- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 86e865dd..5d9ecd92 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -1,4 +1,5 @@ import datetime +import sys from typing import ClassVar from pydantic import ValidationError @@ -37,6 +38,9 @@ TimeRange, ) +# TODO: add TEXTUAL_SERVE dependency if end up using it... + + # TODO: I wonder whether it would be a good idea to be able to recognise all the subcomponents of a pydantic model # TODO: such as the instrument configuration, and be able to loop (?) rather than explicitly type each one out # TODO: maybe in some kind of form where it's stored externally so as to facilitate it being in one place and helps there be more ease of adding instruments @@ -1024,15 +1028,13 @@ def on_mount(self) -> None: self.theme = "textual-light" -def _plan(path: str) -> None: +if __name__ == "__main__": + # parse path + if len(sys.argv) > 1: + path = sys.argv[1] + else: + raise ValueError("No path argument provided") + + # run app app = ScheduleApp(path) app.run() - - -# FOR DEV?! - -# if __name__ == "__main__": -# _plan(".") - -# # FOR RUNNING IN DEV MODE: -# # PYTHONPATH="/Users/Atkin004/Documents/virtualship/src" textual run --dev virtualship.cli._plan diff --git a/src/virtualship/cli/commands.py b/src/virtualship/cli/commands.py index 7fb4b529..1c9ceafb 100644 --- a/src/virtualship/cli/commands.py +++ b/src/virtualship/cli/commands.py @@ -1,10 +1,10 @@ from pathlib import Path import click +from textual_serve.server import Server from virtualship import utils from virtualship.cli._fetch import _fetch -from virtualship.cli._plan import _plan from virtualship.expedition.do_expedition import do_expedition from virtualship.utils import ( SCHEDULE, @@ -75,7 +75,8 @@ def init(path, from_mfp): ) def plan(path): """Launch UI to help build schedule and ship config files.""" - _plan(Path(path)) + server = Server(f"python -m virtualship.cli._plan {Path(path)}") + server.serve() @click.command() From 4abdac4bb1734d9428c25034944035dc6db593be Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 17 Jun 2025 14:29:36 +0200 Subject: [PATCH 08/30] launch automatically to browser --- src/virtualship/cli/commands.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/virtualship/cli/commands.py b/src/virtualship/cli/commands.py index 1c9ceafb..6dabd5f2 100644 --- a/src/virtualship/cli/commands.py +++ b/src/virtualship/cli/commands.py @@ -1,3 +1,4 @@ +import webbrowser from pathlib import Path import click @@ -74,8 +75,13 @@ def init(path, from_mfp): type=click.Path(exists=False, file_okay=False, dir_okay=True), ) def plan(path): - """Launch UI to help build schedule and ship config files.""" - server = Server(f"python -m virtualship.cli._plan {Path(path)}") + """Launch UI to help build schedule and ship config files. Opens in web browser, hosted on the user's local machine only.""" + server = Server( + command=f"python -m virtualship.cli._plan {Path(path)}", + title="VirtualShip plan", + ) + url = "http://localhost:8000" + webbrowser.open(url) server.serve() From ac6af8fb4bee6db468c8b1e63170113bb2f6dc39 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 18 Jun 2025 17:06:18 +0200 Subject: [PATCH 09/30] validation via textual Input arguments working but with bugs --- src/virtualship/cli/_plan.py | 519 +++++++++++++++++++++++++++----- src/virtualship/cli/commands.py | 3 +- 2 files changed, 454 insertions(+), 68 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 5d9ecd92..b87dd9f7 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -7,6 +7,7 @@ from textual.app import App, ComposeResult from textual.containers import Container, Horizontal, VerticalScroll from textual.screen import Screen +from textual.validation import Function from textual.widgets import ( Button, Collapsible, @@ -38,28 +39,31 @@ TimeRange, ) -# TODO: add TEXTUAL_SERVE dependency if end up using it... +# TODO: need to do a big round of edits where tidy up all the error messaging. +# - given a lot of the validation of entry is done natively now in textual, this should mean that all other +# errors associated with inputting to the app should be 'unexpected' and therefore call to the report-to-GitHub workflow +# - errors associated with ship_config and schedule.verify() should, however, provide in-UI messaging ideally +# to notify the user that their selections are not valid... +# TODO: add TEXTUAL_SERVE dependency if end up using it... -# TODO: I wonder whether it would be a good idea to be able to recognise all the subcomponents of a pydantic model -# TODO: such as the instrument configuration, and be able to loop (?) rather than explicitly type each one out -# TODO: maybe in some kind of form where it's stored externally so as to facilitate it being in one place and helps there be more ease of adding instruments -# TODO: down the line without having to make changes in so many places...? -# TODO: but is this possible given the different requirements/units etc. that they all require? +# TODO: add a flag to the `virtualship plan` command which allows toggle between hosting the UI in terminal or web browser.. -# TODO: NEED TO MAKE SURE THE CHANGES OVERWRITTEN TO THE YAML FILES DO IT IN THE RIGHT ORDER, IT DOESN'T SAVE PROPERLY AT CURRENT -# TODO: implement action for 'Save' button -# TODO: probably with a second pop up that says "are you sure you want to save, this will overwrite schedule. and ship_config.yaml" +# TODO: need to handle how to add the start_ and end_ datetimes automatically to Space-Time Region! +# TODO: because I think the .yaml that comes from `virtualship init` will not have these as standard and we don't expect students to go add themselves... +# TODO: look through all error handling in both _plan.py and command.py scripts to check redudancy +# TODO: also add more error handling for 1) if there are no yamls found in the specified *path* +# TODO: and also for if there are errors in the yaml being opened, for example if what should be a float has been manaully changed to something invalid +# TODO: or if indeed any of the pydantic model components associated with the yamls are missing -# TODO: error handling needs to be better! E.g. if elements of the pydantic model are missing then they should be specifically flagged -# TODO: e.g. if you remove ship_speed_knots from the ship_config.yaml it fails but the error message is misleading (starts going on about adcp_config) +# TODO: make sure 'Save' writes the yaml file components in the right order? Or does this not matter? +# TODO: test via full run through of an expedition using yamls edited by `virtualship plan` +# TODO: implement action for 'Save' button # TODO: Can the whole lot be tidied up by moving some classes/methods to a new directory/files?! - -# ------ # TODO: the valid entry + User errors etc. need to be added to the Schedule editor (currently on the Config Editor) @@ -70,7 +74,7 @@ def __init__(self, waypoint: Waypoint, index: int): self.index = index def compose(self) -> ComposeResult: - with Collapsible(title=f"[b]Waypoint {self.index + 1}[/b]", collapsed=False): + with Collapsible(title=f"[b]Waypoint {self.index + 1}[/b]", collapsed=True): if self.index > 0: yield Button( "Copy from Previous", id=f"wp{self.index}_copy", variant="warning" @@ -158,15 +162,7 @@ def copy_from_previous(self) -> None: if self.index > 0: schedule_editor = self.parent if schedule_editor: - prev_lat = schedule_editor.query_one(f"#wp{self.index - 1}_lat") - prev_lon = schedule_editor.query_one(f"#wp{self.index - 1}_lon") - curr_lat = self.query_one(f"#wp{self.index}_lat") - curr_lon = self.query_one(f"#wp{self.index}_lon") - - if prev_lat and prev_lon and curr_lat and curr_lon: - curr_lat.value = prev_lat.value - curr_lon.value = prev_lon.value - + # Only copy time components and instruments, not lat/lon time_components = ["year", "month", "day", "hour", "minute"] for comp in time_components: prev = schedule_editor.query_one(f"#wp{self.index - 1}_{comp}") @@ -201,7 +197,7 @@ def compose(self) -> ComposeResult: yield Rule(line_style="heavy") with Collapsible( - title="[b]Waypoints[/b]", + title="[b]Waypoints & Instrument Selection[/b]", collapsed=True, ): for i, waypoint in enumerate(self.schedule.waypoints): @@ -272,6 +268,8 @@ def compose(self) -> ComposeResult: def save_changes(self) -> bool: """Save changes to schedule.yaml.""" + # TODO: SAVE_CHANGES() NEEDS TO BE LARGELY RE-WORKED NOW THAT MORE VALIDATION IS BUILT INTO THE INPUTS + # TODO: and should proabably now be more focussed on .verify() methods try: # spacetime region spatial_range = SpatialRange( @@ -334,6 +332,138 @@ def save_changes(self) -> bool: return False +# TODO: perhaps these methods could be housed elsewhere for neatness?! + + +def get_field_type(model_class, field_name): + """Get Pydantic model class data type.""" + return model_class.model_fields[field_name].annotation + + +def type_to_textual(field_type): + """Convert data type to str which Textual can interpret for type = setting in Input objects.""" + if field_type in (float, datetime.timedelta): + return "number" + elif field_type is int: + return "integer" + else: + return "text" + + +# Pydantic field constraint attributes used for validation and introspection +FIELD_CONSTRAINT_ATTRS = ("gt", "ge", "lt", "le") + + +def get_field_conditions(model_class, field_name): + """Determine and return what conditions (and associated reference value) a Pydantic model sets on inputs.""" + field_info = model_class.model_fields[field_name] + conditions = {} + for meta in field_info.metadata: + for attr in dir(meta): + if not attr.startswith("_") and getattr(meta, attr) is not None: + if attr in FIELD_CONSTRAINT_ATTRS: + conditions[attr] = getattr(meta, attr) + else: + raise ValueError( + f"Unexpected constraint '{attr}' found on field '{field_name}'. " + f"Allowed constraints: {FIELD_CONSTRAINT_ATTRS}" + ) + return list(conditions.keys()), list(conditions.values()) + + +def make_validator(condition, reference, value_type): + """ + Make a validator function based on the Pydantic model field conditions returned by get_field_conditions(). + + TODO: Textual validator tools do not currently support additional arguments (such as 'reference') being fed into the validator functions (such as is_gt0) at present. + TODO: Therefore, reference values cannot be fed in dynamically and necessitates hard coding depending onthe condition and reference value combination. + TODO: At present, Pydantic models only require gt/ge/lt/le relative to **0.0** so `reference` is always checked as being == 0.0 + TODO: Additional custom conditions can be "hard-coded" as new condition and reference combinations + TODO: if Pydantic model specifications change in the future and/or new instruments are added to VirtualShip etc. + TODO: Perhaps there's scope here though for a more robust implementation in a future PR... + + Note, docstrings describing the conditional are required in the child functions (e.g. is_gt0), and is checked by require_docstring(). + Docstrings will be used to generate informative UI invalid entry messages, so them being informative and accurate is important! + + """ + + def require_docstring(func): + if func.__doc__ is None or not func.__doc__.strip(): + raise ValueError(f"Function '{func.__name__}' must have a docstring.") + return func + + def convert(value): + try: + return value_type(value) + except Exception: + return None + + if value_type in (float, int) and reference == 0.0: + ref_zero = 0.0 + elif value_type is datetime.timedelta and reference == datetime.timedelta(): + ref_zero = datetime.timedelta() + else: + raise ValueError( + f"Unsupported value_type/reference combination: {value_type}, {reference}" + ) + + if condition == "gt" and reference == ref_zero: + + @require_docstring + def is_gt0(value: str) -> bool: + """Greater than 0.""" + v = convert(value) + return v is not None and v > ref_zero + + return is_gt0 + + if condition == "ge" and reference == ref_zero: + + @require_docstring + def is_ge0(value: str) -> bool: + """Greater than or equal to 0.""" + v = convert(value) + return v is not None and v >= ref_zero + + return is_ge0 + + if condition == "lt" and reference == ref_zero: + + @require_docstring + def is_lt0(value: str) -> bool: + """Less than 0.""" + v = convert(value) + return v is not None and v < ref_zero + + return is_lt0 + + if condition == "le" and reference == ref_zero: + + @require_docstring + def is_le0(value: str) -> bool: + """Less than or equal to 0.""" + v = convert(value) + return v is not None and v <= ref_zero + + return is_le0 + + else: + raise ValueError( + f"Unknown condition: {condition} and reference value: {reference} combination." + ) + + +def group_validators(model, attr): + """Bundle all validators for Input into singular list.""" + return [ + make_validator(cond, ref, get_field_type(model, attr)) + for cond, ref in zip( + *get_field_conditions(model, attr), + strict=False, + ) + ] + + class ConfigEditor(Container): DEFAULT_ADCP_CONFIG: ClassVar[dict[str, float]] = { "num_bins": 40, @@ -355,12 +485,21 @@ def compose(self) -> ComposeResult: yield Label("[b]Ship Config Editor[/b]", id="title", markup=True) yield Rule(line_style="heavy") - # ship speed and onboard measurement selection - with Collapsible(title="[b]Ship Speed and Onboard Measurements[/b]"): + with Collapsible(title="[b]Ship Speed & Onboard Measurements[/b]"): + attr = "ship_speed_knots" + validators = group_validators(ShipConfig, attr) with Horizontal(classes="ship_speed"): yield Label("[b]Ship Speed (knots):[/b]") yield Input( id="speed", + type=type_to_textual(get_field_type(ShipConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], classes="ship_speed_input", placeholder="knots", value=str( @@ -369,6 +508,7 @@ def compose(self) -> ComposeResult: else "" ), ) + yield Label("", id="validation-failure-label", classes="-hidden") with Horizontal(classes="ts-section"): yield Label("[b]Onboard Temperature/Salinity:[/b]") @@ -397,24 +537,43 @@ def compose(self) -> ComposeResult: title="[b]Instrument Configurations[/b] (advanced users only)", collapsed=True, ): - # TODO: could all of the below be a loop instead?! Extracting each of the sub parameters for each instrument config? - with Collapsible( title="[b]Onboard ADCP Configuration[/b]", collapsed=True ): with Container(classes="instrument-config"): + attr = "num_bins" + validators = group_validators(ADCPConfig, attr) yield Label("Number of Bins:") yield Input( id="adcp_num_bins", + type=type_to_textual(get_field_type(ADCPConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.adcp_config.num_bins if self.config.adcp_config else "" ), ) + + attr = "period" + validators = group_validators(ADCPConfig, attr) yield Label("Period (minutes):") yield Input( id="adcp_period", + type=type_to_textual(get_field_type(ADCPConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.adcp_config.period.total_seconds() / 60.0 if self.config.adcp_config @@ -427,9 +586,21 @@ def compose(self) -> ComposeResult: collapsed=True, ): with Container(classes="instrument-config"): + attr = "period" + validators = group_validators(ShipUnderwaterSTConfig, attr) yield Label("Period (minutes):") yield Input( id="ts_period", + type=type_to_textual( + get_field_type(ShipUnderwaterSTConfig, attr) + ), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.ship_underwater_st_config.period.total_seconds() / 60.0 @@ -440,27 +611,57 @@ def compose(self) -> ComposeResult: with Collapsible(title="[b]CTD Configuration[/b]", collapsed=True): with Container(classes="instrument-config"): + attr = "max_depth_meter" + validators = group_validators(CTDConfig, attr) yield Label("Maximum Depth (meters):") yield Input( id="ctd_max_depth", + type=type_to_textual(get_field_type(CTDConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.ctd_config.max_depth_meter if self.config.ctd_config else "" ), ) + attr = "min_depth_meter" + validators = group_validators(CTDConfig, attr) yield Label("Minimum Depth (meters):") yield Input( id="ctd_min_depth", + type=type_to_textual(get_field_type(CTDConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.ctd_config.min_depth_meter if self.config.ctd_config else "" ), ) + attr = "stationkeeping_time" + validators = group_validators(CTDConfig, attr) yield Label("Stationkeeping Time (minutes):") yield Input( id="ctd_stationkeeping_time", + type=type_to_textual(get_field_type(CTDConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.ctd_config.stationkeeping_time.total_seconds() / 60.0 @@ -471,27 +672,57 @@ def compose(self) -> ComposeResult: with Collapsible(title="[b]CTD-BGC Configuration[/b]", collapsed=True): with Container(classes="instrument-config"): + attr = "max_depth_meter" + validators = group_validators(CTD_BGCConfig, attr) yield Label("Maximum Depth (meters):") yield Input( id="ctd_bgc_max_depth", + type=type_to_textual(get_field_type(CTD_BGCConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.ctd_bgc_config.max_depth_meter if self.config.ctd_bgc_config else "" ), ) + attr = "min_depth_meter" + validators = group_validators(CTD_BGCConfig, attr) yield Label("Minimum Depth (meters):") yield Input( id="ctd_bgc_min_depth", + type=type_to_textual(get_field_type(CTD_BGCConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.ctd_bgc_config.min_depth_meter if self.config.ctd_bgc_config else "" ), ) + attr = "stationkeeping_time" + validators = group_validators(CTD_BGCConfig, attr) yield Label("Stationkeeping Time (minutes):") yield Input( id="ctd_bgc_stationkeeping_time", + type=type_to_textual(get_field_type(CTD_BGCConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.ctd_bgc_config.stationkeeping_time.total_seconds() / 60.0 @@ -502,36 +733,76 @@ def compose(self) -> ComposeResult: with Collapsible(title="[b]XBT Configuration[/b]", collapsed=True): with Container(classes="instrument-config"): + attr = "max_depth_meter" + validators = group_validators(XBTConfig, attr) yield Label("Maximum Depth (meters):") yield Input( id="xbt_max_depth", + type=type_to_textual(get_field_type(XBTConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.xbt_config.max_depth_meter if self.config.xbt_config else "" ), ) + attr = "min_depth_meter" + validators = group_validators(XBTConfig, attr) yield Label("Minimum Depth (meters):") yield Input( id="xbt_min_depth", + type=type_to_textual(get_field_type(XBTConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.xbt_config.min_depth_meter if self.config.xbt_config else "" ), ) + attr = "fall_speed_meter_per_second" + validators = group_validators(XBTConfig, attr) yield Label("Fall Speed (meters/second):") yield Input( id="xbt_fall_speed", + type=type_to_textual(get_field_type(XBTConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.xbt_config.fall_speed_meter_per_second if self.config.xbt_config else "" ), ) + attr = "deceleration_coefficient" + validators = group_validators(XBTConfig, attr) yield Label("Deceleration Coefficient:") yield Input( id="xbt_decel_coeff", + type=type_to_textual(get_field_type(XBTConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.xbt_config.deceleration_coefficient if self.config.xbt_config @@ -543,54 +814,114 @@ def compose(self) -> ComposeResult: title="[b]Argo Float Configuration[/b]", collapsed=True ): with Container(classes="instrument-config"): + attr = "cycle_days" + validators = group_validators(ArgoFloatConfig, attr) yield Label("Cycle Days:") yield Input( id="argo_cycle_days", + type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.argo_float_config.cycle_days if self.config.argo_float_config else "" ), ) + attr = "drift_days" + validators = group_validators(ArgoFloatConfig, attr) yield Label("Drift Days:") yield Input( id="argo_drift_days", + type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.argo_float_config.drift_days if self.config.argo_float_config else "" ), ) + attr = "drift_depth_meter" + validators = group_validators(ArgoFloatConfig, attr) yield Label("Drift Depth (meters):") yield Input( id="argo_drift_depth", + type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.argo_float_config.drift_depth_meter if self.config.argo_float_config else "" ), ) + attr = "max_depth_meter" + validators = group_validators(ArgoFloatConfig, attr) yield Label("Maximum Depth (meters):") yield Input( id="argo_max_depth", + type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.argo_float_config.max_depth_meter if self.config.argo_float_config else "" ), ) + attr = "min_depth_meter" + validators = group_validators(ArgoFloatConfig, attr) yield Label("Minimum Depth (meters):") yield Input( id="argo_min_depth", + type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.argo_float_config.min_depth_meter if self.config.argo_float_config else "" ), ) + attr = "vertical_speed_meter_per_second" + validators = group_validators(ArgoFloatConfig, attr) yield Label("Vertical Speed (meters/second):") yield Input( id="argo_vertical_speed", + type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.argo_float_config.vertical_speed_meter_per_second if self.config.argo_float_config @@ -600,18 +931,38 @@ def compose(self) -> ComposeResult: with Collapsible(title="[b]Drifter Configuration[/b]", collapsed=True): with Container(classes="instrument-config"): + attr = "depth_meter" + validators = group_validators(DrifterConfig, attr) yield Label("Depth (meters):") yield Input( id="drifter_depth", + type=type_to_textual(get_field_type(DrifterConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.drifter_config.depth_meter if self.config.drifter_config else "" ), ) + attr = "lifetime" + validators = group_validators(DrifterConfig, attr) yield Label("Lifetime (minutes):") yield Input( id="drifter_lifetime", + type=type_to_textual(get_field_type(DrifterConfig, attr)), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], value=str( self.config.drifter_config.lifetime.total_seconds() / 60.0 @@ -623,6 +974,23 @@ def compose(self) -> ComposeResult: except Exception as e: yield Label(f"Error loading ship config: {e!s}") + @on(Input.Changed) + def show_invalid_reasons(self, event: Input.Changed) -> None: + label = self.query_one("#validation-failure-label", Label) + if not event.validation_result.is_valid: + message = ( + "\n".join(event.validation_result.failure_descriptions) + if isinstance(event.validation_result.failure_descriptions, list) + else str(event.validation_result.failure_descriptions) + ) + label.update(message) + label.remove_class("-hidden") + label.add_class("validation-failure") + else: + label.update("") + label.add_class("-hidden") + label.remove_class("validation-failure") + def on_mount(self) -> None: self.show_hide_adcp_type(bool(self.config.adcp_config)) @@ -686,6 +1054,8 @@ def _try_create_config(self, config_class, input_values, config_name): ) from e def save_changes(self) -> bool: + # TODO: SAVE_CHANGES() NEEDS TO BE LARGELY RE-WORKED NOW THAT MORE VALIDATION IS BUILT INTO THE INPUTS + # TODO: and should proabably now be more focussed on .verify() methods """Save changes to ship_config.yaml.""" try: # ship speed @@ -707,12 +1077,14 @@ def save_changes(self) -> bool: "max_depth_meter": -1000.0 if self.query_one("#adcp_deep", Switch).value else -150.0, - "num_bins": int(self.query_one("#adcp_num_bins").value), - "period": float(self.query_one("#adcp_period").value), + "num_bins": self.query_one("#adcp_num_bins").value, + "period": float( + self.query_one("#adcp_period").value + ), # must be inputted as float, "period" in ADCPConfig will not handle str (to convert to timedelta), Pydantic will try to coerce the str to the relevant type in case of other params e.g. num_bins }, "ADCP", ) - except (ValueError, ValidationError) as e: + except (ValueError, ValidationError, TypeError) as e: raise UserError(f"Invalid ADCP configuration: {e!s}") from e else: self.config.adcp_config = None @@ -726,9 +1098,9 @@ def save_changes(self) -> bool: {"period": float(self.query_one("#ts_period").value)}, "Temperature/Salinity", ) - except (ValueError, ValidationError) as e: + except (ValueError, ValidationError, TypeError) as e: raise UserError( - f"Invalid Temperature/Salinity configuration: {e!s}" + f"Invalid Onboard Temperature/Salinity configuration: {e!s}" ) from e else: self.config.ship_underwater_st_config = None @@ -736,81 +1108,81 @@ def save_changes(self) -> bool: # CTD config try: self.config.ctd_config = CTDConfig( - max_depth_meter=float(self.query_one("#ctd_max_depth").value), - min_depth_meter=float(self.query_one("#ctd_min_depth").value), + max_depth_meter=self.query_one("#ctd_max_depth").value, + min_depth_meter=self.query_one("#ctd_min_depth").value, stationkeeping_time=float( self.query_one("#ctd_stationkeeping_time").value ), ) - except (ValueError, ValidationError) as e: + except (ValueError, ValidationError, TypeError) as e: raise UserError(f"Invalid CTD configuration: {e!s}") from e # CTD-BGC config try: self.config.ctd_bgc_config = CTD_BGCConfig( - max_depth_meter=float(self.query_one("#ctd_bgc_max_depth").value), - min_depth_meter=float(self.query_one("#ctd_bgc_min_depth").value), + max_depth_meter=self.query_one("#ctd_bgc_max_depth").value, + min_depth_meter=self.query_one("#ctd_bgc_min_depth").value, stationkeeping_time=float( self.query_one("#ctd_bgc_stationkeeping_time").value ), ) - except (ValueError, ValidationError) as e: + except (ValueError, ValidationError, TypeError) as e: raise UserError(f"Invalid CTD-BGC configuration: {e!s}") from e # XBT config try: self.config.xbt_config = XBTConfig( - min_depth_meter=float(self.query_one("#xbt_min_depth").value), - max_depth_meter=float(self.query_one("#xbt_max_depth").value), - fall_speed_meter_per_second=float( - self.query_one("#xbt_fall_speed").value - ), - deceleration_coefficient=float( - self.query_one("#xbt_decel_coeff").value - ), + min_depth_meter=self.query_one("#xbt_min_depth").value, + max_depth_meter=self.query_one("#xbt_max_depth").value, + fall_speed_meter_per_second=self.query_one("#xbt_fall_speed").value, + deceleration_coefficient=self.query_one("#xbt_decel_coeff").value, ) - except (ValueError, ValidationError) as e: + except (ValueError, ValidationError, TypeError) as e: raise UserError(f"Invalid XBT configuration: {e!s}") from e # Argo config try: self.config.argo_float_config = ArgoFloatConfig( - min_depth_meter=float(self.query_one("#argo_min_depth").value), - max_depth_meter=float(self.query_one("#argo_max_depth").value), - drift_depth_meter=float(self.query_one("#argo_drift_depth").value), - vertical_speed_meter_per_second=float( - self.query_one("#argo_vertical_speed").value - ), - cycle_days=float(self.query_one("#argo_cycle_days").value), - drift_days=float(self.query_one("#argo_drift_days").value), + min_depth_meter=self.query_one("#argo_min_depth").value, + max_depth_meter=self.query_one("#argo_max_depth").value, + drift_depth_meter=self.query_one("#argo_drift_depth").value, + vertical_speed_meter_per_second=self.query_one( + "#argo_vertical_speed" + ).value, + cycle_days=self.query_one("#argo_cycle_days").value, + drift_days=self.query_one("#argo_drift_days").value, ) - except (ValueError, ValidationError) as e: + except (ValueError, ValidationError, TypeError) as e: raise UserError(f"Invalid Argo Float configuration: {e!s}") from e # Drifter config try: self.config.drifter_config = DrifterConfig( - depth_meter=float(self.query_one("#drifter_depth").value), - lifetime=float(self.query_one("#drifter_lifetime").value), + depth_meter=self.query_one("#drifter_depth").value, + lifetime=self.query_one("#drifter_lifetime").value, ) - except (ValueError, ValidationError) as e: + except (ValueError, ValidationError, TypeError) as e: raise UserError(f"Invalid Drifter configuration: {e!s}") from e # save self.config.to_yaml(f"{self.path}/ship_config.yaml") return True - except UserError as e: - self.notify( - f"Input error: {e!s}", severity="error", timeout=60, markup=False - ) - return False + # TODO: error message here which says "Unexpected error, quitting application in x seconds, please report issue and traceback on GitHub" except Exception: self.notify( - "An unexpected error occurred. Please report this issue on GitHub.", + "An unexpected error occurred. The application will quit in 10 seconds.\n" + "Please report this issue and the traceback on the VirtualShip issue tracker at: xyz.com", severity="error", - timeout=60, + timeout=10, ) + import asyncio + + async def quit_app(): + await asyncio.sleep(10) + self.app.exit() + + self._quit_task = asyncio.create_task(quit_app()) return False @@ -842,6 +1214,8 @@ def save_pressed(self) -> None: config_saved = config_editor.save_changes() schedule_saved = schedule_editor.save_changes() + # TODO: don't need this error handling here if it's handled in the respective save_changes() functions for ship and schedule configs?! + if config_saved and schedule_saved: self.notify( "Changes saved successfully", severity="information", timeout=20 @@ -892,6 +1266,13 @@ class ScheduleApp(App): padding: 1; } + Input.-valid { + border: tall $success 60%; + } + Input.-valid:focus { + border: tall $success; + } + Input { margin: 1; } @@ -1017,6 +1398,10 @@ class ScheduleApp(App): .hour-select, .minute-select { width: 15; } + + Label.validation-failure { + color: $error; + } """ def __init__(self, path: str): diff --git a/src/virtualship/cli/commands.py b/src/virtualship/cli/commands.py index 6dabd5f2..fc5d1392 100644 --- a/src/virtualship/cli/commands.py +++ b/src/virtualship/cli/commands.py @@ -82,7 +82,8 @@ def plan(path): ) url = "http://localhost:8000" webbrowser.open(url) - server.serve() + # TODO: remove debug = True when goes live + server.serve(debug=True) @click.command() From 5be6ab4b12d448b4c8665364239a3b4a1d722592 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Thu, 19 Jun 2025 10:06:01 +0200 Subject: [PATCH 10/30] instrument config refactoring --- src/virtualship/cli/_plan.py | 548 ++++++++--------------------------- 1 file changed, 114 insertions(+), 434 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index b87dd9f7..012ee104 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -47,7 +47,7 @@ # TODO: add TEXTUAL_SERVE dependency if end up using it... -# TODO: add a flag to the `virtualship plan` command which allows toggle between hosting the UI in terminal or web browser.. +# TODO: add a flag to the `virtualship plan` command which allows toggle between hosting the UI in terminal or web browser..defo useful for when/if hosted on cloud based JupyterLab type environments/terminals... # TODO: need to handle how to add the start_ and end_ datetimes automatically to Space-Time Region! # TODO: because I think the .yaml that comes from `virtualship init` will not have these as standard and we don't expect students to go add themselves... @@ -472,6 +472,73 @@ class ConfigEditor(Container): DEFAULT_TS_CONFIG: ClassVar[dict[str, float]] = {"period_minutes": 5.0} + INSTRUMENT_FIELDS: ClassVar[dict[str, dict]] = { + "adcp_config": { + "class": ADCPConfig, + "title": "Onboard ADCP", + "attributes": [ + {"name": "max_depth_meter"}, + {"name": "num_bins"}, + {"name": "period", "minutes": True}, + ], + }, + "ship_underwater_st_config": { + "class": ShipUnderwaterSTConfig, + "title": "Onboard Temperature/Salinity", + "attributes": [ + {"name": "period", "minutes": True}, + ], + }, + "ctd_config": { + "class": CTDConfig, + "title": "CTD", + "attributes": [ + {"name": "max_depth_meter"}, + {"name": "min_depth_meter"}, + {"name": "stationkeeping_time", "minutes": True}, + ], + }, + "ctd_bgc_config": { + "class": CTD_BGCConfig, + "title": "CTD-BGC", + "attributes": [ + {"name": "max_depth_meter"}, + {"name": "min_depth_meter"}, + {"name": "stationkeeping_time", "minutes": True}, + ], + }, + "xbt_config": { + "class": XBTConfig, + "title": "XBT", + "attributes": [ + {"name": "min_depth_meter"}, + {"name": "max_depth_meter"}, + {"name": "fall_speed_meter_per_second"}, + {"name": "deceleration_coefficient"}, + ], + }, + "argo_float_config": { + "class": ArgoFloatConfig, + "title": "Argo Float", + "attributes": [ + {"name": "min_depth_meter"}, + {"name": "max_depth_meter"}, + {"name": "drift_depth_meter"}, + {"name": "vertical_speed_meter_per_second"}, + {"name": "cycle_days"}, + {"name": "drift_days"}, + ], + }, + "drifter_config": { + "class": DrifterConfig, + "title": "Drifter", + "attributes": [ + {"name": "depth_meter"}, + {"name": "lifetime", "minutes": True}, + ], + }, + } + # TODO: Also incorporate verify methods! def __init__(self, path: str): @@ -481,6 +548,8 @@ def __init__(self, path: str): def compose(self) -> ComposeResult: try: + ## UI SECTION: Ship Speed & Onboard Measurements + self.config = ShipConfig.from_yaml(f"{self.path}/ship_config.yaml") yield Label("[b]Ship Config Editor[/b]", id="title", markup=True) yield Rule(line_style="heavy") @@ -532,444 +601,55 @@ def compose(self) -> ComposeResult: yield Label(" SeaSeven:") yield Switch(value=not is_deep, id="adcp_shallow") - # specific instrument configurations + ## UI SECTION: Instrument Configurations (advanced users only) + with Collapsible( title="[b]Instrument Configurations[/b] (advanced users only)", collapsed=True, ): - with Collapsible( - title="[b]Onboard ADCP Configuration[/b]", collapsed=True - ): - with Container(classes="instrument-config"): - attr = "num_bins" - validators = group_validators(ADCPConfig, attr) - yield Label("Number of Bins:") - yield Input( - id="adcp_num_bins", - type=type_to_textual(get_field_type(ADCPConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.adcp_config.num_bins - if self.config.adcp_config - else "" - ), - ) - - attr = "period" - validators = group_validators(ADCPConfig, attr) - yield Label("Period (minutes):") - yield Input( - id="adcp_period", - type=type_to_textual(get_field_type(ADCPConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.adcp_config.period.total_seconds() / 60.0 - if self.config.adcp_config - else "" - ), - ) - - with Collapsible( - title="[b]Onboard Temperature/Salinity Configuration[/b]", - collapsed=True, - ): - with Container(classes="instrument-config"): - attr = "period" - validators = group_validators(ShipUnderwaterSTConfig, attr) - yield Label("Period (minutes):") - yield Input( - id="ts_period", - type=type_to_textual( - get_field_type(ShipUnderwaterSTConfig, attr) - ), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.ship_underwater_st_config.period.total_seconds() - / 60.0 - if self.config.ship_underwater_st_config - else "" - ), - ) - - with Collapsible(title="[b]CTD Configuration[/b]", collapsed=True): - with Container(classes="instrument-config"): - attr = "max_depth_meter" - validators = group_validators(CTDConfig, attr) - yield Label("Maximum Depth (meters):") - yield Input( - id="ctd_max_depth", - type=type_to_textual(get_field_type(CTDConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.ctd_config.max_depth_meter - if self.config.ctd_config - else "" - ), - ) - attr = "min_depth_meter" - validators = group_validators(CTDConfig, attr) - yield Label("Minimum Depth (meters):") - yield Input( - id="ctd_min_depth", - type=type_to_textual(get_field_type(CTDConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.ctd_config.min_depth_meter - if self.config.ctd_config - else "" - ), - ) - attr = "stationkeeping_time" - validators = group_validators(CTDConfig, attr) - yield Label("Stationkeeping Time (minutes):") - yield Input( - id="ctd_stationkeeping_time", - type=type_to_textual(get_field_type(CTDConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.ctd_config.stationkeeping_time.total_seconds() - / 60.0 - if self.config.ctd_config - else "" - ), - ) - - with Collapsible(title="[b]CTD-BGC Configuration[/b]", collapsed=True): - with Container(classes="instrument-config"): - attr = "max_depth_meter" - validators = group_validators(CTD_BGCConfig, attr) - yield Label("Maximum Depth (meters):") - yield Input( - id="ctd_bgc_max_depth", - type=type_to_textual(get_field_type(CTD_BGCConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.ctd_bgc_config.max_depth_meter - if self.config.ctd_bgc_config - else "" - ), - ) - attr = "min_depth_meter" - validators = group_validators(CTD_BGCConfig, attr) - yield Label("Minimum Depth (meters):") - yield Input( - id="ctd_bgc_min_depth", - type=type_to_textual(get_field_type(CTD_BGCConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.ctd_bgc_config.min_depth_meter - if self.config.ctd_bgc_config - else "" - ), - ) - attr = "stationkeeping_time" - validators = group_validators(CTD_BGCConfig, attr) - yield Label("Stationkeeping Time (minutes):") - yield Input( - id="ctd_bgc_stationkeeping_time", - type=type_to_textual(get_field_type(CTD_BGCConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.ctd_bgc_config.stationkeeping_time.total_seconds() - / 60.0 - if self.config.ctd_bgc_config - else "" - ), - ) - - with Collapsible(title="[b]XBT Configuration[/b]", collapsed=True): - with Container(classes="instrument-config"): - attr = "max_depth_meter" - validators = group_validators(XBTConfig, attr) - yield Label("Maximum Depth (meters):") - yield Input( - id="xbt_max_depth", - type=type_to_textual(get_field_type(XBTConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.xbt_config.max_depth_meter - if self.config.xbt_config - else "" - ), - ) - attr = "min_depth_meter" - validators = group_validators(XBTConfig, attr) - yield Label("Minimum Depth (meters):") - yield Input( - id="xbt_min_depth", - type=type_to_textual(get_field_type(XBTConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.xbt_config.min_depth_meter - if self.config.xbt_config - else "" - ), - ) - attr = "fall_speed_meter_per_second" - validators = group_validators(XBTConfig, attr) - yield Label("Fall Speed (meters/second):") - yield Input( - id="xbt_fall_speed", - type=type_to_textual(get_field_type(XBTConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.xbt_config.fall_speed_meter_per_second - if self.config.xbt_config - else "" - ), - ) - attr = "deceleration_coefficient" - validators = group_validators(XBTConfig, attr) - yield Label("Deceleration Coefficient:") - yield Input( - id="xbt_decel_coeff", - type=type_to_textual(get_field_type(XBTConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.xbt_config.deceleration_coefficient - if self.config.xbt_config - else "" - ), - ) - - with Collapsible( - title="[b]Argo Float Configuration[/b]", collapsed=True - ): - with Container(classes="instrument-config"): - attr = "cycle_days" - validators = group_validators(ArgoFloatConfig, attr) - yield Label("Cycle Days:") - yield Input( - id="argo_cycle_days", - type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.argo_float_config.cycle_days - if self.config.argo_float_config - else "" - ), - ) - attr = "drift_days" - validators = group_validators(ArgoFloatConfig, attr) - yield Label("Drift Days:") - yield Input( - id="argo_drift_days", - type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.argo_float_config.drift_days - if self.config.argo_float_config - else "" - ), - ) - attr = "drift_depth_meter" - validators = group_validators(ArgoFloatConfig, attr) - yield Label("Drift Depth (meters):") - yield Input( - id="argo_drift_depth", - type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.argo_float_config.drift_depth_meter - if self.config.argo_float_config - else "" - ), - ) - attr = "max_depth_meter" - validators = group_validators(ArgoFloatConfig, attr) - yield Label("Maximum Depth (meters):") - yield Input( - id="argo_max_depth", - type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.argo_float_config.max_depth_meter - if self.config.argo_float_config - else "" - ), - ) - attr = "min_depth_meter" - validators = group_validators(ArgoFloatConfig, attr) - yield Label("Minimum Depth (meters):") - yield Input( - id="argo_min_depth", - type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.argo_float_config.min_depth_meter - if self.config.argo_float_config - else "" - ), - ) - attr = "vertical_speed_meter_per_second" - validators = group_validators(ArgoFloatConfig, attr) - yield Label("Vertical Speed (meters/second):") - yield Input( - id="argo_vertical_speed", - type=type_to_textual(get_field_type(ArgoFloatConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", + for instrument_name, info in self.INSTRUMENT_FIELDS.items(): + config_class = info["class"] + attributes = info["attributes"] + config_instance = getattr(self.config, instrument_name, None) + title = info.get("title", instrument_name.replace("_", " ").title()) + + with Collapsible( + title=f"[b]{title}[/b]", + collapsed=True, + ): + with Container(classes="instrument-config"): + for attr_meta in attributes: + attr = attr_meta["name"] + is_minutes = attr_meta.get("minutes", False) + validators = group_validators(config_class, attr) + if config_instance: + raw_value = getattr(config_instance, attr, "") + if is_minutes and raw_value != "": + try: + value = str( + raw_value.total_seconds() / 60.0 + ) + except AttributeError: + value = str(raw_value) + else: + value = str(raw_value) + else: + value = "" + yield Label(f"{attr.replace('_', ' ').title()}:") + yield Input( + id=f"{instrument_name}_{attr}", + type=type_to_textual( + get_field_type(config_class, attr) + ), + validators=[ + Function( + validator, + f"*INVALID*: entry must be {validator.__doc__.lower()}", + ) + for validator in validators + ], + value=value, ) - for validator in validators - ], - value=str( - self.config.argo_float_config.vertical_speed_meter_per_second - if self.config.argo_float_config - else "" - ), - ) - - with Collapsible(title="[b]Drifter Configuration[/b]", collapsed=True): - with Container(classes="instrument-config"): - attr = "depth_meter" - validators = group_validators(DrifterConfig, attr) - yield Label("Depth (meters):") - yield Input( - id="drifter_depth", - type=type_to_textual(get_field_type(DrifterConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.drifter_config.depth_meter - if self.config.drifter_config - else "" - ), - ) - attr = "lifetime" - validators = group_validators(DrifterConfig, attr) - yield Label("Lifetime (minutes):") - yield Input( - id="drifter_lifetime", - type=type_to_textual(get_field_type(DrifterConfig, attr)), - validators=[ - Function( - validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", - ) - for validator in validators - ], - value=str( - self.config.drifter_config.lifetime.total_seconds() - / 60.0 - if self.config.drifter_config - else "" - ), - ) except Exception as e: yield Label(f"Error loading ship config: {e!s}") From 15d15b58d2469d3f0f4de1688698a083c2293407 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Fri, 20 Jun 2025 09:34:34 +0200 Subject: [PATCH 11/30] refarctoring and update config save_changes method --- src/virtualship/cli/_plan.py | 492 ++++++++++++++++------------------- 1 file changed, 222 insertions(+), 270 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 012ee104..15d0d73f 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -1,8 +1,9 @@ import datetime +import os import sys +import traceback from typing import ClassVar -from pydantic import ValidationError from textual import on from textual.app import App, ComposeResult from textual.containers import Container, Horizontal, VerticalScroll @@ -19,7 +20,6 @@ Switch, ) -from virtualship.errors import UserError from virtualship.models.location import Location from virtualship.models.schedule import Schedule, Waypoint from virtualship.models.ship_config import ( @@ -39,6 +39,11 @@ TimeRange, ) +# TODO: check: how does it handle (all) the fields being empty upon reading-in? Need to add redundancy here...? +# - currently the application fails on start-up if something is missing or null (apart from ADCP and ST) +# - there is also a bug where if ADCP is null on start up, and then you switch it on in the UI then it crashes, so this bug fix is TODO! +# - otherwise, in terms of missing data on start up handling, best to throw an error to the user stating that x field is missing a value + # TODO: need to do a big round of edits where tidy up all the error messaging. # - given a lot of the validation of entry is done natively now in textual, this should mean that all other # errors associated with inputting to the app should be 'unexpected' and therefore call to the report-to-GitHub workflow @@ -77,7 +82,9 @@ def compose(self) -> ComposeResult: with Collapsible(title=f"[b]Waypoint {self.index + 1}[/b]", collapsed=True): if self.index > 0: yield Button( - "Copy from Previous", id=f"wp{self.index}_copy", variant="warning" + "Copy Time & Instruments from Previous", + id=f"wp{self.index}_copy", + variant="warning", ) yield Label("Location:") yield Label(" Latitude:") @@ -332,138 +339,6 @@ def save_changes(self) -> bool: return False -# TODO: perhaps these methods could be housed elsewhere for neatness?! - - -def get_field_type(model_class, field_name): - """Get Pydantic model class data type.""" - return model_class.model_fields[field_name].annotation - - -def type_to_textual(field_type): - """Convert data type to str which Textual can interpret for type = setting in Input objects.""" - if field_type in (float, datetime.timedelta): - return "number" - elif field_type is int: - return "integer" - else: - return "text" - - -# Pydantic field constraint attributes used for validation and introspection -FIELD_CONSTRAINT_ATTRS = ("gt", "ge", "lt", "le") - - -def get_field_conditions(model_class, field_name): - """Determine and return what conditions (and associated reference value) a Pydantic model sets on inputs.""" - field_info = model_class.model_fields[field_name] - conditions = {} - for meta in field_info.metadata: - for attr in dir(meta): - if not attr.startswith("_") and getattr(meta, attr) is not None: - if attr in FIELD_CONSTRAINT_ATTRS: - conditions[attr] = getattr(meta, attr) - else: - raise ValueError( - f"Unexpected constraint '{attr}' found on field '{field_name}'. " - f"Allowed constraints: {FIELD_CONSTRAINT_ATTRS}" - ) - return list(conditions.keys()), list(conditions.values()) - - -def make_validator(condition, reference, value_type): - """ - Make a validator function based on the Pydantic model field conditions returned by get_field_conditions(). - - TODO: Textual validator tools do not currently support additional arguments (such as 'reference') being fed into the validator functions (such as is_gt0) at present. - TODO: Therefore, reference values cannot be fed in dynamically and necessitates hard coding depending onthe condition and reference value combination. - TODO: At present, Pydantic models only require gt/ge/lt/le relative to **0.0** so `reference` is always checked as being == 0.0 - TODO: Additional custom conditions can be "hard-coded" as new condition and reference combinations - TODO: if Pydantic model specifications change in the future and/or new instruments are added to VirtualShip etc. - TODO: Perhaps there's scope here though for a more robust implementation in a future PR... - - Note, docstrings describing the conditional are required in the child functions (e.g. is_gt0), and is checked by require_docstring(). - Docstrings will be used to generate informative UI invalid entry messages, so them being informative and accurate is important! - - """ - - def require_docstring(func): - if func.__doc__ is None or not func.__doc__.strip(): - raise ValueError(f"Function '{func.__name__}' must have a docstring.") - return func - - def convert(value): - try: - return value_type(value) - except Exception: - return None - - if value_type in (float, int) and reference == 0.0: - ref_zero = 0.0 - elif value_type is datetime.timedelta and reference == datetime.timedelta(): - ref_zero = datetime.timedelta() - else: - raise ValueError( - f"Unsupported value_type/reference combination: {value_type}, {reference}" - ) - - if condition == "gt" and reference == ref_zero: - - @require_docstring - def is_gt0(value: str) -> bool: - """Greater than 0.""" - v = convert(value) - return v is not None and v > ref_zero - - return is_gt0 - - if condition == "ge" and reference == ref_zero: - - @require_docstring - def is_ge0(value: str) -> bool: - """Greater than or equal to 0.""" - v = convert(value) - return v is not None and v >= ref_zero - - return is_ge0 - - if condition == "lt" and reference == ref_zero: - - @require_docstring - def is_lt0(value: str) -> bool: - """Less than 0.""" - v = convert(value) - return v is not None and v < ref_zero - - return is_lt0 - - if condition == "le" and reference == ref_zero: - - @require_docstring - def is_le0(value: str) -> bool: - """Less than or equal to 0.""" - v = convert(value) - return v is not None and v <= ref_zero - - return is_le0 - - else: - raise ValueError( - f"Unknown condition: {condition} and reference value: {reference} combination." - ) - - -def group_validators(model, attr): - """Bundle all validators for Input into singular list.""" - return [ - make_validator(cond, ref, get_field_type(model, attr)) - for cond, ref in zip( - *get_field_conditions(model, attr), - strict=False, - ) - ] - - class ConfigEditor(Container): DEFAULT_ADCP_CONFIG: ClassVar[dict[str, float]] = { "num_bins": 40, @@ -477,7 +352,6 @@ class ConfigEditor(Container): "class": ADCPConfig, "title": "Onboard ADCP", "attributes": [ - {"name": "max_depth_meter"}, {"name": "num_bins"}, {"name": "period", "minutes": True}, ], @@ -539,6 +413,13 @@ class ConfigEditor(Container): }, } + FIELD_CONSTRAINT_ATTRS: ClassVar[tuple] = ( + "gt", + "ge", + "lt", + "le", + ) # Pydantic field constraint attributes used for validation and introspection + # TODO: Also incorporate verify methods! def __init__(self, path: str): @@ -548,7 +429,7 @@ def __init__(self, path: str): def compose(self) -> ComposeResult: try: - ## UI SECTION: Ship Speed & Onboard Measurements + ## SECTION: "Ship Speed & Onboard Measurements" self.config = ShipConfig.from_yaml(f"{self.path}/ship_config.yaml") yield Label("[b]Ship Config Editor[/b]", id="title", markup=True) @@ -556,16 +437,18 @@ def compose(self) -> ComposeResult: with Collapsible(title="[b]Ship Speed & Onboard Measurements[/b]"): attr = "ship_speed_knots" - validators = group_validators(ShipConfig, attr) + validators = self.group_validators(ShipConfig, attr) with Horizontal(classes="ship_speed"): yield Label("[b]Ship Speed (knots):[/b]") yield Input( id="speed", - type=type_to_textual(get_field_type(ShipConfig, attr)), + type=self.type_to_textual( + self.get_field_type(ShipConfig, attr) + ), validators=[ Function( validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", + f"INVALID: value must be {validator.__doc__.lower()}", ) for validator in validators ], @@ -577,7 +460,7 @@ def compose(self) -> ComposeResult: else "" ), ) - yield Label("", id="validation-failure-label", classes="-hidden") + yield Label("", id="validation-failure-label-speed", classes="-hidden") with Horizontal(classes="ts-section"): yield Label("[b]Onboard Temperature/Salinity:[/b]") @@ -601,7 +484,7 @@ def compose(self) -> ComposeResult: yield Label(" SeaSeven:") yield Switch(value=not is_deep, id="adcp_shallow") - ## UI SECTION: Instrument Configurations (advanced users only) + ## SECTION: "Instrument Configurations (advanced users only)"" with Collapsible( title="[b]Instrument Configurations[/b] (advanced users only)", @@ -621,7 +504,7 @@ def compose(self) -> ComposeResult: for attr_meta in attributes: attr = attr_meta["name"] is_minutes = attr_meta.get("minutes", False) - validators = group_validators(config_class, attr) + validators = self.group_validators(config_class, attr) if config_instance: raw_value = getattr(config_instance, attr, "") if is_minutes and raw_value != "": @@ -638,25 +521,32 @@ def compose(self) -> ComposeResult: yield Label(f"{attr.replace('_', ' ').title()}:") yield Input( id=f"{instrument_name}_{attr}", - type=type_to_textual( - get_field_type(config_class, attr) + type=self.type_to_textual( + self.get_field_type(config_class, attr) ), validators=[ Function( validator, - f"*INVALID*: entry must be {validator.__doc__.lower()}", + f"INVALID: value must be {validator.__doc__.lower()}", ) for validator in validators ], value=value, ) + yield Label( + "", + id=f"validation-failure-label-{instrument_name}_{attr}", + classes="-hidden validation-failure", + ) except Exception as e: yield Label(f"Error loading ship config: {e!s}") @on(Input.Changed) def show_invalid_reasons(self, event: Input.Changed) -> None: - label = self.query_one("#validation-failure-label", Label) + input_id = event.input.id + label_id = f"validation-failure-label-{input_id}" + label = self.query_one(f"#{label_id}", Label) if not event.validation_result.is_valid: message = ( "\n".join(event.validation_result.failure_descriptions) @@ -721,128 +611,179 @@ def shallow_changed(self, event: Switch.Changed) -> None: deep = self.query_one("#adcp_deep", Switch) deep.value = False - def _try_create_config(self, config_class, input_values, config_name): - """Helper to create config with error handling.""" - try: - return config_class(**input_values) - except ValueError as e: - field = ( - str(e).split()[0] if str(e).split() else "unknown field" - ) # extract field name from Pydantic error - raise UserError( - f"Invalid {config_name} configuration: {field} - {e!s}" - ) from e + def get_field_type(self, model_class, field_name): + """Get Pydantic model class data type.""" + return model_class.model_fields[field_name].annotation - def save_changes(self) -> bool: - # TODO: SAVE_CHANGES() NEEDS TO BE LARGELY RE-WORKED NOW THAT MORE VALIDATION IS BUILT INTO THE INPUTS - # TODO: and should proabably now be more focussed on .verify() methods - """Save changes to ship_config.yaml.""" - try: - # ship speed - try: - speed = float(self.query_one("#speed").value) - if speed <= 0: - raise UserError("Ship speed must be greater than 0") - self.config.ship_speed_knots = speed - except ValueError: - raise UserError("Ship speed must be a valid number") from None - - # ADCP config - has_adcp = self.query_one("#has_adcp", Switch).value - if has_adcp: - try: - self.config.adcp_config = self._try_create_config( - ADCPConfig, - { - "max_depth_meter": -1000.0 - if self.query_one("#adcp_deep", Switch).value - else -150.0, - "num_bins": self.query_one("#adcp_num_bins").value, - "period": float( - self.query_one("#adcp_period").value - ), # must be inputted as float, "period" in ADCPConfig will not handle str (to convert to timedelta), Pydantic will try to coerce the str to the relevant type in case of other params e.g. num_bins - }, - "ADCP", - ) - except (ValueError, ValidationError, TypeError) as e: - raise UserError(f"Invalid ADCP configuration: {e!s}") from e - else: - self.config.adcp_config = None - - # T/S config - has_ts = self.query_one("#has_onboard_ts", Switch).value - if has_ts: - try: - self.config.ship_underwater_st_config = self._try_create_config( - ShipUnderwaterSTConfig, - {"period": float(self.query_one("#ts_period").value)}, - "Temperature/Salinity", - ) - except (ValueError, ValidationError, TypeError) as e: - raise UserError( - f"Invalid Onboard Temperature/Salinity configuration: {e!s}" - ) from e - else: - self.config.ship_underwater_st_config = None + def type_to_textual(self, field_type): + """Convert data type to str which Textual can interpret for type = setting in Input objects.""" + if field_type in (float, datetime.timedelta): + return "number" + elif field_type is int: + return "integer" + else: + return "text" + + def get_field_conditions(self, model_class, field_name): + """Determine and return what conditions (and associated reference value) a Pydantic model sets on inputs.""" + field_info = model_class.model_fields[field_name] + conditions = {} + for meta in field_info.metadata: + for attr in dir(meta): + if not attr.startswith("_") and getattr(meta, attr) is not None: + if attr in self.FIELD_CONSTRAINT_ATTRS: + conditions[attr] = getattr(meta, attr) + else: + raise ValueError( + f"Unexpected constraint '{attr}' found on field '{field_name}'. " + f"Allowed constraints: {self.FIELD_CONSTRAINT_ATTRS}" + ) + return list(conditions.keys()), list(conditions.values()) - # CTD config - try: - self.config.ctd_config = CTDConfig( - max_depth_meter=self.query_one("#ctd_max_depth").value, - min_depth_meter=self.query_one("#ctd_min_depth").value, - stationkeeping_time=float( - self.query_one("#ctd_stationkeeping_time").value - ), - ) - except (ValueError, ValidationError, TypeError) as e: - raise UserError(f"Invalid CTD configuration: {e!s}") from e + def make_validator(self, condition, reference, value_type): + """ + Make a validator function based on the Pydantic model field conditions returned by get_field_conditions(). - # CTD-BGC config - try: - self.config.ctd_bgc_config = CTD_BGCConfig( - max_depth_meter=self.query_one("#ctd_bgc_max_depth").value, - min_depth_meter=self.query_one("#ctd_bgc_min_depth").value, - stationkeeping_time=float( - self.query_one("#ctd_bgc_stationkeeping_time").value - ), - ) - except (ValueError, ValidationError, TypeError) as e: - raise UserError(f"Invalid CTD-BGC configuration: {e!s}") from e + N.B. #1 Textual validator tools do not currently support additional arguments (such as 'reference') being fed into the validator functions (such as is_gt0) at present. + Therefore, reference values cannot be fed in dynamically and necessitates hard coding depending onthe condition and reference value combination. + At present, Pydantic models only require gt/ge/lt/le relative to **0.0** so `reference` is always checked as being == 0.0 + Additional custom conditions can be "hard-coded" as new condition and reference combinations if Pydantic model specifications change in the future and/or new instruments are added to VirtualShip etc. + TODO: Perhaps there's scope here though for a more robust implementation in a future PR... - # XBT config - try: - self.config.xbt_config = XBTConfig( - min_depth_meter=self.query_one("#xbt_min_depth").value, - max_depth_meter=self.query_one("#xbt_max_depth").value, - fall_speed_meter_per_second=self.query_one("#xbt_fall_speed").value, - deceleration_coefficient=self.query_one("#xbt_decel_coeff").value, - ) - except (ValueError, ValidationError, TypeError) as e: - raise UserError(f"Invalid XBT configuration: {e!s}") from e + N.B. #2, docstrings describing the conditional are required in the child functions (e.g. is_gt0), and presence is checked by require_docstring(). + Docstrings will be used to generate informative UI invalid entry messages, so them being informative and accurate is important! - # Argo config - try: - self.config.argo_float_config = ArgoFloatConfig( - min_depth_meter=self.query_one("#argo_min_depth").value, - max_depth_meter=self.query_one("#argo_max_depth").value, - drift_depth_meter=self.query_one("#argo_drift_depth").value, - vertical_speed_meter_per_second=self.query_one( - "#argo_vertical_speed" - ).value, - cycle_days=self.query_one("#argo_cycle_days").value, - drift_days=self.query_one("#argo_drift_days").value, - ) - except (ValueError, ValidationError, TypeError) as e: - raise UserError(f"Invalid Argo Float configuration: {e!s}") from e + """ - # Drifter config + def require_docstring(func): + if func.__doc__ is None or not func.__doc__.strip(): + raise ValueError(f"Function '{func.__name__}' must have a docstring.") + return func + + def convert(value): try: - self.config.drifter_config = DrifterConfig( - depth_meter=self.query_one("#drifter_depth").value, - lifetime=self.query_one("#drifter_lifetime").value, - ) - except (ValueError, ValidationError, TypeError) as e: - raise UserError(f"Invalid Drifter configuration: {e!s}") from e + if value_type is datetime.timedelta: + return datetime.timedelta(minutes=float(value)) + return value_type(value) + except Exception: + return None + + if value_type in (float, int) and reference == 0.0: + ref_zero = 0.0 + elif value_type is datetime.timedelta and reference == datetime.timedelta(): + ref_zero = datetime.timedelta() + else: + raise ValueError( + f"Unsupported value_type/reference combination: {value_type}, {reference}" + ) + + if condition == "gt" and reference == ref_zero: + + @require_docstring + def is_gt0(value: str) -> bool: + """Greater than 0.""" + v = convert(value) + return v is not None and v > ref_zero + + return is_gt0 + + if condition == "ge" and reference == ref_zero: + + @require_docstring + def is_ge0(value: str) -> bool: + """Greater than or equal to 0.""" + v = convert(value) + return v is not None and v >= ref_zero + + return is_ge0 + + if condition == "lt" and reference == ref_zero: + + @require_docstring + def is_lt0(value: str) -> bool: + """Less than 0.""" + v = convert(value) + return v is not None and v < ref_zero + + return is_lt0 + + if condition == "le" and reference == ref_zero: + + @require_docstring + def is_le0(value: str) -> bool: + """Less than or equal to 0.""" + v = convert(value) + return v is not None and v <= ref_zero + + return is_le0 + + else: + raise ValueError( + f"Unknown condition: {condition} and reference value: {reference} combination." + ) + + def group_validators(self, model, attr): + """Bundle all validators for Input into singular list.""" + return [ + self.make_validator(cond, ref, self.get_field_type(model, attr)) + for cond, ref in zip( + *self.get_field_conditions(model, attr), + strict=False, + ) + ] + + def save_changes(self) -> bool: + # TODO: SAVE_CHANGES() NEEDS TO BE LARGELY RE-WORKED NOW THAT MORE VALIDATION IS BUILT INTO THE INPUTS + # TODO: and should proabably now be more focussed on .verify() methods + """Save changes to ship_config.yaml.""" + try: + # ship speed + attr = "ship_speed_knots" + field_type = self.get_field_type(self.config, attr) + value = field_type(self.query_one("#speed").value) + ShipConfig.model_validate( + {**self.config.model_dump(), attr: value} + ) # validate using a temporary model (raises if invalid) + self.config.ship_speed_knots = value + + # individual instrument configurations + for instrument_name, info in self.INSTRUMENT_FIELDS.items(): + config_class = info["class"] + attributes = info["attributes"] + kwargs = {} + for attr_meta in attributes: + attr = attr_meta["name"] + is_minutes = attr_meta.get("minutes", False) + input_id = f"{instrument_name}_{attr}" + value = self.query_one(f"#{input_id}").value + field_type = self.get_field_type(config_class, attr) + if is_minutes and field_type is datetime.timedelta: + value = datetime.timedelta(minutes=float(value)) + else: + value = field_type(value) + kwargs[attr] = value + + # TODO: the special handling does not seem to work for both instruments + # TODO: in the ADCP case it causes an error, in the T/S case it saves but doesn't actually write anything... + + # special handling for onboard ADCP, conditional on whether it's selected by user in Switch (overwrite with None if switch is off) + if instrument_name == "adcp_config": + has_adcp = self.query_one("#has_adcp", Switch).value + if has_adcp: # determine max_depth_meter value based on sub-Switch selection by user, i.e. deep or shallow ADCP + if self.query_one("#adcp_deep", Switch).value: + kwargs["max_depth_meter"] = -1000.0 + else: + kwargs["max_depth_meter"] = -150.0 + else: + setattr(self.config, instrument_name, None) + + # special handling for onboard T/S, conditional on whether it's selected by user (overwrite with None if switch is off) + if instrument_name == "ship_underwater_st_config": + has_ts = self.query_one("#has_onboard_ts", Switch).value + if not has_ts: + setattr(self.config, instrument_name, None) + + setattr(self.config, instrument_name, config_class(**kwargs)) # save self.config.to_yaml(f"{self.path}/ship_config.yaml") @@ -850,19 +791,31 @@ def save_changes(self) -> bool: # TODO: error message here which says "Unexpected error, quitting application in x seconds, please report issue and traceback on GitHub" except Exception: + # TODO: this traceback strategy is not working... + # print traceback in terminal if running in a terminal, else (e.g. running in browser) save to file + if sys.stdout.isatty(): + traceback.print_exc() + else: + error_log_path = os.path.join(self.path, "virtualship_error.txt") + with open(error_log_path, "a") as f: + traceback.print_exc(file=f) + self.notify( - "An unexpected error occurred. The application will quit in 10 seconds.\n" - "Please report this issue and the traceback on the VirtualShip issue tracker at: xyz.com", + "ERROR: An unexpected error occurred.\n" + "\nPlease ensure that all entries are valid (all typed entry boxes must have green borders and no warnings).\n" + "\nIf the problem persists, please report this issue, with a description and the traceback (to be printed in the terminal upon exiting the application) " + "on the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues", + # The application will quit in 10 seconds.\n" severity="error", timeout=10, ) - import asyncio + # import asyncio - async def quit_app(): - await asyncio.sleep(10) - self.app.exit() + # async def quit_app(): + # await asyncio.sleep(10) + # self.app.exit() - self._quit_task = asyncio.create_task(quit_app()) + # self._quit_task = asyncio.create_task(quit_app()) return False @@ -875,7 +828,6 @@ def compose(self) -> ComposeResult: with VerticalScroll(): yield ConfigEditor(self.path) yield ScheduleEditor(self.path) - # Move buttons to screen level with Horizontal(): yield Button("Save Changes", id="save_button", variant="success") yield Button("Exit", id="exit_button", variant="error") From 26d0ecdcf8d124f50b233b641437c1cbf5e91b59 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Fri, 20 Jun 2025 10:45:38 +0200 Subject: [PATCH 12/30] improved error handling --- src/virtualship/cli/_plan.py | 55 +++++++++++++----------------------- src/virtualship/errors.py | 4 +-- 2 files changed, 22 insertions(+), 37 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 15d0d73f..9e042cda 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -20,6 +20,7 @@ Switch, ) +from virtualship.errors import UnexpectedError from virtualship.models.location import Location from virtualship.models.schedule import Schedule, Waypoint from virtualship.models.ship_config import ( @@ -733,8 +734,6 @@ def group_validators(self, model, attr): ] def save_changes(self) -> bool: - # TODO: SAVE_CHANGES() NEEDS TO BE LARGELY RE-WORKED NOW THAT MORE VALIDATION IS BUILT INTO THE INPUTS - # TODO: and should proabably now be more focussed on .verify() methods """Save changes to ship_config.yaml.""" try: # ship speed @@ -789,34 +788,21 @@ def save_changes(self) -> bool: self.config.to_yaml(f"{self.path}/ship_config.yaml") return True - # TODO: error message here which says "Unexpected error, quitting application in x seconds, please report issue and traceback on GitHub" - except Exception: - # TODO: this traceback strategy is not working... - # print traceback in terminal if running in a terminal, else (e.g. running in browser) save to file - if sys.stdout.isatty(): - traceback.print_exc() - else: - error_log_path = os.path.join(self.path, "virtualship_error.txt") - with open(error_log_path, "a") as f: - traceback.print_exc(file=f) - - self.notify( - "ERROR: An unexpected error occurred.\n" - "\nPlease ensure that all entries are valid (all typed entry boxes must have green borders and no warnings).\n" - "\nIf the problem persists, please report this issue, with a description and the traceback (to be printed in the terminal upon exiting the application) " - "on the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues", - # The application will quit in 10 seconds.\n" - severity="error", - timeout=10, - ) - # import asyncio - - # async def quit_app(): - # await asyncio.sleep(10) - # self.app.exit() + except Exception as e: + # write error log + error_log_path = os.path.join(self.path, "virtualship_error.txt") + with open(error_log_path, "w") as f: + f.write("Error saving ship config:\n") + traceback.print_exception( + type(e), e, e.__traceback__, file=f, chain=True + ) + f.write("\n") - # self._quit_task = asyncio.create_task(quit_app()) - return False + raise UnexpectedError( + "\n1) Please ensure that all entries are valid (all typed entry boxes must have green borders and no warnings).\n" + "\n2) If the problem persists, please report this issue, with a description and the traceback, " + "to the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues" + ) from None class ScheduleScreen(Screen): @@ -838,7 +824,7 @@ def exit_pressed(self) -> None: @on(Button.Pressed, "#save_button") def save_pressed(self) -> None: - """Handle save button press.""" + """Save button press.""" config_editor = self.query_one(ConfigEditor) schedule_editor = self.query_one(ScheduleEditor) @@ -846,19 +832,18 @@ def save_pressed(self) -> None: config_saved = config_editor.save_changes() schedule_saved = schedule_editor.save_changes() - # TODO: don't need this error handling here if it's handled in the respective save_changes() functions for ship and schedule configs?! - if config_saved and schedule_saved: self.notify( "Changes saved successfully", severity="information", timeout=20 ) + except Exception as e: self.notify( - f"Error saving changes: {e!s}", + f"*** Error saving changes ***:\n\n{e}\n\nTraceback will be logged in `{self.path}/virtualship_error.txt`. Please copy the file and/or its contents when submitting an issue.", severity="error", - timeout=60, - markup=False, + timeout=20, ) + return False class ScheduleApp(App): diff --git a/src/virtualship/errors.py b/src/virtualship/errors.py index 882091fa..f98f2a41 100644 --- a/src/virtualship/errors.py +++ b/src/virtualship/errors.py @@ -28,7 +28,7 @@ class ConfigError(RuntimeError): pass -class UserError(Exception): - """Error raised when user input is invalid.""" +class UnexpectedError(Exception): + """Error raised when there is an unexpected problem.""" pass From ee26959ddda69ad69cf6e482616d0686f20c0bfe Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Mon, 23 Jun 2025 16:28:41 +0200 Subject: [PATCH 13/30] Refactor error handling and validation; introduce UserError for user-related issues --- src/virtualship/cli/_plan.py | 579 +++++++++++++------------ src/virtualship/cli/commands.py | 34 +- src/virtualship/cli/validator_utils.py | 172 ++++++++ src/virtualship/errors.py | 6 + 4 files changed, 497 insertions(+), 294 deletions(-) create mode 100644 src/virtualship/cli/validator_utils.py diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 9e042cda..85705a6d 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -1,6 +1,5 @@ import datetime import os -import sys import traceback from typing import ClassVar @@ -20,7 +19,14 @@ Switch, ) -from virtualship.errors import UnexpectedError +from virtualship.cli.validator_utils import ( + get_field_type, + group_validators, + is_valid_lat, + is_valid_lon, + type_to_textual, +) +from virtualship.errors import UnexpectedError, UserError from virtualship.models.location import Location from virtualship.models.schedule import Schedule, Waypoint from virtualship.models.ship_config import ( @@ -40,6 +46,12 @@ TimeRange, ) +# TODO: need to distinguish in error handling between usererrors such as faulty yaml read in add pointers to the User to make fixes, +# TODO: and errors which are genuinely unexpected, are likely bugs and need attention on GitHub...! + +# TODO: will need to some scenario testing where introduce a bug which affects save_changes() to show that the GitHub issues related error +# TODO: message is definitely thrown when the error is unexpected. + # TODO: check: how does it handle (all) the fields being empty upon reading-in? Need to add redundancy here...? # - currently the application fails on start-up if something is missing or null (apart from ADCP and ST) # - there is also a bug where if ADCP is null on start up, and then you switch it on in the UI then it crashes, so this bug fix is TODO! @@ -50,10 +62,14 @@ # errors associated with inputting to the app should be 'unexpected' and therefore call to the report-to-GitHub workflow # - errors associated with ship_config and schedule.verify() should, however, provide in-UI messaging ideally # to notify the user that their selections are not valid... +# TODO: perhaps error messaging could be wrapped into one of the higher level functions i.e. ScheduleScreen?! # TODO: add TEXTUAL_SERVE dependency if end up using it... -# TODO: add a flag to the `virtualship plan` command which allows toggle between hosting the UI in terminal or web browser..defo useful for when/if hosted on cloud based JupyterLab type environments/terminals... +# TODO: I think remove web browser option for now, build stable version for terminal only (especially because VSC being mostly run in browser atm) +# TODO: Textual-serve / browser support for a future PR... + +# TODO: test in JupyterLab terminal!... # TODO: need to handle how to add the start_ and end_ datetimes automatically to Space-Time Region! # TODO: because I think the .yaml that comes from `virtualship init` will not have these as standard and we don't expect students to go add themselves... @@ -63,6 +79,8 @@ # TODO: and also for if there are errors in the yaml being opened, for example if what should be a float has been manaully changed to something invalid # TODO: or if indeed any of the pydantic model components associated with the yamls are missing +# TODO: or indeed how error messages such as those in make_validators are handled in the hypothetical situation that they are thrown... + # TODO: make sure 'Save' writes the yaml file components in the right order? Or does this not matter? # TODO: test via full run through of an expedition using yamls edited by `virtualship plan` @@ -73,6 +91,38 @@ # TODO: the valid entry + User errors etc. need to be added to the Schedule editor (currently on the Config Editor) +UNEXPECTED_MSG = ( + "\n1) Please ensure that all entries are valid (all typed entry boxes must have green borders and no warnings).\n" + "\n2) If the problem persists, please report this issue, with a description and the traceback, " + "to the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues" +) + + +def log_exception_to_file( + exception: Exception, + path: str, + filename: str = "virtualship_error.txt", + context_message: str = "Error occurred:", +): + """ + Log an exception and its traceback to a file. + + Args: + exception (Exception): The exception to log. + path (str): Directory where the log file will be saved. + filename (str, optional): Log file name. Defaults to 'virtualship_error.txt'. + context_message (str, optional): Message to write before the traceback. + + """ + error_log_path = os.path.join(path, filename) + with open(error_log_path, "w") as f: + f.write(f"{context_message}\n") + traceback.print_exception( + type(exception), exception, exception.__traceback__, file=f, chain=True + ) + f.write("\n") + + class WaypointWidget(Static): def __init__(self, waypoint: Waypoint, index: int): super().__init__() @@ -80,111 +130,175 @@ def __init__(self, waypoint: Waypoint, index: int): self.index = index def compose(self) -> ComposeResult: - with Collapsible(title=f"[b]Waypoint {self.index + 1}[/b]", collapsed=True): - if self.index > 0: - yield Button( - "Copy Time & Instruments from Previous", - id=f"wp{self.index}_copy", - variant="warning", - ) - yield Label("Location:") - yield Label(" Latitude:") - yield Input( - id=f"wp{self.index}_lat", - value=str(self.waypoint.location.lat), - ) - yield Label(" Longitude:") - yield Input( - id=f"wp{self.index}_lon", - value=str(self.waypoint.location.lon), - ) - yield Label("Time:") - with Horizontal(): - yield Label("Year:") - yield Select( - [ - (str(year), year) - for year in range( - datetime.datetime.now().year - 3, - datetime.datetime.now().year + 1, + try: + with Collapsible(title=f"[b]Waypoint {self.index + 1}[/b]", collapsed=True): + if self.index > 0: + yield Button( + "Copy Time & Instruments from Previous", + id=f"wp{self.index}_copy", + variant="warning", + ) + yield Label("Location:") + yield Label(" Latitude:") + yield Input( + id=f"wp{self.index}_lat", + value=str(self.waypoint.location.lat) + if self.waypoint.location.lat + else "", + validators=[ + Function( + is_valid_lat, + f"INVALID: value must be {is_valid_lat.__doc__.lower()}", ) ], - id=f"wp{self.index}_year", - value=int(self.waypoint.time.year) - if self.waypoint.time - else Select.BLANK, - prompt="YYYY", - classes="year-select", + type="number", + placeholder="°N", + classes="latitude-input", ) - yield Label("Month:") - yield Select( - [(f"{m:02d}", m) for m in range(1, 13)], - id=f"wp{self.index}_month", - value=int(self.waypoint.time.month) - if self.waypoint.time - else Select.BLANK, - prompt="MM", - classes="month-select", + yield Label( + "", + id=f"validation-failure-label-wp{self.index}_lat", + classes="-hidden validation-failure", ) - yield Label("Day:") - yield Select( - [(f"{d:02d}", d) for d in range(1, 32)], - id=f"wp{self.index}_day", - value=int(self.waypoint.time.day) - if self.waypoint.time - else Select.BLANK, - prompt="DD", - classes="day-select", - ) - yield Label("Hour:") - yield Select( - [(f"{h:02d}", h) for h in range(24)], - id=f"wp{self.index}_hour", - value=int(self.waypoint.time.hour) - if self.waypoint.time - else Select.BLANK, - prompt="hh", - classes="hour-select", + + yield Label(" Longitude:") + yield Input( + id=f"wp{self.index}_lon", + value=str(self.waypoint.location.lon) + if self.waypoint.location.lon + else "", + validators=[ + Function( + is_valid_lon, + f"INVALID: value must be {is_valid_lon.__doc__.lower()}", + ) + ], + type="number", + placeholder="°W", + classes="longitude-input", ) - yield Label("Min:") - yield Select( - [(f"{m:02d}", m) for m in range(0, 60, 5)], - id=f"wp{self.index}_minute", - value=int(self.waypoint.time.minute) - if self.waypoint.time - else Select.BLANK, - prompt="mm", - classes="minute-select", + yield Label( + "", + id=f"validation-failure-label-wp{self.index}_lon", + classes="-hidden validation-failure", ) - yield Label("Instruments:") - for instrument in InstrumentType: - is_selected = instrument in (self.waypoint.instrument or []) + yield Label("Time:") with Horizontal(): - yield Label(instrument.value) - yield Switch( - value=is_selected, id=f"wp{self.index}_{instrument.value}" + yield Label("Year:") + yield Select( + [ + (str(year), year) + for year in range( + datetime.datetime.now().year - 3, + datetime.datetime.now().year + 1, + ) + ], + id=f"wp{self.index}_year", + value=int(self.waypoint.time.year) + if self.waypoint.time + else Select.BLANK, + prompt="YYYY", + classes="year-select", + ) + yield Label("Month:") + yield Select( + [(f"{m:02d}", m) for m in range(1, 13)], + id=f"wp{self.index}_month", + value=int(self.waypoint.time.month) + if self.waypoint.time + else Select.BLANK, + prompt="MM", + classes="month-select", + ) + yield Label("Day:") + yield Select( + [(f"{d:02d}", d) for d in range(1, 32)], + id=f"wp{self.index}_day", + value=int(self.waypoint.time.day) + if self.waypoint.time + else Select.BLANK, + prompt="DD", + classes="day-select", + ) + yield Label("Hour:") + yield Select( + [(f"{h:02d}", h) for h in range(24)], + id=f"wp{self.index}_hour", + value=int(self.waypoint.time.hour) + if self.waypoint.time + else Select.BLANK, + prompt="hh", + classes="hour-select", ) + yield Label("Min:") + yield Select( + [(f"{m:02d}", m) for m in range(0, 60, 5)], + id=f"wp{self.index}_minute", + value=int(self.waypoint.time.minute) + if self.waypoint.time + else Select.BLANK, + prompt="mm", + classes="minute-select", + ) + + yield Label("Instruments:") + for instrument in InstrumentType: + is_selected = instrument in (self.waypoint.instrument or []) + with Horizontal(): + yield Label(instrument.value) + yield Switch( + value=is_selected, id=f"wp{self.index}_{instrument.value}" + ) + + except Exception: + raise # raise the exception to prevent incomplete UI build + # TODO: could follow this through to be included in a generic 'unexpected error' please post on Github issue in say ScheduleApp? def copy_from_previous(self) -> None: - if self.index > 0: - schedule_editor = self.parent - if schedule_editor: - # Only copy time components and instruments, not lat/lon - time_components = ["year", "month", "day", "hour", "minute"] - for comp in time_components: - prev = schedule_editor.query_one(f"#wp{self.index - 1}_{comp}") - curr = self.query_one(f"#wp{self.index}_{comp}") - if prev and curr: - curr.value = prev.value + try: + if self.index > 0: + schedule_editor = self.parent + if schedule_editor: + # only copy time components and instruments, not lat/lon + time_components = ["year", "month", "day", "hour", "minute"] + for comp in time_components: + prev = schedule_editor.query_one(f"#wp{self.index - 1}_{comp}") + curr = self.query_one(f"#wp{self.index}_{comp}") + if prev and curr: + curr.value = prev.value + + for instrument in InstrumentType: + prev_switch = schedule_editor.query_one( + f"#wp{self.index - 1}_{instrument.value}" + ) + curr_switch = self.query_one( + f"#wp{self.index}_{instrument.value}" + ) + if prev_switch and curr_switch: + curr_switch.value = prev_switch.value + except Exception: + raise # raise the exception to prevent incomplete UI build + # TODO: could follow this through to be included in a generic 'unexpected error' please post on Github issue in say ScheduleApp? - for instrument in InstrumentType: - prev_switch = schedule_editor.query_one( - f"#wp{self.index - 1}_{instrument.value}" - ) - curr_switch = self.query_one(f"#wp{self.index}_{instrument.value}") - if prev_switch and curr_switch: - curr_switch.value = prev_switch.value + @on(Input.Changed) + def show_invalid_reasons(self, event: Input.Changed) -> None: + input_id = event.input.id + label_id = f"validation-failure-label-{input_id}" + label = self.query_one(f"#{label_id}", Label) + if not event.validation_result.is_valid: + message = ( + "\n".join(event.validation_result.failure_descriptions) + if isinstance(event.validation_result.failure_descriptions, list) + else str(event.validation_result.failure_descriptions) + ) + label.update(message) + label.remove_class("-hidden") + label.add_class("validation-failure") + else: + label.update("") + label.add_class("-hidden") + label.remove_class("validation-failure") @on(Button.Pressed, "Button") def button_pressed(self, event: Button.Pressed) -> None: @@ -201,9 +315,18 @@ def __init__(self, path: str): def compose(self) -> ComposeResult: try: self.schedule = Schedule.from_yaml(f"{self.path}/schedule.yaml") + except Exception: + # TODO: this error message needs far more detail, just a placeholder for now! + raise UserError( + "There is something wrong with schedule.yaml. Please fix." + ) from None + + try: yield Label("[b]Schedule Editor[/b]", id="title", markup=True) yield Rule(line_style="heavy") + # SECTION: "Waypoints & Instrument Selection" + with Collapsible( title="[b]Waypoints & Instrument Selection[/b]", collapsed=True, @@ -211,7 +334,8 @@ def compose(self) -> ComposeResult: for i, waypoint in enumerate(self.schedule.waypoints): yield WaypointWidget(waypoint, i) - # Space-Time Region Section + # SECTION: "Space-Time Region" + # TODO: MAY NEED TO ADD A FEATURE ON SAVE CHANGES WHICH AUTOMATICALLY DETECTS MAX AND MIN TIME # TODO: FOR THE SCENARIO WHERE YAML LOADED IN IS NULL AND USER DOES NOT EDIT THEMSELVES with Collapsible( @@ -271,8 +395,8 @@ def compose(self) -> ComposeResult: ), ) - except Exception as e: - yield Label(f"Error loading schedule: {e!s}") + except Exception: + raise UnexpectedError(UNEXPECTED_MSG) from None def save_changes(self) -> bool: """Save changes to schedule.yaml.""" @@ -414,13 +538,6 @@ class ConfigEditor(Container): }, } - FIELD_CONSTRAINT_ATTRS: ClassVar[tuple] = ( - "gt", - "ge", - "lt", - "le", - ) # Pydantic field constraint attributes used for validation and introspection - # TODO: Also incorporate verify methods! def __init__(self, path: str): @@ -429,23 +546,28 @@ def __init__(self, path: str): self.config = None def compose(self) -> ComposeResult: + try: + self.config = ShipConfig.from_yaml(f"{self.path}/ship_config.yaml") + except Exception: + # TODO: this error message needs far more detail, just a placeholder for now! + raise UserError( + "There is something wrong with ship_config.yaml. Please fix." + ) from None + try: ## SECTION: "Ship Speed & Onboard Measurements" - self.config = ShipConfig.from_yaml(f"{self.path}/ship_config.yaml") yield Label("[b]Ship Config Editor[/b]", id="title", markup=True) yield Rule(line_style="heavy") with Collapsible(title="[b]Ship Speed & Onboard Measurements[/b]"): attr = "ship_speed_knots" - validators = self.group_validators(ShipConfig, attr) + validators = group_validators(ShipConfig, attr) with Horizontal(classes="ship_speed"): yield Label("[b]Ship Speed (knots):[/b]") yield Input( id="speed", - type=self.type_to_textual( - self.get_field_type(ShipConfig, attr) - ), + type=type_to_textual(get_field_type(ShipConfig, attr)), validators=[ Function( validator, @@ -496,16 +618,22 @@ def compose(self) -> ComposeResult: attributes = info["attributes"] config_instance = getattr(self.config, instrument_name, None) title = info.get("title", instrument_name.replace("_", " ").title()) - with Collapsible( title=f"[b]{title}[/b]", collapsed=True, ): + if instrument_name in ( + "adcp_config", + "ship_underwater_st_config", + ): + yield Label( + f"NOTE: entries will be ignored here if {info['title']} is OFF in Ship Speed & Onboard Measurements." + ) with Container(classes="instrument-config"): for attr_meta in attributes: attr = attr_meta["name"] is_minutes = attr_meta.get("minutes", False) - validators = self.group_validators(config_class, attr) + validators = group_validators(config_class, attr) if config_instance: raw_value = getattr(config_instance, attr, "") if is_minutes and raw_value != "": @@ -522,8 +650,8 @@ def compose(self) -> ComposeResult: yield Label(f"{attr.replace('_', ' ').title()}:") yield Input( id=f"{instrument_name}_{attr}", - type=self.type_to_textual( - self.get_field_type(config_class, attr) + type=type_to_textual( + get_field_type(config_class, attr) ), validators=[ Function( @@ -540,8 +668,9 @@ def compose(self) -> ComposeResult: classes="-hidden validation-failure", ) - except Exception as e: - yield Label(f"Error loading ship config: {e!s}") + except Exception: + raise # raise the exception to prevent incomplete UI build + # TODO: could follow this through to be included in a generic 'unexpected error' please post on Github issue in say ScheduleApp? @on(Input.Changed) def show_invalid_reasons(self, event: Input.Changed) -> None: @@ -563,7 +692,10 @@ def show_invalid_reasons(self, event: Input.Changed) -> None: label.remove_class("validation-failure") def on_mount(self) -> None: - self.show_hide_adcp_type(bool(self.config.adcp_config)) + adcp_present = ( + getattr(self.config, "adcp_config", None) if self.config else False + ) + self.show_hide_adcp_type(bool(adcp_present)) def show_hide_adcp_type(self, show: bool) -> None: container = self.query_one("#adcp_type_container") @@ -573,17 +705,17 @@ def show_hide_adcp_type(self, show: bool) -> None: container.add_class("-hidden") def _set_adcp_default_values(self): - self.query_one("#adcp_num_bins").value = str( + self.query_one("#adcp_config_num_bins").value = str( self.DEFAULT_ADCP_CONFIG["num_bins"] ) - self.query_one("#adcp_period").value = str( + self.query_one("#adcp_config_period").value = str( self.DEFAULT_ADCP_CONFIG["period_minutes"] ) - self.query_one("#adcp_shallow").value = True - self.query_one("#adcp_deep").value = False + self.query_one("#adcp_shallow").value = False + self.query_one("#adcp_deep").value = True def _set_ts_default_values(self): - self.query_one("#ts_period").value = str( + self.query_one("#ship_underwater_st_config_period").value = str( self.DEFAULT_TS_CONFIG["period_minutes"] ) @@ -612,133 +744,12 @@ def shallow_changed(self, event: Switch.Changed) -> None: deep = self.query_one("#adcp_deep", Switch) deep.value = False - def get_field_type(self, model_class, field_name): - """Get Pydantic model class data type.""" - return model_class.model_fields[field_name].annotation - - def type_to_textual(self, field_type): - """Convert data type to str which Textual can interpret for type = setting in Input objects.""" - if field_type in (float, datetime.timedelta): - return "number" - elif field_type is int: - return "integer" - else: - return "text" - - def get_field_conditions(self, model_class, field_name): - """Determine and return what conditions (and associated reference value) a Pydantic model sets on inputs.""" - field_info = model_class.model_fields[field_name] - conditions = {} - for meta in field_info.metadata: - for attr in dir(meta): - if not attr.startswith("_") and getattr(meta, attr) is not None: - if attr in self.FIELD_CONSTRAINT_ATTRS: - conditions[attr] = getattr(meta, attr) - else: - raise ValueError( - f"Unexpected constraint '{attr}' found on field '{field_name}'. " - f"Allowed constraints: {self.FIELD_CONSTRAINT_ATTRS}" - ) - return list(conditions.keys()), list(conditions.values()) - - def make_validator(self, condition, reference, value_type): - """ - Make a validator function based on the Pydantic model field conditions returned by get_field_conditions(). - - N.B. #1 Textual validator tools do not currently support additional arguments (such as 'reference') being fed into the validator functions (such as is_gt0) at present. - Therefore, reference values cannot be fed in dynamically and necessitates hard coding depending onthe condition and reference value combination. - At present, Pydantic models only require gt/ge/lt/le relative to **0.0** so `reference` is always checked as being == 0.0 - Additional custom conditions can be "hard-coded" as new condition and reference combinations if Pydantic model specifications change in the future and/or new instruments are added to VirtualShip etc. - TODO: Perhaps there's scope here though for a more robust implementation in a future PR... - - N.B. #2, docstrings describing the conditional are required in the child functions (e.g. is_gt0), and presence is checked by require_docstring(). - Docstrings will be used to generate informative UI invalid entry messages, so them being informative and accurate is important! - - """ - - def require_docstring(func): - if func.__doc__ is None or not func.__doc__.strip(): - raise ValueError(f"Function '{func.__name__}' must have a docstring.") - return func - - def convert(value): - try: - if value_type is datetime.timedelta: - return datetime.timedelta(minutes=float(value)) - return value_type(value) - except Exception: - return None - - if value_type in (float, int) and reference == 0.0: - ref_zero = 0.0 - elif value_type is datetime.timedelta and reference == datetime.timedelta(): - ref_zero = datetime.timedelta() - else: - raise ValueError( - f"Unsupported value_type/reference combination: {value_type}, {reference}" - ) - - if condition == "gt" and reference == ref_zero: - - @require_docstring - def is_gt0(value: str) -> bool: - """Greater than 0.""" - v = convert(value) - return v is not None and v > ref_zero - - return is_gt0 - - if condition == "ge" and reference == ref_zero: - - @require_docstring - def is_ge0(value: str) -> bool: - """Greater than or equal to 0.""" - v = convert(value) - return v is not None and v >= ref_zero - - return is_ge0 - - if condition == "lt" and reference == ref_zero: - - @require_docstring - def is_lt0(value: str) -> bool: - """Less than 0.""" - v = convert(value) - return v is not None and v < ref_zero - - return is_lt0 - - if condition == "le" and reference == ref_zero: - - @require_docstring - def is_le0(value: str) -> bool: - """Less than or equal to 0.""" - v = convert(value) - return v is not None and v <= ref_zero - - return is_le0 - - else: - raise ValueError( - f"Unknown condition: {condition} and reference value: {reference} combination." - ) - - def group_validators(self, model, attr): - """Bundle all validators for Input into singular list.""" - return [ - self.make_validator(cond, ref, self.get_field_type(model, attr)) - for cond, ref in zip( - *self.get_field_conditions(model, attr), - strict=False, - ) - ] - def save_changes(self) -> bool: """Save changes to ship_config.yaml.""" try: # ship speed attr = "ship_speed_knots" - field_type = self.get_field_type(self.config, attr) + field_type = get_field_type(self.config, attr) value = field_type(self.query_one("#speed").value) ShipConfig.model_validate( {**self.config.model_dump(), attr: value} @@ -750,37 +761,38 @@ def save_changes(self) -> bool: config_class = info["class"] attributes = info["attributes"] kwargs = {} + + # special handling for onboard ADCP and T/S + # will skip to next instrument if toggle is off + if instrument_name == "adcp_config": + has_adcp = self.query_one("#has_adcp", Switch).value + if not has_adcp: + setattr(self.config, instrument_name, None) + continue + if instrument_name == "ship_underwater_st_config": + has_ts = self.query_one("#has_onboard_ts", Switch).value + if not has_ts: + setattr(self.config, instrument_name, None) + continue + for attr_meta in attributes: attr = attr_meta["name"] is_minutes = attr_meta.get("minutes", False) input_id = f"{instrument_name}_{attr}" value = self.query_one(f"#{input_id}").value - field_type = self.get_field_type(config_class, attr) + field_type = get_field_type(config_class, attr) if is_minutes and field_type is datetime.timedelta: value = datetime.timedelta(minutes=float(value)) else: value = field_type(value) kwargs[attr] = value - # TODO: the special handling does not seem to work for both instruments - # TODO: in the ADCP case it causes an error, in the T/S case it saves but doesn't actually write anything... - - # special handling for onboard ADCP, conditional on whether it's selected by user in Switch (overwrite with None if switch is off) + # ADCP max_depth_meter based on deep/shallow switch if instrument_name == "adcp_config": - has_adcp = self.query_one("#has_adcp", Switch).value - if has_adcp: # determine max_depth_meter value based on sub-Switch selection by user, i.e. deep or shallow ADCP - if self.query_one("#adcp_deep", Switch).value: - kwargs["max_depth_meter"] = -1000.0 - else: - kwargs["max_depth_meter"] = -150.0 + if self.query_one("#adcp_deep", Switch).value: + kwargs["max_depth_meter"] = -1000.0 else: - setattr(self.config, instrument_name, None) - - # special handling for onboard T/S, conditional on whether it's selected by user (overwrite with None if switch is off) - if instrument_name == "ship_underwater_st_config": - has_ts = self.query_one("#has_onboard_ts", Switch).value - if not has_ts: - setattr(self.config, instrument_name, None) + kwargs["max_depth_meter"] = -150.0 setattr(self.config, instrument_name, config_class(**kwargs)) @@ -790,19 +802,11 @@ def save_changes(self) -> bool: except Exception as e: # write error log - error_log_path = os.path.join(self.path, "virtualship_error.txt") - with open(error_log_path, "w") as f: - f.write("Error saving ship config:\n") - traceback.print_exception( - type(e), e, e.__traceback__, file=f, chain=True - ) - f.write("\n") + log_exception_to_file( + e, self.path, context_message="Error saving ship config:" + ) - raise UnexpectedError( - "\n1) Please ensure that all entries are valid (all typed entry boxes must have green borders and no warnings).\n" - "\n2) If the problem persists, please report this issue, with a description and the traceback, " - "to the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues" - ) from None + raise UnexpectedError(UNEXPECTED_MSG) from None class ScheduleScreen(Screen): @@ -839,7 +843,7 @@ def save_pressed(self) -> None: except Exception as e: self.notify( - f"*** Error saving changes ***:\n\n{e}\n\nTraceback will be logged in `{self.path}/virtualship_error.txt`. Please copy the file and/or its contents when submitting an issue.", + f"*** Error saving changes ***:\n\n{e}\n\nTraceback will be logged in `{self.path}/virtualship_error.txt`. Please attach the file and/or its contents when submitting an issue.\n", severity="error", timeout=20, ) @@ -1030,13 +1034,20 @@ def on_mount(self) -> None: self.theme = "textual-light" -if __name__ == "__main__": - # parse path - if len(sys.argv) > 1: - path = sys.argv[1] - else: - raise ValueError("No path argument provided") - - # run app +def _plan(path: str) -> None: + """Run UI in terminal.""" app = ScheduleApp(path) app.run() + + +# if __name__ == "__main__": +# """Used if running UI in browser via `python -m ...` command.""" +# # parse path +# if len(sys.argv) > 1: +# path = sys.argv[1] +# else: +# raise ValueError("No path argument provided") + +# # run app +# app = ScheduleApp(path) +# app.run() diff --git a/src/virtualship/cli/commands.py b/src/virtualship/cli/commands.py index fc5d1392..531556a0 100644 --- a/src/virtualship/cli/commands.py +++ b/src/virtualship/cli/commands.py @@ -74,16 +74,30 @@ def init(path, from_mfp): "path", type=click.Path(exists=False, file_okay=False, dir_okay=True), ) -def plan(path): - """Launch UI to help build schedule and ship config files. Opens in web browser, hosted on the user's local machine only.""" - server = Server( - command=f"python -m virtualship.cli._plan {Path(path)}", - title="VirtualShip plan", - ) - url = "http://localhost:8000" - webbrowser.open(url) - # TODO: remove debug = True when goes live - server.serve(debug=True) +@click.option( + "--terminal/--web", + default=False, + help="Run the VirtualShip planner in the terminal (default: web browser).", +) + +# TODO: implement auto detection of whether running remotely e.g. in JupyterLab (already in browser) to automatically run in terminal +# TODO: rather than launching in browser from the browser... + +def plan(path, terminal): + """Launch UI to help build schedule and ship config files. Opens in web browser by default, or in the terminal with --terminal.""" + if terminal: + from virtualship.cli._plan import _plan + + _plan(Path(path)) + else: + server = Server( + command=f"python -m virtualship.cli._plan {Path(path)}", + title="VirtualShip plan", + ) + url = "http://localhost:8000" + webbrowser.open(url) + # TODO: remove debug = True when goes live + server.serve(debug=True) @click.command() diff --git a/src/virtualship/cli/validator_utils.py b/src/virtualship/cli/validator_utils.py new file mode 100644 index 00000000..9f95f4b9 --- /dev/null +++ b/src/virtualship/cli/validator_utils.py @@ -0,0 +1,172 @@ +""" + +Utils for validating inputs to Pydantic model fields in a Textual setting and generating Textual input validators. + +Note, all validator functions require docstrings which describe the condition (used in error messaging). Presence is checked +by require_docstring() helper function. + +""" + +import datetime + + +def require_docstring(func): + if func.__doc__ is None or not func.__doc__.strip(): + raise ValueError(f"Function '{func.__name__}' must have a docstring.") + return func + + +# SCHEDULE INUTS VALIDATION + + +@require_docstring +def is_valid_lat(value: str) -> bool: + """(-90° < lat < 90°).""" + try: + v = float(value) + except ValueError: + return None + + return -90 < v < 90 + + +@require_docstring +def is_valid_lon(value: str) -> bool: + """(-180 < lon < 360).""" + try: + v = float(value) + except ValueError: + return None + + return -180 < v < 360 + + +# SHIP CONFIG INPUTS VALIDATION + +FIELD_CONSTRAINT_ATTRS = ( + "gt", + "ge", + "lt", + "le", +) # pydantic field constraint attributes used for validation and introspection + + +def get_field_type(model_class, field_name): + """Get Pydantic model class data type.""" + return model_class.model_fields[field_name].annotation + + +def type_to_textual(field_type): + """Convert data type to str which Textual can interpret for type = setting in Input objects.""" + if field_type in (float, datetime.timedelta): + return "number" + elif field_type is int: + return "integer" + else: + return "text" + + +def get_field_conditions(model_class, field_name): + """Determine and return what conditions (and associated reference value) a Pydantic model sets on inputs.""" + field_info = model_class.model_fields[field_name] + conditions = {} + for meta in field_info.metadata: + for attr in dir(meta): + if not attr.startswith("_") and getattr(meta, attr) is not None: + if attr in FIELD_CONSTRAINT_ATTRS: + conditions[attr] = getattr(meta, attr) + else: + raise ValueError( + f"Unexpected constraint '{attr}' found on field '{field_name}'. " + f"Allowed constraints: {FIELD_CONSTRAINT_ATTRS}" + ) + return list(conditions.keys()), list(conditions.values()) + + +def make_validator(condition, reference, value_type): + """ + Make a validator function based on the Pydantic model field conditions returned by get_field_conditions(). + + N.B. #1, docstrings describing the conditional are required in the child functions (e.g. is_gt0), and presence is checked by require_docstring(). + Docstrings will be used to generate informative UI invalid entry messages, so them being informative and accurate is important! + + N.B. #2 Textual validator tools do not currently support additional arguments (such as 'reference' values) being fed into the validator functions (such as is_gt0) at present. + Therefore, reference values for the conditions cannot be fed in dynamically and necessitates "hard-coding" the condition and reference value combination. + At present, Pydantic models in VirtualShip only require gt/ge/lt/le relative to **0.0** so "reference" is always checked as being == 0.0 + Additional custom conditions can be "hard-coded" as new condition and reference combinations if Pydantic model specifications change in the future and/or new instruments are added to VirtualShip etc. + TODO: Perhaps there's scope here though for a more flexible implementation in a future PR... + + """ + + def convert(value): + try: + if value_type is datetime.timedelta: + return datetime.timedelta(minutes=float(value)) + return value_type(value) + except Exception: + return None + + if value_type in (float, int) and reference == 0.0: + ref_zero = 0.0 + elif value_type is datetime.timedelta and reference == datetime.timedelta(): + ref_zero = datetime.timedelta() + else: + raise ValueError( + f"Unsupported value_type/reference combination: {value_type}, {reference}" + ) + + if condition == "gt" and reference == ref_zero: + + @require_docstring + def is_gt0(value: str) -> bool: + """Greater than 0.""" + v = convert(value) + return v is not None and v > ref_zero + + return is_gt0 + + if condition == "ge" and reference == ref_zero: + + @require_docstring + def is_ge0(value: str) -> bool: + """Greater than or equal to 0.""" + v = convert(value) + return v is not None and v >= ref_zero + + return is_ge0 + + if condition == "lt" and reference == ref_zero: + + @require_docstring + def is_lt0(value: str) -> bool: + """Less than 0.""" + v = convert(value) + return v is not None and v < ref_zero + + return is_lt0 + + if condition == "le" and reference == ref_zero: + + @require_docstring + def is_le0(value: str) -> bool: + """Less than or equal to 0.""" + v = convert(value) + return v is not None and v <= ref_zero + + return is_le0 + + else: + raise ValueError( + f"Unknown condition: {condition} and reference value: {reference} combination." + ) + + +def group_validators(model, attr): + """Bundle all validators for Input into singular list.""" + return [ + make_validator(cond, ref, get_field_type(model, attr)) + for cond, ref in zip( + *get_field_conditions(model, attr), + strict=False, + ) + ] diff --git a/src/virtualship/errors.py b/src/virtualship/errors.py index f98f2a41..cdd58349 100644 --- a/src/virtualship/errors.py +++ b/src/virtualship/errors.py @@ -28,6 +28,12 @@ class ConfigError(RuntimeError): pass +class UserError(Exception): + """Error raised when there is an user error.""" + + pass + + class UnexpectedError(Exception): """Error raised when there is an unexpected problem.""" From 1f7475f88540df80ba35b4ffd1a59f47908d5772 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 24 Jun 2025 20:14:00 +0200 Subject: [PATCH 14/30] fix potential bug in longitude validation --- src/virtualship/cli/validator_utils.py | 4 ++-- src/virtualship/models/location.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/virtualship/cli/validator_utils.py b/src/virtualship/cli/validator_utils.py index 9f95f4b9..44796d74 100644 --- a/src/virtualship/cli/validator_utils.py +++ b/src/virtualship/cli/validator_utils.py @@ -32,13 +32,13 @@ def is_valid_lat(value: str) -> bool: @require_docstring def is_valid_lon(value: str) -> bool: - """(-180 < lon < 360).""" + """(-180 < lon < 180).""" try: v = float(value) except ValueError: return None - return -180 < v < 360 + return -180 < v < 180 # SHIP CONFIG INPUTS VALIDATION diff --git a/src/virtualship/models/location.py b/src/virtualship/models/location.py index 793e5312..d3657829 100644 --- a/src/virtualship/models/location.py +++ b/src/virtualship/models/location.py @@ -22,8 +22,8 @@ def __post_init__(self) -> None: raise ValueError("Latitude cannot be larger than 90.") if self.lon < -180: raise ValueError("Longitude cannot be smaller than -180.") - if self.lon > 360: - raise ValueError("Longitude cannot be larger than 360.") + if self.lon > 180: + raise ValueError("Longitude cannot be larger than 180.") @property def lat(self) -> float: From 22b36915e2ffa7aa5af4451b8f5aacd216497bbf Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 24 Jun 2025 21:17:20 +0200 Subject: [PATCH 15/30] add validators to space time region inputs --- src/virtualship/cli/_plan.py | 148 ++++++++++++++++++-- src/virtualship/cli/validator_utils.py | 25 ++++ src/virtualship/models/space_time_region.py | 1 + 3 files changed, 165 insertions(+), 9 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 85705a6d..25706555 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -22,8 +22,10 @@ from virtualship.cli.validator_utils import ( get_field_type, group_validators, + is_valid_depth, is_valid_lat, is_valid_lon, + is_valid_timestr, type_to_textual, ) from virtualship.errors import UnexpectedError, UserError @@ -174,7 +176,7 @@ def compose(self) -> ComposeResult: ) ], type="number", - placeholder="°W", + placeholder="°E", classes="longitude-input", ) yield Label( @@ -315,10 +317,10 @@ def __init__(self, path: str): def compose(self) -> ComposeResult: try: self.schedule = Schedule.from_yaml(f"{self.path}/schedule.yaml") - except Exception: + except Exception as e: # TODO: this error message needs far more detail, just a placeholder for now! raise UserError( - "There is something wrong with schedule.yaml. Please fix." + f"There is something wrong with schedule.yaml. Please fix. More detail: {e}" ) from None try: @@ -334,8 +336,6 @@ def compose(self) -> ComposeResult: for i, waypoint in enumerate(self.schedule.waypoints): yield WaypointWidget(waypoint, i) - # SECTION: "Space-Time Region" - # TODO: MAY NEED TO ADD A FEATURE ON SAVE CHANGES WHICH AUTOMATICALLY DETECTS MAX AND MIN TIME # TODO: FOR THE SCENARIO WHERE YAML LOADED IN IS NULL AND USER DOES NOT EDIT THEMSELVES with Collapsible( @@ -344,36 +344,123 @@ def compose(self) -> ComposeResult: ): if self.schedule.space_time_region: str_data = self.schedule.space_time_region + yield Label("Minimum Latitude:") yield Input( id="min_lat", - value=str(str_data.spatial_range.minimum_latitude), + value=str(str_data.spatial_range.minimum_latitude) + if str_data.spatial_range.minimum_latitude + else "", + validators=[ + Function( + is_valid_lat, + f"INVALID: value must be {is_valid_lat.__doc__.lower()}", + ) + ], + type="number", + placeholder="°N", ) + yield Label( + "", + id="validation-failure-label-min_lat", + classes="-hidden validation-failure", + ) + yield Label("Maximum Latitude:") yield Input( id="max_lat", value=str(str_data.spatial_range.maximum_latitude), + validators=[ + Function( + is_valid_lat, + f"INVALID: value must be {is_valid_lat.__doc__.lower()}", + ) + ], + type="number", + placeholder="°N", + ) + yield Label( + "", + id="validation-failure-label-max_lat", + classes="-hidden validation-failure", ) + yield Label("Minimum Longitude:") yield Input( id="min_lon", value=str(str_data.spatial_range.minimum_longitude), + validators=[ + Function( + is_valid_lon, + f"INVALID: value must be {is_valid_lon.__doc__.lower()}", + ) + ], + type="number", + placeholder="°E", + ) + yield Label( + "", + id="validation-failure-label-min_lon", + classes="-hidden validation-failure", ) + yield Label("Maximum Longitude:") yield Input( id="max_lon", value=str(str_data.spatial_range.maximum_longitude), + validators=[ + Function( + is_valid_lon, + f"INVALID: value must be {is_valid_lon.__doc__.lower()}", + ) + ], + type="number", + placeholder="°E", ) + yield Label( + "", + id="validation-failure-label-max_lon", + classes="-hidden validation-failure", + ) + yield Label("Minimum Depth (meters):") yield Input( id="min_depth", value=str(str_data.spatial_range.minimum_depth), + validators=[ + Function( + is_valid_depth, + f"INVALID: value must be {is_valid_depth.__doc__.lower()}", + ) + ], + type="number", + placeholder="m", + ) + yield Label( + "", + id="validation-failure-label-min_depth", + classes="-hidden validation-failure", ) + yield Label("Maximum Depth (meters):") yield Input( id="max_depth", value=str(str_data.spatial_range.maximum_depth), + validators=[ + Function( + is_valid_depth, + f"INVALID: value must be {is_valid_depth.__doc__.lower()}", + ) + ], + type="number", + placeholder="m", + ) + yield Label( + "", + id="validation-failure-label-max_depth", + classes="-hidden validation-failure", ) + yield Label("Start Time:") yield Input( id="start_time", @@ -383,7 +470,20 @@ def compose(self) -> ComposeResult: if str_data.time_range and str_data.time_range.start_time else "" ), + validators=[ + Function( + is_valid_timestr, + f"INVALID: value must be {is_valid_timestr.__doc__.lower()}", + ) + ], + type="text", ) + yield Label( + "", + id="validation-failure-label-start_time", + classes="-hidden validation-failure", + ) + yield Label("End Time:") yield Input( id="end_time", @@ -393,11 +493,41 @@ def compose(self) -> ComposeResult: if str_data.time_range and str_data.time_range.end_time else "" ), + validators=[ + Function( + is_valid_timestr, + f"INVALID: value must be {is_valid_timestr.__doc__.lower()}", + ) + ], + type="text", + ) + yield Label( + "", + id="validation-failure-label-end_time", + classes="-hidden validation-failure", ) - except Exception: raise UnexpectedError(UNEXPECTED_MSG) from None + @on(Input.Changed) + def show_invalid_reasons(self, event: Input.Changed) -> None: + input_id = event.input.id + label_id = f"validation-failure-label-{input_id}" + label = self.query_one(f"#{label_id}", Label) + if not event.validation_result.is_valid: + message = ( + "\n".join(event.validation_result.failure_descriptions) + if isinstance(event.validation_result.failure_descriptions, list) + else str(event.validation_result.failure_descriptions) + ) + label.update(message) + label.remove_class("-hidden") + label.add_class("validation-failure") + else: + label.update("") + label.add_class("-hidden") + label.remove_class("validation-failure") + def save_changes(self) -> bool: """Save changes to schedule.yaml.""" # TODO: SAVE_CHANGES() NEEDS TO BE LARGELY RE-WORKED NOW THAT MORE VALIDATION IS BUILT INTO THE INPUTS @@ -548,10 +678,10 @@ def __init__(self, path: str): def compose(self) -> ComposeResult: try: self.config = ShipConfig.from_yaml(f"{self.path}/ship_config.yaml") - except Exception: + except Exception as e: # TODO: this error message needs far more detail, just a placeholder for now! raise UserError( - "There is something wrong with ship_config.yaml. Please fix." + f"There is something wrong with ship_config.yaml. Please fix. More detail: {e}" ) from None try: diff --git a/src/virtualship/cli/validator_utils.py b/src/virtualship/cli/validator_utils.py index 44796d74..9aa4b1cd 100644 --- a/src/virtualship/cli/validator_utils.py +++ b/src/virtualship/cli/validator_utils.py @@ -41,6 +41,31 @@ def is_valid_lon(value: str) -> bool: return -180 < v < 180 +@require_docstring +def is_valid_depth(value: str) -> bool: + """Float.""" + try: + v = float(value) + except ValueError: + return None + + # NOTE: depth model in space_time_region.py ONLY specifies that depth must be float (and no conditions < 0) + # NOTE: therefore, this condition is carried forward here to match what currently exists + # NOTE: however, there is a TODO in space_time_region.py to add conditions as Pydantic Field + # TODO: update validator here if/when depth model is updated in space_time_region.py + return isinstance(v, float) + + +@require_docstring +def is_valid_timestr(value: str) -> bool: + """Format YYYY-MM-DD hh:mm:ss.""" + try: + datetime.datetime.strptime(value, "%Y-%m-%d %H:%M:%S") + return True + except Exception: + return False + + # SHIP CONFIG INPUTS VALIDATION FIELD_CONSTRAINT_ATTRS = ( diff --git a/src/virtualship/models/space_time_region.py b/src/virtualship/models/space_time_region.py index 22008805..48ad5699 100644 --- a/src/virtualship/models/space_time_region.py +++ b/src/virtualship/models/space_time_region.py @@ -9,6 +9,7 @@ Longitude = Annotated[float, Field(..., ge=-180, le=180)] Latitude = Annotated[float, Field(..., ge=-90, le=90)] Depth = float # TODO: insert a minimum depth here? e.g., `Annotated[float, Field(..., ge=0)]` +# TODO: is_valid_depth in validator_utils.py will alse need to be updated if this TODO is implemented class SpatialRange(BaseModel): From 64e34862f9763fd580ea0daabf2c65af688900d1 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 25 Jun 2025 14:52:03 +0100 Subject: [PATCH 16/30] Update save_changes method in ScheduleEditor --- src/virtualship/cli/_plan.py | 74 +++++++++----------------- src/virtualship/cli/validator_utils.py | 2 +- 2 files changed, 26 insertions(+), 50 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 25706555..0b452d7b 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -43,11 +43,13 @@ XBTConfig, ) from virtualship.models.space_time_region import ( - SpaceTimeRegion, SpatialRange, TimeRange, ) +# TODO: need to build TESTS for the UI! +# - look into best way of testing this kind of thing... + # TODO: need to distinguish in error handling between usererrors such as faulty yaml read in add pointers to the User to make fixes, # TODO: and errors which are genuinely unexpected, are likely bugs and need attention on GitHub...! @@ -92,10 +94,14 @@ # TODO: the valid entry + User errors etc. need to be added to the Schedule editor (currently on the Config Editor) +# TODO: add a check if instruments selections are left empty? + UNEXPECTED_MSG = ( - "\n1) Please ensure that all entries are valid (all typed entry boxes must have green borders and no warnings).\n" - "\n2) If the problem persists, please report this issue, with a description and the traceback, " + "Please ensure that:\n" + "\n1) All typed entries are valid (all boxes in all sections must have green borders and no warnings).\n" + "\n2) Time selections exist for all waypoints.\n" + "\nIf the problem persists, please report this issue, with a description and the traceback, " "to the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues" ) @@ -318,10 +324,7 @@ def compose(self) -> ComposeResult: try: self.schedule = Schedule.from_yaml(f"{self.path}/schedule.yaml") except Exception as e: - # TODO: this error message needs far more detail, just a placeholder for now! - raise UserError( - f"There is something wrong with schedule.yaml. Please fix. More detail: {e}" - ) from None + raise UserError(f"There is an issue in schedule.yaml:\n\n{e}") from None try: yield Label("[b]Schedule Editor[/b]", id="title", markup=True) @@ -530,10 +533,10 @@ def show_invalid_reasons(self, event: Input.Changed) -> None: def save_changes(self) -> bool: """Save changes to schedule.yaml.""" - # TODO: SAVE_CHANGES() NEEDS TO BE LARGELY RE-WORKED NOW THAT MORE VALIDATION IS BUILT INTO THE INPUTS # TODO: and should proabably now be more focussed on .verify() methods try: # spacetime region + spatial_range = SpatialRange( minimum_longitude=self.query_one("#min_lon").value, maximum_longitude=self.query_one("#max_lon").value, @@ -548,20 +551,16 @@ def save_changes(self) -> bool: end_time=self.query_one("#end_time").value, ) - space_time_region = SpaceTimeRegion( - spatial_range=spatial_range, time_range=time_range - ) - - self.schedule.space_time_region = space_time_region + self.schedule.space_time_region.spatial_range = spatial_range + self.schedule.space_time_region.time_range = time_range # waypoints - waypoints = [] - for i in range(len(self.schedule.waypoints)): - location = Location( + for i, wp in enumerate(self.schedule.waypoints): + wp.location = Location( latitude=float(self.query_one(f"#wp{i}_lat").value), longitude=float(self.query_one(f"#wp{i}_lon").value), ) - time = datetime.datetime( + wp.time = datetime.datetime( int(self.query_one(f"#wp{i}_year").value), int(self.query_one(f"#wp{i}_month").value), int(self.query_one(f"#wp{i}_day").value), @@ -569,29 +568,23 @@ def save_changes(self) -> bool: int(self.query_one(f"#wp{i}_minute").value), 0, ) - instruments = [ + wp.instrument = [ instrument for instrument in InstrumentType if self.query_one(f"#wp{i}_{instrument.value}").value ] - waypoints.append( - Waypoint(location=location, time=time, instrument=instruments) - ) - # save - self.schedule.waypoints = waypoints self.schedule.to_yaml(f"{self.path}/schedule.yaml") return True except Exception as e: - self.notify( - f"Error saving schedule: {e!r}", - severity="error", - timeout=60, - markup=False, + # write error log + log_exception_to_file( + e, self.path, context_message="Error saving ship config:" ) - return False + + raise UnexpectedError(UNEXPECTED_MSG) from None class ConfigEditor(Container): @@ -679,10 +672,7 @@ def compose(self) -> ComposeResult: try: self.config = ShipConfig.from_yaml(f"{self.path}/ship_config.yaml") except Exception as e: - # TODO: this error message needs far more detail, just a placeholder for now! - raise UserError( - f"There is something wrong with ship_config.yaml. Please fix. More detail: {e}" - ) from None + raise UserError(f"There is an issue in ship_config.yaml:\n\n{e}") from None try: ## SECTION: "Ship Speed & Onboard Measurements" @@ -987,7 +977,6 @@ class ScheduleApp(App): } ConfigEditor { - background: $panel; padding: 1; margin-bottom: 1; height: auto; @@ -1009,7 +998,7 @@ class ScheduleApp(App): WaypointWidget > Collapsible { margin: 1; - background: $boost; + background: $panel; border: solid $primary; } @@ -1097,7 +1086,7 @@ class ScheduleApp(App): Collapsible > .collapsible--content > Collapsible { margin: 0 1; - background: $surface; + background: $panel; } .-hidden { @@ -1168,16 +1157,3 @@ def _plan(path: str) -> None: """Run UI in terminal.""" app = ScheduleApp(path) app.run() - - -# if __name__ == "__main__": -# """Used if running UI in browser via `python -m ...` command.""" -# # parse path -# if len(sys.argv) > 1: -# path = sys.argv[1] -# else: -# raise ValueError("No path argument provided") - -# # run app -# app = ScheduleApp(path) -# app.run() diff --git a/src/virtualship/cli/validator_utils.py b/src/virtualship/cli/validator_utils.py index 9aa4b1cd..3e14daff 100644 --- a/src/virtualship/cli/validator_utils.py +++ b/src/virtualship/cli/validator_utils.py @@ -16,7 +16,7 @@ def require_docstring(func): return func -# SCHEDULE INUTS VALIDATION +# SCHEDULE INPUTS VALIDATION @require_docstring From 7e5ff857f03def948b1b8cdd60be100195d57432 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 25 Jun 2025 15:31:48 +0100 Subject: [PATCH 17/30] Refactor error handling to provide detailed unexpected error messages; unify exception raising in ScheduleEditor and ConfigEditor. --- src/virtualship/cli/_plan.py | 53 ++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 0b452d7b..c76ea3e3 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -50,9 +50,6 @@ # TODO: need to build TESTS for the UI! # - look into best way of testing this kind of thing... -# TODO: need to distinguish in error handling between usererrors such as faulty yaml read in add pointers to the User to make fixes, -# TODO: and errors which are genuinely unexpected, are likely bugs and need attention on GitHub...! - # TODO: will need to some scenario testing where introduce a bug which affects save_changes() to show that the GitHub issues related error # TODO: message is definitely thrown when the error is unexpected. @@ -81,23 +78,27 @@ # TODO: look through all error handling in both _plan.py and command.py scripts to check redudancy # TODO: also add more error handling for 1) if there are no yamls found in the specified *path* # TODO: and also for if there are errors in the yaml being opened, for example if what should be a float has been manaully changed to something invalid -# TODO: or if indeed any of the pydantic model components associated with the yamls are missing # TODO: or indeed how error messages such as those in make_validators are handled in the hypothetical situation that they are thrown... - # TODO: make sure 'Save' writes the yaml file components in the right order? Or does this not matter? # TODO: test via full run through of an expedition using yamls edited by `virtualship plan` # TODO: implement action for 'Save' button -# TODO: Can the whole lot be tidied up by moving some classes/methods to a new directory/files?! - # TODO: the valid entry + User errors etc. need to be added to the Schedule editor (currently on the Config Editor) # TODO: add a check if instruments selections are left empty? -UNEXPECTED_MSG = ( +def unexpected_msg_compose(e): + return ( + f"\n\nUNEXPECTED ERROR:\n\n{e}" + "\n\nPlease report this issue, with a description and the traceback, " + "to the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues" + ) + + +UNEXPECTED_MSG_ONSAVE = ( "Please ensure that:\n" "\n1) All typed entries are valid (all boxes in all sections must have green borders and no warnings).\n" "\n2) Time selections exist for all waypoints.\n" @@ -259,9 +260,11 @@ def compose(self) -> ComposeResult: value=is_selected, id=f"wp{self.index}_{instrument.value}" ) - except Exception: - raise # raise the exception to prevent incomplete UI build - # TODO: could follow this through to be included in a generic 'unexpected error' please post on Github issue in say ScheduleApp? + except Exception as e: + raise UnexpectedError(unexpected_msg_compose(e)) from None + + # raise the exception to prevent incomplete UI build + # TODO: could follow this through to be included in a generic 'unexpected error' please post on Github issue in say ScheduleApp? def copy_from_previous(self) -> None: try: @@ -509,8 +512,8 @@ def compose(self) -> ComposeResult: id="validation-failure-label-end_time", classes="-hidden validation-failure", ) - except Exception: - raise UnexpectedError(UNEXPECTED_MSG) from None + except Exception as e: + raise UnexpectedError(unexpected_msg_compose(e)) from None @on(Input.Changed) def show_invalid_reasons(self, event: Input.Changed) -> None: @@ -584,7 +587,7 @@ def save_changes(self) -> bool: e, self.path, context_message="Error saving ship config:" ) - raise UnexpectedError(UNEXPECTED_MSG) from None + raise UnexpectedError(UNEXPECTED_MSG_ONSAVE) from None class ConfigEditor(Container): @@ -788,9 +791,8 @@ def compose(self) -> ComposeResult: classes="-hidden validation-failure", ) - except Exception: - raise # raise the exception to prevent incomplete UI build - # TODO: could follow this through to be included in a generic 'unexpected error' please post on Github issue in say ScheduleApp? + except Exception as e: + raise UnexpectedError(unexpected_msg_compose(e)) from None @on(Input.Changed) def show_invalid_reasons(self, event: Input.Changed) -> None: @@ -926,7 +928,7 @@ def save_changes(self) -> bool: e, self.path, context_message="Error saving ship config:" ) - raise UnexpectedError(UNEXPECTED_MSG) from None + raise UnexpectedError(UNEXPECTED_MSG_ONSAVE) from None class ScheduleScreen(Screen): @@ -935,12 +937,15 @@ def __init__(self, path: str): self.path = path def compose(self) -> ComposeResult: - with VerticalScroll(): - yield ConfigEditor(self.path) - yield ScheduleEditor(self.path) - with Horizontal(): - yield Button("Save Changes", id="save_button", variant="success") - yield Button("Exit", id="exit_button", variant="error") + try: + with VerticalScroll(): + yield ConfigEditor(self.path) + yield ScheduleEditor(self.path) + with Horizontal(): + yield Button("Save Changes", id="save_button", variant="success") + yield Button("Exit", id="exit_button", variant="error") + except Exception as e: + raise UnexpectedError(unexpected_msg_compose(e)) from None @on(Button.Pressed, "#exit_button") def exit_pressed(self) -> None: From c7097e011e69ae68991ff36ba2532a358f86ce22 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 1 Jul 2025 13:28:40 +0100 Subject: [PATCH 18/30] Refactor TODO comments for clarity and organization; update unexpected error message handling in ScheduleEditor. --- src/virtualship/cli/_plan.py | 93 ++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 40 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index c76ea3e3..fda4d8f7 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -47,55 +47,35 @@ TimeRange, ) -# TODO: need to build TESTS for the UI! -# - look into best way of testing this kind of thing... - -# TODO: will need to some scenario testing where introduce a bug which affects save_changes() to show that the GitHub issues related error -# TODO: message is definitely thrown when the error is unexpected. +# TODO list: -# TODO: check: how does it handle (all) the fields being empty upon reading-in? Need to add redundancy here...? -# - currently the application fails on start-up if something is missing or null (apart from ADCP and ST) -# - there is also a bug where if ADCP is null on start up, and then you switch it on in the UI then it crashes, so this bug fix is TODO! -# - otherwise, in terms of missing data on start up handling, best to throw an error to the user stating that x field is missing a value +# 1) for future PR...remove instrument config from ship_config.yaml if instrument not selected in schedule? -# TODO: need to do a big round of edits where tidy up all the error messaging. -# - given a lot of the validation of entry is done natively now in textual, this should mean that all other -# errors associated with inputting to the app should be 'unexpected' and therefore call to the report-to-GitHub workflow -# - errors associated with ship_config and schedule.verify() should, however, provide in-UI messaging ideally -# to notify the user that their selections are not valid... -# TODO: perhaps error messaging could be wrapped into one of the higher level functions i.e. ScheduleScreen?! +# 2) when testing full workflow, through to `run` etc, check if can handle e.g. ADCP being null or whether needs to be removed from .yaml if off -# TODO: add TEXTUAL_SERVE dependency if end up using it... - -# TODO: I think remove web browser option for now, build stable version for terminal only (especially because VSC being mostly run in browser atm) -# TODO: Textual-serve / browser support for a future PR... +# 3) need to build TESTS for the UI! +# - look into best way of testing this kind of thing... -# TODO: test in JupyterLab terminal!... +# 4) errors associated with ship_config and schedule.verify() should, however, provide in-UI messaging ideally -# TODO: need to handle how to add the start_ and end_ datetimes automatically to Space-Time Region! -# TODO: because I think the .yaml that comes from `virtualship init` will not have these as standard and we don't expect students to go add themselves... +# 5) I think remove web browser option for now, build stable version for terminal only (especially because VSC being mostly run in browser atm) +# - Textual-serve / browser support for a future PR... -# TODO: look through all error handling in both _plan.py and command.py scripts to check redudancy -# TODO: also add more error handling for 1) if there are no yamls found in the specified *path* -# TODO: and also for if there are errors in the yaml being opened, for example if what should be a float has been manaully changed to something invalid +# 6) test in JupyterLab terminal!... -# TODO: or indeed how error messages such as those in make_validators are handled in the hypothetical situation that they are thrown... +# 7) need to handle how to add the start_ and end_ datetimes automatically to Space-Time Region! +# - because I think the .yaml that comes from `virtualship init` will not have these as standard and we don't expect students to go add themselves... -# TODO: make sure 'Save' writes the yaml file components in the right order? Or does this not matter? -# TODO: test via full run through of an expedition using yamls edited by `virtualship plan` -# TODO: implement action for 'Save' button +# 8) add more error handling for if there are no yamls found in the specified *path* -# TODO: the valid entry + User errors etc. need to be added to the Schedule editor (currently on the Config Editor) +# 9) make sure 'Save' writes the yaml file components in the right order? Or does this not matter? +# - test via full run through of an expedition using yamls edited by `virtualship plan` -# TODO: add a check if instruments selections are left empty? +# 10) the valid entry + User errors etc. need to be added to the Schedule editor (currently on the Config Editor) +# 11) add a check if instruments selections are left empty? -def unexpected_msg_compose(e): - return ( - f"\n\nUNEXPECTED ERROR:\n\n{e}" - "\n\nPlease report this issue, with a description and the traceback, " - "to the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues" - ) +# 12) will need to update quickstart guide and documentation to handle the fact that no longer adding column to excel for `virtualship init` UNEXPECTED_MSG_ONSAVE = ( @@ -107,6 +87,14 @@ def unexpected_msg_compose(e): ) +def unexpected_msg_compose(e): + return ( + f"\n\nUNEXPECTED ERROR:\n\n{e}" + "\n\nPlease report this issue, with a description and the traceback, " + "to the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues" + ) + + def log_exception_to_file( exception: Exception, path: str, @@ -263,9 +251,6 @@ def compose(self) -> ComposeResult: except Exception as e: raise UnexpectedError(unexpected_msg_compose(e)) from None - # raise the exception to prevent incomplete UI build - # TODO: could follow this through to be included in a generic 'unexpected error' please post on Github issue in say ScheduleApp? - def copy_from_previous(self) -> None: try: if self.index > 0: @@ -577,6 +562,34 @@ def save_changes(self) -> bool: if self.query_one(f"#wp{i}_{instrument.value}").value ] + # --------------------------------------------------------------------------- + + # TODO: this block is currently not working as expected! Needs attention. + # - Not sure if it's verifying properly, actually checking that in timing order or just causing other errors and being picked up as timing error + # - And can't dynamically load in ship speed yet + + # verify schedule and save + + # # TODO: NEEDS TO BE ABLE TO TAKE SHIP SPEED VALUE FROM INPUT IN CONFIG EDITOR...!!! + + # try: + # self.schedule.verify( + # 10.0, # TODO: currently hardcoded for dev purposes only!! + # input_data=None, + # check_space_time_region=True, + # ignore_missing_fieldsets=True, + # ) + + # # save + # self.schedule.to_yaml(f"{self.path}/schedule.yaml") + # return True + + # except Exception as e: + # self.notify(f"SCHEDULE ERROR: {e}", severity="error", timeout=20) + # return False + + # --------------------------------------------------------------------------- + # save self.schedule.to_yaml(f"{self.path}/schedule.yaml") return True From 36702522ed3db5e52037400b97f13311b4173044 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 1 Jul 2025 14:51:02 +0100 Subject: [PATCH 19/30] remove reading in instrument selection from Excel for schedule --- src/virtualship/cli/_plan.py | 2 ++ src/virtualship/utils.py | 35 +++-------------------------------- 2 files changed, 5 insertions(+), 32 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index fda4d8f7..eb4c691f 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -77,6 +77,8 @@ # 12) will need to update quickstart guide and documentation to handle the fact that no longer adding column to excel for `virtualship init` +# 13) incorp functionality to determine the max depth for space time region based on instrument choice (as previously existed in mfp_to_yaml) + UNEXPECTED_MSG_ONSAVE = ( "Please ensure that:\n" diff --git a/src/virtualship/utils.py b/src/virtualship/utils.py index 3b53a68d..1f334f06 100644 --- a/src/virtualship/utils.py +++ b/src/virtualship/utils.py @@ -76,16 +76,11 @@ def load_coordinates(file_path): def validate_coordinates(coordinates_data): # Expected column headers - expected_columns = {"Station Type", "Name", "Latitude", "Longitude", "Instrument"} + expected_columns = {"Station Type", "Name", "Latitude", "Longitude"} # Check if the headers match the expected ones actual_columns = set(coordinates_data.columns) - if "Instrument" not in actual_columns: - raise ValueError( - "Error: Missing column 'Instrument'. Have you added this column after exporting from MFP?" - ) - missing_columns = expected_columns - actual_columns if missing_columns: raise ValueError( @@ -140,7 +135,6 @@ def mfp_to_yaml(coordinates_file_path: str, yaml_output_path: str): # noqa: D41 """ from virtualship.models import ( - InstrumentType, Location, Schedule, SpaceTimeRegion, @@ -163,24 +157,13 @@ def mfp_to_yaml(coordinates_file_path: str, yaml_output_path: str): # noqa: D41 "ARGO_FLOAT": 2000, } - unique_instruments = set() - - for instrument_list in coordinates_data["Instrument"]: - instruments = instrument_list.split(", ") - unique_instruments |= set(instruments) - - # Determine the maximum depth based on the unique instruments - maximum_depth = max( - instrument_max_depths.get(instrument, 0) for instrument in unique_instruments - ) - spatial_range = SpatialRange( minimum_longitude=coordinates_data["Longitude"].min(), maximum_longitude=coordinates_data["Longitude"].max(), minimum_latitude=coordinates_data["Latitude"].min(), maximum_latitude=coordinates_data["Latitude"].max(), minimum_depth=0, - maximum_depth=maximum_depth, + maximum_depth=max(instrument_max_depths.values()), ) # Create space-time region object @@ -192,21 +175,9 @@ def mfp_to_yaml(coordinates_file_path: str, yaml_output_path: str): # noqa: D41 # Generate waypoints waypoints = [] for _, row in coordinates_data.iterrows(): - try: - instruments = [ - InstrumentType(instrument) - for instrument in row["Instrument"].split(", ") - ] - except ValueError as err: - raise ValueError( - f"Error: Invalid instrument type in row {row.name}. " - "Please ensure that the instrument type is one of: " - f"{[instrument.name for instrument in InstrumentType]}. " - "Also be aware that these are case-sensitive." - ) from err waypoints.append( Waypoint( - instrument=instruments, + instrument=None, # instruments blank, to be built by user using `virtualship plan` UI or by interacting directly with YAML files location=Location(latitude=row["Latitude"], longitude=row["Longitude"]), ) ) From 286f9f7095708ad77f7ed718048702a6bb7785c8 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Mon, 7 Jul 2025 15:03:59 +0200 Subject: [PATCH 20/30] add .verify() methods to Save logic, remove browser launching, update `virtualship init` user messaging --- src/virtualship/cli/_plan.py | 77 +++++++++++++++------------------ src/virtualship/cli/commands.py | 38 ++++------------ 2 files changed, 44 insertions(+), 71 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index eb4c691f..c4d546ff 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -56,23 +56,14 @@ # 3) need to build TESTS for the UI! # - look into best way of testing this kind of thing... -# 4) errors associated with ship_config and schedule.verify() should, however, provide in-UI messaging ideally - -# 5) I think remove web browser option for now, build stable version for terminal only (especially because VSC being mostly run in browser atm) -# - Textual-serve / browser support for a future PR... - # 6) test in JupyterLab terminal!... # 7) need to handle how to add the start_ and end_ datetimes automatically to Space-Time Region! # - because I think the .yaml that comes from `virtualship init` will not have these as standard and we don't expect students to go add themselves... -# 8) add more error handling for if there are no yamls found in the specified *path* - # 9) make sure 'Save' writes the yaml file components in the right order? Or does this not matter? # - test via full run through of an expedition using yamls edited by `virtualship plan` -# 10) the valid entry + User errors etc. need to be added to the Schedule editor (currently on the Config Editor) - # 11) add a check if instruments selections are left empty? # 12) will need to update quickstart guide and documentation to handle the fact that no longer adding column to excel for `virtualship init` @@ -188,8 +179,9 @@ def compose(self) -> ComposeResult: yield Select( [ (str(year), year) + # TODO: change from hard coding...flexibility for different datasets... for year in range( - datetime.datetime.now().year - 3, + 2022, datetime.datetime.now().year + 1, ) ], @@ -499,6 +491,7 @@ def compose(self) -> ComposeResult: id="validation-failure-label-end_time", classes="-hidden validation-failure", ) + except Exception as e: raise UnexpectedError(unexpected_msg_compose(e)) from None @@ -523,7 +516,6 @@ def show_invalid_reasons(self, event: Input.Changed) -> None: def save_changes(self) -> bool: """Save changes to schedule.yaml.""" - # TODO: and should proabably now be more focussed on .verify() methods try: # spacetime region @@ -564,34 +556,6 @@ def save_changes(self) -> bool: if self.query_one(f"#wp{i}_{instrument.value}").value ] - # --------------------------------------------------------------------------- - - # TODO: this block is currently not working as expected! Needs attention. - # - Not sure if it's verifying properly, actually checking that in timing order or just causing other errors and being picked up as timing error - # - And can't dynamically load in ship speed yet - - # verify schedule and save - - # # TODO: NEEDS TO BE ABLE TO TAKE SHIP SPEED VALUE FROM INPUT IN CONFIG EDITOR...!!! - - # try: - # self.schedule.verify( - # 10.0, # TODO: currently hardcoded for dev purposes only!! - # input_data=None, - # check_space_time_region=True, - # ignore_missing_fieldsets=True, - # ) - - # # save - # self.schedule.to_yaml(f"{self.path}/schedule.yaml") - # return True - - # except Exception as e: - # self.notify(f"SCHEDULE ERROR: {e}", severity="error", timeout=20) - # return False - - # --------------------------------------------------------------------------- - # save self.schedule.to_yaml(f"{self.path}/schedule.yaml") return True @@ -679,8 +643,6 @@ class ConfigEditor(Container): }, } - # TODO: Also incorporate verify methods! - def __init__(self, path: str): super().__init__() self.path = path @@ -962,6 +924,23 @@ def compose(self) -> ComposeResult: except Exception as e: raise UnexpectedError(unexpected_msg_compose(e)) from None + def sync_ui_waypoints(self): + """Update the waypoints models with current UI values (spacetime only) from the live UI inputs.""" + schedule_editor = self.query_one(ScheduleEditor) + for i, wp in enumerate(schedule_editor.schedule.waypoints): + wp.location = Location( + latitude=float(schedule_editor.query_one(f"#wp{i}_lat").value), + longitude=float(schedule_editor.query_one(f"#wp{i}_lon").value), + ) + wp.time = datetime.datetime( + int(schedule_editor.query_one(f"#wp{i}_year").value), + int(schedule_editor.query_one(f"#wp{i}_month").value), + int(schedule_editor.query_one(f"#wp{i}_day").value), + int(schedule_editor.query_one(f"#wp{i}_hour").value), + int(schedule_editor.query_one(f"#wp{i}_minute").value), + 0, + ) + @on(Button.Pressed, "#exit_button") def exit_pressed(self) -> None: self.app.exit() @@ -973,6 +952,20 @@ def save_pressed(self) -> None: schedule_editor = self.query_one(ScheduleEditor) try: + ship_speed_value = float( + config_editor.query_one("#speed").value + ) # get ship speed from config + + self.sync_ui_waypoints() # call to ensure waypoint inputs are synced + + # verify schedule + schedule_editor.schedule.verify( + ship_speed_value, + input_data=None, + check_space_time_region=True, + ignore_missing_fieldsets=True, + ) + config_saved = config_editor.save_changes() schedule_saved = schedule_editor.save_changes() @@ -983,7 +976,7 @@ def save_pressed(self) -> None: except Exception as e: self.notify( - f"*** Error saving changes ***:\n\n{e}\n\nTraceback will be logged in `{self.path}/virtualship_error.txt`. Please attach the file and/or its contents when submitting an issue.\n", + f"*** Error saving changes ***:\n\n{e}\n", severity="error", timeout=20, ) diff --git a/src/virtualship/cli/commands.py b/src/virtualship/cli/commands.py index 531556a0..2b1c491f 100644 --- a/src/virtualship/cli/commands.py +++ b/src/virtualship/cli/commands.py @@ -1,11 +1,10 @@ -import webbrowser from pathlib import Path import click -from textual_serve.server import Server from virtualship import utils from virtualship.cli._fetch import _fetch +from virtualship.cli._plan import _plan from virtualship.expedition.do_expedition import do_expedition from virtualship.utils import ( SCHEDULE, @@ -56,10 +55,12 @@ def init(path, from_mfp): click.echo(f"Generating schedule from {mfp_file}...") mfp_to_yaml(mfp_file, schedule) click.echo( - "\n⚠️ The generated schedule does not contain time values. " - "\nPlease either use the 'virtualship plan` app to complete the schedule configuration, " - "\nOR edit 'schedule.yaml' and manually add the necessary time values." - "\n🕒 Expected time format: 'YYYY-MM-DD HH:MM:SS' (e.g., '2023-10-20 01:00:00').\n" + "\n⚠️ The generated schedule does not contain TIME values or INSTRUMENT selections. ⚠️" + "\n\nNow please either use the 'virtualship plan` app to complete the schedule configuration, " + "\nOR edit 'schedule.yaml' and manually add the necessary time values and instrument selections." + "\n\n🕒 Expected time format: 'YYYY-MM-DD HH:MM:SS' (e.g., '2023-10-20 01:00:00')." + "\n\n🌡️ Expected instrument(s) format: one line per instrument e.g." + f"\n\n{' ' * 15}waypoints:\n{' ' * 15}- instrument:\n{' ' * 19}- CTD\n{' ' * 19}- ARGO_FLOAT\n" ) else: # Create a default example schedule @@ -74,30 +75,9 @@ def init(path, from_mfp): "path", type=click.Path(exists=False, file_okay=False, dir_okay=True), ) -@click.option( - "--terminal/--web", - default=False, - help="Run the VirtualShip planner in the terminal (default: web browser).", -) - -# TODO: implement auto detection of whether running remotely e.g. in JupyterLab (already in browser) to automatically run in terminal -# TODO: rather than launching in browser from the browser... - -def plan(path, terminal): +def plan(path): """Launch UI to help build schedule and ship config files. Opens in web browser by default, or in the terminal with --terminal.""" - if terminal: - from virtualship.cli._plan import _plan - - _plan(Path(path)) - else: - server = Server( - command=f"python -m virtualship.cli._plan {Path(path)}", - title="VirtualShip plan", - ) - url = "http://localhost:8000" - webbrowser.open(url) - # TODO: remove debug = True when goes live - server.serve(debug=True) + _plan(Path(path)) @click.command() From 1b22634576487a2abd7ca8d54c2b9cc49f0bd4cd Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Mon, 7 Jul 2025 16:10:59 +0200 Subject: [PATCH 21/30] auto fill start and end times in SpaceTimeRegion if left blank in UI --- src/virtualship/cli/_plan.py | 60 ++++++++++++++++---------- src/virtualship/cli/validator_utils.py | 4 ++ 2 files changed, 42 insertions(+), 22 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index c4d546ff..a2fad675 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -49,26 +49,21 @@ # TODO list: -# 1) for future PR...remove instrument config from ship_config.yaml if instrument not selected in schedule? - -# 2) when testing full workflow, through to `run` etc, check if can handle e.g. ADCP being null or whether needs to be removed from .yaml if off +# 2) when testing full workflow, through to `run` etc, check if can handle e.g. +# - ADCP being null or whether needs to be removed from .yaml if off +# - instrument lists being empty... # 3) need to build TESTS for the UI! # - look into best way of testing this kind of thing... # 6) test in JupyterLab terminal!... -# 7) need to handle how to add the start_ and end_ datetimes automatically to Space-Time Region! -# - because I think the .yaml that comes from `virtualship init` will not have these as standard and we don't expect students to go add themselves... - # 9) make sure 'Save' writes the yaml file components in the right order? Or does this not matter? # - test via full run through of an expedition using yamls edited by `virtualship plan` # 11) add a check if instruments selections are left empty? -# 12) will need to update quickstart guide and documentation to handle the fact that no longer adding column to excel for `virtualship init` - -# 13) incorp functionality to determine the max depth for space time region based on instrument choice (as previously existed in mfp_to_yaml) +#! 13) incorp functionality to determine the max depth for space time region based on instrument choice (as previously existed in mfp_to_yaml) UNEXPECTED_MSG_ONSAVE = ( @@ -181,7 +176,7 @@ def compose(self) -> ComposeResult: (str(year), year) # TODO: change from hard coding...flexibility for different datasets... for year in range( - 2022, + 2023, datetime.datetime.now().year + 1, ) ], @@ -267,9 +262,8 @@ def copy_from_previous(self) -> None: ) if prev_switch and curr_switch: curr_switch.value = prev_switch.value - except Exception: - raise # raise the exception to prevent incomplete UI build - # TODO: could follow this through to be included in a generic 'unexpected error' please post on Github issue in say ScheduleApp? + except Exception as e: + raise UnexpectedError(unexpected_msg_compose(e)) from None @on(Input.Changed) def show_invalid_reasons(self, event: Input.Changed) -> None: @@ -321,8 +315,6 @@ def compose(self) -> ComposeResult: for i, waypoint in enumerate(self.schedule.waypoints): yield WaypointWidget(waypoint, i) - # TODO: MAY NEED TO ADD A FEATURE ON SAVE CHANGES WHICH AUTOMATICALLY DETECTS MAX AND MIN TIME - # TODO: FOR THE SCENARIO WHERE YAML LOADED IN IS NULL AND USER DOES NOT EDIT THEMSELVES with Collapsible( title="[b]Space-Time Region[/b] (advanced users only)", collapsed=True, @@ -446,7 +438,9 @@ def compose(self) -> ComposeResult: classes="-hidden validation-failure", ) - yield Label("Start Time:") + yield Label( + "Start Time (will be auto determined from waypoints if left blank):" + ) yield Input( id="start_time", placeholder="YYYY-MM-DD hh:mm:ss", @@ -469,7 +463,9 @@ def compose(self) -> ComposeResult: classes="-hidden validation-failure", ) - yield Label("End Time:") + yield Label( + "End Time (will be auto determined from waypoints if left blank):" + ) yield Input( id="end_time", placeholder="YYYY-MM-DD hh:mm:ss", @@ -517,8 +513,7 @@ def show_invalid_reasons(self, event: Input.Changed) -> None: def save_changes(self) -> bool: """Save changes to schedule.yaml.""" try: - # spacetime region - + ## spacetime region spatial_range = SpatialRange( minimum_longitude=self.query_one("#min_lon").value, maximum_longitude=self.query_one("#max_lon").value, @@ -528,15 +523,36 @@ def save_changes(self) -> bool: maximum_depth=self.query_one("#max_depth").value, ) + # auto fill start and end times if input is blank + start_time_input = self.query_one("#start_time").value + end_time_input = self.query_one("#end_time").value + waypoint_times = [ + wp.time + for wp in self.schedule.waypoints + if hasattr(wp, "time") and wp.time + ] + + if not start_time_input and waypoint_times: + start_time = min(waypoint_times) + else: + start_time = start_time_input + + if not end_time_input and waypoint_times: + end_time = max(waypoint_times) + datetime.timedelta( + days=3 + ) # with buffer + else: + end_time = end_time_input + time_range = TimeRange( - start_time=self.query_one("#start_time").value, - end_time=self.query_one("#end_time").value, + start_time=start_time, + end_time=end_time, ) self.schedule.space_time_region.spatial_range = spatial_range self.schedule.space_time_region.time_range = time_range - # waypoints + ## waypoints for i, wp in enumerate(self.schedule.waypoints): wp.location = Location( latitude=float(self.query_one(f"#wp{i}_lat").value), diff --git a/src/virtualship/cli/validator_utils.py b/src/virtualship/cli/validator_utils.py index 3e14daff..060ce95a 100644 --- a/src/virtualship/cli/validator_utils.py +++ b/src/virtualship/cli/validator_utils.py @@ -59,6 +59,10 @@ def is_valid_depth(value: str) -> bool: @require_docstring def is_valid_timestr(value: str) -> bool: """Format YYYY-MM-DD hh:mm:ss.""" + if ( + not value.strip() + ): # return as valid if blank, UI logic will auto fill on save if so + return True try: datetime.datetime.strptime(value, "%Y-%m-%d %H:%M:%S") return True From 111b4106d87e529d4a71b09e88b610e9629255ea Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 8 Jul 2025 10:35:47 +0200 Subject: [PATCH 22/30] Update TODO comments; enhance user messaging in schedule configuration instructions. --- src/virtualship/cli/_plan.py | 27 ++++++++------------------- src/virtualship/cli/commands.py | 3 ++- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index a2fad675..4917b3cb 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -49,23 +49,9 @@ # TODO list: -# 2) when testing full workflow, through to `run` etc, check if can handle e.g. -# - ADCP being null or whether needs to be removed from .yaml if off -# - instrument lists being empty... - -# 3) need to build TESTS for the UI! +# Need to build TESTS for the UI! # - look into best way of testing this kind of thing... -# 6) test in JupyterLab terminal!... - -# 9) make sure 'Save' writes the yaml file components in the right order? Or does this not matter? -# - test via full run through of an expedition using yamls edited by `virtualship plan` - -# 11) add a check if instruments selections are left empty? - -#! 13) incorp functionality to determine the max depth for space time region based on instrument choice (as previously existed in mfp_to_yaml) - - UNEXPECTED_MSG_ONSAVE = ( "Please ensure that:\n" "\n1) All typed entries are valid (all boxes in all sections must have green borders and no warnings).\n" @@ -176,7 +162,7 @@ def compose(self) -> ComposeResult: (str(year), year) # TODO: change from hard coding...flexibility for different datasets... for year in range( - 2023, + 2022, datetime.datetime.now().year + 1, ) ], @@ -539,8 +525,8 @@ def save_changes(self) -> bool: if not end_time_input and waypoint_times: end_time = max(waypoint_times) + datetime.timedelta( - days=3 - ) # with buffer + minutes=60480.0 + ) # with buffer (corresponds to default drifter lifetime) else: end_time = end_time_input @@ -582,7 +568,10 @@ def save_changes(self) -> bool: e, self.path, context_message="Error saving ship config:" ) - raise UnexpectedError(UNEXPECTED_MSG_ONSAVE) from None + raise UnexpectedError( + UNEXPECTED_MSG_ONSAVE + + f"\n\nTraceback will be logged in {self.path}/virtualship_error.txt. Please attach this/copy the contents to any issue submitted." + ) from None class ConfigEditor(Container): diff --git a/src/virtualship/cli/commands.py b/src/virtualship/cli/commands.py index 2b1c491f..7948fd49 100644 --- a/src/virtualship/cli/commands.py +++ b/src/virtualship/cli/commands.py @@ -56,8 +56,9 @@ def init(path, from_mfp): mfp_to_yaml(mfp_file, schedule) click.echo( "\n⚠️ The generated schedule does not contain TIME values or INSTRUMENT selections. ⚠️" - "\n\nNow please either use the 'virtualship plan` app to complete the schedule configuration, " + "\n\nNow please either use the `\033[4mvirtualship plan\033[0m` app to complete the schedule configuration, " "\nOR edit 'schedule.yaml' and manually add the necessary time values and instrument selections." + "\n\nIf editing 'schedule.yaml' manually:" "\n\n🕒 Expected time format: 'YYYY-MM-DD HH:MM:SS' (e.g., '2023-10-20 01:00:00')." "\n\n🌡️ Expected instrument(s) format: one line per instrument e.g." f"\n\n{' ' * 15}waypoints:\n{' ' * 15}- instrument:\n{' ' * 19}- CTD\n{' ' * 19}- ARGO_FLOAT\n" From b5eb357c05e0e1ea59fc8c0efe0860e2c690ada5 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 9 Jul 2025 10:11:11 +0200 Subject: [PATCH 23/30] add tests for UI; add pytest-asyncio dependency --- environment.yml | 1 + src/virtualship/cli/_plan.py | 27 ++++++--- tests/cli/test_plan.py | 113 +++++++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 8 deletions(-) create mode 100644 tests/cli/test_plan.py diff --git a/environment.yml b/environment.yml index bce0d80f..2580056c 100644 --- a/environment.yml +++ b/environment.yml @@ -23,6 +23,7 @@ dependencies: # Testing - pytest - pytest-cov + - pytest-asyncio - codecov - seabird - setuptools diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 4917b3cb..1f160b95 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -102,7 +102,11 @@ def __init__(self, waypoint: Waypoint, index: int): def compose(self) -> ComposeResult: try: - with Collapsible(title=f"[b]Waypoint {self.index + 1}[/b]", collapsed=True): + with Collapsible( + title=f"[b]Waypoint {self.index + 1}[/b]", + collapsed=True, + id=f"wp{self.index + 1}", + ): if self.index > 0: yield Button( "Copy Time & Instruments from Previous", @@ -115,6 +119,7 @@ def compose(self) -> ComposeResult: id=f"wp{self.index}_lat", value=str(self.waypoint.location.lat) if self.waypoint.location.lat + is not None # is not None to handle if lat is 0.0 else "", validators=[ Function( @@ -137,6 +142,7 @@ def compose(self) -> ComposeResult: id=f"wp{self.index}_lon", value=str(self.waypoint.location.lon) if self.waypoint.location.lon + is not None # is not None to handle if lon is 0.0 else "", validators=[ Function( @@ -296,6 +302,7 @@ def compose(self) -> ComposeResult: with Collapsible( title="[b]Waypoints & Instrument Selection[/b]", + id="waypoints", collapsed=True, ): for i, waypoint in enumerate(self.schedule.waypoints): @@ -665,7 +672,9 @@ def compose(self) -> ComposeResult: yield Label("[b]Ship Config Editor[/b]", id="title", markup=True) yield Rule(line_style="heavy") - with Collapsible(title="[b]Ship Speed & Onboard Measurements[/b]"): + with Collapsible( + title="[b]Ship Speed & Onboard Measurements[/b]", id="speed_collapsible" + ): attr = "ship_speed_knots" validators = group_validators(ShipConfig, attr) with Horizontal(classes="ship_speed"): @@ -853,7 +862,7 @@ def save_changes(self) -> bool: try: # ship speed attr = "ship_speed_knots" - field_type = get_field_type(self.config, attr) + field_type = get_field_type(type(self.config), attr) value = field_type(self.query_one("#speed").value) ShipConfig.model_validate( {**self.config.model_dump(), attr: value} @@ -913,7 +922,7 @@ def save_changes(self) -> bool: raise UnexpectedError(UNEXPECTED_MSG_ONSAVE) from None -class ScheduleScreen(Screen): +class PlanScreen(Screen): def __init__(self, path: str): super().__init__() self.path = path @@ -976,7 +985,9 @@ def save_pressed(self) -> None: if config_saved and schedule_saved: self.notify( - "Changes saved successfully", severity="information", timeout=20 + "Changes saved successfully", + severity="information", + timeout=20, ) except Exception as e: @@ -988,7 +999,7 @@ def save_pressed(self) -> None: return False -class ScheduleApp(App): +class PlanApp(App): CSS = """ Screen { align: center middle; @@ -1167,11 +1178,11 @@ def __init__(self, path: str): self.path = path def on_mount(self) -> None: - self.push_screen(ScheduleScreen(self.path)) + self.push_screen(PlanScreen(self.path)) self.theme = "textual-light" def _plan(path: str) -> None: """Run UI in terminal.""" - app = ScheduleApp(path) + app = PlanApp(path) app.run() diff --git a/tests/cli/test_plan.py b/tests/cli/test_plan.py new file mode 100644 index 00000000..bad21282 --- /dev/null +++ b/tests/cli/test_plan.py @@ -0,0 +1,113 @@ +import os +import shutil +import tempfile +from importlib.resources import files +from pathlib import Path +from unittest.mock import MagicMock + +import pytest +import yaml +from textual.widgets import Button, Collapsible, Input + +from virtualship.cli._plan import ConfigEditor, PlanApp, ScheduleEditor + +NEW_SPEED = "8.0" +NEW_LAT = "0.05" +NEW_LON = "0.05" + + +async def simulate_input(pilot, box, new_value): + """Simulate inputs to the UI.""" + box.focus() + await pilot.pause() + box.clear() + await pilot.pause() + for char in new_value: + await pilot.press(char) + await pilot.pause(0.05) + + +@pytest.mark.asyncio +async def test_UI_changes(): + """Test making changes to UI inputs and saving to YAML.""" + tmpdir = Path(tempfile.mkdtemp()) + + shutil.copy( + files("virtualship.static").joinpath("ship_config.yaml"), + tmpdir / "ship_config.yaml", + ) + shutil.copy( + files("virtualship.static").joinpath("schedule.yaml"), + tmpdir / "schedule.yaml", + ) + + app = PlanApp(path=tmpdir) + + async with app.run_test(size=(120, 100)) as pilot: + await pilot.pause(0.5) + + plan_screen = pilot.app.screen + config_editor = plan_screen.query_one(ConfigEditor) + schedule_editor = plan_screen.query_one(ScheduleEditor) + + # get mock of UI notify method + plan_screen.notify = MagicMock() + + # change ship speed + speed_collapsible = config_editor.query_one("#speed_collapsible", Collapsible) + if speed_collapsible.collapsed: + speed_collapsible.collapsed = False + await pilot.pause() + ship_speed_input = config_editor.query_one("#speed", Input) + await simulate_input(pilot, ship_speed_input, NEW_SPEED) + + # change waypoint lat/lon (e.g. first waypoint) + waypoints_collapsible = schedule_editor.query_one("#waypoints", Collapsible) + if waypoints_collapsible.collapsed: + waypoints_collapsible.collapsed = False + await pilot.pause() + wp_collapsible = waypoints_collapsible.query_one("#wp1", Collapsible) + if wp_collapsible.collapsed: + wp_collapsible.collapsed = False + await pilot.pause() + lat_input, lon_input = ( + wp_collapsible.query_one("#wp0_lat", Input), + wp_collapsible.query_one("#wp0_lat", Input), + ) + await simulate_input(pilot, lat_input, NEW_LAT) + await simulate_input(pilot, lon_input, NEW_LON) + wp_collapsible.collapsed = True + await pilot.pause() + waypoints_collapsible.collapsed = True + await pilot.pause() + + # press save button + save_button = plan_screen.query_one("#save_button", Button) + await pilot.click(save_button) + await pilot.pause(0.5) + + # verify success notification received in UI (also useful for displaying potential debugging messages) + plan_screen.notify.assert_called_once_with( + "Changes saved successfully", + severity="information", + timeout=20, + ) + + # verify changes to speed, lat, lon in saved YAML + ship_config_path = os.path.join(tmpdir, "ship_config.yaml") + with open(ship_config_path) as f: + saved_config = yaml.safe_load(f) + + assert saved_config["ship_speed_knots"] == float(NEW_SPEED) + + # check schedule.verify() methods are working by purposefully making invalid schedule (i.e. ship speed too slow to reach waypoints) + invalid_speed = "0.0001" + await simulate_input(pilot, ship_speed_input, invalid_speed) + await pilot.click(save_button) + await pilot.pause(0.5) + + args, _ = plan_screen.notify.call_args + assert "*** Error saving changes ***" in args[0] + + # cleanup + shutil.rmtree(tmpdir) From 7e8775ab7142a5ae1eb5ecc36a2f8a3f6e264774 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 9 Jul 2025 13:19:02 +0200 Subject: [PATCH 24/30] update tests to reflect changes to how users select instruments --- tests/cli/test_plan.py | 12 ++++++++- tests/test_mfp_to_yaml.py | 51 ++++----------------------------------- 2 files changed, 16 insertions(+), 47 deletions(-) diff --git a/tests/cli/test_plan.py b/tests/cli/test_plan.py index bad21282..6fef90a1 100644 --- a/tests/cli/test_plan.py +++ b/tests/cli/test_plan.py @@ -29,7 +29,7 @@ async def simulate_input(pilot, box, new_value): @pytest.mark.asyncio async def test_UI_changes(): - """Test making changes to UI inputs and saving to YAML.""" + """Test making changes to UI inputs and saving to YAML (simulated botton presses and typing inputs).""" tmpdir = Path(tempfile.mkdtemp()) shutil.copy( @@ -76,6 +76,16 @@ async def test_UI_changes(): ) await simulate_input(pilot, lat_input, NEW_LAT) await simulate_input(pilot, lon_input, NEW_LON) + + # toggle CTD on first waypoint + await pilot.click("#wp0_CTD") + await pilot.pause(0.1) + + # toggle XBT on first waypoint + await pilot.click("#wp0_XBT") + await pilot.pause(0.1) + + # re-collapse widget editors to make save button visible on screen wp_collapsible.collapsed = True await pilot.pause() waypoints_collapsible.collapsed = True diff --git a/tests/test_mfp_to_yaml.py b/tests/test_mfp_to_yaml.py index e230d2db..d242d30a 100644 --- a/tests/test_mfp_to_yaml.py +++ b/tests/test_mfp_to_yaml.py @@ -3,7 +3,7 @@ import pandas as pd import pytest -from virtualship.models import InstrumentType, Schedule +from virtualship.models import Schedule from virtualship.utils import mfp_to_yaml @@ -14,7 +14,6 @@ def valid_mfp_data(): "Name": ["Station1", "Station2", "Station3"], "Latitude": [30.8, 31.2, 32.5], "Longitude": [-44.3, -45.1, -46.7], - "Instrument": ["CTD, DRIFTER", "ARGO_FLOAT", "XBT, CTD, DRIFTER"], } ) @@ -65,13 +64,6 @@ def nonexistent_mfp_file(tmp_path): return path -@pytest.fixture -def missing_instruments_column_mfp_file(tmp_path): - path = tmp_path / "file.xlsx" - valid_mfp_data().drop(columns=["Instrument"]).to_excel(path, index=False) - return path - - @pytest.fixture def missing_columns_mfp_file(tmp_path): path = tmp_path / "file.xlsx" @@ -108,13 +100,6 @@ def test_mfp_to_yaml_success(request, fixture_name, tmp_path): data = Schedule.from_yaml(yaml_output_path) assert len(data.waypoints) == 3 - assert data.waypoints[0].instrument == [InstrumentType.CTD, InstrumentType.DRIFTER] - assert data.waypoints[1].instrument == [InstrumentType.ARGO_FLOAT] - assert data.waypoints[2].instrument == [ - InstrumentType.XBT, - InstrumentType.CTD, - InstrumentType.DRIFTER, - ] @pytest.mark.parametrize( @@ -134,22 +119,16 @@ def test_mfp_to_yaml_success(request, fixture_name, tmp_path): ), pytest.param( "invalid_mfp_file", - RuntimeError, - "Could not read coordinates data from the provided file. Ensure it is either a csv or excel file.", - id="InvalidFile", - ), - pytest.param( - "missing_instruments_column_mfp_file", ValueError, - "Error: Missing column 'Instrument'. Have you added this column after exporting from MFP?", - id="MissingInstruments", + r"Error: Found columns \['Station Type\|Name\|Latitude\|Longitude'\], but expected columns \[.*('Station Type'|'Longitude'|'Latitude'|'Name').*\]. Are you sure that you're using the correct export from MFP\?", + id="InvalidFile", ), pytest.param( "missing_columns_mfp_file", ValueError, ( - r"Error: Found columns \[.*?('Station Type'| 'Name'| 'Latitude'| 'Instrument').*?\], " - r"but expected columns \[.*?('Station Type'| 'Name'| 'Latitude'| 'Instrument'| 'Longitude').*?\]." + r"Error: Found columns \[.*?('Station Type'| 'Name'| 'Latitude').*?\], " + r"but expected columns \[.*?('Station Type'| 'Name'| 'Latitude'| 'Longitude').*?\]." ), id="MissingColumns", ), @@ -171,23 +150,3 @@ def test_mfp_to_yaml_extra_headers(unexpected_header_mfp_file, tmp_path): with pytest.warns(UserWarning, match="Found additional unexpected columns.*"): mfp_to_yaml(unexpected_header_mfp_file, yaml_output_path) - - -def test_mfp_to_yaml_instrument_conversion(valid_excel_mfp_file, tmp_path): - """Test that instruments are correctly converted into InstrumentType enums.""" - yaml_output_path = tmp_path / "schedule.yaml" - - # Run function - mfp_to_yaml(valid_excel_mfp_file, yaml_output_path) - - # Load the generated YAML - data = Schedule.from_yaml(yaml_output_path) - - assert isinstance(data.waypoints[0].instrument, list) - assert data.waypoints[0].instrument == [InstrumentType.CTD, InstrumentType.DRIFTER] - assert data.waypoints[1].instrument == [InstrumentType.ARGO_FLOAT] - assert data.waypoints[2].instrument == [ - InstrumentType.XBT, - InstrumentType.CTD, - InstrumentType.DRIFTER, - ] From a0ef9fdab99cbd608d9ee01385d9ce78ce0e4c10 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 9 Jul 2025 13:23:07 +0200 Subject: [PATCH 25/30] tidy up `plan` command docstring --- src/virtualship/cli/commands.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/virtualship/cli/commands.py b/src/virtualship/cli/commands.py index 7948fd49..72d37866 100644 --- a/src/virtualship/cli/commands.py +++ b/src/virtualship/cli/commands.py @@ -77,7 +77,11 @@ def init(path, from_mfp): type=click.Path(exists=False, file_okay=False, dir_okay=True), ) def plan(path): - """Launch UI to help build schedule and ship config files. Opens in web browser by default, or in the terminal with --terminal.""" + """ + Launch UI to help build schedule and ship config files. + + Should you encounter any issues with using this tool, please report an issue describing the problem to the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues" + """ _plan(Path(path)) From 71bdd2d00a4dc1a81aa6d7d76e64297e094b8ff5 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Wed, 9 Jul 2025 13:33:14 +0200 Subject: [PATCH 26/30] tidy up --- src/virtualship/cli/_plan.py | 26 ++++++-------------------- src/virtualship/cli/validator_utils.py | 6 +++--- 2 files changed, 9 insertions(+), 23 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 1f160b95..413544d5 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -47,11 +47,6 @@ TimeRange, ) -# TODO list: - -# Need to build TESTS for the UI! -# - look into best way of testing this kind of thing... - UNEXPECTED_MSG_ONSAVE = ( "Please ensure that:\n" "\n1) All typed entries are valid (all boxes in all sections must have green borders and no warnings).\n" @@ -75,16 +70,7 @@ def log_exception_to_file( filename: str = "virtualship_error.txt", context_message: str = "Error occurred:", ): - """ - Log an exception and its traceback to a file. - - Args: - exception (Exception): The exception to log. - path (str): Directory where the log file will be saved. - filename (str, optional): Log file name. Defaults to 'virtualship_error.txt'. - context_message (str, optional): Message to write before the traceback. - - """ + """Log an exception and its traceback to a file.""" error_log_path = os.path.join(path, filename) with open(error_log_path, "w") as f: f.write(f"{context_message}\n") @@ -166,7 +152,7 @@ def compose(self) -> ComposeResult: yield Select( [ (str(year), year) - # TODO: change from hard coding...flexibility for different datasets... + # TODO: change from hard coding? ...flexibility for different datasets... for year in range( 2022, datetime.datetime.now().year + 1, @@ -233,11 +219,11 @@ def compose(self) -> ComposeResult: raise UnexpectedError(unexpected_msg_compose(e)) from None def copy_from_previous(self) -> None: + """Copy inputs from previous waypoint widget (time and instruments only, not lat/lon).""" try: if self.index > 0: schedule_editor = self.parent if schedule_editor: - # only copy time components and instruments, not lat/lon time_components = ["year", "month", "day", "hour", "minute"] for comp in time_components: prev = schedule_editor.query_one(f"#wp{self.index - 1}_{comp}") @@ -308,6 +294,8 @@ def compose(self) -> ComposeResult: for i, waypoint in enumerate(self.schedule.waypoints): yield WaypointWidget(waypoint, i) + # SECTION: "Space-Time Region" + with Collapsible( title="[b]Space-Time Region[/b] (advanced users only)", collapsed=True, @@ -570,7 +558,6 @@ def save_changes(self) -> bool: return True except Exception as e: - # write error log log_exception_to_file( e, self.path, context_message="Error saving ship config:" ) @@ -721,7 +708,7 @@ def compose(self) -> ComposeResult: yield Label(" SeaSeven:") yield Switch(value=not is_deep, id="adcp_shallow") - ## SECTION: "Instrument Configurations (advanced users only)"" + ## SECTION: "Instrument Configurations"" with Collapsible( title="[b]Instrument Configurations[/b] (advanced users only)", @@ -914,7 +901,6 @@ def save_changes(self) -> bool: return True except Exception as e: - # write error log log_exception_to_file( e, self.path, context_message="Error saving ship config:" ) diff --git a/src/virtualship/cli/validator_utils.py b/src/virtualship/cli/validator_utils.py index 060ce95a..b2d4095a 100644 --- a/src/virtualship/cli/validator_utils.py +++ b/src/virtualship/cli/validator_utils.py @@ -120,9 +120,9 @@ def make_validator(condition, reference, value_type): Docstrings will be used to generate informative UI invalid entry messages, so them being informative and accurate is important! N.B. #2 Textual validator tools do not currently support additional arguments (such as 'reference' values) being fed into the validator functions (such as is_gt0) at present. - Therefore, reference values for the conditions cannot be fed in dynamically and necessitates "hard-coding" the condition and reference value combination. - At present, Pydantic models in VirtualShip only require gt/ge/lt/le relative to **0.0** so "reference" is always checked as being == 0.0 - Additional custom conditions can be "hard-coded" as new condition and reference combinations if Pydantic model specifications change in the future and/or new instruments are added to VirtualShip etc. + Therefore, reference values for the conditions cannot be fed in dynamically and necessitates 'hard-coding' the condition and reference value combination. + At present, Pydantic models in VirtualShip only require gt/ge/lt/le relative to **0.0** so the 'reference' value is always checked as being == 0.0 + Additional custom conditions can be 'hard-coded' as new condition and reference combinations if Pydantic model specifications change in the future and/or new instruments are added to VirtualShip etc. TODO: Perhaps there's scope here though for a more flexible implementation in a future PR... """ From 4512df0495dbe1150166fe9c334652b30fb681ef Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Fri, 25 Jul 2025 15:52:47 +0200 Subject: [PATCH 27/30] post-review edits and fixes --- src/virtualship/cli/_plan.py | 255 ++++++++++++++++++++----- src/virtualship/cli/validator_utils.py | 4 +- src/virtualship/models/location.py | 4 +- src/virtualship/models/schedule.py | 10 +- 4 files changed, 215 insertions(+), 58 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 413544d5..218533bd 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -7,7 +7,7 @@ from textual.app import App, ComposeResult from textual.containers import Container, Horizontal, VerticalScroll from textual.screen import Screen -from textual.validation import Function +from textual.validation import Function, Integer from textual.widgets import ( Button, Collapsible, @@ -47,10 +47,23 @@ TimeRange, ) +#! ---------------------------------------------------------------------------------------------- +#! ---------------------------------------------------------------------------------------------- +#! ---------------------------------------------------------------------------------------------- + +# TODO list: + +# - Do final sift through of all other TODOs in the script! + +#! ---------------------------------------------------------------------------------------------- +#! ---------------------------------------------------------------------------------------------- +#! ---------------------------------------------------------------------------------------------- + + UNEXPECTED_MSG_ONSAVE = ( "Please ensure that:\n" "\n1) All typed entries are valid (all boxes in all sections must have green borders and no warnings).\n" - "\n2) Time selections exist for all waypoints.\n" + "\n2) Complete time selections (YYYY-MM-DD hh:mm) exist for all waypoints.\n" "\nIf the problem persists, please report this issue, with a description and the traceback, " "to the VirtualShip issue tracker at: https://github.com/OceanParcels/virtualship/issues" ) @@ -215,9 +228,35 @@ def compose(self) -> ComposeResult: value=is_selected, id=f"wp{self.index}_{instrument.value}" ) + if instrument.value == "DRIFTER": + yield Label("Count") + yield Input( + id=f"wp{self.index}_drifter_count", + value=str( + self.get_drifter_count() if is_selected else "" + ), + type="integer", + placeholder="# of drifters", + validators=Integer( + minimum=1, + failure_description="INVALID: value must be > 0", + ), + classes="drifter-count-input", + ) + yield Label( + "", + id=f"validation-failure-label-wp{self.index}_drifter_count", + classes="-hidden validation-failure", + ) + except Exception as e: raise UnexpectedError(unexpected_msg_compose(e)) from None + def get_drifter_count(self) -> int: + return sum( + 1 for inst in self.waypoint.instrument if inst == InstrumentType.DRIFTER + ) + def copy_from_previous(self) -> None: """Copy inputs from previous waypoint widget (time and instruments only, not lat/lon).""" try: @@ -243,30 +282,23 @@ def copy_from_previous(self) -> None: except Exception as e: raise UnexpectedError(unexpected_msg_compose(e)) from None - @on(Input.Changed) - def show_invalid_reasons(self, event: Input.Changed) -> None: - input_id = event.input.id - label_id = f"validation-failure-label-{input_id}" - label = self.query_one(f"#{label_id}", Label) - if not event.validation_result.is_valid: - message = ( - "\n".join(event.validation_result.failure_descriptions) - if isinstance(event.validation_result.failure_descriptions, list) - else str(event.validation_result.failure_descriptions) - ) - label.update(message) - label.remove_class("-hidden") - label.add_class("validation-failure") - else: - label.update("") - label.add_class("-hidden") - label.remove_class("validation-failure") - @on(Button.Pressed, "Button") def button_pressed(self, event: Button.Pressed) -> None: if event.button.id == f"wp{self.index}_copy": self.copy_from_previous() + @on(Switch.Changed) + def on_switch_changed(self, event: Switch.Changed) -> None: + if event.switch.id == f"wp{self.index}_DRIFTER": + drifter_count_input = self.query_one( + f"#wp{self.index}_drifter_count", Input + ) + if not event.value: + drifter_count_input.value = "" + else: + if not drifter_count_input.value: + drifter_count_input.value = "1" + class ScheduleEditor(Static): def __init__(self, path: str): @@ -285,14 +317,21 @@ def compose(self) -> ComposeResult: yield Rule(line_style="heavy") # SECTION: "Waypoints & Instrument Selection" - with Collapsible( title="[b]Waypoints & Instrument Selection[/b]", id="waypoints", collapsed=True, ): - for i, waypoint in enumerate(self.schedule.waypoints): - yield WaypointWidget(waypoint, i) + yield Horizontal( + Button("Add Waypoint", id="add_waypoint", variant="primary"), + Button( + "Remove Last Waypoint", + id="remove_waypoint", + variant="error", + ), + ) + + yield VerticalScroll(id="waypoint_list", classes="waypoint-list") # SECTION: "Space-Time Region" @@ -472,11 +511,30 @@ def compose(self) -> ComposeResult: except Exception as e: raise UnexpectedError(unexpected_msg_compose(e)) from None + def on_mount(self) -> None: + self.refresh_waypoint_widgets() + + def refresh_waypoint_widgets(self): + waypoint_list = self.query_one("#waypoint_list", VerticalScroll) + waypoint_list.remove_children() + for i, waypoint in enumerate(self.schedule.waypoints): + waypoint_list.mount(WaypointWidget(waypoint, i)) + @on(Input.Changed) def show_invalid_reasons(self, event: Input.Changed) -> None: input_id = event.input.id label_id = f"validation-failure-label-{input_id}" label = self.query_one(f"#{label_id}", Label) + if input_id.endswith("_drifter_count"): + wp_index = int(input_id.split("_")[0][2:]) + drifter_switch = self.query_one(f"#wp{wp_index}_DRIFTER") + if not drifter_switch.value: + label.update("") + label.add_class("-hidden") + label.remove_class("validation-failure") + event.input.remove_class("-valid") + event.input.remove_class("-invalid") + return if not event.validation_result.is_valid: message = ( "\n".join(event.validation_result.failure_descriptions) @@ -491,6 +549,46 @@ def show_invalid_reasons(self, event: Input.Changed) -> None: label.add_class("-hidden") label.remove_class("validation-failure") + @on(Button.Pressed, "#add_waypoint") + def add_waypoint(self) -> None: + """Add a new waypoint to the schedule. Copies time from last waypoint if possible (Lat/lon and instruments blank).""" + try: + if self.schedule.waypoints: + last_wp = self.schedule.waypoints[-1] + new_time = last_wp.time if last_wp.time else None + new_wp = Waypoint( + location=Location( + latitude=0.0, + longitude=0.0, + ), + time=new_time, + instrument=[], + ) + else: + new_wp = Waypoint( + location=Location(latitude=0.0, longitude=0.0), + time=None, + instrument=[], + ) + self.schedule.waypoints.append(new_wp) + self.refresh_waypoint_widgets() + + except Exception as e: + raise UnexpectedError(unexpected_msg_compose(e)) from None + + @on(Button.Pressed, "#remove_waypoint") + def remove_waypoint(self) -> None: + """Remove the last waypoint from the schedule.""" + try: + if self.schedule.waypoints: + self.schedule.waypoints.pop() + self.refresh_waypoint_widgets() + else: + self.notify("No waypoints to remove.", severity="error", timeout=5) + + except Exception as e: + raise UnexpectedError(unexpected_msg_compose(e)) from None + def save_changes(self) -> bool: """Save changes to schedule.yaml.""" try: @@ -547,11 +645,17 @@ def save_changes(self) -> bool: int(self.query_one(f"#wp{i}_minute").value), 0, ) - wp.instrument = [ - instrument - for instrument in InstrumentType - if self.query_one(f"#wp{i}_{instrument.value}").value - ] + + wp.instrument = [] + for instrument in InstrumentType: + switch_on = self.query_one(f"#wp{i}_{instrument.value}").value + if instrument.value == "DRIFTER" and switch_on: + count_str = self.query_one(f"#wp{i}_drifter_count").value + count = int(count_str) + assert count > 0 + wp.instrument.extend([InstrumentType.DRIFTER] * count) + elif switch_on: + wp.instrument.append(instrument) # save self.schedule.to_yaml(f"{self.path}/schedule.yaml") @@ -559,7 +663,7 @@ def save_changes(self) -> bool: except Exception as e: log_exception_to_file( - e, self.path, context_message="Error saving ship config:" + e, self.path, context_message="Error saving schedule:" ) raise UnexpectedError( @@ -707,6 +811,7 @@ def compose(self) -> ComposeResult: yield Switch(value=is_deep, id="adcp_deep") yield Label(" SeaSeven:") yield Switch(value=not is_deep, id="adcp_shallow") + yield Button("?", id="info_button", variant="warning") ## SECTION: "Instrument Configurations"" @@ -748,7 +853,12 @@ def compose(self) -> ComposeResult: value = str(raw_value) else: value = "" - yield Label(f"{attr.replace('_', ' ').title()}:") + label = f"{attr.replace('_', ' ').title()}:" + yield Label( + label + if not is_minutes + else label.replace(":", " Minutes:") + ) yield Input( id=f"{instrument_name}_{attr}", type=type_to_textual( @@ -772,6 +882,15 @@ def compose(self) -> ComposeResult: except Exception as e: raise UnexpectedError(unexpected_msg_compose(e)) from None + @on(Button.Pressed, "#info_button") + def info_pressed(self) -> None: + self.notify( + "[b]SeaSeven[/b]:\nShallow ADCP profiler capable of providing information to a depth of 150 m every 4 meters (300kHz)" + "\n\n[b]OceanObserver[/b]:\nLong-range ADCP profiler capable of providing ~ 1000m of range every 24 meters (38kHz)", + severity="warning", + timeout=20, + ) + @on(Input.Changed) def show_invalid_reasons(self, event: Input.Changed) -> None: input_id = event.input.id @@ -927,19 +1046,33 @@ def compose(self) -> ComposeResult: def sync_ui_waypoints(self): """Update the waypoints models with current UI values (spacetime only) from the live UI inputs.""" schedule_editor = self.query_one(ScheduleEditor) + errors = [] for i, wp in enumerate(schedule_editor.schedule.waypoints): - wp.location = Location( - latitude=float(schedule_editor.query_one(f"#wp{i}_lat").value), - longitude=float(schedule_editor.query_one(f"#wp{i}_lon").value), - ) - wp.time = datetime.datetime( - int(schedule_editor.query_one(f"#wp{i}_year").value), - int(schedule_editor.query_one(f"#wp{i}_month").value), - int(schedule_editor.query_one(f"#wp{i}_day").value), - int(schedule_editor.query_one(f"#wp{i}_hour").value), - int(schedule_editor.query_one(f"#wp{i}_minute").value), - 0, + try: + wp.location = Location( + latitude=float(schedule_editor.query_one(f"#wp{i}_lat").value), + longitude=float(schedule_editor.query_one(f"#wp{i}_lon").value), + ) + wp.time = datetime.datetime( + int(schedule_editor.query_one(f"#wp{i}_year").value), + int(schedule_editor.query_one(f"#wp{i}_month").value), + int(schedule_editor.query_one(f"#wp{i}_day").value), + int(schedule_editor.query_one(f"#wp{i}_hour").value), + int(schedule_editor.query_one(f"#wp{i}_minute").value), + 0, + ) + except Exception as e: + errors.append(f"Waypoint {i + 1}: {e}") + if errors: + log_exception_to_file( + Exception("\n".join(errors)), + self.path, + context_message="Error syncing waypoints:", ) + raise UnexpectedError( + UNEXPECTED_MSG_ONSAVE + + f"\n\nTraceback will be logged in {self.path}/virtualship_error.txt. Please attach this/copy the contents to any issue submitted." + ) from None @on(Button.Pressed, "#exit_button") def exit_pressed(self) -> None: @@ -952,9 +1085,7 @@ def save_pressed(self) -> None: schedule_editor = self.query_one(ScheduleEditor) try: - ship_speed_value = float( - config_editor.query_one("#speed").value - ) # get ship speed from config + ship_speed_value = self.get_ship_speed(config_editor) self.sync_ui_waypoints() # call to ensure waypoint inputs are synced @@ -984,6 +1115,20 @@ def save_pressed(self) -> None: ) return False + def get_ship_speed(self, config_editor): + try: + ship_speed = float(config_editor.query_one("#speed").value) + assert ship_speed > 0 + except Exception as e: + log_exception_to_file( + e, self.path, context_message="Error saving schedule:" + ) + raise UnexpectedError( + UNEXPECTED_MSG_ONSAVE + + f"\n\nTraceback will be logged in {self.path}/virtualship_error.txt. Please attach this/copy the contents to any issue submitted." + ) from None + return ship_speed + class PlanApp(App): CSS = """ @@ -1044,7 +1189,6 @@ class PlanApp(App): Button.-primary { background: $primary; - width: 100%; } Button.-default { @@ -1059,10 +1203,6 @@ class PlanApp(App): background: $error; } - Button#exit_button { - margin-left: 1; - } - Horizontal { height: auto; align: left middle; @@ -1081,6 +1221,21 @@ class PlanApp(App): padding: 1; } + #info_button { + margin-top: 0; + margin-left: 8; + } + + #waypoint_list { + height: auto; + } + + .drifter-count-input { + width: auto; + margin-left: 1; + margin-right: 1; + } + .path { color: $text-muted; text-style: italic; diff --git a/src/virtualship/cli/validator_utils.py b/src/virtualship/cli/validator_utils.py index b2d4095a..83239ac8 100644 --- a/src/virtualship/cli/validator_utils.py +++ b/src/virtualship/cli/validator_utils.py @@ -32,13 +32,13 @@ def is_valid_lat(value: str) -> bool: @require_docstring def is_valid_lon(value: str) -> bool: - """(-180 < lon < 180).""" + """(-180 < lon < 360).""" try: v = float(value) except ValueError: return None - return -180 < v < 180 + return -180 < v < 360 @require_docstring diff --git a/src/virtualship/models/location.py b/src/virtualship/models/location.py index d3657829..793e5312 100644 --- a/src/virtualship/models/location.py +++ b/src/virtualship/models/location.py @@ -22,8 +22,8 @@ def __post_init__(self) -> None: raise ValueError("Latitude cannot be larger than 90.") if self.lon < -180: raise ValueError("Longitude cannot be smaller than -180.") - if self.lon > 180: - raise ValueError("Longitude cannot be larger than 180.") + if self.lon > 360: + raise ValueError("Longitude cannot be larger than 360.") @property def lat(self) -> float: diff --git a/src/virtualship/models/schedule.py b/src/virtualship/models/schedule.py index 0118bbd0..3de44f09 100644 --- a/src/virtualship/models/schedule.py +++ b/src/virtualship/models/schedule.py @@ -137,11 +137,13 @@ def verify( # check waypoint times are in ascending order timed_waypoints = [wp for wp in self.waypoints if wp.time is not None] - if not all( - [next.time >= cur.time for cur, next in itertools.pairwise(timed_waypoints)] - ): + checks = [ + next.time >= cur.time for cur, next in itertools.pairwise(timed_waypoints) + ] + if not all(checks): + invalid_i = [i for i, c in enumerate(checks) if c] raise ScheduleError( - "Each waypoint should be timed after all previous waypoints" + f"Waypoint(s) {', '.join(f'#{i + 1}' for i in invalid_i)}: each waypoint should be timed after all previous waypoints", ) # check if all waypoints are in water From 9ca37ba3802dcbea2b24abf56bf4e4c889425600 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Fri, 25 Jul 2025 16:33:59 +0200 Subject: [PATCH 28/30] update test for new schedule error messaging --- tests/expedition/test_schedule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/expedition/test_schedule.py b/tests/expedition/test_schedule.py index d45900e0..f4a8532e 100644 --- a/tests/expedition/test_schedule.py +++ b/tests/expedition/test_schedule.py @@ -103,7 +103,7 @@ def test_get_instruments() -> None: ), False, ScheduleError, - "Each waypoint should be timed after all previous waypoints", + "Waypoint\\(s\\) : each waypoint should be timed after all previous waypoints", id="SequentialWaypoints", ), pytest.param( From be20ce86b6c5841ba272a6759ba707a1783d39e4 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Fri, 25 Jul 2025 17:23:46 +0200 Subject: [PATCH 29/30] tidy up old TODOs --- src/virtualship/cli/_plan.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index 218533bd..b34facde 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -47,19 +47,6 @@ TimeRange, ) -#! ---------------------------------------------------------------------------------------------- -#! ---------------------------------------------------------------------------------------------- -#! ---------------------------------------------------------------------------------------------- - -# TODO list: - -# - Do final sift through of all other TODOs in the script! - -#! ---------------------------------------------------------------------------------------------- -#! ---------------------------------------------------------------------------------------------- -#! ---------------------------------------------------------------------------------------------- - - UNEXPECTED_MSG_ONSAVE = ( "Please ensure that:\n" "\n1) All typed entries are valid (all boxes in all sections must have green borders and no warnings).\n" From 1ee1f83fb2bc59ae0f0c508a7952e0512dda85c4 Mon Sep 17 00:00:00 2001 From: j-atkins <106238905+j-atkins@users.noreply.github.com> Date: Tue, 29 Jul 2025 10:37:31 +0200 Subject: [PATCH 30/30] catch no matches error --- src/virtualship/cli/_plan.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/virtualship/cli/_plan.py b/src/virtualship/cli/_plan.py index b34facde..85539e3f 100644 --- a/src/virtualship/cli/_plan.py +++ b/src/virtualship/cli/_plan.py @@ -6,6 +6,7 @@ from textual import on from textual.app import App, ComposeResult from textual.containers import Container, Horizontal, VerticalScroll +from textual.dom import NoMatches from textual.screen import Screen from textual.validation import Function, Integer from textual.widgets import ( @@ -511,7 +512,13 @@ def refresh_waypoint_widgets(self): def show_invalid_reasons(self, event: Input.Changed) -> None: input_id = event.input.id label_id = f"validation-failure-label-{input_id}" - label = self.query_one(f"#{label_id}", Label) + + # avoid errors when button pressed too rapidly + try: + label = self.query_one(f"#{label_id}", Label) + except NoMatches: + return + if input_id.endswith("_drifter_count"): wp_index = int(input_id.split("_")[0][2:]) drifter_switch = self.query_one(f"#wp{wp_index}_DRIFTER")