Split widget callbacks into update and commit so only the latter adds a history state #1584

zhiyuang · 2024-01-23T03:34:48Z

Closes #972

try to fix ColorButton and NumberInput widgets.

0HyperCube

Thanks for this amazing editing improvement.

0HyperCube · 2024-01-24T17:21:15Z

editor/src/messages/layout/layout_message_handler.rs

+					return;
+				}
+
+				responses.add(DocumentMessage::StartTransaction);


It appears this is mistakenly adding undo commits when modifying the tool options (e.g. dragging the weight and stroke colour with the line tool at the top).

oh, i see. I missed this situation, maybe I need to do something like on_update callback

github-actions · 2024-01-27T11:05:18Z

📦 Build Complete for `c32103c`
https://766f7f4e.graphite.pages.dev

Keavon · 2024-01-27T11:13:17Z

I'll give this a detailed code review tomorrow, bedtime now though! Thanks @0HyperCube for the review so far.

Keavon · 2024-02-05T02:30:22Z

frontend/src/components/widgets/WidgetSpan.svelte

 		{/if}
 		{@const colorInput = narrowWidgetProps(component.props, "ColorButton")}
 		{#if colorInput}
-			<ColorButton {...exclude(colorInput)} on:value={({ detail }) => updateLayout(index, detail)} />
+			<ColorButton {...exclude(colorInput)} on:value={({ detail }) => updateLayout(index, detail)} on:start={() => commitLayout(index)} />


Can you please help me understand why we are committing the layout upon the start of the user's dragging? Why doesn't this happen upon completion of the dragging?

it also seems unreasonable to me first. After tried myself, I try to understand like this: for undo history, we always need to commit a undo history and keep a snapshot before we begin new changes. Then upon the start of dragging means commit a undo history before user begin new dragging changes(then we can properly rollback to this history).

Actually we commit the layout of a last status, not a layout changed by this widget. Maybe we could consider change a name of this.

But doesn't that mean that we don't save a history step upon completing the drag? So if you change a number from 1 to 10 by dragging its NumberInput slider, that value of 10 never gets saved as a history step? So the user could move on to other things, and if they undo several times, the history jumps back two operations at once (the operation that moved from 1 to 10, and whatever the other operation was adjacent to that).

But doesn't that mean that we don't save a history step upon completing the drag?

saving a history step upon completing the drag is a responsibility belongs to the next widget's commit_layout or next action that trigger Document.startTransaction. That's also why common widgets use updateAndCommitLayout.

So if you change a number from 1 to 10 by dragging its NumberInput slider, that value of 10 never gets saved as a history step?

e.g. if user's next action is to add a path using pen tool. During we adding a path, we will trigger Document.StartTransanction then save a history step. (Our existing history related code have handled this properly I think)

I see! I think part of my confusion is because I don't have a full understanding of how StartTransaction/AbortTransaction/CommitTransaction work (since the original author of that never documented them). I did test this all out and it seems to work well so I trust that you're right about this. I'll keep going with my code review.

CommitTransaction is a noop. StartTransaction clones the document and stores it in a list. AbortTransaction pops a document from the list and makes it current.

That's interesting that CommitTransaction is a noop. Is there a reason we have it? Do you think we should we just remove it, then find more intuitive names for the remaining two?

Your welcome to modify it.

Keavon · 2024-02-05T03:05:19Z

editor/src/messages/layout/layout_message_handler.rs

+				let layout = if let Some(layout) = self.layouts.get_mut(layout_target as usize) {
+					layout
+				} else {
+					warn!("CommitLayout was called referencing an invalid layout. `widget_id: {widget_id}`, `layout_target: {layout_target:?}`",);
+					return;
+				};
+
+				let widget_holder = if let Some(widget_holder) = layout.iter_mut().find(|widget| widget.widget_id == widget_id) {
+					widget_holder
+				} else {
+					warn!("CommitLayout was called referencing an invalid widget ID, although the layout target was valid. `widget_id: {widget_id}`, `layout_target: {layout_target:?}`",);
+					return;
+				};


These can be simplified with let else syntax.

Suggested change

let layout = if let Some(layout) = self.layouts.get_mut(layout_target as usize) {

layout

} else {

warn!("CommitLayout was called referencing an invalid layout. `widget_id: {widget_id}`, `layout_target: {layout_target:?}`",);

return;

};

let widget_holder = if let Some(widget_holder) = layout.iter_mut().find(|widget| widget.widget_id == widget_id) {

widget_holder

} else {

warn!("CommitLayout was called referencing an invalid widget ID, although the layout target was valid. `widget_id: {widget_id}`, `layout_target: {layout_target:?}`",);

return;

};

let Some(layout) = self.layouts.get_mut(layout_target as usize) else {

warn!("CommitLayout was called referencing an invalid layout. `widget_id: {widget_id}`, `layout_target: {layout_target:?}`",);

return;

};

let Some(widget_holder) = layout.iter_mut().find(|widget| widget.widget_id == widget_id) else {

warn!("CommitLayout was called referencing an invalid widget ID, although the layout target was valid. `widget_id: {widget_id}`, `layout_target: {layout_target:?}`",);

return;

};

Keavon · 2024-02-05T03:06:57Z

editor/src/messages/layout/layout_message_handler.rs

+					return;
+				};
+
+				#[remain::sorted]


I'm not a big fan of having this match statement for all the widget types exist twice (also on lines 163-291). Would you mind breaking it out into a function and combining the two use cases into one so it does the update or commit logic depending on the function's arguments?

sure, I could refactor it. But not that quick, I will find some time later today.

Keavon · 2024-02-05T09:10:18Z

I have a couple remaining improvements that I have in mind related to this feature but I think it's better to merge this today and save those as follow-ups. But here are my comments for those:

I'd like it so hovering over the entries of a DropdownMenu sends value (but not commit) updates, so dropdowns like Blend Mode could be previewed. Also, same for TextInput and TextAreaInput that should send updates as the user types but only commits when that currently happens, so some things are able to display updates live on a case-by-case basis.
This may pertain a bit to our discussion above about start, but I'd like to have color changes saved to history. So if you edit a color, once you release the mouse after dragging any of the sliders, it should send a commit at that time.

0HyperCube reviewed Jan 24, 2024

View reviewed changes

Keavon changed the title ~~Try to Split Update and Commit Layout to handle undo operations~~ Split Update and Commit Layout to avoid spamming history steps Jan 26, 2024

zhiyuang force-pushed the feature-update-commit branch from 614f909 to c32103c Compare January 26, 2024 03:08

GraphiteEditor deleted a comment from github-actions bot Jan 27, 2024

Keavon force-pushed the feature-update-commit branch from c32103c to fdea013 Compare February 5, 2024 01:48

Keavon reviewed Feb 5, 2024

View reviewed changes

Keavon changed the title ~~Split Update and Commit Layout to avoid spamming history steps~~ Split widget callbacks into update and commit so only the latter adds a history state Feb 5, 2024

zhiyuang and others added 4 commits February 5, 2024 01:19

feat: split commit and update layout

9c9abdd

feat: add on_commit callback

6d25855

Code review

e6de639

fix: refactor

d07bab3

Keavon force-pushed the feature-update-commit branch from 2acb858 to d07bab3 Compare February 5, 2024 09:20

Keavon approved these changes Feb 5, 2024

View reviewed changes

Keavon merged commit 9530e55 into GraphiteEditor:master Feb 5, 2024
2 checks passed

zhiyuang mentioned this pull request Feb 8, 2024

DropdownInput preview support and ColorButton history improvements #1598

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split widget callbacks into update and commit so only the latter adds a history state #1584

Split widget callbacks into update and commit so only the latter adds a history state #1584

zhiyuang commented Jan 23, 2024

0HyperCube left a comment

0HyperCube Jan 24, 2024

zhiyuang Jan 26, 2024 •

edited

github-actions bot commented Jan 27, 2024

Keavon commented Jan 27, 2024

Keavon Feb 5, 2024

zhiyuang Feb 5, 2024 •

edited

Keavon Feb 5, 2024

zhiyuang Feb 5, 2024 •

edited

zhiyuang Feb 5, 2024 •

edited

Keavon Feb 5, 2024

0HyperCube Feb 5, 2024

Keavon Feb 5, 2024

0HyperCube Feb 5, 2024

Keavon Feb 5, 2024 •

edited

Keavon Feb 5, 2024

zhiyuang Feb 5, 2024

Keavon commented Feb 5, 2024

Split widget callbacks into update and commit so only the latter adds a history state #1584

Split widget callbacks into update and commit so only the latter adds a history state #1584

Conversation

zhiyuang commented Jan 23, 2024

0HyperCube left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiyuang Jan 26, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Jan 27, 2024

Keavon commented Jan 27, 2024

Choose a reason for hiding this comment

zhiyuang Feb 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiyuang Feb 5, 2024 • edited

Choose a reason for hiding this comment

zhiyuang Feb 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keavon Feb 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keavon commented Feb 5, 2024

zhiyuang Jan 26, 2024 •

edited

zhiyuang Feb 5, 2024 •

edited

zhiyuang Feb 5, 2024 •

edited

zhiyuang Feb 5, 2024 •

edited

Keavon Feb 5, 2024 •

edited