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): draw regions (DSP-1845) #524

Merged
merged 15 commits into from Sep 15, 2021
Merged

Conversation

@EricSommerhalder
Copy link
Contributor

@EricSommerhalder EricSommerhalder commented Sep 7, 2021

resolves DSP-1845

@EricSommerhalder EricSommerhalder changed the title feat(resource): Draw regions (DSP-1845) feat(resource): draw regions (DSP-1845) Sep 7, 2021
Copy link
Collaborator

@kilchenmann kilchenmann left a comment

The code looks good, but I also want to try it

Loading

src/app/workspace/resource/resource.component.html Outdated Show resolved Hide resolved
Loading
<form [formGroup]="regionForm">
<mat-form-field>
<mat-label>Comment</mat-label>
<input matInput formControlName="comment">
Copy link
Collaborator

@kilchenmann kilchenmann Sep 9, 2021

Choose a reason for hiding this comment

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

Comment field should be a textarea. But I think the form style could also be done in a separate, style refactoring PR.

Loading

Copy link
Contributor Author

@EricSommerhalder EricSommerhalder Sep 9, 2021

Choose a reason for hiding this comment

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

I changed it to a textarea for now, but I agree that a style refactoring PR will be needed.

Loading

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented Sep 9, 2021

After creating a region, it does not update the resource viewer. Only after refreshing the page I got the region in the annotation tab.

And for any reason, I don't know why, it does not display the "is region of" in the region properties. I've checked and it's not an issue of the create region function. This has to be resolved in another PR; in the same PR we should also fix the typo in the german label "is Region von".

Screen Shot 2021-09-09 at 07 47 23

Loading

ngOnInit(): void {
this.regionForm = this._fb.group({
color: ['#ff3333', [Validators.required, Validators.pattern(this.colorPattern)]],
comment: [null, Validators.required],
Copy link
Contributor

@mdelez mdelez Sep 9, 2021

Choose a reason for hiding this comment

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

is a comment actually required to submit the region?

Loading

Copy link
Contributor Author

@EricSommerhalder EricSommerhalder Sep 9, 2021

Choose a reason for hiding this comment

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

Yes, otherwise the api is not happy. Not sure why though.

Loading

@@ -91,12 +114,14 @@ export class StillImageComponent implements OnChanges, OnDestroy {

@Output() regionClicked = new EventEmitter<string>();

private _regionDrawMode: Boolean = false; // stores whether viewer is currently drawing a region
private _regionDragInfo; // stores the information of the first click for drawing a region
private _mouseTracker; // stores the MouseTracker that allows drawing regions
Copy link
Contributor

@mdelez mdelez Sep 9, 2021

Choose a reason for hiding this comment

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

is this used anywhere? My IDE says it's unused so it can probably be removed

Loading

Copy link
Contributor Author

@EricSommerhalder EricSommerhalder Sep 9, 2021

Choose a reason for hiding this comment

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

At some point this got the whole thing running, but apparently it is not needed anymore. I have removed it.

Loading

@mdelez
Copy link
Contributor

@mdelez mdelez commented Sep 9, 2021

After creating a region, it does not update the resource viewer. Only after refreshing the page I got the region in the annotation tab.

+1 this should be handled in this PR. I've also noticed that editing the color of the region afterwards does not change the actual color of the region, even after refreshing but this can be handled in a different PR.

Loading

mdelez
mdelez approved these changes Sep 14, 2021
Copy link
Contributor

@mdelez mdelez left a comment

Thanks!

Loading

@EricSommerhalder EricSommerhalder merged commit f08706b into main Sep 15, 2021
12 checks passed
Loading
@EricSommerhalder EricSommerhalder deleted the wip/DSP-1845-draw-regions branch Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants