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

feat(resource): display region annotations in still images (DSP-1585) #445

Merged
merged 9 commits into from May 26, 2021

Conversation

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented May 21, 2021

resolves DSP-1585

@kilchenmann kilchenmann self-assigned this May 21, 2021
@kilchenmann kilchenmann marked this pull request as draft May 21, 2021
@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 21, 2021

Screen.Recording.2021-05-21.at.18.37.50.mov

Region on book page

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 21, 2021

Screen.Recording.2021-05-21.at.18.41.20.mov

Region in compound object

Loading

@kilchenmann kilchenmann marked this pull request as ready for review May 25, 2021
@kilchenmann kilchenmann requested review from flavens, mdelez and waychal May 25, 2021
@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 25, 2021

@flavens @mdelez @waychal @gautschr if you like to try this locally you can do it with the following steps:

  • Run the app with the test-server environment: ng s --configuration=test-server This way you have also all the images.

1. Resource with annotation

2. Compound object

Or you could search for other resources which have annotations. Good Luck

p.s. about the design. The design of the annotation metadata could be better but it has to be developed first in Figma.

Loading

@kilchenmann kilchenmann requested a review from gautschr May 25, 2021
Copy link
Collaborator

@flavens flavens left a comment

I have a few questions:

  • why having the Annotations tab displayed in a resource without a media representation?
  • when I want to show all props of an annotation, it opens them for all the existing annotations, but I would have expected to open the complete list of props only for the selected annotation. What is the expected behaviour here? If it is not the correct one, I would like a fix in this PR.
  • about the info of the project, is it possible that 2 different annotations belong to 2 different projects? If not, I would clean up the info (after each annotation header).

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 25, 2021

I have a few questions:

  • why having the Annotations tab displayed in a resource without a media representation?
  • when I want to show all props of an annotation, it opens them for all the existing annotations, but I would have expected to open the complete list of props only for the selected annotation. What is the expected behaviour here? If it is not the correct one, I would like a fix in this PR.
  • about the info of the project, is it possible that 2 different annotations belong to 2 different projects? If not, I would clean up the info (after each annotation header).
  1. Because it will also be used to create a new annotation. In this case we have a starting point.
  2. Yes it opens all. This is because they are handled in one component. Any suggestion how to solve it? I will also think about it but I'm happy for all constructive inputs.
  3. ok, makes sense. But the line will still stay there because of the creator information. Is that correct? --> btw this is the point I mentioned about the "design" here #445 (comment)

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 25, 2021

@flavens about the second item: I found a solution! My fault. I will fix it...but it will not be a quick solution! Maybe it would be better to do it in a separate PR. Would you like to create a task on youtrack? Thanks

Loading

@flavens
Copy link
Collaborator

@flavens flavens commented May 25, 2021

  1. Because it will also be used to create a new annotation. In this case we have a starting point.

ok

  1. ok, makes sense. But the line will still stay there because of the creator information. Is that correct? --> btw this is the point I mentioned about the "design" here #445 (comment)

yes

Loading

@flavens
Copy link
Collaborator

@flavens flavens commented May 25, 2021

@flavens about the second item: I found a solution! My fault. I will fix it...but it will not be a quick solution! Maybe it would be better to do it in a separate PR. Would you like to create a task on youtrack? Thanks

ok I will create the issue > DSP-1663

Loading

Copy link
Contributor

@waychal waychal left a comment

I have few queries:

  1. When I select annotation on image, in result it shows me the selected one in annotation tab. Is it possible to do it other way round? I would like to select the annotation in annotation tab and in result it should highlight the selected one on image.
  2. When I tried to remove property from annotation, it asked me for the confirmation which is fine. In confirmation dialog, it shows value in html format (see attached screen shot). Is it a requirement or can we display value in text format?
  3. The annotation belongs to compound object is shown in red colour. When I checked annotation tab first time, it was confusing to me why 2 colours are used until I read your comment in this PR.

Screenshot 2021-05-25 at 17 12 22

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 25, 2021

I have few queries:

  1. When I select annotation on image, in result it shows me the selected one in annotation tab. Is it possible to do it other way round? I would like to select the annotation in annotation tab and in result it should highlight the selected one on image.

Yes, that's also what I thought. But it should be done in a separate task. (s. p.s. in comment)

  1. When I tried to remove property from annotation, it asked me for the confirmation which is fine. In confirmation dialog, it shows value in html format (see attached screen shot). Is it a requirement or can we display value in text format?

This is not part of this PR and belongs to the value component. Would you like to create an issue task on Youtrack? Thanks

  1. The annotation belongs to compound object is shown in red colour. When I checked annotation tab first time, it was confusing to me why 2 colours are used until I read your comment in this PR.

What do you mean by 2 colours? The color of the region can be selected by the creator or editor. But create and edit region is not part of this PR.

Loading

@waychal
Copy link
Contributor

@waychal waychal commented May 25, 2021

Yes, that's also what I thought. But it should be done in a separate task. (s. p.s. in comment)

Ok.

This is not part of this PR and belongs to the value component. Would you like to create an issue task on Youtrack? Thanks

Sure.

  1. The annotation belongs to compound object is shown in red colour. When I checked annotation tab first time, it was confusing to me why 2 colours are used until I read your comment in this PR.

What do you mean by 2 colours? The color of the region can be selected by the creator or editor. But create and edit region is not part of this PR.

Sorry, my mistake. Farbe in German is Colour. I didn't know that. I thought red is used for compound object and green for others.

Screenshot 2021-05-25 at 18 24 40

Loading

@waychal
Copy link
Contributor

@waychal waychal commented May 25, 2021

So far everything is good from my side. If you add me as reviewer again, I will approve the PR.

Loading

@kilchenmann kilchenmann requested a review from waychal May 25, 2021
@kilchenmann kilchenmann requested a review from flavens May 25, 2021
Copy link
Contributor

@gautschr gautschr left a comment

It works in both cases. After a bit of trial I figured out how to search for resources which have annotations. I also realised what was mentioned already concerning the hide/show properties option, that it doesn't work for single annotations but for all on a certain page. It took me a little bit of time to figure out which polygon corresponds to which annotation below in cases where there were e.g. three green polygons.

What I wonder about: would it be possible easily to avoid that the html color code is printed so prominently on top of the color in the case of all color values? To me it's rather distracting.
Bildschirmfoto 2021-05-25 um 19 53 16

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 25, 2021

What I wonder about: would it be possible easily to avoid that the html color code is printed so prominently on top of the color in the case of all color values? To me it's rather distracting.

I totally agree. The color value is done in the value component and so, it has to be resolved in an other task. Would you like to create an issue on youtrack? Thanks

Loading

@gautschr
Copy link
Contributor

@gautschr gautschr commented May 26, 2021

Yes, I'll create a new task.

Loading

@kilchenmann kilchenmann requested a review from gautschr May 26, 2021
@kilchenmann kilchenmann merged commit 86e75b9 into main May 26, 2021
8 checks passed
Loading
@kilchenmann kilchenmann deleted the wip/dsp-1585-region-annotations branch May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants