Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Single-update undo/redo #1374

Merged
merged 25 commits into from
Dec 20, 2022
Merged

Single-update undo/redo #1374

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1575413
Use drafts, remove throttling
apedroferreira Nov 22, 2022
ee45df3
Forgot one dom
apedroferreira Nov 22, 2022
9a8d071
Merge remote-tracking branch 'origin/master' into atomic-undo-redo
apedroferreira Nov 23, 2022
a3bb650
Fix types
apedroferreira Nov 23, 2022
28d5799
Try not using nested functions
apedroferreira Nov 23, 2022
6b332f7
Atomic column normalization
apedroferreira Nov 23, 2022
4c667d8
Use draft page nodes
apedroferreira Nov 23, 2022
65e7e7f
Fix element removal
apedroferreira Nov 24, 2022
a157576
Merge remote-tracking branch 'origin/master' into atomic-undo-redo
apedroferreira Nov 24, 2022
ea42bfb
Fix moving elements
apedroferreira Nov 24, 2022
4ae076d
Re-add throttling
apedroferreira Nov 28, 2022
d5f6b69
Remove some domAPI methods
apedroferreira Nov 29, 2022
6bc3985
Merge remote-tracking branch 'origin/master' into atomic-undo-redo
apedroferreira Nov 30, 2022
0237579
Fix type errors
apedroferreira Nov 30, 2022
d7a5664
Hide selected node while dragging new node
apedroferreira Dec 2, 2022
1e3722e
Merge remote-tracking branch 'origin/master' into atomic-undo-redo
apedroferreira Dec 2, 2022
892e2ec
Fix not being able to drop in selected node when dragging new node
apedroferreira Dec 2, 2022
203070d
Merge remote-tracking branch 'origin/master' into atomic-undo-redo
apedroferreira Dec 12, 2022
34f59a1
Merge remote-tracking branch 'origin/master' into atomic-undo-redo
apedroferreira Dec 14, 2022
baabb44
Remove batching test
apedroferreira Dec 14, 2022
ab64a1c
Debounce text input changes (#1459)
apedroferreira Dec 15, 2022
8c10a84
Merge remote-tracking branch 'origin/master' into atomic-undo-redo
apedroferreira Dec 15, 2022
1f7779e
Use updater function in DOM API updates
apedroferreira Dec 16, 2022
5d1fd30
Readd method
apedroferreira Dec 16, 2022
1d8b49e
Merge remote-tracking branch 'origin/master' into atomic-undo-redo
apedroferreira Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/toolpad-app/src/appDom/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,13 +714,13 @@ export function setQueryProp<Q, K extends keyof Q>(
export function setNodeNamespacedProp<
Node extends AppDomNode,
Namespace extends PropNamespaces<Node>,
Prop extends keyof Node[Namespace] & string,
Prop extends keyof NonNullable<Node[Namespace]> & string,
>(
dom: AppDom,
node: Node,
namespace: Namespace,
prop: Prop,
value: Node[Namespace][Prop] | null,
value: NonNullable<Node[Namespace]>[Prop] | null,
): AppDom {
if (value) {
return update(dom, {
Expand All @@ -736,7 +736,7 @@ export function setNodeNamespacedProp<
return update(dom, {
nodes: update(dom.nodes, {
[node.id]: update(node, {
[namespace]: omit(node[namespace], prop) as Partial<Node[Namespace]>,
[namespace]: omit(node[namespace]!, prop) as Partial<Node[Namespace]>,
} as Partial<Node>),
}),
});
Expand Down
7 changes: 5 additions & 2 deletions packages/toolpad-app/src/toolpad/AppEditor/ConnectionEditor/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,21 @@ function ConnectionEditorContent<P>({
className,
connectionNode,
}: ConnectionEditorContentProps<P>) {
const { dom } = useDom();
const domApi = useDomApi();

const handleConnectionChange = React.useCallback(
(connectionParams: P | null) => {
domApi.setNodeNamespacedProp(
const updatedDom = appDom.setNodeNamespacedProp(
dom,
connectionNode,
'attributes',
'params',
appDom.createSecret(connectionParams),
);
domApi.update(updatedDom);
},
[connectionNode, domApi],
[connectionNode, dom, domApi],
Copy link
Member

@Janpot Janpot Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙂 I think this is why I suggested to use a function in domApi.update. This callback invalidates on every dom change, and it's also not 100% sure it will get the last dom. e.g. imagine a function:

const addConnection = React.useCallback(() => domApi.update(appDom.addConnection(dom)), [dom]);

// somewhere else, this function called twice
addConnection();
addConnection();

Those would get the same initial version of dom, meaning only one connection is added. While I would expect to have two connections. It would be different if

const addConnection = React.useCallback(
  () => domApi.update((draft) => appDom.addConnection(draft)),
  [],
);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it work to restore and use methods such as domApi.setNodeNamespacedProp (which I had deleted) in cases like this?
And use domApi.update as I'm using it but only when a temporary DOM is needed? domApi.update replaces the whole DOM anyway so in those cases maybe it's fine?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still thinking of better alternatives, your proposal does seem more "atomic", it's just more difficult to use in RenderOverlay.

Copy link
Member Author

@apedroferreira apedroferreira Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try supporting domApi.update((draft) => ... and use it as much as possible where I can, except in the few cases where it's hard to use, I think that should be possible.

Copy link
Member

@Janpot Janpot Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still thinking of better alternatives, your proposal does seem more "atomic", it's just more difficult to use in RenderOverlay.

We can support both methods, like

update: (draft: React.SetStateAction<AppDom>) => void

Also, wouldn't necessarily recommend it, but nothing prevents you from doing:

const addConnection = React.useCallback(
  () => domApi.update(() => appDom.addConnection(dom)),
  [dom],
);

Copy link
Member Author

@apedroferreira apedroferreira Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I was able to use an updater function everywhere after all!
Commit: 1f7779e
After the refactoring I had made in RenderOverlay I guess it wasn't so difficult there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this PR is using the updater function now I will merge this part already!

);

const dataSourceId = connectionNode.attributes.dataSource.value;
Expand Down
5 changes: 4 additions & 1 deletion ...ges/toolpad-app/src/toolpad/AppEditor/HierarchyExplorer/CreateCodeComponentNodeDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,10 @@ export default function CreateCodeComponentDialog({
},
});
const appNode = appDom.getApp(dom);
domApi.addNode(newNode, appNode, 'codeComponents');

const updatedDom = appDom.addNode(dom, newNode, appNode, 'codeComponents');
domApi.update(updatedDom);

onClose();
navigate(`/app/${appId}/codeComponents/${newNode.id}`);
}}
Expand Down
5 changes: 4 additions & 1 deletion packages/toolpad-app/src/toolpad/AppEditor/HierarchyExplorer/CreateConnectionNodeDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ export default function CreateConnectionDialog({
},
});
const appNode = appDom.getApp(dom);
domApi.addNode(newNode, appNode, 'connections');

const updatedDom = appDom.addNode(dom, newNode, appNode, 'connections');
domApi.update(updatedDom);

onClose();
navigate(`/app/${appId}/connections/${newNode.id}`);
}}
Expand Down
4 changes: 3 additions & 1 deletion packages/toolpad-app/src/toolpad/AppEditor/HierarchyExplorer/CreatePageNodeDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ export default function CreatePageDialog({
},
});
const appNode = appDom.getApp(dom);
domApi.addNode(newNode, appNode, 'pages');

const updatedDom = appDom.addNode(dom, newNode, appNode, 'pages');
domApi.update(updatedDom);

onClose();
navigate(`/app/${appId}/pages/${newNode.id}`);
Expand Down
7 changes: 5 additions & 2 deletions packages/toolpad-app/src/toolpad/AppEditor/HierarchyExplorer/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,8 @@ export default function HierarchyExplorer({ appId, className }: HierarchyExplore
}
}

domApi.removeNode(nodeId);
const updatedDom = appDom.removeNode(dom, nodeId);
domApi.update(updatedDom);

if (redirectAfterDelete) {
navigate(redirectAfterDelete);
Expand All @@ -255,7 +256,9 @@ export default function HierarchyExplorer({ appId, className }: HierarchyExplore
);

const fragment = appDom.cloneFragment(dom, nodeId);
domApi.addFragment(fragment, node.parentId, node.parentProp);

const updatedDom = appDom.addFragment(dom, fragment, node.parentId, node.parentProp);
domApi.update(updatedDom);

const newNode = appDom.getNode(fragment, fragment.root);
const editorLink = getLinkToNodeEditor(appId, newNode);
Expand Down
4 changes: 1 addition & 3 deletions packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentCatalog/ComponentCatalog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import invariant from 'invariant';
import ComponentCatalogItem from './ComponentCatalogItem';
import CreateCodeComponentNodeDialog from '../../HierarchyExplorer/CreateCodeComponentNodeDialog';
import * as appDom from '../../../../appDom';
import { useDom, useDomApi } from '../../../DomLoader';
import { useDom } from '../../../DomLoader';
import { usePageEditorApi, usePageEditorState } from '../PageEditorProvider';
import { useToolpadComponents } from '../../toolpadComponents';
import useLocalStorageState from '../../../../utils/useLocalStorageState';
Expand Down Expand Up @@ -49,7 +49,6 @@ export default function ComponentCatalog({ className }: ComponentCatalogProps) {
const api = usePageEditorApi();
const pageState = usePageEditorState();
const { dom } = useDom();
const domApi = useDomApi();

const [openStart, setOpenStart] = React.useState(0);
const [openCustomComponents, setOpenCustomComponents] = useLocalStorageState(
Expand Down Expand Up @@ -90,7 +89,6 @@ export default function ComponentCatalog({ className }: ComponentCatalogProps) {
const handleDragStart = (componentType: string) => (event: React.DragEvent<HTMLElement>) => {
event.dataTransfer.dropEffect = 'copy';
const newNode = appDom.createElement(dom, componentType, {});
domApi.deselectNode();
api.newNodeDragStart(newNode);
closeDrawer(0);
};
Expand Down
8 changes: 5 additions & 3 deletions packages/toolpad-app/src/toolpad/AppEditor/PageEditor/NodeAttributeEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';
import { ArgTypeDefinition, BindableAttrValue } from '@mui/toolpad-core';
import { Alert } from '@mui/material';
import * as appDom from '../../../appDom';
import { useDomApi } from '../../DomLoader';
import { useDom, useDomApi } from '../../DomLoader';
import BindableEditor from './BindableEditor';
import { usePageEditorState } from './PageEditorProvider';
import { getDefaultControl } from '../../propertyControls';
Expand All @@ -22,13 +22,15 @@ export default function NodeAttributeEditor<P extends object>({
argType,
props,
}: NodeAttributeEditorProps<P>) {
const { dom } = useDom();
const domApi = useDomApi();

const handlePropChange = React.useCallback(
(newValue: BindableAttrValue<unknown> | null) => {
domApi.setNodeNamespacedProp(node, namespace as any, name, newValue);
const updatedDom = appDom.setNodeNamespacedProp(dom, node, namespace as any, name, newValue);
domApi.update(updatedDom);
},
[domApi, node, namespace, name],
[dom, node, namespace, name, domApi],
);

const propValue: BindableAttrValue<unknown> | null = (node as any)[namespace]?.[name] ?? null;
Expand Down
12 changes: 10 additions & 2 deletions packages/toolpad-app/src/toolpad/AppEditor/PageEditor/PageModuleEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,16 @@ function PageModuleEditorDialog({ pageNodeId, open, onClose }: PageModuleEditorD
const handleSave = React.useCallback(() => {
const pretty = tryFormat(input);
setInput(pretty);
domApi.setNodeNamespacedProp(page, 'attributes', 'module', appDom.createConst(pretty));
}, [domApi, input, page]);

const updatedDom = appDom.setNodeNamespacedProp(
dom,
page,
'attributes',
'module',
appDom.createConst(pretty),
);
domApi.update(updatedDom);
}, [dom, domApi, input, page]);

const handleSaveButton = React.useCallback(() => {
handleSave();
Expand Down
9 changes: 6 additions & 3 deletions packages/toolpad-app/src/toolpad/AppEditor/PageEditor/QueryEditor/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ export default function QueryEditor() {
if (appDom.nodeExists(dom, node.id)) {
domApi.saveNode(node);
} else {
domApi.addNode(node, page, 'queries');
const updatedDom = appDom.addNode(dom, node, page, 'queries');
domApi.update(updatedDom);
}
setDialogState({ node, isDraft: false });
},
Expand All @@ -141,10 +142,12 @@ export default function QueryEditor() {

const handleDeleteNode = React.useCallback(
(nodeId: NodeId) => {
domApi.removeNode(nodeId);
const updatedDom = appDom.removeNode(dom, nodeId);
domApi.update(updatedDom);

handleEditStateDialogClose();
},
[domApi, handleEditStateDialogClose],
[dom, domApi, handleEditStateDialogClose],
);

const handleRemove = React.useCallback(
Expand Down
Loading