Annotations filters UI #2743

vnishukov · 2021-01-29T13:13:29Z

Motivation and context

1418: CVAT new UI: find the way to simplify the syntax for filters

How has this been tested?

Go to annotation page
Click to filters input
Play around with form
Shift+Left click: add open bracket for item
Shift+Right click: add close bracket for item
Alt+Left click: remove open bracket for item
Alt+Right click: remove close bracket for item
Right click on filter input open confirm dialog to clean up filters

Checklist

I submit my changes into the develop branch
I have added description of my changes into CHANGELOG file
I have updated the documentation accordingly
I have added tests to cover my changes
I have linked related issues (read github docs)
I have increased versions of npm packages if it is necessary (cvat-canvas,
cvat-core, cvat-data and cvat-ui)

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) 2020 Intel Corporation
#
# SPDX-License-Identifier: MIT

vnishukov · 2021-01-29T13:18:51Z

FiltersInput component has to be removed after review

cvat-ui/package-lock.json

nmanovic · 2021-02-03T13:15:50Z

@vnishukov , by a reason first image is zoomed when I open a job. Can it be connected with your changes?

nmanovic · 2021-02-03T13:52:44Z

In help message:

Can we replace [Right] and [Left] on icons for left and right buttons?
Do we need ServerID and ClientID? The user can see ID. I will prefer to keep only ID = ClientID.
CVAT uses json queries to perform search.. Why do we need mention that? Is it important?

nmanovic · 2021-02-03T13:57:01Z

@nmanovic

Can we have an icon to clear filter somewhere instead of context menu?

We actually can, but I'm pretty sure that current approach is more comfortable for users, and allow to keep filters pane with no extra elements. It actually not a big deal to change, so could we give users a chance to test it?

nmanovic · 2021-02-03T13:57:40Z

Why did you decide to use hotkeys to add ( and )?

nmanovic · 2021-02-03T14:09:19Z

@vnishukov , did you look at https://github.com/ukrbublik/react-awesome-query-builder? Can it help?

vnishukov · 2021-02-03T14:55:32Z

@nmanovic

In help message:

Can we replace [Right] and [Left] on icons for left and right buttons?
This is a dynamic values fetched from global shortcuts map, don't think we shall hardcode it

Do we need ServerID and ClientID? The user can see ID. I will prefer to keep only ID = ClientID.
This developed as it was requested in concept. Maybe we can ask @bsekachev?

CVAT uses json queries to perform search.. Why do we need mention that? Is it important?
This was mentioned before. JSON queries is still used to apply the filters, that's why it is still present. Should I remove it?

vnishukov · 2021-02-03T15:14:24Z

@nmanovic

Why did you decide to use hotkeys to add ( and )?

I'm absolutely sure this is more comfortable than it was described in concept (additional combine button in change filter panel). We should improve UX and make it possible for users to achieve theirs goals using minimum of clicks, navigations, etc. In current approach user can group filters by single clicks for each of them. In the approach described in concept it will be couple of clicks, and extra button. But the main benefit here is the multigrouping (two or more braces) in the current approach we still handle it by two clicks, in other approach it requires a "by hand" input in filters pane which is acts slowly and takes more efforts from user. Let's give a chance to test it by users as well?

vnishukov · 2021-02-03T15:16:32Z

@nmanovic

@vnishukov , did you look at https://github.com/ukrbublik/react-awesome-query-builder? Can it help?

Current logic of json queries was not changed, as it works fine. What benefits will we have to change it with something else instead?

nmanovic · 2021-02-03T16:17:32Z

@nmanovic

@vnishukov , did you look at https://github.com/ukrbublik/react-awesome-query-builder? Can it help?

Current logic of json queries was not changed, as it works fine. What benefits will we have to change it with something else instead?

@vnishukov , I see your point. @bsekachev , @ActiveChooN , what do you think?

bsekachev · 2021-02-04T13:59:17Z

@ActiveChooN Can I ask you to review the PR when it is ready?

vnishukov · 2021-02-08T06:35:42Z

@vnishukov , by a reason first image is zoomed when I open a job. Can it be connected with your changes?

This is the one of the reason of failed tests. I didn't make any specific with images, changes affect only right side panel, but seems somehow it affects annotation area as well

bsekachev · 2021-02-09T16:04:29Z

Hi, I tried the patch and here are some thoughts:

Why do we need "@babel/plugin-proposal-nullish-coalescing-operator"? Let's just use "||" to get default value.
Bug: Image scaled after opening a job (already mentioned)
Delete by right mouse click: not obvious (why not left icon as it was?)
Disappeared feature: the latest 10 filters list to quick select on of them
Personally, it took me 10 minutes to understand how to add brackets in current implementation
Bug: the object with ID 6 is inaccessible (literally it is out of scroll area, scroll is set to maximum on the screenshot)
Pressing right button (to remove a bracket) looks quite risky because in case when I pressed it before Alt (or missed the key in some reasons) it opens popover with suggestion to remove filter and button "Yes" close to the cursor
Bug: In attribute annotation mode we still have old component for filters
Bug: Not critical but looks strange when a label does not have any attributes
Bug: I set empty_frame=="false" and CVAT still navigates by empty frames. How is it expected to work? We already have another feature to empty frames navigation actually
Probably bug: boolean attributes have only one option (I understand that it could be replaced with ==, !==, but for me personally it looks strange (maybe @ActiveChooN think in other way)
Disappeared feature: filters like width > height; height > width; height == width, etc
Type filter does not contain "Tag" option
Bug: Filter: occluded=="true" hides occluded object (occluded=="false" also hides occluded object as it expected)

I also looked at https://github.com/ukrbublik/react-awesome-query-builder and I would say it looks promising. IMO UX is simplier and the builder is quite flexible. I have some concerns related with output format (out of the box it is different, but GitHub says "Export to MongoDb, SQL, JsonLogic or your custom format", so maybe we will be able to setup it properly or adjust format that cvat-core expects (it is also possible because cvat-core expects custom format, not jsonpath and we have our code for convertation which could be adjusted). And additionally we need to be sure that this library is able to provide necessary flexibility (in possible queries). After quick review it looks like it is able more or less, but need more investigations.

bsekachev · 2021-02-09T16:05:35Z

@vnishukov @ActiveChooN

Let's have a meeting tomorrow about the patch, guys. I'll send an invite.

bsekachev · 2021-02-09T16:46:18Z

@nmanovic

Do we need ServerID and ClientID? The user can see ID. I will prefer to keep only ID = ClientID.

Previously when dump file included server IDs some people used serverID to find objects from dump file in UI.
Now a dump file does not include id anymore, so I think it can be removed.

bsekachev · 2021-02-10T07:52:49Z

cvat-canvas/src/typescript/canvasView.ts

@@ -1,4 +1,4 @@
-// Copyright (C) 2019-2020 Intel Corporation
+// Copyright (C) 2021 Intel Corporation


It should be 2019-2021

bsekachev · 2021-02-10T08:02:27Z

...mponents/annotation-page/annotation-filter/annotation-filter-item/annotation-filter-item.tsx

+//
+// SPDX-License-Identifier: MIT
+
+import { Tag } from 'antd';


Let's use the same import policy, now project uses modular import: import Tag from /antd/lib/Tag
For information: https://nsisodiya.medium.com/reduce-size-in-ant-design-bundle-size-c35dad9ce3a8

bsekachev · 2021-02-10T08:05:35Z

...mponents/annotation-page/annotation-filter/annotation-filter-item/annotation-filter-item.tsx

+};
+function AnnotationFilterItem({
+    item, onEdit, onDelete, onGrouping,
+}: Props): ReactElement {


ReactElement > JSX.Element

bsekachev · 2021-02-10T08:11:41Z

...mponents/annotation-page/annotation-filter/annotation-filter-item/annotation-filter-item.tsx

+        case ActionType.addRight:
+            return { ...state, right: [...state.right, ')'] };
+        case ActionType.removeLeft:
+            state.left.pop();


state.right property before reducing is equal to state.right property after.
It is better to use something destructive like that:
return { ...state, left: state.left.slice(0, -1) };

bsekachev · 2021-02-10T08:11:46Z

...mponents/annotation-page/annotation-filter/annotation-filter-item/annotation-filter-item.tsx

+            state.right.pop();
+            return { ...state };


bsekachev · 2021-02-10T08:23:26Z

cvat-ui/src/cvat-canvas-wrapper.ts

@@ -1,4 +1,4 @@
-// Copyright (C) 2020 Intel Corporation
+// Copyright (C) 2021 Intel Corporation


Please, abort changes in this file

bsekachev · 2021-02-10T08:23:41Z

cvat-ui/src/icons.tsx

@@ -1,4 +1,4 @@
-// Copyright (C) 2020 Intel Corporation
+// Copyright (C) 2021 Intel Corporation


Please, abort changes in this file

bsekachev · 2021-02-10T08:24:01Z

...ui/src/components/annotation-page/standard-workspace/controls-side-bar/controls-side-bar.tsx

@@ -1,4 +1,4 @@
-// Copyright (C) 2020 Intel Corporation
+// Copyright (C) 2021 Intel Corporation


Should be 2020-2021

bsekachev · 2021-02-10T08:24:04Z

...i/src/components/annotation-page/standard-workspace/objects-side-bar/objects-list-header.tsx

@@ -1,22 +1,21 @@
-// Copyright (C) 2020 Intel Corporation
+// Copyright (C) 2021 Intel Corporation


Should be 2020-2021

bsekachev · 2021-02-10T08:24:59Z

...i/src/components/annotation-page/standard-workspace/objects-side-bar/objects-list-header.tsx

 import StatesOrderingSelector from 'components/annotation-page/standard-workspace/objects-side-bar/states-ordering-selector';
+import React from 'react';


Please, do 3rdparty import before internal import
What is a reason to change import order in this file?

bsekachev · 2021-02-10T08:34:25Z

cvat-ui/src/components/annotation-page/annotation-filter/annotation-filter.enum.ts

+    concatenator,
+    filterBy,
+    operator,
+    value,
+    attribute,
+    attributeOperator,
+    attributeValue,
+    anotherAttributeLabel,
+    anotherAttributeValue,
+    fillState,
+    partialReset,
+    reset,


I expect all actions to be a verb (or contain a verb)
For example I cannot understand what such actions like concatenator, value, operator, attribute, etc. do.
Also to follow the current codestyle let actions to be written in upper case: FILTER_BY, FILL_STATE, PARTIAL_RESET.

bsekachev · 2021-02-10T08:35:46Z

Some common comments after code review:

Using any and some abstract types is too often, we lose the main benefit of TypeScript in this case
I see code style and header changes in some files which are not expected to be affected by the patch

vnishukov · 2021-02-10T10:05:25Z

@bsekachev

Why do we need "@babel/plugin-proposal-nullish-coalescing-operator"? Let's just use "||" to get default value.

Using || we apply right branch value when the left branch value is false but not only includes undefined and null but also 0 and ''.
I believe ?? is much more specific as to apply only for undefined and null and not for 0 and '' (which more like a js assertions side effect)

bsekachev · 2021-02-10T10:09:07Z

Using || we apply right branch value when the left

I read about the difference, but in the code I do not see where it is important in our case. Could you please direct me?

ActiveChooN

Most problems was described by @bsekachev and i have to add only few things:

It's better to save state of the filter input for reducing clicks for similar filters (in case we want to select some obejcts with differnt clientID).
There is no way to insert filter in the middle of the query, only remove secand half of the query.
I guess it can be really nice feature to insert query as a string like it works for label editor. It can be helpful for common filters.

Probably bug: boolean attributes have only one option (I understand that it could be replaced with ==, !==, but for me personally it looks strange (maybe @ActiveChooN think in other way)

I did not notice anithing strange.

ActiveChooN · 2021-02-10T11:39:10Z

...mponents/annotation-page/annotation-filter/annotation-filter-pane/annotation-filter-pane.tsx

+    const { changeAnnotationsFilters } = props;
+
+    const [editItem, setEditItem] = useState();
+    const [filters, setFilters] = useState([] as any);


Can we use filter type from annotation-filter-item.tsx and reduce using any type as possible?

ActiveChooN · 2021-02-10T12:07:51Z

...onents/annotation-page/annotation-filter/annotation-filter-panel/annotation-filter-panel.tsx

+    }, [state]);
+
+    useEffect(() => {
+        setTimeout(() => {


I guess it's better to use recap for timeout in useEffect hook

vnishukov · 2021-02-27T09:02:58Z

Reimplemented with new UI in #2871 using https://github.com/ukrbublik/react-awesome-query-builder

Filters UI feature implemented

0f79d3a

vnishukov requested review from azhavoro and bsekachev as code owners January 29, 2021 13:13

This was referenced Jan 29, 2021

[WIP] Annotations filters UI #2726

Closed

Annotation filters UI #2603

Closed

CHANGELOG, version increment, babel nullish-coalescing-operator

6bf4f59

vnishukov requested a review from nmanovic as a code owner January 29, 2021 13:40

bsekachev reviewed Feb 1, 2021

View reviewed changes

cvat-ui/package-lock.json Outdated Show resolved Hide resolved

bsekachev added the enhancement New feature or request label Feb 1, 2021

vnishukov added 5 commits February 1, 2021 11:30

Build error fix

050eb94

Build error fix

79feb87

package-lock update

a70e607

Merge branch 'develop' into feature/1418-annotations-filters-ui

f4f3e79

Merged with current develop

4817065

vnishukov added 2 commits February 4, 2021 16:33

Old filters e2e tests skip

93407c8

Merged with current develop

16a828f

vnishukov changed the title ~~Annotations filters UI~~ [WIP] Annotations filters UI Feb 4, 2021

bsekachev requested a review from ActiveChooN February 5, 2021 10:03

bsekachev assigned ActiveChooN and bsekachev Feb 8, 2021

vnishukov added 2 commits February 9, 2021 15:21

Merged with current develop

73785ce

Old filters-input tests

3eb8af8

bsekachev reviewed Feb 10, 2021

View reviewed changes

ActiveChooN reviewed Feb 10, 2021

View reviewed changes

vnishukov changed the title ~~[WIP] Annotations filters UI~~ Annotations filters UI Feb 26, 2021

vnishukov closed this Feb 27, 2021

bsekachev deleted the feature/1418-annotations-filters-ui branch April 1, 2021 05:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotations filters UI #2743

Annotations filters UI #2743

vnishukov commented Jan 29, 2021 •

edited

Loading

vnishukov commented Jan 29, 2021

nmanovic commented Feb 3, 2021

nmanovic commented Feb 3, 2021

nmanovic commented Feb 3, 2021 •

edited by vnishukov

Loading

nmanovic commented Feb 3, 2021

nmanovic commented Feb 3, 2021

vnishukov commented Feb 3, 2021 •

edited

Loading

vnishukov commented Feb 3, 2021 •

edited

Loading

vnishukov commented Feb 3, 2021

nmanovic commented Feb 3, 2021

bsekachev commented Feb 4, 2021 •

edited

Loading

vnishukov commented Feb 8, 2021

bsekachev commented Feb 9, 2021

bsekachev commented Feb 9, 2021

bsekachev commented Feb 9, 2021

bsekachev Feb 10, 2021

bsekachev Feb 10, 2021

bsekachev Feb 10, 2021

bsekachev Feb 10, 2021

bsekachev Feb 10, 2021

bsekachev Feb 10, 2021

bsekachev Feb 10, 2021

bsekachev Feb 10, 2021

bsekachev Feb 10, 2021

bsekachev Feb 10, 2021

bsekachev Feb 10, 2021

bsekachev commented Feb 10, 2021

vnishukov commented Feb 10, 2021 •

edited

Loading

bsekachev commented Feb 10, 2021

ActiveChooN left a comment

ActiveChooN Feb 10, 2021

ActiveChooN Feb 10, 2021

vnishukov commented Feb 27, 2021

		@@ -1,4 +1,4 @@
		// Copyright (C) 2019-2020 Intel Corporation
		// Copyright (C) 2021 Intel Corporation

		@@ -1,4 +1,4 @@
		// Copyright (C) 2020 Intel Corporation
		// Copyright (C) 2021 Intel Corporation

		@@ -1,22 +1,21 @@
		// Copyright (C) 2020 Intel Corporation
		// Copyright (C) 2021 Intel Corporation

		import StatesOrderingSelector from 'components/annotation-page/standard-workspace/objects-side-bar/states-ordering-selector';
		import React from 'react';

Annotations filters UI #2743

Annotations filters UI #2743

Conversation

vnishukov commented Jan 29, 2021 • edited Loading

Motivation and context

How has this been tested?

Checklist

License

vnishukov commented Jan 29, 2021

nmanovic commented Feb 3, 2021

nmanovic commented Feb 3, 2021

nmanovic commented Feb 3, 2021 • edited by vnishukov Loading

nmanovic commented Feb 3, 2021

nmanovic commented Feb 3, 2021

vnishukov commented Feb 3, 2021 • edited Loading

vnishukov commented Feb 3, 2021 • edited Loading

vnishukov commented Feb 3, 2021

nmanovic commented Feb 3, 2021

bsekachev commented Feb 4, 2021 • edited Loading

vnishukov commented Feb 8, 2021

bsekachev commented Feb 9, 2021

bsekachev commented Feb 9, 2021

bsekachev commented Feb 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsekachev commented Feb 10, 2021

vnishukov commented Feb 10, 2021 • edited Loading

bsekachev commented Feb 10, 2021

ActiveChooN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnishukov commented Feb 27, 2021

vnishukov commented Jan 29, 2021 •

edited

Loading

nmanovic commented Feb 3, 2021 •

edited by vnishukov

Loading

vnishukov commented Feb 3, 2021 •

edited

Loading

vnishukov commented Feb 3, 2021 •

edited

Loading

bsekachev commented Feb 4, 2021 •

edited

Loading

vnishukov commented Feb 10, 2021 •

edited

Loading