-
Notifications
You must be signed in to change notification settings - Fork 559
Add label for transaction commits for undo/redo grouping. #25938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,6 +87,21 @@ export class SchematizingSimpleTreeView< | |
| in out TRootSchema extends ImplicitFieldSchema | UnsafeUnknownSchema, | ||
| > implements TreeViewAlpha<TRootSchema>, WithBreakable | ||
| { | ||
| /** | ||
| * Optional user-provided label associated with the transaction as a commit is being applied, if provided. | ||
| * This value is intended to be read within event handlers like `commitApplied`. | ||
| * This value is cleared after each transaction to prevent providing stale/incorrect labels. | ||
| */ | ||
| private static _currentTransactionLabel: unknown | undefined; | ||
|
||
|
|
||
| public static get currentTransactionLabel(): unknown | undefined { | ||
| return this._currentTransactionLabel; | ||
| } | ||
|
|
||
| public static setCurrentTransactionLabel(label: unknown | undefined): void { | ||
| this._currentTransactionLabel = label; | ||
| } | ||
|
|
||
| /** | ||
| * This is set to undefined when this object is disposed or the view schema does not support viewing the document's stored schema. | ||
| * | ||
|
|
@@ -319,7 +334,13 @@ export class SchematizingSimpleTreeView< | |
| transactionCallbackStatus?.preconditionsOnRevert, | ||
| ); | ||
|
|
||
| this.checkout.transaction.commit(); | ||
| // Set the label before commit, and clear the label after commit. | ||
| SchematizingSimpleTreeView.setCurrentTransactionLabel(params?.label); | ||
| try { | ||
| this.checkout.transaction.commit(); | ||
| } finally { | ||
| SchematizingSimpleTreeView.setCurrentTransactionLabel(undefined); | ||
| } | ||
| return value !== undefined | ||
| ? { success: true, value: value as TSuccessValue } | ||
| : { success: true }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, there is no benefit to having a private property here and then making a getter and setter, because the getter and setter do no validation or transformation of the value.
I do think we would benefit from constraining the way that this field can be set. How about this:
Then it's impossible for any code to ever accidentally set the label and forget to clear it, or forget to properly handle errors.
The passing of
labelto thefnand the resulting generic parameter is not strictly necessary but just a nice to have, IMO.