fix: Updates based on feedback.

Signed-off-by: Karthik Bekal Pattathana <[email protected]>

src/deadline/client/ui/dataclasses/timeouts.py

@@ -4,7 +4,7 @@
 from dataclasses import dataclass
 from datetime import timedelta
 
-from deadline.client.exceptions import NonValidInputError
+from ...exceptions import NonValidInputError
 
 
 @dataclass
@@ -14,7 +14,7 @@ class TimeoutEntry:
 
     Attributes:
         tooltip (str): Description or explanation of the timeout entry.
-        is_activated (bool): Flag indicating if the timeout is active. Defaults to True.
+        is_activated (bool): Flag indicating if the timeout is activated. Defaults to True.
         seconds (int): Timeout duration in seconds. Defaults to 24 hours (86400 seconds).
 
     Raises:
@@ -32,10 +32,15 @@ def __post_init__(self) -> None:
         if not self.tooltip:
             raise ValueError("Timeout tooltip cannot be empty.")
 
-    def to_dict(self) -> Dict[str, Any]:
+    def to_sticky_settings_dict(self) -> Dict[str, Any]:
         """
-        Converts the timeout entry to a dictionary representation.
-        We skip tooltip as it not necessary to be saved in sticky settings.
+        Returns the timeout entry as a dictionary suitable for sticky settings.
+
+        The returned dictionary contains only the essential attributes needed for saving:
+        - is_activated: The activation status
+        - seconds: The timeout duration
+
+        The tooltip is intentionally excluded as it's not necessary for sticky settings.
 
         Returns:
             Dict[str, Any]: Dictionary containing activation status and timeout duration.
@@ -44,31 +49,43 @@ def to_dict(self) -> Dict[str, Any]:
 
 
 @dataclass
-class TimeoutEntries:
+class TimeoutTableEntries:
     """
-    Manages a collection of timeout entries.
+    Manages a collection of named timeout entries.
 
     This class provides functionality to handle multiple TimeoutEntry instances,
-    including serialization and validation operations.
+    including serialization to/from sticky settings and validation operations.
+    It maintains a mapping of labels to timeout configurations and ensures
+    all timeout values are valid when activated.
 
     Attributes:
         entries (Dict[str, TimeoutEntry]): Dictionary mapping labels to TimeoutEntry instances.
     """
 
     entries: Dict[str, TimeoutEntry]
 
-    def to_dict(self) -> Dict[str, Dict[str, Any]]:
+    def to_sticky_settings_dict(self) -> Dict[str, Dict[str, Any]]:
         """
-        Converts all timeout entries to a dictionary format.
+        Returns all timeout entries as a dictionary format suitable for sticky settings.
 
         Returns:
-            Dict[str, Dict[str, Any]]: Dictionary mapping labels to entry configurations.
+            Dict[str, Dict[str, Any]]: A dictionary mapping labels to entry configurations.
+                Example: {
+                    "Env1 enter": {"is_activated": True, "seconds": 300},
+                    "Env1 exit": {"is_activated": True, "seconds": 60},
+                    ...
+                }
         """
-        return {label: entry.to_dict() for label, entry in self.entries.items()}
+        return {label: entry.to_sticky_settings_dict() for label, entry in self.entries.items()}
 
-    def update_from_dict(self, data: Dict[str, Dict[str, Any]]) -> None:
+    def update_from_sticky_settings(self, data: Dict[str, Dict[str, Any]]) -> None:
         """
-        Updates existing timeout entries from a dictionary of configurations.
+        Updates existing timeout entries with values from sticky settings.
+
+        This method performs a selective update where:
+        - Only existing entries are updated (unknown labels in input data are ignored)
+        - Missing attributes preserve their current values. So adding a new timeout is
+            still a backwards compatible change.
 
         Args:
             data (Dict[str, Dict[str, Any]]): Dictionary containing timeout configurations.
@@ -81,10 +98,16 @@ def update_from_dict(self, data: Dict[str, Dict[str, Any]]) -> None:
 
     def validate_entries(self) -> None:
         """
-        Validates all timeout entries in the collection.
+        Validates all timeout entries in the table.
+
+        Performs validation checks for all entries:
+        - Identifies any activated entries with zero duration
+        - Collects labels of non valid entries
+        - Provides detailed error message for configuration correction
 
         Raises:
-            NonValidInputError: If any activated timeout has a zero duration.
+            NonValidInputError: If any activated timeout has a zero duration. The error message
+                includes the specific labels of non valid entries and instructions for correction.
         """
         zero_timeouts = [
             label

src/deadline/client/ui/widgets/job_timeouts_widget.py

@@ -18,7 +18,8 @@
     QGridLayout,
     QWidget,
 )
-from ..dataclasses.timeouts import TimeoutEntries
+from qtpy.QtCore import Signal
+from ..dataclasses.timeouts import TimeoutTableEntries
 
 # UI Constants
 WARNING_ICON = "⚠️"
@@ -35,6 +36,9 @@ class TimeoutEntryWidget(QWidget):
     checkbox state, validation indicators, and time value calculations.
     """
 
+    # Signal to indicate any change in the widget
+    changed = Signal()
+
     def __init__(self, label: str, tooltip: str):
         super().__init__()
 
@@ -58,6 +62,7 @@ def __init__(self, label: str, tooltip: str):
         self.minutes_box.setFixedWidth(90)
 
         self._build_ui()
+        self._connect_signals()
 
     def _build_ui(self):
         """
@@ -72,6 +77,23 @@ def _build_ui(self):
         layout.addWidget(self.hours_box, 0, 3)
         layout.addWidget(self.minutes_box, 0, 4)
 
+    def _connect_signals(self):
+        """
+        Connects all internal widgets to emit the changed signal.
+        """
+        self.checkbox.clicked.connect(self._on_change)
+        self.days_box.valueChanged.connect(self._on_change)
+        self.hours_box.valueChanged.connect(self._on_change)
+        self.minutes_box.valueChanged.connect(self._on_change)
+
+    def _on_change(self):
+        """
+        Handler for any changes in the widget's state.
+        Updates the widget state and emits the changed signal.
+        """
+        self.update_state()
+        self.changed.emit()
+
     def set_enabled(self, enabled: bool):
         """
         Enables or deactivates all time input fields in this row.
@@ -134,10 +156,10 @@ def set_status_icon(self, status: str):
 
     def set_error_style(self, is_error: bool):
         """
-        Sets the status icon (warning or error) next to the checkbox.
+        Sets the error styling for the time input fields.
 
         Args:
-            status: The icon to display (WARNING_ICON, ERROR_ICON, or empty string)
+            is_error: If True, applies error styling; if False, removes it
         """
         style = f"QSpinBox {{ background-color: {ERROR_BG_COLOR}; }}" if is_error else ""
         for box in [self.days_box, self.hours_box, self.minutes_box]:
@@ -162,18 +184,35 @@ def update_state(self):
         self.update_suffix()
 
 
-class JobTimeoutsWidget(QGroupBox):
+class TimeoutTableWidget(QGroupBox):
     """
-    UI element to hold timeout settings of the submission
+    A widget for managing multiple timeout settings in a tabular format.
+
+    This widget provides a user interface for configuring multiple timeout entries,
+    each consisting of a checkbox for activation/deactivation and time input fields
+    for days, hours, and minutes. It includes validation, warning indicators, and
+    error messages for invalid configurations.
+
+    Features:
+    - Individual timeout rows with checkbox activation
+    - Time input fields for days, hours, and minutes
+    - Visual indicators for warnings and errors
+    - Real-time validation and feedback
+    - Automatic suffix updates (singular/plural)
+    - Comprehensive error messaging
+
+    Args:
+        timeouts (TimeoutTableEntries): Configuration object containing timeout entries
+        parent (QWidget, optional): Parent widget. Defaults to None.
     """
 
-    def __init__(self, *, timeouts: TimeoutEntries, parent=None):
+    def __init__(self, *, timeouts: TimeoutTableEntries, parent=None):
         super().__init__("Timeouts", parent=parent)
         self.timeout_rows: Dict[str, TimeoutEntryWidget] = {}
         self._build_ui(timeouts)
         self.refresh_ui(timeouts)
 
-    def _build_ui(self, timeouts: TimeoutEntries):
+    def _build_ui(self, timeouts: TimeoutTableEntries):
         """
         Constructs the complete UI layout with all timeout rows and message labels.
 
@@ -190,7 +229,7 @@ def _build_ui(self, timeouts: TimeoutEntries):
             timeout_row = TimeoutEntryWidget(label, entry.tooltip)
             timeouts_layout.addWidget(timeout_row, index, 0, 1, 4)
             self.timeout_rows[label] = timeout_row
-            self._hookup_change_callback(timeout_row)
+            timeout_row.changed.connect(self._update_ui_state)
 
         self.error_label = self._create_message_label(ERROR_BG_COLOR)
         self.warning_label = self._create_message_label(WARNING_BG_COLOR)
@@ -222,24 +261,12 @@ def _create_message_label(self, bg_color: str) -> QLabel:
         label.setWordWrap(True)
         return label
 
-    def _hookup_change_callback(self, timeout_row: TimeoutEntryWidget):
-        """
-        Connects UI element signals to update handlers for a timeout row.
-
-        Args:
-            timeout_row: The TimeoutRow instance to hook up callbacks for
-        """
-        timeout_row.checkbox.clicked.connect(self._update_ui_state)
-        for spinbox in [timeout_row.days_box, timeout_row.hours_box, timeout_row.minutes_box]:
-            spinbox.valueChanged.connect(self._update_ui_state)
-
     def _update_ui_state(self):
         """
-        Updates the complete UI state including all timeout rows and message labels.
+        Updates the UI state for error and warning messages.
         """
         self._update_error_states()
         self._update_warning_states()
-        self._update_row_states()
 
     def _update_error_states(self):
         """
@@ -266,14 +293,7 @@ def _update_warning_states(self):
         self.warning_label.setText(warning_text)
         self.warning_label.setVisible(any_deactivated)
 
-    def _update_row_states(self):
-        """
-        Updates the state of all timeout rows.
-        """
-        for row in self.timeout_rows.values():
-            row.update_state()
-
-    def refresh_ui(self, timeouts: TimeoutEntries):
+    def refresh_ui(self, timeouts: TimeoutTableEntries):
         """
         Refreshes all UI elements to reflect the current timeout settings.
 
@@ -287,12 +307,12 @@ def refresh_ui(self, timeouts: TimeoutEntries):
                 row.set_timeout(entry.seconds)
         self._update_ui_state()
 
-    def update_settings(self, settings: TimeoutEntries):
+    def update_settings(self, timeouts: TimeoutTableEntries):
         """
-        Updates the settings object with the current values from the UI.
+        Updates the timeouts with the current values from the UI.
         """
         for label, row in self.timeout_rows.items():
-            if label in settings.entries:
-                entry = settings.entries[label]
+            if label in timeouts.entries:
+                entry = timeouts.entries[label]
                 entry.is_activated = row.checkbox.isChecked()
                 entry.seconds = row.get_timeout_seconds()

test/unit/deadline_client/ui/dataclasses/test_timeouts.py

@@ -1,7 +1,7 @@
 # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 
 import pytest
-from deadline.client.ui.dataclasses.timeouts import TimeoutEntry, TimeoutEntries
+from deadline.client.ui.dataclasses.timeouts import TimeoutEntry, TimeoutTableEntries
 from datetime import timedelta
 from deadline.client.exceptions import NonValidInputError
 
@@ -66,19 +66,19 @@ def test_empty_fields(tooltip, error_message):
 
 
 @pytest.fixture
-def valid_timeout_entries() -> TimeoutEntries:
-    """Fixture providing a TimeoutEntries instance with valid entries."""
+def valid_timeout_entries() -> TimeoutTableEntries:
+    """Fixture providing a TimeoutTableEntries instance with valid entries."""
     entries = {
         "entry1": TimeoutEntry(tooltip="First timeout", is_activated=True, seconds=3600),
         "entry2": TimeoutEntry(tooltip="Second timeout", is_activated=False, seconds=7200),
     }
-    return TimeoutEntries(entries=entries)
+    return TimeoutTableEntries(entries=entries)
 
 
-class TestTimeoutEntries:
-    def test_to_dict(self, valid_timeout_entries):
-        """Test conversion of TimeoutEntries to dictionary format, including value checks."""
-        result = valid_timeout_entries.to_dict()
+class TestTimeoutTableEntries:
+    def test_to_sticky_settings_dict(self, valid_timeout_entries):
+        """Test conversion of TimeoutTableEntries to dictionary format, including value checks."""
+        result = valid_timeout_entries.to_sticky_settings_dict()
 
         assert isinstance(result, dict)
         assert len(result) == 2
@@ -91,13 +91,13 @@ def test_to_dict(self, valid_timeout_entries):
         assert "entry2" in result
         assert result["entry2"] == {"is_activated": False, "seconds": 7200}
 
-    def test_update_from_dict(self, valid_timeout_entries):
-        """Test updating TimeoutEntries from dictionary data."""
+    def test_update_from_sticky_settings(self, valid_timeout_entries):
+        """Test updating TimeoutTableEntries from dictionary data."""
         update_data = {
             "entry1": {"is_activated": False, "seconds": 1800},
         }
 
-        valid_timeout_entries.update_from_dict(update_data)
+        valid_timeout_entries.update_from_sticky_settings(update_data)
 
         # Check entry1 updates
         assert valid_timeout_entries.entries["entry1"].is_activated is False
@@ -107,12 +107,12 @@ def test_update_from_dict(self, valid_timeout_entries):
         assert valid_timeout_entries.entries["entry2"].is_activated is False  # unchanged
         assert valid_timeout_entries.entries["entry2"].seconds == 7200
 
-    def test_update_from_dict_with_nonexistent_entry(self, valid_timeout_entries):
+    def test_update_from_sticky_settings_with_nonexistent_entry(self, valid_timeout_entries):
         """Test updating with data for non-existent entries."""
         update_data = {"nonexistent_entry": {"is_activated": False, "seconds": 1800}}
 
         # Should not raise an error, should simply ignore non-existent entries
-        valid_timeout_entries.update_from_dict(update_data)
+        valid_timeout_entries.update_from_sticky_settings(update_data)
         assert "nonexistent_entry" not in valid_timeout_entries.entries
 
     def test_validate_entries_success(self, valid_timeout_entries):
@@ -141,11 +141,10 @@ def test_validate_entries_with_multiple_zero_timeouts(self, valid_timeout_entrie
         assert "entry1" in str(exc_info.value)
         assert "entry2" not in str(exc_info.value)
 
-        # Check that both entries are present.
         valid_timeout_entries.entries["entry2"].is_activated = True
         with pytest.raises(NonValidInputError) as exc_info:
             valid_timeout_entries.validate_entries()
 
-        # Check that only entry1 is in error.
+        # Check that both entries are in error.
         assert "entry1" in str(exc_info.value)
         assert "entry2" in str(exc_info.value)