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

Annotations filters new UI #2871

Merged
merged 43 commits into from
Mar 22, 2021
Merged

Annotations filters new UI #2871

merged 43 commits into from
Mar 22, 2021

Conversation

vnishukov
Copy link
Contributor

@vnishukov vnishukov commented Feb 26, 2021

Motivation and context

Resolved #1418

How has this been tested?

Go to annotation page, click filter Icon on top bar right (see examples images below)
image
image
image
image

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

@nmanovic
Copy link
Contributor

@vnishukov , need to fix the problem with CI build.

@vnishukov vnishukov mentioned this pull request Feb 27, 2021
8 tasks
@bsekachev
Copy link
Member

Hi, I tried the patch. Mostly working fine, but several comments/questions below.
@ActiveChooN Please, try to run also this patch and left some comments about.

  1. When I open CVAT in small windows (first screenshot), layout looks alright (except of minor issues with position of sort by selector, it is expected to be right-aligned)
    Screenshot from 2021-03-01 16-46-10

But when I open CVAT in my different configuration (Full HD), something wrong with layout
Screenshot from 2021-03-01 16-46-18

  1. I have attributes type (radio) and color (select) and I want to set equality filter on them (like on the screenshot below), but I can't because there are not option to select color attribute, only text.
    Screenshot from 2021-03-01 16-50-27

  2. I would vote for changing color to blue
    Screenshot from 2021-03-01 16-54-35

  3. Probably "Tag" is missed in possible types. (I see "Tag" in "Shapes", but "Tag" is not a shape at all)
    Screenshot from 2021-03-01 16-57-04

  4. Is comparison label with these fields valid? (the same for type and shape)
    Screenshot from 2021-03-01 17-02-14

  5. Is raw filter editor difficult to implement? (I see it was cut from the interface)

cvat-core/package.json Show resolved Hide resolved
.github/workflows/eslint.yml Outdated Show resolved Hide resolved
cvat-core/tests/internal/filter.js.bkp Outdated Show resolved Hide resolved
cvat-ui/package-lock.json Outdated Show resolved Hide resolved
@vnishukov
Copy link
Contributor Author

vnishukov commented Mar 1, 2021

@bsekachev

Hi, I tried the patch. Mostly working fine, but several comments/questions below.
@ActiveChooN Please, try to run also this patch and left some comments about.

  1. When I open CVAT in small windows (first screenshot), layout looks alright (except of minor issues with position of sort by selector, it is expected to be right-aligned)
    Screenshot from 2021-03-01 16-46-10

But when I open CVAT in my different configuration (Full HD), something wrong with layout
Screenshot from 2021-03-01 16-46-18

  1. I have attributes type (radio) and color (select) and I want to set equality filter on them (like on the screenshot below), but I can't because there are not option to select color attribute, only text.
    Screenshot from 2021-03-01 16-50-27
  2. I would vote for changing color to blue
    Screenshot from 2021-03-01 16-54-35
  3. Probably "Tag" is missed in possible types. (I see "Tag" in "Shapes", but "Tag" is not a shape at all)
    Screenshot from 2021-03-01 16-57-04
  4. Is comparison label with these fields valid? (the same for type and shape)
    Screenshot from 2021-03-01 17-02-14
  5. Is raw filter editor difficult to implement? (I see it was cut from the interface)
  1. Will check top-bar alignment
  2. As I understood query-builder allow to compare same types of fields. In our case radio will cast to text
  3. Will change to blue
  4. Probably yes. It was developed based on initial concept
  5. Will doublecheck what happening here
  6. I don't think is difficult but can we make it with separate enhancement PR?

@bsekachev
Copy link
Member

@vnishukov

As I understood query-builder allow to compare same types of fields. In our case radio will cast to text

Radio attribute and text attribute are the same type, I agree (string). But we also have select attribute (see at right on the screenshot, color attribute) that is also string, but the list is lack of this select attribute. It contains only the text attribute

I don't think is difficult but can we make it with separate enhancement PR?

Sure.

@bsekachev
Copy link
Member

@vnishukov Please, work with @dvkruchinin if you have difficulties with CI.

@vnishukov
Copy link
Contributor Author

@bsekachev

@vnishukov Please, work with @dvkruchinin if you have difficulties with CI.

Root cause already defined. Will be fixed soon

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.

Guess it would be a little bit better if button will be gray with more margin at the top.

изображение

If i click on one of these empty dropdown values, UI will crush.

изображение

Guess it can happend due to old values in filtersHistory in local storage.

изображение

@bsekachev
Copy link
Member

One more possible issue found:

I have "label 1" which contains attribute "type" (radio button)
I have "label 2" which contains attribute "type" (number)

Looks like constructor doesn't work correctly in this case. It considers "type" as number attribute only. Should not we link attributes with their parent labels? Something like: "type (label 1)", "type (label 2)"?

@nmanovic
Copy link
Contributor

nmanovic commented Mar 16, 2021

- [ ] Server ID should be removed

  • Client ID should be renamed to Shape ID
  • Shape ID should be a number (not select). Nobody will select from 1000 values. If you don't know the exact number, nothing to select.
  • Fix bug: add rule for "Server ID" or "Client ID", as select you will see only 1 variant (let's say "1"). Enter "2" and it will crash UI.
    TypeError
    (t.children || t.value).toLowerCase is not a function

TypeError: (t.children || t.value).toLowerCase is not a function
at t.filterOption (http://localhost:8080/cvat-ui.dffe796fde25f2595563.min.js:388:377502)
at http://localhost:8080/cvat-ui.dffe796fde25f2595563.min.js:24:146169
at Array.forEach ()
at y (http://localhost:8080/cvat-ui.dffe796fde25f2595563.min.js:24:145984)
at http://localhost:8080/cvat-ui.dffe796fde25f2595563.min.js:36:225400
at Object.pi [as useMemo] (http://localhost:8080/cvat-ui.dffe796fde25f2595563.min.js:348:61567)
at t.useMemo (http://localhost:8080/cvat-ui.dffe796fde25f2595563.min.js:332:5984)
at x (http://localhost:8080/cvat-ui.dffe796fde25f2595563.min.js:36:225346)
at Ko (http://localhost:8080/cvat-ui.dffe796fde25f2595563.min.js:348:57930)
at Pi (http://localhost:8080/cvat-ui.dffe796fde25f2595563.min.js:348:65886)

  • Should select and radio have the same type (select)? No radio is converted to string

@bsekachev
Copy link
Member

bsekachev commented Mar 16, 2021

@nmanovic

Server ID should be removed

It cannot be removed because "Create object URL" works via serverID.

Shape ID should be a number (not select). Nobody will select from 1000 values

It was Data Annotation Team request

@nmanovic
Copy link
Contributor

nmanovic commented Mar 17, 2021

@nmanovic

Server ID should be removed

It cannot be removed because "Create object URL" works via serverID.

Shape ID should be a number (not select). Nobody will select from 1000 values

It was Data Annotation Team request

@bsekachev , let's discuss it one more time. Need to understand how they are using the feature. Also need to unify server and client IDs in the future. I really don't see any value to have multiple IDs. I don't have a solution in my mind, but I'm sure that should not have both.

@bsekachev
Copy link
Member

@nmanovic

Server ID should be removed

It cannot be removed because "Create object URL" works via serverID.

Shape ID should be a number (not select). Nobody will select from 1000 values

It was Data Annotation Team request

@bsekachev , let's discuss it one more time. Need to understand how they are using the feature.

I've already sent a letter to Andrey. Agree, that this using looks strange.

@bsekachev
Copy link
Member

@nmanovic @ActiveChooN

Issues with this patch:

  • It is not clear how to implement input for Object ID that support both predefined and custom values "out-of-the-box"
    Related issue: AllowCustomValues for select (in addition to multiselect)? ukrbublik/react-awesome-query-builder#191 ('select' type does not support custom values since antd does not support them)
    I tried to use 'multiselect' instead of 'select' because it supports allowCustomValues configuration. Nevertheless it does not work with JsonLogic, returning false when comparison.

  • When we compare two attributes (like: an attribute "color" of label "car" equals to attribute "wheel color" of label "car" ), it shows also objects which are lack of these attributes because formally they are equal in this case (undefined === undefined). It is workaround-ed by adding label name with && operator to the filter expression.

  • If a label/an attribute contains . in it's name filters don't work
    Good news. It was also workaround-ed by replacing all . characters by another dot-like character (internally only, no any changes for a user).

@bsekachev bsekachev assigned ActiveChooN and unassigned bsekachev Mar 18, 2021
@bsekachev bsekachev added the enhancement New feature or request label Mar 18, 2021
@bsekachev bsekachev merged commit 11d818d into develop Mar 22, 2021
@bsekachev bsekachev deleted the annotations-filters-new-ui branch March 24, 2021 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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