Add DocumentMode groundwork with MaskMode and Q toggle#4001
Add DocumentMode groundwork with MaskMode and Q toggle#4001krVatsal wants to merge 10 commits intoGraphiteEditor:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new DocumentMode enum, including a MaskMode variant, and integrates it into the DocumentMessageHandler to allow toggling and setting the document mode. The review feedback suggests ensuring UI consistency by triggering PortfolioMessage::UpdateDocumentWidgets when the mode changes and recommends uncommenting and updating the fmt::Display implementation for DocumentMode to avoid leaving dead code.
ca57043 to
268ec73
Compare
|
I wanted to continue with the work on part of my proposed changes but that needs these changes, should i continue adding further commits to this PR only? It would be great if you review this and then i can follow up with further changes for marquee selection. @Keavon |
|
These changes comprise a relatively small change set, so you can and should continue adding functionality since this is so far just the user's entry point to a not-yet-built feature. |
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/portfolio/document/document_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/document_message_handler.rs:1161">
P1: Mask group creation can self-parent because parent resolution happens after entering MaskMode and storing the same mask group ID used for the new layer.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/document_message_handler.rs:1192">
P1: Applying mask on `ExitMaskMode` mutates the graph without transaction boundaries, unlike the discard branch, causing inconsistent undo/history behavior.</violation>
<violation number="3" location="editor/src/messages/portfolio/document/document_message_handler.rs:1194">
P1: Mask apply path discards authored mask content by using a placeholder 1x1 white image and then deleting the mask group.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
b14dbd9 to
cbb615e
Compare
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:447">
P2: `ApplyMaskStencil` uses `ClipModeToggle`, making mask application state-dependent and capable of un-clipping already clipped target layers.</violation>
</file>
<file name="editor/src/messages/portfolio/document/document_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/document_message_handler.rs:1194">
P1: ExitMaskMode uses a placeholder mask image and deletes the mask group, so user-drawn mask content is not applied.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:443">
P2: `ApplyMaskStencil` now accepts `mask_group` but immediately discards it, so the operation ignores the provided mask source and does not perform stencil application as implied.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| modify_inputs.transform_set(placement_transform, TransformIn::Local, false); | ||
| } | ||
| GraphOperationMessage::ApplyMaskStencil { layers, mask_group } => { | ||
| let _ = mask_group; |
There was a problem hiding this comment.
P2: ApplyMaskStencil now accepts mask_group but immediately discards it, so the operation ignores the provided mask source and does not perform stencil application as implied.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, line 443:
<comment>`ApplyMaskStencil` now accepts `mask_group` but immediately discards it, so the operation ignores the provided mask source and does not perform stencil application as implied.</comment>
<file context>
@@ -439,7 +439,10 @@ impl MessageHandler<GraphOperationMessage, GraphOperationMessageContext<'_>> for
}
- GraphOperationMessage::ApplyMaskStencil { layers } => {
+ GraphOperationMessage::ApplyMaskStencil { layers, mask_group } => {
+ let _ = mask_group;
+
+ // TODO: Rasterize `mask_group` into a stencil image and apply that stencil to each target layer.
</file context>
This PR implements the early groundwork for Document Mode as planned for community bonding / week 1 in Marquee Selection Masking.
It introduces MaskMode and wires a keyboard toggle, while intentionally avoiding rendering or visual feature work.
What this PR changes?
Scope and intent