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

fix: LEAP-1191: Fix drawing tools scenarios with ctrl pressed #6056

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Gondragos
Copy link
Collaborator

@Gondragos Gondragos commented Jul 3, 2024

This fix changes behaviour into:

  1. We can draw region over another region with keeping ctrl/cmd pressed only when there is no selected region yet.
  2. We can add region to selection with ctrl/cmd pressed, in case if there is(are) some selected region(s).

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

  • Product design
  • Backend (Database)
  • Backend (API)
  • Frontend

Describe the reason for change

There is an intersection of two expected behaviours:

  1. We need to draw regions over other regions. And that was originally solved with adding ability to draw with pressed ctrl/cmd key to ignore interactions with existing regions.
  2. We want to be able to select more than one region to work with all them at once (hiding, deleting, moving and so on). And we use ctrl/cmd pressing as well.

So that, with keeping ctrl/cmd pressed and clicking on the region we got:

  1. Starting drawing region by clicking.
  2. Adding region to the current selection.
    But all that in the same time.

What feature flags were used to cover this change?

N/A

Does this PR introduce a breaking change?

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

  • e2e
  • integration
  • unit

Which logical domain(s) does this change affect?

Image, Drawing tools

@github-actions github-actions bot added the fix label Jul 3, 2024
Copy link

netlify bot commented Jul 3, 2024

Deploy Preview for label-studio-docs-new-theme canceled.

Name Link
🔨 Latest commit c8fe64b
🔍 Latest deploy log https://app.netlify.com/sites/label-studio-docs-new-theme/deploys/669528fa2c620e0008b4f4d4

Copy link

netlify bot commented Jul 3, 2024

Deploy Preview for heartex-docs canceled.

Name Link
🔨 Latest commit c8fe64b
🔍 Latest deploy log https://app.netlify.com/sites/heartex-docs/deploys/669528fac40c770008b87406

Copy link
Contributor

@bmartel bmartel left a comment

Choose a reason for hiding this comment

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

Looks great 🔥

const isCtrlPressed = e.evt && (e.evt.metaKey || e.evt.ctrlKey);
const hasSelection = self.control.annotation.hasSelection;

return isCtrlPressed && !hasSelection;
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be misunderstanding something but based on the description if there is no selection we create a new region overtop existing regions. (this seems to allow it as coded)
but if there is a selection then we add to the selection - would that not go through here and fail? or does that go through another spot?

Copy link
Contributor

@bmartel bmartel Jul 11, 2024

Choose a reason for hiding this comment

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

based on the description if there is no selection we create a new region overtop existing regions
if there is a selection then we add to the selection

Only when the meta key is pressed, otherwise its the default behaviour as before which depends on the state as to the operation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we are going to skip the interaction, the event will be processed inside the tool and it will draw region

if (
// create regions over another regions with Cmd/Ctrl pressed
item.getSkipInteractions() ||
e.target === item.stageRef ||
findClosestParent(e.target, isRightElementToCatchToolInteractions)
) {
window.addEventListener("mousemove", this.handleGlobalMouseMove);
window.addEventListener("mouseup", this.handleGlobalMouseUp);
const { offsetX: x, offsetY: y } = e.evt;
// store the canvas coords for calculations in further events
const { left, top } = item.containerRef.getBoundingClientRect();
this.canvasX = left;
this.canvasY = top;
if (this.skipNextMouseDown) {
this.skipNextMouseDown = false;
return true;
}
item.event("mousedown", e, x, y);
return true;
}
};

With the currently selected region, we will skip this interaction, but clicking on the region will still be processed.
onClick={(e) => {
if (item.parent.getSkipInteractions()) return;
if (store.annotationStore.selected.relationMode) {
stage.container().style.cursor = Constants.DEFAULT_CURSOR;
}
item.setHighlight(false);
item.onClickRegion(e);
}}

And it affects selection
const selectAction = () => {
const allowedDrawingThroughRegion = self.object.allowedDrawingThroughRegion;
// Prevent selecting interactions with an existing region
// If we have intention to draw a new region instead
if (additiveMode && allowedDrawingThroughRegion) return;
self._selectArea(additiveMode);
deferredSelectId = null;
};

So with ctrl/cmd key pressed and drawing tool selected it might draw over existed region if there is no selection and add/remove to/from selection if there is already selected regions.

I hope that I've mentioned here all meaningful spots,
'cause it's really a bit complicated. 😅

@yyassi-heartex
Copy link
Contributor

yyassi-heartex commented Jul 11, 2024

/selenium test

'/selenium' is an unknown pull-request command.
See '/help'

@Gondragos
Copy link
Collaborator Author

Gondragos commented Jul 15, 2024

/git merge develop

Workflow run
Successfully merged: create mode 100644 web/libs/editor/src/components/TopBar/tests/CurrentTask.test.tsx

@Gondragos
Copy link
Collaborator Author

Gondragos commented Jul 16, 2024

/git merge develop

Workflow run
Successfully merged: Already up to date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants