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.

Sign up for GitHub

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

Show tooltips (rdfs:comment or skos:definition) for custom properties #995

Merged
merged 6 commits into from
Jun 12, 2020
Merged

Show tooltips (rdfs:comment or skos:definition) for custom properties #995

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c66deb
Show tooltips (rdfs:comment or skos:definition) for custom properties…
osma Jun 12, 2020
cbdb8ff
Add missing variable declaration & change variable name to tooltip
osma Jun 12, 2020
5e4ae2c
Add unit test for custom property tooltip/description
osma Jun 12, 2020
8e1aa16
Add unit test for custom property missing a description
osma Jun 12, 2020
161af06
add missing phpdoc parameter declarations
osma Jun 12, 2020
c1ab089
simplify template a little bit by cutting an else expression
osma Jun 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion model/Concept.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,12 @@ public function getProperties()
$propres = new EasyRdf\Resource($prop, $this->graph);
$proplabel = $propres->label($this->getEnvLang()) ? $propres->label($this->getEnvLang()) : $propres->label();

$prophelp = $propres->getLiteral('rdfs:comment|skos:definition', $this->getEnvLang());
if ($prophelp === null) {
// try again without language restriction
$prophelp = $propres->getLiteral('rdfs:comment|skos:definition');
}

// check if the property is one of the well-known properties for which we have a gettext translation
// if it is then we can skip the additional lookups in the default graph
$propkey = (substr($prop, 0, 5) == 'dc11:') ?
Expand Down Expand Up @@ -554,7 +560,7 @@ public function getProperties()
$superprop = EasyRdf\RdfNamespace::shorten($superprop) ? EasyRdf\RdfNamespace::shorten($superprop) : $superprop;
}
$sort_by_notation = $this->vocab->getConfig()->sortByNotation();
$propobj = new ConceptProperty($prop, $proplabel, $superprop, $sort_by_notation);
$propobj = new ConceptProperty($prop, $proplabel, $prophelp, $superprop, $sort_by_notation);

if ($propobj->getLabel() !== null) {
// only display properties for which we have a label
Expand Down
23 changes: 19 additions & 4 deletions model/ConceptProperty.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ class ConceptProperty
private $super;
/** stores the property label */
private $label;
/** stores the property tooltip */
private $tooltip;
/** stores the property values */
private $values;
/** flag whether the values are sorted, as we do lazy sorting */
Expand All @@ -22,10 +24,11 @@ class ConceptProperty
* @param string $prop property type eg. 'rdf:type'.
* @param string $label
*/
public function __construct($prop, $label, $super=null, $sort_by_notation=false)
public function __construct($prop, $label, $tooltip=null, $super=null, $sort_by_notation=false)
Copy link
Contributor

Choose a reason for hiding this comment

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

One could document any new parameter(s) here.
On a side note, it's also a bit risky to add a parameter in the middle of a constructor call parameter list, but in this case it does not cause trouble. It may very well be intended, though, if one thinks it is a more important one to have.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, the phpdoc declaration for the new parameter is missing.

I added the parameter in the middle because in PHP, only the last parameters can be omitted. Since there is a need to call the constructor both with and without the last two parameters, this was the only clean option. I did check all calls to the constructor.

{
$this->prop = $prop;
$this->label = $label;
$this->tooltip = $tooltip;
$this->values = array();
$this->is_sorted = true;
$this->super = $super;
Expand All @@ -47,7 +50,7 @@ public function getLabel()
}

// if not, see if there was a label for the property in the graph
if ($this->label) {
if ($this->label !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a side note: I can see why this was done, but does not - nevertheless - change the code behavior a bit. But one can argument for code quality measures, and I agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied and pasted some of the code for the label method into description and noticed this potential problem, so I fixed it in both places. In PHP, a string containing a zero "0" evaluates as false. It's an unlikely label, I know, but still, the behavior does change a little bit here ;)

return $this->label;
}

Expand All @@ -56,14 +59,26 @@ public function getLabel()
}

/**
* Returns a gettext translation for the property tooltip.
* Returns text for the property tooltip.
* @return string
*/
public function getDescription()
{
$helpprop = $this->prop . "_help";

return gettext($helpprop); // can't use string constant, it'd be picked up by xgettext
// see if we have a translation with the help text
$help = gettext($helpprop);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will prefer Skosmos translations, but that will must likely be OK.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's the intent (same as for property labels)

if ($help != $helpprop) {
return $help;
}

// if not, see if there was a comment/definition for the property in the graph
if ($this->tooltip !== null) {
return $this->tooltip;
}

// when nothing is found, don't show the tooltip at all
return null;
}

/**
Expand Down
6 changes: 6 additions & 0 deletions model/sparql/GenericSparql.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,8 @@ private function generateConceptInfoQuery($uris, $arrayClass, $vocabs) {
?sp ?uri ?op .
?uri ?p ?o .
?p rdfs:label ?proplabel .
?p rdfs:comment ?propcomm .
?p skos:definition ?propdef .
?p rdfs:subPropertyOf ?pp .
?pp rdfs:label ?plabel .
?o a ?ot .
Expand Down Expand Up @@ -422,6 +424,10 @@ private function generateConceptInfoQuery($uris, $arrayClass, $vocabs) {
OPTIONAL {
{ ?p rdfs:label ?proplabel . }
UNION
{ ?p rdfs:comment ?propcomm . }
UNION
{ ?p skos:definition ?propdef . }
UNION
{ ?p rdfs:subPropertyOf ?pp . }
}
OPTIONAL {
Expand Down
28 changes: 28 additions & 0 deletions tests/ConceptPropertyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,34 @@ public function testGetLabelReturnsNullWhenThereIsNoLabel() {
$this->assertEquals(null, $prop->getLabel());
}

/**
* @covers ConceptProperty::getLabel
* @covers ConceptProperty::getDescription
*/
public function testGetDescriptionAndLabelForCustomProperty() {
$vocab = $this->model->getVocabulary('test');
$concepts = $vocab->getConceptInfo('http://www.skosmos.skos/test/ta112', 'en');
$concept = $concepts[0];
$props = $concept->getProperties();
$prop = $props["http://www.skosmos.skos/testprop"];
$this->assertEquals('Skosmos test property', $prop->getLabel());
$this->assertEquals('description for Skosmos test property', $prop->getDescription());
}

/**
* @covers ConceptProperty::getLabel
* @covers ConceptProperty::getDescription
*/
public function testGetDescriptionAndLabelForCustomPropertyMissingDesc() {
$vocab = $this->model->getVocabulary('test-notation-sort');
$concepts = $vocab->getConceptInfo('http://www.skosmos.skos/test/ta0112', 'en');
$concept = $concepts[0];
$props = $concept->getProperties();
$prop = $props["http://www.skosmos.skos/testprop"];
$this->assertEquals('Skosmos test property', $prop->getLabel());
$this->assertEquals(null, $prop->getDescription());
}

/**
* @covers Concept::getProperties
* @covers ConceptProperty::getType
Expand Down
3 changes: 2 additions & 1 deletion tests/test-vocab-data/test.ttl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ meta:TestClass a owl:Class ;
rdfs:label "Test class"@en .

skosmos:testprop a rdf:Property ;
rdfs:label "Skosmos test property"@en .
rdfs:label "Skosmos test property"@en ;
rdfs:comment "description for Skosmos test property"@en .

skos:prefLabel a rdf:Property ;
rdfs:label "preferred label"@en .
Expand Down
2 changes: 1 addition & 1 deletion view/concept-shared.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
{% if property.getSubPropertyOf != 'skos:hiddenLabel' %}
<div class="row{% if property.type == 'dc:isReplacedBy' %} replaced-by{% endif%}">
<div class="property-label">
<span class="versal{% if property.type == 'rdf:type' %}-bold{% endif %}{% if not (property.type in property.description and '_help' in property.description) %} property-click" title="{{ property.description }}"{% else %}"{% endif %}>{{ property.label|upper }}</span>
<span class="versal{% if property.type == 'rdf:type' %}-bold{% endif %}{% if property.description %} property-click" title="{{ property.description }}"{% else %}"{% endif %}>{{ property.label|upper }}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

This could only have an if part.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

</div>
<div class="property-value-column"><div class="property-value-wrapper">
{% if request.vocab.config.hasMultiLingualProperty(property.type) %}
Expand Down