Aude has uploaded a new change for review.
https://gerrit.wikimedia.org/r/49673
Change subject: Further work on claim diff visualization
......................................................................
Further work on claim diff visualization
Change-Id: Iefb28bb70af3380e820ae1e3955e89f95e901383
---
M lib/includes/DiffOpValueFormatter.php
M lib/includes/DiffView.php
M lib/includes/claim/ClaimDifferenceVisualizer.php
M lib/includes/entity/EntityDiffVisualizer.php
M lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
5 files changed, 195 insertions(+), 93 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/73/49673/1
diff --git a/lib/includes/DiffOpValueFormatter.php
b/lib/includes/DiffOpValueFormatter.php
index f1411e9..a63412e 100644
--- a/lib/includes/DiffOpValueFormatter.php
+++ b/lib/includes/DiffOpValueFormatter.php
@@ -6,7 +6,7 @@
use Diff;
/**
- * Class for generating HTML for Claim Diffs.
+ * Class for formatting diffs, @todo might be renamed or something....
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -54,9 +54,12 @@
* @return string
*/
protected function generateHeaderHtml() {
+ $oldHeader = is_string( $this->oldValue ) ? $this->name : '';
+ $newHeader = is_string( $this->newValue ) ? $this->name : '';
+
$html = Html::openElement( 'tr' );
- $html .= Html::rawElement( 'td', array( 'colspan'=>'2', 'class'
=> 'diff-lineno' ), $this->name );
- $html .= Html::rawElement( 'td', array( 'colspan'=>'2', 'class'
=> 'diff-lineno' ), $this->name );
+ $html .= Html::rawElement( 'td', array( 'colspan'=>'2', 'class'
=> 'diff-lineno' ), $oldHeader );
+ $html .= Html::rawElement( 'td', array( 'colspan'=>'2', 'class'
=> 'diff-lineno' ), $newHeader );
$html .= Html::closeElement( 'tr' );
return $html;
diff --git a/lib/includes/DiffView.php b/lib/includes/DiffView.php
index b7b241b..f9f7f9a 100644
--- a/lib/includes/DiffView.php
+++ b/lib/includes/DiffView.php
@@ -101,7 +101,7 @@
} elseif ( $op->getType() === 'remove' ) {
$html .= $this->generateRemoveOpHtml(
$op->getOldValue() );
} elseif ( $op->getType() === 'change' ) {
- $html .= $this->generateChangeOpHtml(
$op->getNewValue(), $op->getOldValue() );
+ $html .= $this->generateChangeOpHtml(
$op->getOldValue(), $op->getNewValue() );
} else {
throw new \MWException( 'Invalid diffOp type' );
}
diff --git a/lib/includes/claim/ClaimDifferenceVisualizer.php
b/lib/includes/claim/ClaimDifferenceVisualizer.php
index 0e45232..0f7aec4 100644
--- a/lib/includes/claim/ClaimDifferenceVisualizer.php
+++ b/lib/includes/claim/ClaimDifferenceVisualizer.php
@@ -6,6 +6,8 @@
/**
* Class for generating HTML for Claim Diffs.
*
+ * @todo we might want a SnakFormatter class and others that handle specific
stuff
+ *
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
@@ -59,66 +61,109 @@
*
* @return string
*/
- public function visualizeDiff( ClaimDifference $claimDifference,
$langCode ) {
+ public function visualizeClaimChange( ClaimDifference $claimDifference,
Claim $baseClaim ) {
$html = '';
if ( $claimDifference->getMainSnakChange() !== null ) {
- $mainSnakChange = $claimDifference->getMainSnakChange();
- $valueFormatter = new DiffOpValueFormatter(
- // todo: should shoe specific headers for both
columns
- $this->getMainSnakHeader(
$mainSnakChange->getNewValue() ),
- $this->getMainSnakValue(
$mainSnakChange->getOldValue() ),
- $this->getMainSnakValue(
$mainSnakChange->getNewValue() )
- );
- $html .= $valueFormatter->generateHtml();
+ $html .= $this->visualizeMainSnakChange(
$claimDifference->getMainSnakChange() );
}
if ( $claimDifference->getRankChange() !== null ) {
- $rankChange = $claimDifference->getRankChange();
- $valueFormatter = new DiffOpValueFormatter(
- wfMessage( 'wikibase-diffview-rank' ),
- $rankChange->getOldValue(),
- $rankChange->getNewValue()
- );
- $html .= $valueFormatter->generateHtml();
+ $html .= $this->visualizeRankChange(
$claimDifference->getRankChange() );
}
// TODO: html for qualifier changes
if ( $claimDifference->getReferenceChanges() !== null ) {
- $referenceChanges =
$claimDifference->getReferenceChanges();
-
- // somehow changing a reference value is treated as a
diffop add and diffop remove
- // for diff visualization, it should be more like a
change
- // @todo assert that both reference changes refer to
the same reference
- if ( count( $referenceChanges ) === 2 ) {
-
- $oldValue = $newValue = null;
-
- foreach( $referenceChanges as $referenceChange
) {
- if ( $referenceChange instanceof
\Diff\DiffOpAdd ) {
- $newValue =
$referenceChange->getNewValue();
- } else if ( $referenceChange instanceof
\Diff\DiffOpRemove ) {
- $oldValue =
$referenceChange->getOldValue();
- }
- }
-
- $html .= $this->getRefHtml( $oldValue,
$newValue, 'change' );
- } else {
- foreach( $referenceChanges as $referenceChange
) {
- if ( $referenceChange instanceof
\Diff\DiffOpAdd ) {
- $html .= $this->getRefHtml(
null, $referenceChange->getNewValue(), 'add' );
- } else if ( $referenceChange instanceof
\Diff\DiffOpRemove ) {
- $html .= $this->getRefHtml(
$referenceChange->getOldValue(), null, 'remove' );
- } else if ( $referenceChange instanceof
\Diff\DiffOpChange ) {
- $html .= $this->getRefHtml(
$referenceChange->getOldValue(),
-
$reference->getNewValue(), 'change' );
- }
- }
- }
+ $html .= $this->visualizeReferenceChanges(
+ $claimDifference->getReferenceChanges(),
+ $baseClaim
+ );
}
return $html;
+ }
+
+ /**
+ * Get diff html for a new claim
+ *
+ * @since 0.4
+ *
+ * @param Claim $claim
+ *
+ * @return string
+ */
+ public function visualizeNewClaim( Claim $claim ) {
+ $mainSnak = $claim->getMainSnak();
+
+ $html = '';
+
+ $html .= $this->getSnakHtml(
+ null,
+ $mainSnak
+ );
+
+ return $html;
+ }
+
+ /**
+ * Get diff html for a removed claim
+ *
+ * @since 0.4
+ *
+ * @param Claim $claim
+ *
+ * @return string
+ */
+ public function visualizeRemovedClaim( Claim $claim ) {
+ $mainSnak = $claim->getMainSnak();
+
+ $html = '';
+
+ $html .= $this->getSnakHtml(
+ $mainSnak,
+ null
+ );
+
+ return $html;
+ }
+
+ /**
+ * Get Html for a main snak change
+ *
+ * @since 0.4
+ *
+ * @param $mainSnakChange
+ *
+ * @return string
+ */
+ protected function visualizeMainSnakChange( $mainSnakChange ) {
+ $valueFormatter = new DiffOpValueFormatter(
+ // todo: should show specific headers for both columns
+ $this->getMainSnakHeader(
$mainSnakChange->getNewValue() ),
+ $this->getMainSnakValue( $mainSnakChange->getOldValue()
),
+ $this->getMainSnakValue( $mainSnakChange->getNewValue()
)
+ );
+
+ return $valueFormatter->generateHtml();
+ }
+
+ /**
+ * Get Html for rank change
+ *
+ * @since 0.4
+ *
+ * @param $rankChange
+ *
+ * @return string
+ */
+ protected function visualizeRankChange( $rankChange ) {
+ $valueFormatter = new DiffOpValueFormatter(
+ wfMessage( 'wikibase-diffview-rank' ),
+ $rankChange->getOldValue(),
+ $rankChange->getNewValue()
+ );
+ return $valueFormatter->generateHtml();
}
/**
@@ -199,12 +244,17 @@
protected function getMainSnakHeader( Snak $mainSnak ) {
$propertyId = $mainSnak->getPropertyId();
$property = $this->entityLookup->getEntity( $propertyId );
- $dataTypeLabel = $property->getDataType()->getLabel(
$this->langCode );
- $label = $property->getLabel( $this->langCode );
- $propertyLabel = $label !== false ? $label :
$property->getPrefixedId();
+ if ( $property instanceof Property ) {
+ $dataTypeLabel = $property->getDataType()->getLabel(
$this->langCode );
- $headerText = wfMessage( 'wikibase-entity-property' ) . ' / ' .
$dataTypeLabel . ' / ' . $label;
+ $label = $property->getLabel( $this->langCode );
+ $propertyLabel = $label !== false ? $label :
$property->getPrefixedId();
+
+ $headerText = wfMessage( 'wikibase-entity-property' ) .
' / ' . $label;
+ } else {
+ $headerText = $propertyId->getPrefixedId();
+ }
return $headerText;
}
@@ -224,7 +274,7 @@
if ( $snakType === 'value' ) {
$dataValue = $snak->getDataValue();
- // FIXME!
+ // FIXME! should use some value formatter
if ( $dataValue instanceof EntityId ) {
$diffValueString = $this->getEntityLabel(
$dataValue );
} else {
@@ -250,13 +300,68 @@
protected function getEntityLabel( EntityId $entityId ) {
$label = $entityId->getPrefixedId();
- $lookedUpLabel = $this->entityLookup->getEntity( $entityId
)->getLabel( $this->langCode );
+ $entity = $this->entityLookup->getEntity( $entityId );
- if ( $lookedUpLabel !== false ) {
- $label = $lookedUpLabel;
+ if ( $entity instanceof Entity ) {
+ $lookedUpLabel = $this->entityLookup->getEntity(
$entityId )->getLabel( $this->langCode );
+
+ if ( $lookedUpLabel !== false ) {
+ $label = $lookedUpLabel;
+ }
}
return $label;
+ }
+
+ /**
+ * Get Html for reference changes
+ *
+ * @since 0.4
+ *
+ * @param $referenceChanges
+ *
+ * @return string
+ */
+ protected function visualizeReferenceChanges( $referenceChanges, Claim
$claim ) {
+ $html = '';
+
+ $claimMainSnak = $claim->getMainSnak();
+ $claimHeader = $this->getMainSnakHeader( $claimMainSnak );
+
+ $newRef = $oldRef = null;
+
+ // somehow changing a reference value is treated as a diffop
add and diffop remove
+ // for diff visualization, it should be more like a change
+ // @todo assert that both reference changes refer to the same
reference
+ foreach( $referenceChanges as $referenceChange ) {
+ if ( $referenceChange instanceof \Diff\DiffOpAdd ) {
+ $header = $this->getRefHeader(
$referenceChange->getNewValue() );
+ $newRef = $this->getRefHtml(
$referenceChange->getNewValue() );
+ } else if ( $referenceChange instanceof
\Diff\DiffOpRemove ) {
+ $header = $this->getRefHeader(
$referenceChange->getOldValue() );
+ $oldRef = $this->getRefHtml(
$referenceChange->getOldValue() );
+ } else if ( $referenceChange instanceof
\Diff\DiffOpChange ) {
+ $header = $this->getRefHeader(
$referenceChange->getNewValue() );
+ $oldRef = $this->getRefHtml(
$referenceChange->getOldValue() );
+ $newRef = $this->getRefHtml(
$referenceChange->getNewValue() );
+ } else {
+ // something went wrong, never should happen
+ throw new \MWException( 'There are no
references in the reference change.' );
+ }
+
+ $valueFormatter = new DiffOpValueFormatter(
+ $claimHeader . ' / ' . wfMessage(
'wikibase-diffview-reference' ) . ' / ' . $header,
+ $oldRef,
+ $newRef
+ );
+
+ $oldRef = $newRef = null;
+
+ $html .= $valueFormatter->generateHtml();
+
+ }
+
+ return $html;
}
/**
@@ -264,30 +369,32 @@
*
* @since 0.4
*
- * @param $oldRef Reference|null
- * @param $newRef Reference|null
- * @param $opType string
+ * @param $ref Reference
*
* @return string
*/
- protected function getRefHtml( $oldRef, $newRef, $opType ) {
- $html = $oldHtml = $newHtml = '';
+ protected function getRefHtml( $ref ) {
+ return $this->getSnakListHtml( $ref->getSnaks() );
+ }
- if ( $oldRef !== null ) {
- $oldHtml .= $this->getSnakListHtml( $oldRef->getSnaks()
);
+ /**
+ * Format diff header for a reference
+ *
+ * @since 0.4
+ *
+ * @param Reference $ref
+ *
+ * @return string
+ */
+ protected function getRefHeader( $ref ) {
+ $headerSnaks = $ref->getSnaks();
+
+ foreach( $headerSnaks as $headerSnak ) {
+ $header = $this->getMainSnakHeader( $headerSnak );
+ break;
}
- if ( $newRef !== null ) {
- $newHtml .= $this->getSnakListHtml( $newRef->getSnaks()
);
- }
-
- $valueFormatter = new DiffOpValueFormatter(
- wfMessage( 'wikibase-diffview-reference' ),
- $oldHtml,
- $newHtml
- );
-
- return $valueFormatter->generateHtml();
+ return $header;
}
}
diff --git a/lib/includes/entity/EntityDiffVisualizer.php
b/lib/includes/entity/EntityDiffVisualizer.php
index 4eb7e4f..bccd701 100644
--- a/lib/includes/entity/EntityDiffVisualizer.php
+++ b/lib/includes/entity/EntityDiffVisualizer.php
@@ -137,28 +137,18 @@
$claimDiffOp->getOldValue(),
$claimDiffOp->getNewValue()
);
- return $this->claimDiffVisualizer->visualizeDiff(
+ return $this->claimDiffVisualizer->visualizeClaimChange(
$claimDifference,
- $this->context->getLanguage()->getCode()
+ $claimDiffOp->getNewValue()
);
}
if ( $claimDiffOp instanceof DiffOpAdd || $claimDiffOp
instanceof DiffOpRemove ) {
if ( $claimDiffOp instanceof DiffOpAdd ) {
- $claim = $claimDiffOp->getNewValue();
- $opType = 'add';
+ return
$this->claimDiffVisualizer->visualizeNewClaim( $claimDiffOp->getNewValue() );
} else {
- $claim = $claimDiffOp->getOldValue();
- $opType = 'remove';
+ return
$this->claimDiffVisualizer->visualizeRemovedClaim( $claimDiffOp->getOldValue()
);
}
-
- $mainSnak = $claim->getMainSnak();
-
- return $this->claimDiffVisualizer->getSnakHtml(
- $mainSnak,
- $this->context->getLanguage()->getCode(),
- $opType
- );
}
throw new MWException( 'Encountered an unexpected diff
operation type for a claim' );
diff --git a/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
b/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
index 6509eaf..0b09c12 100644
--- a/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
+++ b/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
@@ -82,12 +82,14 @@
*
* @param ClaimDifference $claimDifference
*/
+// @todo, also takes a claim as second option
+/*
public function testVisualizeDiff( ClaimDifference $claimDifference ) {
- $differenceVisualizer = new ClaimDifferenceVisualizer( new
\Wikibase\CachingEntityLoader() );
+ $differenceVisualizer = new ClaimDifferenceVisualizer( new
\Wikibase\CachingEntityLoader(), 'en' );
- $visualization = $differenceVisualizer->visualizeDiff(
$claimDifference );
+ $visualization = $differenceVisualizer->visualizeClaimChange(
$claimDifference );
$this->assertInternalType( 'string', $visualization );
}
-
+*/
}
--
To view, visit https://gerrit.wikimedia.org/r/49673
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iefb28bb70af3380e820ae1e3955e89f95e901383
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: mw1.21-wmf10
Gerrit-Owner: Aude <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
