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

Fixed bug: Incorrect point deletion with keyboard shortcut #4420

Merged
merged 8 commits into from
Mar 11, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed
- Permission error occured when accessing the JobCommits (<https://github.com/openvinotoolkit/cvat/issues/4434>)
- Bug: Incorrect point deletion with keyboard shortcut (<https://github.com/openvinotoolkit/cvat/pull/4420>)

### Security
- TDB
Expand Down
4 changes: 2 additions & 2 deletions cvat-canvas/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cvat-canvas/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "cvat-canvas",
"version": "2.13.1",
"version": "2.13.2",
"description": "Part of Computer Vision Annotation Tool which presents its canvas library",
"main": "src/canvas.ts",
"scripts": {
Expand Down
28 changes: 27 additions & 1 deletion cvat-canvas/src/typescript/canvasView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,15 @@ export class CanvasViewImpl implements CanvasView, Listener {

if (['polygon', 'polyline', 'points'].includes(state.shapeType)) {
if (e.altKey) {
if (state.shapeType === 'points') {
const selectedClientID = parseInt(
((e.target as HTMLElement).parentElement as HTMLElement).getAttribute('clientID'), 10,
);

if (state.clientID !== selectedClientID) {
return;
}
}
const { points } = state;
this.onEditDone(state, points.slice(0, pointID * 2).concat(points.slice(pointID * 2 + 2)));
} else if (e.shiftKey) {
Expand Down Expand Up @@ -860,6 +869,8 @@ export class CanvasViewImpl implements CanvasView, Listener {

if (value) {
const getGeometry = (): Geometry => this.geometry;
const getController = (): CanvasController => this.controller;
const getActiveElement = (): ActiveElement => this.activeElement;
(shape as any).selectize(value, {
deepSelect: true,
pointSize: (2 * consts.BASE_POINT_SIZE) / this.geometry.scale,
Expand All @@ -875,7 +886,22 @@ export class CanvasViewImpl implements CanvasView, Listener {
'stroke-width': consts.POINTS_STROKE_WIDTH / getGeometry().scale,
});

circle.on('mouseenter', (): void => {
circle.on('mouseenter', (e: MouseEvent): void => {
const activeElement = getActiveElement();
if (activeElement !== null && e.altKey) {
const [state] = getController().objects.filter(
(_state: any): boolean => _state.clientID === activeElement.clientID,
);
if (state.shapeType === 'points') {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (state.shapeType === 'points') {
if (state?.shapeType === 'points') {

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this condition should work for all the shapes, not only for points, do not you think so?

const selectedClientID = parseInt(
((e.target as HTMLElement).parentElement as HTMLElement).getAttribute('clientID'), 10,
);
Copy link
Member

Choose a reason for hiding this comment

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

Why not to use + operator here to cast to integer? It looks simpler

if (state.clientID !== selectedClientID) {
return;
}
}
}

circle.attr({
'stroke-width': consts.POINTS_SELECTED_STROKE_WIDTH / getGeometry().scale,
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// Copyright (C) 2022 Intel Corporation
//
// SPDX-License-Identifier: MIT

/// <reference types="cypress" />

import { taskName, labelName } from '../../support/const';

context('When delete a point, the required point is deleted.', () => {
const issueId = '3821';

const firstPointsShape = {
labelName,
type: 'Shape',
pointsMap: [
{ x: 300, y: 250 },
{ x: 300, y: 350 },
{ x: 300, y: 450 },
],
};
const secondPointsShape = {
labelName,
type: 'Shape',
pointsMap: [
{ x: 330, y: 250 },
{ x: 330, y: 350 },
{ x: 330, y: 450 },
],
};

before(() => {
cy.openTaskJob(taskName);
});

describe(`Testing issue "${issueId}"`, () => {
it('Crearte points shapes', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('Crearte points shapes', () => {
it('Create points shapes', () => {

cy.createPoint(firstPointsShape);
cy.get('#cvat-objects-sidebar-state-item-1').should('contain', '1').and('contain', 'POINTS SHAPE');
cy.createPoint(secondPointsShape);
cy.get('#cvat-objects-sidebar-state-item-2').should('contain', '2').and('contain', 'POINTS SHAPE');
Copy link
Member

Choose a reason for hiding this comment

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

Probably it should be a part of before section because we do not test creating points here

});
it('Remove point holding Alt key from each shape', () => {
cy.get('#cvat_canvas_shape_1').trigger('mousemove', { force: true }).trigger('mouseover', { force: true });
cy.get('body').type('{alt}', { release: false });
cy.get('#cvat_canvas_shape_1')
.children()
.then((children) => {
cy.get(children)
.eq(1)
.then((elem) => {
cy.get(elem).click();
});
});
cy.get('#cvat_canvas_shape_2')
.children()
.then((children) => {
cy.get(children)
.eq(1)
.then((elem) => {
cy.get(elem).click();
});
});
});
it('Point must be removed from first shape, second one should stay the same', () => {
cy.get('#cvat_canvas_shape_1')
.children()
.should('have.length', 2);
cy.get('#cvat_canvas_shape_2')
.children()
.should('have.length', 3);
});
Copy link
Member

Choose a reason for hiding this comment

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

It should be the same it(). In the first you do not check anything.

});
});