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

DSP-1002 Update standoff link value #228

Merged
merged 22 commits into from
Nov 19, 2020

Conversation

tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Nov 13, 2020

resolves DSP-1002

When adding an internal link to a text value, Knora creates a corresponding standoff link value.
When clicking on or hovering over a standoff link, this information can be used to emit the ReadLinkValue. Since this requires access to the standoff link value from the text value, @Input() propArray: PropertyInfoValues[]; was added to DisplayEditComponent.

@tobiasschweizer tobiasschweizer added the enhancement New feature or request label Nov 13, 2020
@tobiasschweizer tobiasschweizer self-assigned this Nov 13, 2020
@tobiasschweizer tobiasschweizer added the breaking Breaking change label Nov 16, 2020
@tobiasschweizer
Copy link
Contributor Author

@mdelez Since updating the standoff link value is a bit complex, I think I should mention it in the design docs. But please go ahead with the review.

@tobiasschweizer
Copy link
Contributor Author

@mdelez Feel free to correct the English in 192a732

Copy link
Contributor

@mdelez mdelez left a comment

Choose a reason for hiding this comment

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

Nice! Just a little comment/question :)

private _updateStandoffLinkValue() {

if (this.resource === undefined) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

should this throw an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should actually never occur: if the user is able to click on a standoff link, then the resource must have been initialised before in resource-view comp.

I added a comment in 76e251a

However, this is an easy way to know that this.resource.id won't cause an error and that no malformed query will be sent to Knora.

If the method is left immediately, then simply the standoff link value would not be updated.

I implemented DisplayEditComponent._getStandoffLinkValueForResource so it would not fail if it cannot find the standoff link value, see:

/**
* Given a resource Iri, finds the corresponding standoff link value.
* Returns an empty array if the standoff link cannot be found.
*
* @param resIri the Iri of the resource.
*/
private _getStandoffLinkValueForResource(resIri: string): ReadLinkValue[] {
// find the PropertyInfoValues for the standoff link value
const standoffLinkPropInfoVals: PropertyInfoValues[] = this.propArray.filter(
resPropInfoVal => {
return resPropInfoVal.propDef.id === "http://api.knora.org/ontology/knora-api/v2#hasStandoffLinkToValue";
}
);
if (standoffLinkPropInfoVals.length === 1) {
// find the corresponding standoff link value
const referredResStandoffLinkVal: ReadValue[] = standoffLinkPropInfoVals[0].values.filter(
(standoffLinkVal: ReadValue) => {
return standoffLinkVal instanceof ReadLinkValue
&& (standoffLinkVal as ReadLinkValue).linkedResourceIri === resIri;
}
);
// if no corresponding standoff link value was found,
// this array is empty
return referredResStandoffLinkVal as ReadLinkValue[];
} else {
// this should actually never happen
// because all resource types have a cardinality for a standoff link value
return [];
}
}

Now if it cannot find a corresponding standoff link value, there will simply be no event emitted:

/**
* React when a standoff link in a text has received a click event.
*
* @param resIri the Iri of the resource the standoff link refers to.
*/
standoffLinkClicked(resIri: string): void {
// find the corresponding standoff link value
const referredResStandoffLinkVal: ReadLinkValue[] = this._getStandoffLinkValueForResource(resIri);
// only emit an event if the corresponding standoff link value could be found
if (referredResStandoffLinkVal.length === 1) {
this.referredResourceClicked.emit(referredResStandoffLinkVal[0]);
}
}
/**
* React when a standoff link in a text has received a hover event.
*
* @param resIri the Iri of the resource the standoff link refers to.
*/
standoffLinkHovered(resIri: string): void {
// find the corresponding standoff link value
const referredResStandoffLinkVal: ReadLinkValue[] = this._getStandoffLinkValueForResource(resIri);
// only emit an event if the corresponding standoff link value could be found
if (referredResStandoffLinkVal.length === 1) {
this.referredResourceHovered.emit(referredResStandoffLinkVal[0]);
}
}

This could be the case because of timing. The standoff link values are only queried after the values have been updated, and it is also async. And it that case the user would just click again and by then the values should be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will write a test that makes sure that display-edit does not break when there is no corresponding standoff link value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdelez I added two tests in 3812be6


// one resource is expected
if (res.resources.length !== 1) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should never happen since the Optional is used: also if there is no standoff link value, the resource should be returned. However, being paranoid res.resources[0] without an assertion seems dangerous to me.

Copy link
Contributor Author

@tobiasschweizer tobiasschweizer Nov 18, 2020

Choose a reason for hiding this comment

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

let's discus the whole flow of data together today or tomorrow to make sure I did not make an error in reasoning.

@tobiasschweizer
Copy link
Contributor Author

@mdelez I actually found the test for "non reacting" to be too weak and improved them in 112bf76

I set the init value of the parent comp. to "init" so I can check that no undefined was emitted by mistake (if the code would accidentally access the first entry of an empty array).

@tobiasschweizer tobiasschweizer merged commit 99c9539 into main Nov 19, 2020
@tobiasschweizer tobiasschweizer deleted the wip/dsp-1002-update-standoff-link-value branch November 19, 2020 12:31
@kilchenmann kilchenmann changed the title Update standoff link value DSP-1002 Update standoff link value Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants