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

Conversation

klakhov
Copy link
Contributor

@klakhov klakhov commented Mar 4, 2022

Motivation and context

The point deletion with shortcut works not quite well if we start deleting points from another shape holding Alt
bug

Solution: implement the same behavior with points shape as with polylines. If we are holding Alt over one object, another one shouldn't be activated.
polyline

Result:
solution

Resolved #3821

How has this been tested?

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2022 Intel Corporation
#
# SPDX-License-Identifier: MIT

@klakhov klakhov requested a review from bsekachev as a code owner March 4, 2022 10:25
@klakhov klakhov requested a review from nmanovic as a code owner March 5, 2022 07:13
@klakhov
Copy link
Contributor Author

klakhov commented Mar 5, 2022

@ActiveChooN Could you look on this fix, please?

ActiveChooN
ActiveChooN previously approved these changes Mar 5, 2022
Copy link
Contributor

@ActiveChooN ActiveChooN left a comment

Choose a reason for hiding this comment

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

LGTM, would you add tests for the issue in this PR?

@klakhov
Copy link
Contributor Author

klakhov commented Mar 5, 2022

Yes, as a next step in this pr test will be added

@klakhov
Copy link
Contributor Author

klakhov commented Mar 10, 2022

@bsekachev Could you look on this pr?

@bsekachev
Copy link
Member

Still reproducable using Ctrl
test

});

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', () => {

Comment on lines 42 to 71
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.

Comment on lines 36 to 40
it('Crearte 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

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?

Comment on lines 896 to 898
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

@bsekachev bsekachev merged commit 53f6699 into develop Mar 11, 2022
@bsekachev bsekachev deleted the kl/fix-point-delete-bug branch March 11, 2022 11:01
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Jun 22, 2022
)

* fixed incorrect deletion

* update changelog

* added test

* applied comments

* added small check
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.

point delete bug
3 participants