Added handles for gradient tool (#1533)#1773
Conversation
|
Thanks for working on this! I won't have time to review this until early next week |
|
|
||
| private void UpdateCursor (PointD canvasPoint, Document document) | ||
| { | ||
| if (handle.UpdateHoverHandle (canvasPoint, out RectangleI handleDirtyRegion)) |
There was a problem hiding this comment.
I think this should ask the gradient handle for which cursor to display (similar to how the rectangle handle in the selection tool works), since that's part of the handle's UI rather than something specific to the tool
| handles = [new MoveHandle (workspace), new MoveHandle (workspace)]; | ||
| } | ||
|
|
||
| public GradientHandle PartialClone () |
There was a problem hiding this comment.
The implementation of the handle looks generally good to me, but I have a couple thoughts:
-
I'd like to have the handle be more generic, e.g. a "LineHandle", that way it's possible to reuse it for other tools in the future. For example, the rectangle handle is used by the Rectangle Select tool but will also be used in the future for the Move Selected Pixels tool to scale.
-
I'm not a fan of storing the GradientHandle in the history item, which feels like mixing up UI code with the underlying data model. I think it'd be preferable to just store the required data (e.g. the start/end positions) and then the tool can update its handle when changes to the data are made. This approach would also be easier to port to whatever the eventual future API is for re-editable tools (Non Destructive Editing - Plans and roadmap #1405)
| base.OnDeactivated (document, newTool); | ||
| } | ||
|
|
||
| private void Finalize (Document? document) |
There was a problem hiding this comment.
Are there cases where the finalization is necessary? It seems like the only effect it would have is hiding the handle, since the tool isn't leaving any uncommitted edits around that need to be flattened into the layer.
There was a problem hiding this comment.
The reason I did this is to avoid this scenario:
- I move the handles and a new history item gets added saving the previous state.
- I switch to another tool and the handles become deactivated.
- Undo. The old state gets swapped with the new state. The handles are back to there previous positions.
- Redo. The states are swapped again. The problem is in the new state handles are deactivated, meaning you lose the possibility to move them again.
Adding a finalizing history item creates a distinction between the state of the handles right after being moved and the state where they become deactivated.
Re-thinking it, a possible workaround could be making so after redoing a gradient history item, the handles are always active.
There was a problem hiding this comment.
Adding a finalizing history item creates a distinction between the state of the handles right after being moved and the state where they become deactivated
Yeah, after some more thought I think you're probably right that this is the most straightforward solution to dealing with switching tools and then possibly making some further edits. It's the same model as other tools like the text and shape tools, so we're not inventing some different UX behaviours
|
|
||
| private bool is_newly_created = false; | ||
|
|
||
| //The 'button' variable was split into two so handles can be moved with a different button to the one used when creating the gradient that defines its colors. |
There was a problem hiding this comment.
There are some undo-related issues I noticed with these.
- Left click + drag to create a gradient
- Then right click + drag to create a new reversed gradient
- Undo
- Click and drag one of the handles from the first gradient - the first gradient now becomes reversed
Related to the other comment about what should be stored in the history item, if you add some sort of "GradientData" struct to hold the necessary info it could store whether the gradient should be reversed or not
|
|
||
| private GradientData gradient_data; | ||
|
|
||
| public GradientHistoryItem (string icon, string text, ImageSurface passedUserSurface, UserLayer passedUserLayer, GradientData passedData, GradientTool passedTool) : base (icon, text) |
There was a problem hiding this comment.
Since some of this code is the same as the original SimpleHistoryItem used in the old version of the tool, I think it might be possible to avoid that duplicate code:
- You could have the gradient history item have a SimpleHistoryItem internally that it delegates to, and then you only need to implement the extra stuff specific to the gradient tool
- Or, a CompoundHistoryItem could be used to combine the two
| } | ||
| } | ||
|
|
||
| // It's a class instead of a struct so that GradientTool can have a null ref when tool hasn't been used yet and there's no undo data |
There was a problem hiding this comment.
Note it is also possible to have a nullable struct, so that is an option if it would simplify some code
| private bool is_newly_created = false; | ||
|
|
||
| //The 'button' variable was split into two so handles can be moved with a different button to the one used when creating the gradient that defines its colors. | ||
| public MouseButton color_button; |
There was a problem hiding this comment.
Would it be more clear to just store whether the gradient should be reversed, rather than the mouse button that was originally used?
|
Thanks for all the updates! I think this is looking good now to merge and get more testing |
OnDeactivatedorOnCommit