[BrushGraph 2/7] Add converters and repository

[maxmmitchell]

Description

This is the second PR in the Brush Graph stack. It introduces persistence and conversion logic to translate the interactive graph data model into Android Ink Proto formats.

Details

• Mapping definitions between the UI-friendly BrushGraph and the tree-structured ProtoBrushFamily.
• Deduplication algorithms to reduce the number of identical nodes on-screen, while maintaining semantically identical brushes.
• BrushGraphRepository abstracts the persistence and conversion details away from the ViewModel, providing a clean API for loading and saving.
• Centralizes failure points during serialization/deserialization.

Dependencies

• Target: brush-graph/1-data

[gemini-code-assist]

Code Review

This pull request introduces a graph-based data model for custom brushes, providing utilities to convert between the BrushGraph model and the functional BrushFamily proto. It includes a BrushGraphRepository for state management, validation, and persistence, along with comprehensive unit tests. The review feedback highlights several performance concerns regarding $O(N^2)$ complexity in graph conversion and deduplication logic. Additionally, there are recommendations to improve testability by injecting coroutine scopes, enhance maintainability by removing magic strings, and increase robustness by decoupling port reordering logic from UI-specific index offsets.

[gemini-code-assist]

The validate() method is called synchronously on every graph modification (add node, add edge, etc.). Since GraphValidator.validateAll performs extensive checks over all nodes and edges, this can block the main thread and cause UI stutters for larger graphs. Consider offloading validation to a background thread or debouncing the updates if immediate synchronous feedback isn't required for all operations.

[maxmmitchell]

I think we do need validate() to be called on every graph modification, as we are constantly updating the brush being used. Delaying validation could cause a gap between when the brush family updates, and when the user receives notice their brush has errors. So a brush could be broken and the user wouldn't know before using it. Validate also needs to block conversion (to avoid creating an invalid brush), so there'd be a lag between when the graph is modified and when the brush is updated.

[gemini-code-assist]

The logic for reordering Paint ports relies on hardcoded assumptions about the index offsets (e.g., T + 1). This tightly couples the repository logic to the specific visual representation of ports in the UI. If the UI layout changes, this logic will break. Consider passing the specific port IDs or using a more robust way to identify the target list.

[maxmmitchell]

There's an argument to be made that this logic belongs at the UI layer. But although it does wind up influencing the UI, it is fundamentally enforcing rules about how the ports are stored in the data layer, so I think using the indices is OK. Perhaps this ordering should be completely maintained at the UI layer instead, but then we need to hold onto the portIds in the data layer, but then also hold an ordering for them in the UI layer. I'm not sure this is any better.

[cka-dev]

lgtm with comments.
They are non-blocking but need to be addressed as a fast follow if not addressable right now