Skip to content

WIP: Scope-Based Visual Feedback + Context Stack Registry#56

Closed
trissim wants to merge 4 commits intomainfrom
feature/pr55-visual-feedback-integration
Closed

WIP: Scope-Based Visual Feedback + Context Stack Registry#56
trissim wants to merge 4 commits intomainfrom
feature/pr55-visual-feedback-integration

Conversation

@trissim
Copy link
Collaborator

@trissim trissim commented Dec 1, 2025

Overview

This PR implements a comprehensive scope-based visual feedback system with a centralized context resolution architecture. It reimplements the goals of the original PR #55 while building on the solid foundation established by PR #44.

Status: 🚧 Work In Progress - All components implemented, debugging in progress

What This PR Does

1. Scope-Based Visual Feedback System ✅ (Working)

Perceptually distinct visual identification for scope hierarchy:

  • Scope Color Generation - MD5-based deterministic colors, WCAG AA compliant
  • Flash Animations - 60fps smooth transitions when resolved values change
    • WidgetFlashAnimator - GroupBox backgrounds
    • TreeItemFlashAnimator - Tree widget items
    • ListItemFlashAnimator - List widget items
  • Layered Borders - Scope-colored window borders
    • Simple solid borders for orchestrator scopes
    • Layered patterned borders for step scopes
  • Integration Mixins
    • ScopedBorderMixin - Renders scope borders on dialogs
    • TreeFormFlashMixin - Coordinates tree+form flash animations

2. Context Stack Registry 🚧 (Implemented, Debugging)

Centralized resolution architecture replacing forms building their own context stacks:

class ContextStackRegistry(QObject):
    value_changed = pyqtSignal(str, str, object, object)  # scope_id, field_path, old, new
    
    def register_scope(scope_id, context_obj, dataclass_type) -> None
    def unregister_scope(scope_id) -> None
    def set(scope_id, field_name, value, dataclass_type) -> None  # Immutable
    def resolve(scope_id, dataclass_type, field_name) -> Any  # Cached
    def get_materialized_object(scope_id) -> Any  # For compilation
    def get_resolved_state(scope_id) -> Dict[str, Any]  # For dirty tracking

Key Design Decisions:

  • Immutability: set() stores concrete values but does NOT mutate context_obj
  • Materialization: get_materialized_object() creates new instances with resolved values
  • Single Source of Truth: All resolution goes through registry, not forms

3. Reactive Flash/Dirty Tracking 🚧 (Implemented, Debugging)

ResolvedItemStateService listens to registry changes:

  • Flash list items when visible scope values change
  • Dirty tracking via baseline comparison

Architecture Comparison: Main vs This Branch

Aspect Main Branch This Branch
Context Resolution Forms build own stacks via build_context_stack() Centralized via ContextStackRegistry
Value Storage Direct mutation via setattr() Immutable via registry.set()
Flash Trigger N/A registry.value_changed signal
Dirty Tracking N/A Baseline comparison via get_resolved_state()
Preview Labels LiveContextResolver registry.resolve()
Scope Colors N/A MD5-based WCAG AA colors

Plan Files (Implementation Roadmap)

All plans implemented in WIP commit, need debugging:

plans/context-registry/plan_01_context_stack_registry.md

Create a ContextStackRegistry service that serves as the single source of truth for all resolved configuration values across the application. This eliminates the current architecture where:

  • ParameterFormManager builds its own context stacks
  • Preview labels resolve values through a separate path
  • Flash/dirty tracking would need yet another resolution path

Key Findings from Plan:

There are exactly 2 ways configuration state changes:

  1. ParameterFormManager edits (field-level changes) → registry.set()
  2. Code mode edits (instance replacement) → registry.register_scope() (re-registration)

No other mutation paths exist.

plans/context-registry/plan_02_form_manager_integration.md

Migrate ParameterFormManager to use ContextStackRegistry as the single source of truth:

  • Write to registry when user edits a field
  • Read from registry for placeholder resolution
  • No longer build their own context stacks

Key Change:

Remove setattr(self.step, field_name, value) from step_parameter_editor.py (no more in-place mutation)

plans/context-registry/plan_03_preview_label_integration.md

Preview labels will:

  • Read from registry for resolved values
  • React to registry.value_changed signal for updates
  • No longer resolve through separate LiveContextResolver path

plans/context-registry/plan_04_flash_dirty_integration.md

Flash: Registry emits (scope_id, field_path, old_value, new_value) - flash if old != new
Dirty: Compare registry.get_resolved_state(scope_id) to saved baseline

User's Exact Specification:

  1. Flash: "only flash if the resolved state changes from before the signal and after the signal"
  2. Preview text: "just reads the current resolved state always, simple as"
  3. Dirty: "checks if the current resolved state is different from the saved resolved state"

Known Issues (from WIP commit)

The end-to-end integration is not yet working. Known bugs:

  • Flash animation timing and visibility filtering needs work
  • Dirty tracking baseline comparison may have edge cases
  • Scope visibility rules need validation across all scenarios
  • Registry cache invalidation may be incomplete
  • Nested manager scope handling needs testing
  • Cross-window update propagation untested
  • Performance impact of registry lookups unknown

Files Changed

New Files (34 total)

Config Framework:

  • context_stack_registry.py - Central registry (406 lines)

Visual Feedback:

  • scope_color_utils.py - Color generation utilities (258 lines)
  • scope_color_strategy.py - Strategy pattern for colors (128 lines)
  • scope_visual_config.py - Color scheme dataclass (148 lines)
  • scope_color_service.py - Reactive color service (151 lines)

Flash Animations:

  • smooth_flash_base.py - Base animator (128 lines)
  • widget_flash_animation.py - Widget flash (121 lines)
  • tree_item_flash_animation.py - Tree item flash (166 lines)
  • list_item_flash_animation.py - List item flash (168 lines)

Mixins:

  • scoped_border_mixin.py - Border rendering (134 lines)
  • tree_form_flash_mixin.py - Flash coordination (165 lines)

Services:

  • resolved_item_state_service.py - Reactive flash/dirty (165 lines)

Docs:

  • scope_visual_feedback_system.rst - Architecture docs

Modified Files

  • abstract_manager_widget.py - Registry connection, flash handlers
  • parameter_form_manager.py - Registry registration
  • parameter_ops_service.py - Use registry.resolve()
  • field_change_dispatcher.py - Update registry on changes
  • live_context_service.py - Track changed field
  • pipeline_editor.py - Scope ID builder, baselines
  • plate_manager.py - Baselines, abstract methods
  • step_parameter_editor.py - Registry for immutable storage
  • config_window.py, dual_editor_window.py - Mixins
  • list_item_delegate.py - Scope border rendering

Commits

  1. a4f8805 - feat(ui): Implement scope-based visual feedback system
  2. 21da0eb - Add plans for context-registry refactor
  3. 3109421 - WIP: Context Stack Registry integration - many bugs to fix
  4. e8b4fbf - Add dependencies to pyproject.toml for color generation

For Reviewers

@smagruder - This PR serves as a starting point for understanding the visual feedback + registry architecture. The visual feedback system (commit 1) is working. The registry refactor (commit 3) implements all 4 plans but has bugs preventing end-to-end functionality.

Key questions for architectural review:

  1. Is the immutability pattern (registry stores values, never mutates context_obj) the right approach?
  2. Should materialization happen lazily or eagerly?
  3. Is the scope visibility filtering logic correct for cross-window updates?
  4. How should we handle nested manager registration (currently only root managers register)?

Dependencies Added

disinctipy = "^1.3.4"  # Perceptually distinct colors
wcag-contrast-ratio = "^0.9"  # WCAG AA compliance

Pull Request opened by Augment Code with guidance from the PR author

Add comprehensive visual feedback for scope identification:

## New Components

### Scope Color System
- ScopeColorService: Singleton managing MD5-based color generation for scope_ids
- ScopeColorScheme: Dataclass holding derived colors (border, background, flash)
- ScopeColorStrategy: Strategy pattern for deriving colors from base scope color
- scope_color_utils: Utility functions for accessing scope colors

### Flash Animation System
- SmoothFlashAnimatorBase: Base class using QVariantAnimation for 60fps transitions
  - 100ms fade-in (OutQuad), 150ms hold at max, 350ms fade-out (InOutCubic)
- WidgetFlashAnimator: Flashes QWidget backgrounds (GroupBoxes)
- TreeItemFlashAnimator: Flashes QTreeWidgetItem backgrounds
- ListItemFlashAnimator: Flashes QListWidgetItem backgrounds

### Integration Mixins
- ScopedBorderMixin: Renders scope-colored borders on windows
  - Simple solid borders for orchestrator scopes
  - Layered patterned borders for step scopes
- TreeFormFlashMixin: Coordinates flash animations for tree+form windows
  - Flashes GroupBox on tree navigation (scroll to section)
  - Flashes tree item + GroupBox when placeholder values change cross-window

## Modified Components

### ParameterFormManager
- Added _on_placeholder_changed_callbacks hook list
- Added _on_build_complete_callbacks for async nested manager registration

### ParameterOpsService
- Fires placeholder change callbacks only when value actually changes
- Passes (config_name, field_name, dataclass_type) to callbacks

### ListItemDelegate
- Renders scope-colored borders on list items using ScopeColorService

### AbstractManagerWidget
- Applies scope colors to list item backgrounds and selection colors

### Windows (ConfigWindow, DualEditorWindow, BaseFormDialog, StepParameterEditor)
- Integrate ScopedBorderMixin and TreeFormFlashMixin
- Initialize scope borders and flash hooks

## Documentation
- Added scope_visual_feedback_system.rst to sphinx docs
- Updated architecture index to include new documentation
- Implement ContextStackRegistry as single source of truth for resolved values
- Add ResolvedItemStateService for reactive flash/dirty tracking
- Refactor ParameterOpsService to use registry.resolve()
- Update scope_id format (remove @position suffix)
- Add baseline tracking for dirty detection
- Connect registry to AbstractManagerWidget for reactive updates

Known issues:
- Flash animation timing and visibility filtering needs work
- Dirty tracking baseline comparison may have edge cases
- Scope visibility rules need validation across all scenarios
- Registry cache invalidation may be incomplete
- Nested manager scope handling needs testing
- Cross-window update propagation untested
- Performance impact of registry lookups unknown

This is a major architectural refactor - extensive testing and debugging needed.
@OpenHCSDev OpenHCSDev deleted a comment from continue bot Dec 2, 2025
trissim added a commit that referenced this pull request Dec 9, 2025
…window cache invalidation

SUMMARY
=======
Three interconnected fixes: (1) Add resolved value change notification system to ObjectState
enabling UI flash animations when inherited values change, (2) Move dirty marker logic to
AbstractManagerWidget ABC so all implementations get it automatically, (3) Fix cache
invalidation when closing windows with unsaved changes so other windows see correct values.

Also fixes false dirty detection when setting explicit value equal to resolved default,
and fixes `func` field reappearing after close/reopen in DualEditorWindow.

ARCHITECTURAL CHANGES
=====================

## 1. Resolved Value Change Notification System

New callback mechanism in ObjectState for UI components to subscribe to actual value changes:

```python
# Subscribe to notifications
state.on_resolved_changed(self._on_values_changed)

def _on_values_changed(self, changed_paths: Set[str]):
    # changed_paths: {'processing_config.group_by', 'well_filter_config.well_filter'}
    for path in changed_paths:
        self._flash_widget_for_path(path)
```

Implementation:
- `on_resolved_changed(callback)` / `off_resolved_changed(callback)` subscription methods
- `_recompute_invalid_fields()` now returns `Set[str]` of paths that actually changed
- `_ensure_live_resolved(notify=True)` calls callbacks when changes detected
- Callbacks only fire when values ACTUALLY change, not just on cache invalidation

## 2. Automatic Dirty Markers in ABC

BEFORE: Each subclass manually added dirty markers
```python
# PipelineEditorWidget._format_list_item()
dirty_marker = " *" if state.is_dirty() else ""
display_text = f"▶ {step_name}{dirty_marker}  ({preview})"
```

AFTER: ABC handles dirty markers automatically
```python
# AbstractManagerWidget (concrete method)
def _format_list_item(self, item, index, context):
    base_text = self._format_item_content(item, index, context)  # abstract
    dirty_marker = self._get_dirty_marker(item)
    return f"{dirty_marker}{base_text}{dirty_marker}"

# Subclasses implement _format_item_content() without dirty marker
```

## 3. Close-Window Cache Invalidation Fix

PROBLEM: Closing ConfigWindow/DualEditorWindow without saving left stale values in
descendant caches, so other windows showed incorrect inherited values.

FIX: BaseFormDialog.closeEvent() now:
1. Calls `_apply_state_action('restore_saved')` to revert unsaved changes
2. Triggers `ObjectStateRegistry.increment_token()` for global refresh

Additionally, `restore_saved()` now:
1. Compares current parameters to `_saved_parameters` snapshot
2. Invalidates descendant caches for each changed parameter
3. Uses `invalidate_by_type_and_scope()` with proper container types

## 4. False Dirty Detection Fix

PROBLEM: Setting `well_filter_mode` from None (resolves to INCLUDE) to explicit INCLUDE
showed as dirty, even though resolved values are identical.

ROOT CAUSE: Container entries (nested dataclass instances) were being stored in both
`parameters` and `_saved_resolved`, causing false positives when comparing containers
with None values vs containers with explicit values.

FIX: Skip container entries from resolved snapshots:
```python
# In _compute_resolved_snapshot() and _recompute_invalid_fields():
is_container = '.' not in name and is_dataclass(container_type)
if is_container:
    continue  # Skip - only compare leaf fields
```

## 5. `func` Field Reappearing Fix

PROBLEM: After closing DualEditorWindow and reopening, `func` field appeared in step
editor even though it should be excluded.

ROOT CAUSE: `restore_saved()` re-extracted parameters without passing `exclude_params`,
causing excluded fields to reappear.

FIX: Store `_exclude_param_names` in `__init__` and pass to `_extract_all_parameters_flat()`
in `restore_saved()`.

## 6. build_context_stack() Simplification

Removed `context_obj` parameter - `ancestor_objects` is now the single source of truth
for parent hierarchy. Simplifies call sites and eliminates redundant context layering.

FILE-BY-FILE CHANGES
====================

## Config Framework (2 files)

openhcs/config_framework/context_manager.py (+15, -22)
- FIX: `_merge_nested_dataclass()` uses object.__getattribute__ for base_value
- FIX: `config_context()` no longer calls `to_base_config()` which broke None preservation
- SIMPLIFY: `build_context_stack()` removes `context_obj` parameter
- ADD: Handle ancestor GlobalConfigBase in stack building

openhcs/config_framework/object_state.py (+120, -50)
- ADD: `_on_resolved_changed_callbacks` list for change notifications
- ADD: `on_resolved_changed()` / `off_resolved_changed()` subscription methods
- ADD: `_saved_parameters` snapshot for restore diff
- ADD: `_exclude_param_names` storage for restore_saved()
- UPDATE: `_ensure_live_resolved(notify=True)` calls callbacks on change
- UPDATE: `_recompute_invalid_fields()` returns Set[str] of changed paths
- UPDATE: `restore_saved()` invalidates descendant caches for changed params
- FIX: Skip container entries from snapshots (false dirty fix)
- ADD: Debug logging for dirty field comparison

## PyQt GUI - Widgets (4 files)

openhcs/pyqt_gui/widgets/shared/abstract_manager_widget.py (+30)
- ADD: `_format_list_item()` concrete method with automatic dirty marker
- ADD: `_format_item_content()` abstract method for subclasses
- ADD: `_get_dirty_marker()` helper using ObjectState.is_dirty()

openhcs/pyqt_gui/widgets/pipeline_editor.py (-8)
- REMOVE: Manual dirty marker from format_item_for_display()
- RENAME: `_format_list_item()` → `_format_item_content()`

openhcs/pyqt_gui/widgets/plate_manager.py (-2)
- RENAME: `_format_list_item()` → `_format_item_content()`

openhcs/pyqt_gui/widgets/function_list_editor.py (-1)
- UPDATE: Remove `context_obj` parameter from build_context_stack() call

## PyQt GUI - Services (1 file)

openhcs/pyqt_gui/widgets/shared/services/widget_service.py (-1)
- UPDATE: Remove `context_obj` parameter from build_context_stack() call

## PyQt GUI - Windows (1 file)

openhcs/pyqt_gui/windows/base_form_dialog.py (+16)
- UPDATE: closeEvent() calls restore_saved() + triggers global refresh
- Ensures descendant caches invalidated when closing with unsaved changes

## Processing (1 file)

openhcs/processing/backends/lib_registry/unified_registry.py (+2)
- RE-ENABLE: well_filter_config and step_well_filter_config injectable params

TESTING
=======
Verified manually:
- Setting explicit value equal to resolved default no longer shows dirty
- `func` field stays excluded after close/reopen
- Closing config window without saving correctly refreshes other windows
- Dirty markers appear on both PlateManager and PipelineEditor items
- Resolved change callback fires with correct changed paths

PREPARED FOR FUTURE WORK
========================
The resolved change notification system enables UI flash animations:
- PR #56 contains flash animation infrastructure (SmoothFlashAnimatorBase, etc.)
- ObjectState.on_resolved_changed() provides the hook for wiring up flashing
- Next branch will integrate flash animations with this notification system
@trissim trissim closed this Jan 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant