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.

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

Allow moving layers using the keyboard arrow keys #7202

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

hippalectryon-0
Copy link
Contributor

@hippalectryon-0 hippalectryon-0 commented Oct 25, 2024

Summary

When the move tool is selected, this PR allows to use the keyboard keys to move the selected layer by 1px.

Related Issues / Discussions

#7120

Known issues

  • The layer moves even if it's disabled fixed
  • If we are on another menu, then we have double inputs, e.g. if we click on the "Layers" tab then pressing right arrow moves the selected layer AND changes the tab from "Layers" to "Gallery" -> Probably an issue with the focus manager itself

Checklist

  • The PR has a short but descriptive title, suitable for a changelog

@github-actions github-actions bot added the frontend PRs that change frontend files label Oct 25, 2024
Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Thanks! Users will be very pleased to have this feature - its been requested quite a few times.

I noticed two issues, and suggested some changes for efficiency and clarity.

Comment on lines +34 to +40
const OFFSET = 1; // How much to move, in px
const offsets: Record<string, { x: number; y: number }> = {
ArrowLeft: { x: -OFFSET, y: 0 },
ArrowRight: { x: OFFSET, y: 0 },
ArrowUp: { x: 0, y: -OFFSET },
ArrowDown: { x: 0, y: OFFSET },
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move these constants outside the callback, either to class property or outside the class, so we don't need to recreate them every keydown

};
const { key } = e;
const selectedEntity = this.manager.stateApi.getSelectedEntityAdapter();
const { x: offsetX = 0, y: offsetY = 0 } = offsets[key] || {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's clearer to do this:

const offsets = KEY_TO_OFFSETS[e.key]
if (!offsets) {
  return
}

// ...

selectedEntity.transformer.nudgePosition(offset.x, offset.y);

const { x: offsetX = 0, y: offsetY = 0 } = offsets[key] || {};

if (
!(selectedEntity && selectedEntity.$isInteractable.get() && $focusedRegion.get() === 'canvas') ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Getting the selected entity is the most expensive part check, so we can be a bit more verbose to improve perf when checking if we should bail early. For example:

if ($focusedRegion.get() !== 'canvas') {
  return;
}
const offsets = KEY_TO_OFFSETS[e.key];
if (!offsets) {
  return;
}
const selectedEntity = this.manager.stateApi.getSelectedEntityAdapter();
if (!selectedEntity || !selectedEntity.$isInteractable.get()) {
  return;
}

Also, I forgot to mention we should also bail if this.manager.$isBusy.get() === true. This flag indicates that something else is happening on the canvas.

Copy link
Collaborator

@psychedelicious psychedelicious Oct 29, 2024

Choose a reason for hiding this comment

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

This doesn't work quite how we need it to, because an entity's objects are positioned relative to the entity's position. For example:
Screenshot 2024-10-29 at 1 11 51 pm

Here, the entity position is 0,0, but the objects - represented by the interaction proxy rect - are at a different position.

As a result, when we use do a single-pixel nudge, we offset the entity by the object position + the single-pixel nudge offsets.

Screen.Recording.2024-10-29.at.1.24.59.pm.mov

Instead of handling this with logic in the transformer class, I think it's best to create a redux action specifically for moving an entity by a certain amount. Proposal:

Optionally, you could do something like this to immediately move the entity and its objects before waiting for the redux round-trip (untested):

nudgePosition = (offset: Coordinate) => {

  //#region Optional perf optimization
  
  // We can immediately move both the proxy rect and layer objects so we don't have to wait for a redux round-trip,
  // which can take up to 2ms in my testing. This is optional, but can make the interaction feel more responsive,
  // especially on lower-end devices.
  // Get the relative position of the layer's objects, according to konva
  const position = this.konva.proxyRect.position();
  // Offset the position by the nudge amount
  const newPosition = offsetCoord(position, offset);
  // Set the new position of the proxy rect - this doesn't move the layer objects - only the outline rect
  this.konva.proxyRect.setAttrs(newPosition);
  // Sync the layer objects with the proxy rect - moves them to the new position
  this.syncObjectGroupWithProxyRect();
  
  //#endregion
  
  // Push to redux. The state change will do a round-trip, and eventually make it back to the canvas classes, at
  // which point the layer will be moved to the new position.
  this.manager.stateApi.moveEntityBy({ entityIdentifier: this.parent.entityIdentifier, offset });
};

@psychedelicious psychedelicious mentioned this pull request Nov 21, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend PRs that change frontend files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants