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

React UI: Point deletion context menu #1292

Merged
merged 11 commits into from
Mar 24, 2020
Merged

React UI: Point deletion context menu #1292

merged 11 commits into from
Mar 24, 2020

Conversation

ActiveChooN
Copy link
Contributor

Delete point with simple context menu and without using Ctrl+double click
изображение

@ActiveChooN ActiveChooN changed the title Point deletion context menu React UI: Point deletion context menu Mar 18, 2020
@bsekachev
Copy link
Member

@ActiveChooN

Hmm..
Whenever I try to press right button click on a point, UI fails with

Uncaught Error: Canvas is busy. Action: resize
    at CanvasModelImpl.activate (canvasModel.ts:336)
    at CanvasImpl.activate (canvas.ts:98)
    at CanvasWrapperComponent.activateOnCanvas (canvas-wrapper.tsx:498)
    at CanvasWrapperComponent.componentDidUpdate (canvas-wrapper.tsx:203)
    at commitLifeCycles (react-dom.development.js:22084)
    at commitLayoutEffects (react-dom.development.js:25302)
    at HTMLUnknownElement.callCallback (react-dom.development.js:336)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:385)
    at invokeGuardedCallback (react-dom.development.js:440)
    at commitRootImpl (react-dom.development.js:25040)
activate @ canvasModel.ts:336
activate @ canvas.ts:98
activateOnCanvas @ canvas-wrapper.tsx:498
componentDidUpdate @ canvas-wrapper.tsx:203
commitLifeCycles @ react-dom.development.js:22084
commitLayoutEffects @ react-dom.development.js:25302
callCallback @ react-dom.development.js:336
invokeGuardedCallbackDev @ react-dom.development.js:385
invokeGuardedCallback @ react-dom.development.js:440
commitRootImpl @ react-dom.development.js:25040
unstable_runWithPriority @ scheduler.development.js:818
runWithPriority$2 @ react-dom.development.js:12130
commitRoot @ react-dom.development.js:24889
finishSyncRender @ react-dom.development.js:24296
performSyncWorkOnRoot @ react-dom.development.js:24274
(anonymous) @ react-dom.development.js:12180
unstable_runWithPriority @ scheduler.development.js:818
runWithPriority$2 @ react-dom.development.js:12130
flushSyncCallbackQueueImpl @ react-dom.development.js:12175
flushSyncCallbackQueue @ react-dom.development.js:12163
batchedUpdates$1 @ react-dom.development.js:24359
notify @ Subscription.js:23
notifyNestedSubs @ Subscription.js:65
handleChangeWrapper @ Subscription.js:70
dispatch @ redux.js:221
(anonymous) @ redux-logger.js:1
(anonymous) @ index.js:11
onUpdateContextMenu @ canvas-wrapper.tsx:263
(anonymous) @ canvas-wrapper.tsx:473
contextmenuHandler @ canvasView.ts:465
react-dom.development.js:21810 The above error occurred in the <CanvasWrapperComponent> component:
    in CanvasWrapperComponent (created by ConnectFunction)
    in ConnectFunction (created by StandardWorkspaceComponent)
    in section (created by BasicLayout)
    in BasicLayout (created by Context.Consumer)
    in Adapter (created by StandardWorkspaceComponent)
    in StandardWorkspaceComponent (created by AnnotationPageComponent)
    in main (created by Basic)
    in Basic (created by Context.Consumer)
    in Adapter (created by AnnotationPageComponent)
    in section (created by BasicLayout)
    in BasicLayout (created by Context.Consumer)
    in Adapter (created by AnnotationPageComponent)
    in AnnotationPageComponent (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in withRouter(Connect(AnnotationPageComponent)) (created by Context.Consumer)
    in Route (created by CVATApplication)
    in Switch (created by CVATApplication)
    in GlobalHotKeys (created by CVATApplication)
    in main (created by Basic)
    in Basic (created by Context.Consumer)
    in Adapter (created by CVATApplication)
    in section (created by BasicLayout)
    in BasicLayout (created by Context.Consumer)
    in Adapter (created by CVATApplication)
    in CVATApplication (created by Context.Consumer)
    in withRouter(CVATApplication) (created by ConnectFunction)
    in ConnectFunction
    in Router (created by BrowserRouter)
    in BrowserRouter
    in Provider

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

Can you confirm if it works for you?

@ActiveChooN
Copy link
Contributor Author

ActiveChooN commented Mar 19, 2020

@bsekachev

Can you confirm if it works for you?

Hmm, everything works fine for me, but i'll try to figure it out.

point_deletion

Also found that there is ability to open context menu for box points also. Will fix it.

@ActiveChooN
Copy link
Contributor Author

@bsekachev please confirm that you still have an issue. And if so can you please provide a case in which it happens?

@bsekachev
Copy link
Member

@ActiveChooN

Was you able to reproduce it on ubuntu?

@ActiveChooN
Copy link
Contributor Author

@bsekachev

@ActiveChooN

Was you able to reproduce it on ubuntu?

Yes, working on it

@bsekachev
Copy link
Member

Some issues:

  • When I open shape menu for a shape, then move cursor to another shape, I cannot open menu for another shape in a new place
  • gif below
    Peek 2020-03-23 15-02

Looks like when we remove some points of a shape, the shape updates immediately, when we remove others points of the shape, the shape doesn't update immediately.

  • one more issue demonstrated on the following gif
    Peek 2020-03-23 15-07

  • and I cannot remove the first point of a shape (a point that was first during drawing). Literally, I cannot even press the button 'Delete point'

Comment on lines 8 to 14
import {
Layout,
Slider,
Icon,
Tooltip,
} from 'antd';

Copy link
Member

Choose a reason for hiding this comment

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

Let's import exact components in future code
Not from antd wrapper
It common recommendation to reduce bundle size

Comment on lines 123 to 134
.cvat-canvas-context-menu {
opacity: 0.6;
position: fixed;
width: 300px;
z-index: 10;
max-height: 50%;
overflow-y: auto;

&:hover {
opacity: 1;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why have you duplicated style from src/components/annotation-page/styles.scss?

Comment on lines 151 to 195
.cvat-canvas-z-axis-wrapper {
position: absolute;
background: $background-color-2;
bottom: 10px;
right: 10px;
height: 150px;
z-index: 100;
border-radius: 6px;
opacity: 0.5;
border: 1px solid $border-color-3;
display: flex;
flex-direction: column;
justify-content: space-between;
padding: 3px;

&:hover {
opacity: 1;
}

> .ant-slider {
height: 75%;
margin: 5px 3px;

> .ant-slider-rail {
background-color: #979797;
}

> .ant-slider-handle {
transform: none !important;
}
}

> i {
opacity: 0.7;
color: $objects-bar-icons-color;

&:hover {
opacity: 1;
}

&:active {
opacity: 0.7;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

One more duplicate


interface StateToProps {
activatedStateID: number | null;
activatedPointID: number | null | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

activatedPointID?: number | null


interface State {
activatedStateID: number | null | undefined;
activatedPointID: number | null | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

activatedPointID?: number | null

top: number;
}

class CanvasContextMenuContainer extends React.PureComponent<Props, State> {
Copy link
Member

Choose a reason for hiding this comment

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

CanvasPointContextMenuContainer

@ActiveChooN ActiveChooN merged commit 57e8083 into develop Mar 24, 2020
@bsekachev bsekachev deleted the dk/point-deletion branch March 25, 2020 08:57
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.

2 participants