Undo redo improvements

newIDE/app/src/EmbeddedGame/EmbeddedGameFrame.js

@@ -110,6 +110,7 @@ let onSetCameraState:
 let onChangeViewPosition:
   | null
   | ((command: ChangeViewPositionCommand) => void) = null;
+let onFocusEmbeddedGameFrame: null | (() => boolean) = null;
 
 export const setEmbeddedGameFramePreviewLocation = ({
   previewIndexHtmlLocation,
@@ -164,6 +165,15 @@ export const changeViewPosition = (command: ChangeViewPositionCommand) => {
   onChangeViewPosition(command);
 };
 
+/**
+ * Give the keyboard focus to the embedded game frame, so the in-game
+ * editor shortcuts work. Return false if there is no frame to focus.
+ */
+export const focusEmbeddedGameFrame = (): boolean => {
+  if (!onFocusEmbeddedGameFrame) return false;
+  return onFocusEmbeddedGameFrame();
+};
+
 const logSwitchingInfo = ({
   editorId,
   sceneName,
@@ -492,6 +502,12 @@ export const EmbeddedGameFrame = ({
             });
           });
       };
+      onFocusEmbeddedGameFrame = () => {
+        const iframe = iframeRef.current;
+        if (!iframe || !iframe.contentWindow) return false;
+        iframe.contentWindow.focus();
+        return true;
+      };
     },
     [
       previewDebuggerServer,

newIDE/app/src/InstancesEditor/InstancesRenderer/index.js

@@ -192,6 +192,57 @@ export default class InstancesRenderer {
     return this._basicProfilingCounters;
   }
 
+  _getOrCreateLayerRenderer(
+    layer: gdLayer,
+    // $FlowFixMe[value-as-type]
+    pixiRenderer: PIXI.Renderer
+  ): LayerRenderer {
+    const layerName = layer.getName();
+    let layerRenderer = this.layersRenderers[layerName];
+    if (!layerRenderer) {
+      this.layersRenderers[layerName] = layerRenderer = new LayerRenderer({
+        project: this.project,
+        globalObjectsContainer: this.globalObjectsContainer,
+        objectsContainer: this.objectsContainer,
+        instances: this.instances,
+        viewPosition: this.viewPosition,
+        layer: layer,
+        onInstanceClicked: this.onInstanceClicked,
+        onInstanceRightClicked: this.onInstanceRightClicked,
+        onInstanceDoubleClicked: this.onInstanceDoubleClicked,
+        onOverInstance: this.onOverInstance,
+        onOutInstance: this.onOutInstance,
+        onMoveInstance: this.onMoveInstance,
+        onMoveInstanceEnd: this.onMoveInstanceEnd,
+        onDownInstance: this.onDownInstance,
+        onUpInstance: this.onUpInstance,
+        pixiRenderer: pixiRenderer,
+        showObjectInstancesIn3D: this._showObjectInstancesIn3D,
+      });
+      this.pixiContainer.addChild(layerRenderer.getPixiContainer());
+    }
+    return layerRenderer;
+  }
+
+  /**
+   * Create the renderers of the layers that don't have one yet. They are
+   * normally created lazily at the first render: call this to measure
+   * instances (see `getInstanceMeasurer`) before it happened - for example
+   * right after the renderers were remounted, as the measurer falls back
+   * to a zero-sized rectangle at the instance origin without them.
+   */
+  ensureLayerRenderersExist(
+    // $FlowFixMe[value-as-type]
+    pixiRenderer: PIXI.Renderer
+  ) {
+    for (let i = 0; i < this.layersContainer.getLayersCount(); i++) {
+      this._getOrCreateLayerRenderer(
+        this.layersContainer.getLayerAt(i),
+        pixiRenderer
+      );
+    }
+  }
+
   render(
     // $FlowFixMe[value-as-type]
     pixiRenderer: PIXI.Renderer,
@@ -232,31 +283,8 @@ export default class InstancesRenderer {
 
     for (let i = 0; i < this.layersContainer.getLayersCount(); i++) {
       const layer = this.layersContainer.getLayerAt(i);
-      const layerName = layer.getName();
-
-      let layerRenderer = this.layersRenderers[layerName];
-      if (!layerRenderer) {
-        this.layersRenderers[layerName] = layerRenderer = new LayerRenderer({
-          project: this.project,
-          globalObjectsContainer: this.globalObjectsContainer,
-          objectsContainer: this.objectsContainer,
-          instances: this.instances,
-          viewPosition: this.viewPosition,
-          layer: layer,
-          onInstanceClicked: this.onInstanceClicked,
-          onInstanceRightClicked: this.onInstanceRightClicked,
-          onInstanceDoubleClicked: this.onInstanceDoubleClicked,
-          onOverInstance: this.onOverInstance,
-          onOutInstance: this.onOutInstance,
-          onMoveInstance: this.onMoveInstance,
-          onMoveInstanceEnd: this.onMoveInstanceEnd,
-          onDownInstance: this.onDownInstance,
-          onUpInstance: this.onUpInstance,
-          pixiRenderer: pixiRenderer,
-          showObjectInstancesIn3D: this._showObjectInstancesIn3D,
-        });
-        this.pixiContainer.addChild(layerRenderer.getPixiContainer());
-      }
+
+      const layerRenderer = this._getOrCreateLayerRenderer(layer, pixiRenderer);
 
       // /!\ Objects representing layers can be deleted at any moment and replaced
       // by new one, for example when two layers are swapped.

newIDE/app/src/InstancesEditor/index.js

@@ -1771,6 +1771,12 @@ export default class InstancesEditor extends Component<Props, State> {
   ) => {
     if (instances.length === 0) return;
 
+    // Centering can be requested before the first render created the layer
+    // renderers (for example right after an undo/redo remounted them), in
+    // which case the measured rectangle would collapse to the instance
+    // origin (its top-left corner most of the time) instead of its center.
+    this.instancesRenderer.ensureLayerRenderersExist(this.pixiRenderer);
+
     const instanceMeasurer = this.instancesRenderer.getInstanceMeasurer();
     let lastInstanceRectangle = instanceMeasurer.getInstanceAABB(
       instances[instances.length - 1],
@@ -1780,6 +1786,84 @@ export default class InstancesEditor extends Component<Props, State> {
     if (offset) this.scrollBy(offset[0], offset[1]);
   };
 
+  /**
+   * Scroll the view by the smallest amount to make the last instance
+   * visible (with a margin), so the view does a small jump towards it -
+   * keeping the direction of the movement - rather than re-centering on it.
+   */
+  scrollViewToLastInstance = (instances: Array<gdInitialInstance>) => {
+    if (instances.length === 0) return;
+
+    // See `centerViewOnLastInstance` about why this is needed.
+    this.instancesRenderer.ensureLayerRenderersExist(this.pixiRenderer);
+    const aabb = this.instancesRenderer
+      .getInstanceMeasurer()
+      .getInstanceAABB(instances[instances.length - 1], new Rectangle());
+    this._scrollViewMinimallyToRectangle(aabb);
+  };
+
+  /**
+   * Scroll the view by the smallest amount to make the given point visible
+   * (with a margin).
+   */
+  scrollViewToPoint = (x: number, y: number) => {
+    this._scrollViewMinimallyToRectangle(new Rectangle(x, y, x, y));
+  };
+
+  _scrollViewMinimallyToRectangle = (rectangle: Rectangle) => {
+    const [viewLeft, viewTop] = this.viewPosition.toSceneCoordinates(0, 0);
+    const [viewRight, viewBottom] = this.viewPosition.toSceneCoordinates(
+      this.viewPosition.getWidth(),
+      this.viewPosition.getHeight()
+    );
+    // Keep a margin so the revealed target is not stuck to the view border.
+    const marginX = (viewRight - viewLeft) * 0.1;
+    const marginY = (viewBottom - viewTop) * 0.1;
+
+    // Only scroll on an axis where the rectangle is completely out of the
+    // view: an axis where it's already (even partly) visible must not
+    // move, so the jump keeps the direction of the target.
+    let scrollX = 0;
+    if (rectangle.right < viewLeft) {
+      scrollX = rectangle.left - (viewLeft + marginX);
+    } else if (rectangle.left > viewRight) {
+      scrollX = rectangle.right - (viewRight - marginX);
+    }
+    let scrollY = 0;
+    if (rectangle.bottom < viewTop) {
+      scrollY = rectangle.top - (viewTop + marginY);
+    } else if (rectangle.top > viewBottom) {
+      scrollY = rectangle.bottom - (viewBottom - marginY);
+    }
+    if (scrollX || scrollY) this.scrollBy(scrollX, scrollY);
+  };
+
+  /**
+   * Return true if any part of the instance is currently visible in the
+   * viewport.
+   */
+  isInstanceVisibleInViewport = (instance: gdInitialInstance): boolean => {
+    // The layer renderers (needed to measure instances) are created lazily
+    // at the first render: ensure they exist (this can be called right
+    // after a remount, before any render).
+    this.instancesRenderer.ensureLayerRenderersExist(this.pixiRenderer);
+    const aabb = this.instancesRenderer
+      .getInstanceMeasurer()
+      .getInstanceAABB(instance, new Rectangle());
+
+    const viewTopLeft = this.viewPosition.toSceneCoordinates(0, 0);
+    const viewBottomRight = this.viewPosition.toSceneCoordinates(
+      this.viewPosition.getWidth(),
+      this.viewPosition.getHeight()
+    );
+    return (
+      aabb.right >= viewTopLeft[0] &&
+      aabb.left <= viewBottomRight[0] &&
+      aabb.bottom >= viewTopLeft[1] &&
+      aabb.top <= viewBottomRight[1]
+    );
+  };
+
   getLastContextMenuSceneCoordinates = (): any => {
     return this.viewPosition.toSceneCoordinates(
       this.lastContextMenuX,

newIDE/app/src/ObjectEditor/CompactObjectPropertiesEditor/index.js

@@ -246,7 +246,6 @@ export const CompactObjectPropertiesEditor = ({
   projectScopedContainersAccessor,
   unsavedChanges,
   i18n,
-  historyHandler,
   objects,
   onEditObject,
   onObjectsModified,
@@ -871,7 +870,10 @@ export const CompactObjectPropertiesEditor = ({
                         )
                       : []
                   }
-                  historyHandler={historyHandler}
+                  // Don't give the scene editor `historyHandler`: it tracks
+                  // the scene (instances, layers...), not the objects — it
+                  // would record no-op undo steps. Without it, the list
+                  // falls back on its own local history.
                   onVariablesUpdated={onVariablesUpdated}
                   toolbarIconStyle={styles.icon}
                   compactEmptyPlaceholderText={

newIDE/app/src/SceneEditor/CompactScenePropertiesEditor/index.js

@@ -75,6 +75,7 @@ type Props = {|
   unsavedChanges?: ?UnsavedChanges,
   i18n: I18nType,
   historyHandler?: HistoryHandler,
+  onScenePropertiesModified?: () => void,
 |};
 
 export const CompactScenePropertiesEditor = ({
@@ -87,6 +88,7 @@ export const CompactScenePropertiesEditor = ({
   unsavedChanges,
   i18n,
   historyHandler,
+  onScenePropertiesModified,
 }: Props): React.Node => {
   const forceUpdate = useForceUpdate();
   const variablesListRef = React.useRef<?VariablesListInterface>(null);
@@ -201,9 +203,7 @@ export const CompactScenePropertiesEditor = ({
                   project={project}
                   schema={propertiesSchema}
                   instances={[scene]}
-                  onInstancesModified={() => {
-                    // TODO: undo/redo?
-                  }}
+                  onInstancesModified={onScenePropertiesModified || noop}
                   resourceManagementProps={resourceManagementProps}
                   placeholder=""
                   onRefreshAllFields={forceRecomputeSchema}

newIDE/app/src/SceneEditor/DialogUndoShortcutPropagation.spec.js

@@ -0,0 +1,104 @@
+/**
+ * @jest-environment jsdom
+ * @flow
+ */
+import * as React from 'react';
+import { createRoot } from 'react-dom/client';
+import { act } from 'react-dom/test-utils';
+import Dialog from '@material-ui/core/Dialog';
+
+// The scene editor listens to `keydown` on its root div to catch undo/redo
+// shortcuts (see `_onKeyDownInEditor` in `SceneEditor/index.js`). Dialogs are
+// rendered in portals: their DOM lives outside of the root div, but React
+// synthetic events propagate through the REACT tree, not the DOM tree - so a
+// shortcut pressed inside a dialog DOES reach the root div handler.
+// These tests document this behavior and the DOM containment check
+// (`container.contains(event.target)`) used by `_onKeyDownInEditor` to
+// distinguish the two cases.
+describe('keydown propagation from a dialog to a parent onKeyDown', () => {
+  let container: HTMLDivElement;
+  let root;
+
+  beforeEach(() => {
+    container = document.createElement('div');
+    if (!document.body) throw new Error('No document body');
+    document.body.appendChild(container);
+  });
+
+  afterEach(() => {
+    act(() => {
+      if (root) root.unmount();
+    });
+    container.remove();
+  });
+
+  const dispatchCtrlZOn = (element: HTMLElement) => {
+    act(() => {
+      element.focus();
+      element.dispatchEvent(
+        new KeyboardEvent('keydown', {
+          key: 'z',
+          ctrlKey: true,
+          bubbles: true,
+          cancelable: true,
+        })
+      );
+    });
+  };
+
+  it('reaches the parent handler from an element inside the div, which contains the event target', () => {
+    const receivedEvents = [];
+    const onKeyDown = (event: SyntheticKeyboardEvent<HTMLElement>) =>
+      receivedEvents.push({
+        containsTarget:
+          event.target instanceof Node &&
+          event.currentTarget.contains(event.target),
+      });
+    act(() => {
+      root = createRoot(container);
+      root.render(
+        <div onKeyDown={onKeyDown} tabIndex={-1}>
+          <button id="in-div-button">In div</button>
+        </div>
+      );
+    });
+
+    const button = document.getElementById('in-div-button');
+    if (!button) throw new Error('Button not found');
+    dispatchCtrlZOn(button);
+
+    expect(receivedEvents).toEqual([{ containsTarget: true }]);
+  });
+
+  it('reaches the parent handler from inside a portaled dialog, but the div does NOT contain the event target', () => {
+    const receivedEvents = [];
+    const onKeyDown = (event: SyntheticKeyboardEvent<HTMLElement>) =>
+      receivedEvents.push({
+        containsTarget:
+          event.target instanceof Node &&
+          event.currentTarget.contains(event.target),
+      });
+    act(() => {
+      root = createRoot(container);
+      root.render(
+        <div onKeyDown={onKeyDown} tabIndex={-1}>
+          <Dialog open transitionDuration={0} disableEnforceFocus>
+            <button id="dialog-button">In dialog</button>
+          </Dialog>
+        </div>
+      );
+    });
+
+    const button = document.getElementById('dialog-button');
+    if (!button) throw new Error('Dialog button not found');
+    dispatchCtrlZOn(button);
+
+    // The synthetic event DOES reach the parent handler (React tree
+    // propagation): without a guard, shortcuts pressed in a dialog would
+    // wrongly trigger the editor ones.
+    expect(receivedEvents.length).toBe(1);
+    // ...but the DOM containment check tells the two cases apart (the
+    // dialog DOM is in a portal, outside of the div).
+    expect(receivedEvents[0].containsTarget).toBe(false);
+  });
+});

newIDE/app/src/SceneEditor/EditorsDisplay.flow.js

@@ -44,6 +44,7 @@ export type SceneEditorsDisplayProps = {|
     targetPosition?: 'center' | 'upperCenter'
   ) => void,
   onInstancesModified?: (Array<gdInitialInstance>) => void,
+  onScenePropertiesModified?: () => void,
   editInstanceVariables: (instance: ?gdInitialInstance) => void,
   editObjectByName: ({
     objectName: string,
@@ -198,6 +199,9 @@ export type SceneEditorsDisplayInterface = {|
       Array<gdInitialInstance>,
       offset?: ?[number, number]
     ) => void,
+    isInstanceVisibleInViewport: gdInitialInstance => boolean,
+    scrollViewToLastInstance: (Array<gdInitialInstance>) => void,
+    scrollViewToPoint: (x: number, y: number) => void,
     getLastCursorSceneCoordinates: () => [number, number],
     getLastContextMenuSceneCoordinates: () => [number, number],
     getViewPosition: () => ?ViewPosition,