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

Feature: select tool extension feature #46

Merged
merged 43 commits into from
Jul 15, 2023
Merged

Conversation

hunkim98
Copy link
Owner

@hunkim98 hunkim98 commented Jul 8, 2023

🚀 [Related Issue: #43]

Preview

Screen.Recording.2023-07-08.at.8.58.01.AM.mov

Changes

Allow extension (sideways & diagonally) for selected area

  • [🎨Component] Detect extension direction for selected area

    • To make extension happen for selected area, I have added an algorithm to detect the extension direction when the user's mouse is down.
  • [🎨Component] Add algorithm for extending the selected area

    • I have deeply thought about the correct algorithm for extending the selected area. After much thought, I thought of a way to set an originPixelIndex and made each pixel in the area to extend according to the distance with the originPixelIndex.
    • Refer to src/utils/position.ts getOverlappingPixelIndicesForModifiedPixels to check how I made the algorithm to find the pixels to color for the extended area
    • If you see the code, I calculate each pixel's pixel-offset distance to the origin pixel index. Then I calculate the corner positions for each pixel. After I get the corner positions, I find the pixel indices that overlap with the corner positions
      export const getOverlappingPixelIndicesForModifiedPixels = (
      originalPixels: Array<ColorChangeItem>,
      originPixelIndex: { rowIndex: number; columnIndex: number },
      modifyPixelWidthRatio: number,
      modifyPixelHeightRatio: number,
      gridSquareLength: number,
      ) => {
      const pixelsToColor: Array<ColorChangeItem> = [];
      for (const item of originalPixels) {
      const pixelDistanceFromOrigin = {
      rowOffset: item.rowIndex - originPixelIndex.rowIndex,
      columnOffset: item.columnIndex - originPixelIndex.columnIndex,
      };
      const pixelWordPosOffset = {
      x: pixelDistanceFromOrigin.columnOffset * gridSquareLength,
      y: pixelDistanceFromOrigin.rowOffset * gridSquareLength,
      };
      const cornerWorldPos = {
      topLeft: {
      x: pixelWordPosOffset.x * modifyPixelWidthRatio,
      y: pixelWordPosOffset.y * modifyPixelHeightRatio,
      },
      topRight: {
      x: (pixelWordPosOffset.x + gridSquareLength) * modifyPixelWidthRatio,
      y: pixelWordPosOffset.y * modifyPixelHeightRatio,
      },
      bottomLeft: {
      x: pixelWordPosOffset.x * modifyPixelWidthRatio,
      y: (pixelWordPosOffset.y + gridSquareLength) * modifyPixelHeightRatio,
      },
      bottomRight: {
      x: (pixelWordPosOffset.x + gridSquareLength) * modifyPixelWidthRatio,
      y: (pixelWordPosOffset.y + gridSquareLength) * modifyPixelHeightRatio,
      },
      };
      for (
      let i = cornerWorldPos.topLeft.x;
      i < cornerWorldPos.topRight.x;
      i += gridSquareLength
      ) {
      for (
      let j = cornerWorldPos.topLeft.y;
      j < cornerWorldPos.bottomLeft.y;
      j += gridSquareLength
      ) {
      const pixelIndex = {
      rowIndex: Math.round(
      originPixelIndex.rowIndex + Math.floor(j / gridSquareLength),
      ),
      columnIndex: Math.round(
      originPixelIndex.columnIndex + Math.floor(i / gridSquareLength),
      ),
      color: item.color,
      previousColor: item.previousColor,
      };
      // console.log(originPixelIndex, "originPixelIndex");
      // console.log(
      // pixelIndex,
      // "pixelIndex",
      // );
      pixelsToColor.push(pixelIndex);
      }
      }
      }
      return pixelsToColor;
      };
  • [🎨Component] Add event listeners for key down and key up

    • When using extension with select tool, most people expect to extend with an 'Alt' key pressed. The 'Alt' key is commonly perceived to make extension happen for both sides simultaneously. To make this happen, I first needed to let the Dotting component to listen to key events.
    • Adding key event listeners to the canvas element did not seem to work well. Thus, I found a way to instead put the event listener inside the parent div element.
      return (
      <div
      style={{
      width: props.width,
      height: props.height,
      position: "relative",
      outline: "none",
      }}
      ref={containerRef}
      tabIndex={1}
      onMouseDown={() => {
      containerRef.current?.focus();
      }}
      onKeyDown={e => {
      editor?.onKeyDown(e);
      }}
      onKeyUp={e => {
      editor?.onKeyUp(e);
      }}
      >
  • [🔗Other] Add tests for extension select area

    • Since extending select area is a new feature, it needs to be tested. I created some tests for the select area extension functionality. This is the first unit test I have created for Dotting, and I hope future developers write test codes for the code they write from now on! (@Lee-Si-Yoon, @lerrybe)
    • I also created a fake mouse down util for testing mouse clicks for canvas elements
      export interface MouseEventWithOffsets extends MouseEventInit {
      pageX?: number;
      pageY?: number;
      offsetX?: number;
      offsetY?: number;
      x?: number;
      y?: number;
      altKey?: boolean;
      }
      export class FakeMouseEvent extends MouseEvent {
      constructor(type: string, values: MouseEventWithOffsets) {
      const { pageX, pageY, offsetX, offsetY, x, y, ...mouseValues } = values;
      super(type, mouseValues);
      Object.assign(this, {
      offsetX: offsetX || 0,
      offsetY: offsetY || 0,
      pageX: pageX || 0,
      pageY: pageY || 0,
      x: x || 0,
      y: y || 0,
      });
      }
      }
    • People interested in how I implemented test codes for Dotting may refer to src/test/interactionLayer/select.test.tsx
      describe("test for select tool", () => {
      let editor: Editor;
      let canvasElement: HTMLCanvasElement;
      beforeEach(() => {
      const divElement = document.createElement("div");
      const interactionCanvas = divElement.appendChild(
      document.createElement("canvas"),
      );
      const gridCanvas = divElement.appendChild(document.createElement("canvas"));
      const dataCanvas = divElement.appendChild(document.createElement("canvas"));
      const backgroundCanvas = divElement.appendChild(
      document.createElement("canvas"),
      );
      const mockEditor = new Editor({
      gridCanvas,
      interactionCanvas,
      dataCanvas,
      backgroundCanvas,
      });
      divElement.tabIndex = 1;
      divElement.onmousedown = () => {
      divElement.focus();
      };
      divElement.addEventListener("keydown", (e: any) => {
      editor.onKeyDown(e);
      });
      mockEditor.setSize(800, 800);
      editor = mockEditor;
      // initialize the canvas with select tool selecting all the pixels
      canvasElement = editor.getCanvasElement();
      editor.setBrushTool(BrushTool.SELECT);
      fireEvent(
      canvasElement,
      new FakeMouseEvent("mousedown", {
      offsetX: 0,
      offsetY: 0,
      }),
      );
      fireEvent(
      canvasElement,
      new FakeMouseEvent("mousemove", {
      offsetX: canvasElement.width,
      offsetY: canvasElement.height,
      }),
      );
      fireEvent(
      canvasElement,
      new FakeMouseEvent("mouseup", {
      offsetX: canvasElement.width,
      offsetY: canvasElement.height,
      }),
      );
      });
      afterEach(() => {
      jest.clearAllMocks();
      });
      it("selects all the pixels in the canvas", () => {
      const columnCount = editor.getColumnCount();
      const rowCount = editor.getRowCount();
      const selectedArea = editor.getSelectedArea();
      expect(selectedArea?.startPixelIndex).toStrictEqual({
      rowIndex: 0,
      columnIndex: 0,
      });
      expect(selectedArea?.endPixelIndex).toStrictEqual({
      rowIndex: rowCount - 1,
      columnIndex: columnCount - 1,
      });
      });

Notes

  • I had to add lots of code due to the complexity of the issue. I am sorry for writing too much code. If you have any questions on how I implemented the extension functionality, please feel free to ask my anytime! (@lerrybe, @Lee-Si-Yoon)

Next Up?

  • It seems that we should change our world position for pixels. currently the middle pixel in the grid is (0,0) of the canvas. However, I think we should make {rowIndex: 0, columnIndex: 0} to be the (0,0) of the canvas. I will work on that.

@hunkim98 hunkim98 self-assigned this Jul 8, 2023
@github-actions
Copy link

github-actions bot commented Jul 8, 2023

PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-07-15 13:33 UTC

@hunkim98 hunkim98 changed the title Feature/select size change Feature: select tool extension feature Jul 8, 2023
src/utils/position.ts Show resolved Hide resolved
Comment on lines +11 to +25
export class FakeMouseEvent extends MouseEvent {
constructor(type: string, values: MouseEventWithOffsets) {
const { pageX, pageY, offsetX, offsetY, x, y, ...mouseValues } = values;
super(type, mouseValues);

Object.assign(this, {
offsetX: offsetX || 0,
offsetY: offsetY || 0,
pageX: pageX || 0,
pageY: pageY || 0,
x: x || 0,
y: y || 0,
});
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Creating an event class for simulation purposes is impressive.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, the basic jest mouse event does not have offsetX or offsetY!
The solution was brought from testing-library/react-testing-library#268

src/components/Canvas/GridLayer.tsx Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
Comment on lines +388 to +398
export const getRelativeCornerWordPosOfPixelToOrigin = (
// the pixelIndex passed as a parameter should be an integer
pixelIndex: {
rowIndex: number;
columnIndex: number;
},
originPixelIndex: { rowIndex: number; columnIndex: number },
gridSquareLength: number,
) => {
// it will return world pos relative to originpixel word pos
// the origin will be set to 0,0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const getRelativeCornerWordPosOfPixelToOrigin = (
// the pixelIndex passed as a parameter should be an integer
pixelIndex: {
rowIndex: number;
columnIndex: number;
},
originPixelIndex: { rowIndex: number; columnIndex: number },
gridSquareLength: number,
) => {
// it will return world pos relative to originpixel word pos
// the origin will be set to 0,0
/**
* @summary it will return world pos relative to originpixel word pos, the origin will be set to 0,0
* @param pixelIndex - the pixelIndex passed as a parameter should be an integer
*/
export const getRelativeCornerWordPosOfPixelToOrigin = (
pixelIndex: {
rowIndex: number;
columnIndex: number;
},
originPixelIndex: { rowIndex: number; columnIndex: number },
gridSquareLength: number,
) => {
image
how about using jsDoc tags for annotations?

Copy link
Owner Author

Choose a reason for hiding this comment

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

From now on, I will try to use jsDoc tags as much as possible

src/components/Canvas/InteractionLayer.tsx Outdated Show resolved Hide resolved
src/components/Canvas/InteractionLayer.tsx Show resolved Hide resolved
src/components/Canvas/InteractionLayer.tsx Outdated Show resolved Hide resolved
Comment on lines +211 to +217
getExtendingSelectedArea() {
return this.extendingSelectedArea;
}

getExtendingSelectedPixels() {
return this.extendingSelectedPixels;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about adding these to storybook example? let's show off great feature

Copy link
Owner Author

Choose a reason for hiding this comment

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

I will add an example for this in another PR later!

src/components/Canvas/Editor.tsx Outdated Show resolved Hide resolved
src/components/Canvas/Editor.tsx Show resolved Hide resolved
src/components/Canvas/Editor.tsx Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Jul 15, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 30 Code Smells

68.1% 68.1% Coverage
12.2% 12.2% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@hunkim98 hunkim98 merged commit 088a40c into main Jul 15, 2023
@hunkim98 hunkim98 deleted the feature/selectSizeChange branch July 15, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants