Minor GUI refactor - Drag+drop issues introduced
This commit is contained in:
127
Documentation/gui_file_table_refactor_plan.md
Normal file
127
Documentation/gui_file_table_refactor_plan.md
Normal file
@@ -0,0 +1,127 @@
|
||||
# Code Review & Refactoring Plan: GUI File Table
|
||||
|
||||
**Objective:** To identify the root causes of file list discrepancies and persistent empty asset rows in the GUI file table, and to propose refactoring solutions for improved robustness and maintainability.
|
||||
|
||||
**Phase 1: In-Depth Code Review**
|
||||
|
||||
This phase will focus on understanding the current implementation and data flow within the relevant GUI modules.
|
||||
|
||||
1. **Identify Key Modules & Classes:**
|
||||
* **`gui/unified_view_model.py` ([`UnifiedViewModel`](gui/unified_view_model.py:1)):** This is the primary focus. We need to analyze:
|
||||
* How it loads and represents the hierarchical data (`SourceRule` -> `AssetRule` -> `FileRule`).
|
||||
* Methods responsible for updating the model with new data (e.g., from predictions or user edits).
|
||||
* Logic for adding, removing, and modifying rows, especially `AssetRule` and `FileRule` items.
|
||||
* How it handles data consistency when underlying data changes (e.g., after LLM processing or renaming operations).
|
||||
* Signal/slot connections related to data changes.
|
||||
* **`gui/asset_restructure_handler.py` ([`AssetRestructureHandler`](gui/asset_restructure_handler.py:1)):**
|
||||
* How it listens to changes in `AssetRule` names or `FileRule` target asset overrides.
|
||||
* The logic for moving `FileRule` items between `AssetRule` items.
|
||||
* The conditions under which it creates new `AssetRule` items or removes empty ones. This is critical for the "persistent empty asset rows" issue.
|
||||
* **`gui/llm_prediction_handler.py` ([`LLMPredictionHandler`](gui/llm_prediction_handler.py:1)):**
|
||||
* How it parses the LLM response.
|
||||
* How it translates the LLM's (potentially hallucinated) file list into the `SourceRule` structure.
|
||||
* How this new `SourceRule` data is passed to and integrated by the `UnifiedViewModel`. This is key for the "file list discrepancies" issue.
|
||||
* **`gui/prediction_handler.py` ([`RuleBasedPredictionHandler`](gui/prediction_handler.py:1)):**
|
||||
* Similar to the LLM handler, how it generates `SourceRule` data from presets.
|
||||
* How its output is integrated into the `UnifiedViewModel`, especially when "reinterpreting with a systematic approach" restores correct files.
|
||||
* **`gui/main_window.py` ([`MainWindow`](gui/main_window.py:1)) & `gui/main_panel_widget.py` ([`MainPanelWidget`](gui/main_panel_widget.py:1)):**
|
||||
* How these components instantiate and connect the `UnifiedViewModel`, `AssetRestructureHandler`, and prediction handlers.
|
||||
* Event handling related to loading data, triggering predictions, and user interactions that modify the table data.
|
||||
* **`rule_structure.py`:**
|
||||
* Review the definitions of `SourceRule`, `AssetRule`, and `FileRule` to ensure a clear understanding of the data being managed.
|
||||
|
||||
2. **Trace Data Flow & State Management:**
|
||||
* **Initial Load:** How is the initial list of files/assets loaded and represented in the `UnifiedViewModel`?
|
||||
* **LLM Processing:**
|
||||
* Trace the data flow from the LLM response -> `LLMPredictionHandler` -> `UnifiedViewModel`.
|
||||
* How does the `UnifiedViewModel` reconcile the LLM's version of the file list with any existing state? Is there a clear "source of truth" for the file list before and after LLM processing?
|
||||
* **Preset-Based Processing:**
|
||||
* Trace data flow from preset selection -> `RuleBasedPredictionHandler` -> `UnifiedViewModel`.
|
||||
* How does this "systematic approach" correct discrepancies? Does it fully replace the model's data or merge it?
|
||||
* **Renaming/Restructuring:**
|
||||
* Trace the events and actions from a user renaming an asset -> `AssetRestructureHandler` -> `UnifiedViewModel`.
|
||||
* How are `AssetRule` items checked for emptiness and subsequently removed (or not removed)?
|
||||
|
||||
3. **Analyze Event Handling and Signal/Slot Connections:**
|
||||
* Map out the key signals and slots between the `UnifiedViewModel`, `AssetRestructureHandler`, prediction handlers, and the main UI components.
|
||||
* Ensure that signals are emitted and slots are connected correctly to trigger necessary updates and prevent race conditions or missed updates.
|
||||
|
||||
**Phase 2: Identify Issues & Propose Refactoring Strategies**
|
||||
|
||||
Based on the review, we will pinpoint specific areas contributing to the reported problems and suggest improvements.
|
||||
|
||||
1. **For File List Discrepancies (especially post-LLM):**
|
||||
* **Potential Issue:** The `UnifiedViewModel` might be directly replacing its internal data with the LLM's output without proper validation or merging against the original input file list.
|
||||
* **Proposed Strategy:**
|
||||
* Establish a clear "source of truth" for the actual input files that remains independent of the LLM's interpretation.
|
||||
* When the LLM provides its categorized list, the `LLMPredictionHandler` or `UnifiedViewModel` should *map* the LLM's findings onto the *existing* source files rather than creating a new list from scratch based on LLM hallucinations.
|
||||
* If the LLM identifies files not in the original input, these should be flagged or handled as discrepancies, not added as if they were real.
|
||||
* If the LLM *misses* files from the original input, these should remain visible, perhaps marked as "uncategorized by LLM."
|
||||
|
||||
2. **For Persistent Empty Asset Rows:**
|
||||
* **Potential Issue:** The `AssetRestructureHandler`'s logic for detecting and removing empty `AssetRule` items might be flawed or not consistently triggered. It might not correctly count child `FileRule` items after a move, or the signal to check for emptiness might not always fire.
|
||||
* **Proposed Strategy:**
|
||||
* Review and strengthen the logic within `AssetRestructureHandler` that checks if an `AssetRule` is empty after its `FileRule` children are moved or its name changes.
|
||||
* Ensure that this check is reliably performed *after* all relevant model updates have completed.
|
||||
* Consider adding explicit methods to `UnifiedViewModel` or `AssetRule` to query if an asset group is truly empty (has no associated `FileRule` items).
|
||||
* Ensure that the `UnifiedViewModel` correctly emits signals that the `AssetRestructureHandler` can use to trigger cleanup of empty asset rows.
|
||||
|
||||
3. **General Robustness & Maintainability:**
|
||||
* **State Management:** Clarify state management within `UnifiedViewModel`. Ensure data consistency and minimize side effects.
|
||||
* **Modularity:** Ensure clear separation of concerns between the `UnifiedViewModel` (data representation), prediction handlers (data generation), and `AssetRestructureHandler` (data manipulation).
|
||||
* **Logging & Error Handling:** Improve logging in these critical sections to make troubleshooting easier. Add robust error handling for unexpected data states.
|
||||
* **Unit Tests:** Identify areas where unit tests could be added or improved to cover the scenarios causing these bugs, especially around model updates and restructuring.
|
||||
|
||||
**Phase 3: Documentation & Handoff**
|
||||
|
||||
1. Document the findings of the code review.
|
||||
2. Detail the agreed-upon refactoring plan.
|
||||
3. Prepare for handoff to a developer (e.g., by switching to "Code" mode).
|
||||
|
||||
**Visual Plan (Mermaid Diagram):**
|
||||
|
||||
```mermaid
|
||||
graph TD
|
||||
subgraph GUI Interaction
|
||||
UserAction[User Action (Load Files, Rename Asset, Trigger LLM)]
|
||||
end
|
||||
|
||||
subgraph Prediction Layer
|
||||
LLM_Handler([`gui.llm_prediction_handler.LLMPredictionHandler`])
|
||||
Preset_Handler([`gui.prediction_handler.RuleBasedPredictionHandler`])
|
||||
end
|
||||
|
||||
subgraph Core GUI Logic
|
||||
MainWindow([`gui.main_window.MainWindow`])
|
||||
MainPanel([`gui.main_panel_widget.MainPanelWidget`])
|
||||
ViewModel([`gui.unified_view_model.UnifiedViewModel`])
|
||||
RestructureHandler([`gui.asset_restructure_handler.AssetRestructureHandler`])
|
||||
end
|
||||
|
||||
subgraph Data Structures
|
||||
RuleStruct([`rule_structure.py` <br> SourceRule, AssetRule, FileRule])
|
||||
end
|
||||
|
||||
UserAction --> MainWindow
|
||||
MainWindow --> MainPanel
|
||||
MainPanel -- Triggers Predictions --> LLM_Handler
|
||||
MainPanel -- Triggers Predictions --> Preset_Handler
|
||||
MainPanel -- Displays Data From --> ViewModel
|
||||
|
||||
LLM_Handler -- Provides SourceRule Data --> ViewModel
|
||||
Preset_Handler -- Provides SourceRule Data --> ViewModel
|
||||
|
||||
ViewModel -- Manages --> RuleStruct
|
||||
ViewModel -- Signals Changes --> RestructureHandler
|
||||
ViewModel -- Signals Changes --> MainPanel
|
||||
|
||||
|
||||
RestructureHandler -- Modifies --> ViewModel
|
||||
|
||||
%% Issues
|
||||
style LLM_Handler fill:#f9d,stroke:#333,stroke-width:2px %% Highlight LLM Handler for file list issue
|
||||
style ViewModel fill:#f9d,stroke:#333,stroke-width:2px %% Highlight ViewModel for both issues
|
||||
style RestructureHandler fill:#f9d,stroke:#333,stroke-width:2px %% Highlight Restructure Handler for empty row issue
|
||||
|
||||
note right of LLM_Handler: Potential source of file list discrepancies
|
||||
note right of RestructureHandler: Potential source of persistent empty asset rows
|
||||
Reference in New Issue
Block a user