Anja Jentzsch has uploaded a new change for review.
https://gerrit.wikimedia.org/r/51654
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, 241 insertions(+), 116 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/54/51654/1
diff --git a/lib/includes/DiffOpValueFormatter.php
b/lib/includes/DiffOpValueFormatter.php
index f1411e9..1b6cd62 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
@@ -34,12 +34,36 @@
*/
class DiffOpValueFormatter {
+ /**
+ * @since 0.4
+ *
+ * @var string
+ */
protected $name;
+ /**
+ * @since 0.4
+ *
+ * @var string
+ */
protected $oldValue;
+ /**
+ * @since 0.4
+ *
+ * @var string
+ */
protected $newValue;
+ /**
+ * Constructor.
+ *
+ * @since 0.4
+ *
+ * @param string $name
+ * @param string $oldValue
+ * @param string $newValue
+ */
public function __construct( $name, $oldValue, $newValue ) {
$this->name = $name;
$this->oldValue = $oldValue;
@@ -54,9 +78,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::element( 'td', array( 'colspan'=>'2', 'class' =>
'diff-lineno' ), $oldHeader );
+ $html .= Html::element( 'td', array( 'colspan'=>'2', 'class' =>
'diff-lineno' ), $newHeader );
$html .= Html::closeElement( 'tr' );
return $html;
@@ -74,12 +101,12 @@
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-marker' ), '-' );
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-deletedline' ),
Html::rawElement( 'div', array(),
- Html::rawElement( 'del', array( 'class' =>
'diffchange diffchange-inline' ),
+ Html::element( 'del', array( 'class' =>
'diffchange diffchange-inline' ),
$this->oldValue ) ) );
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-marker' ), '+' );
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-addedline' ),
Html::rawElement( 'div', array(),
- Html::rawElement( 'ins', array( 'class' =>
'diffchange diffchange-inline' ),
+ Html::element( 'ins', array( 'class' =>
'diffchange diffchange-inline' ),
$this->newValue ) ) );
$html .= Html::closeElement( 'tr' );
$html .= Html::closeElement( 'tr' );
@@ -100,7 +127,7 @@
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-marker' ), '+' );
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-addedline' ),
Html::rawElement( 'div', array(),
- Html::rawElement( 'ins', array( 'class' =>
'diffchange diffchange-inline' ),
+ Html::element( 'ins', array( 'class' =>
'diffchange diffchange-inline' ),
$this->newValue )
)
);
@@ -121,7 +148,7 @@
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-marker' ), '-' );
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-deletedline' ),
Html::rawElement( 'div', array(),
- Html::rawElement( 'del', array( 'class' =>
'diffchange diffchange-inline' ),
+ Html::element( 'del', array( 'class' =>
'diffchange diffchange-inline' ),
$this->oldValue ) ) );
$html .= Html::rawElement( 'td', array( 'colspan'=>'2' ),
' ' );
$html .= Html::closeElement( 'tr' );
@@ -129,6 +156,13 @@
return $html;
}
+ /**
+ * Generates HTML for a diffOP
+ *
+ * @since 0.4
+ *
+ * @return string
+ */
public function generateHtml() {
$html = '';
diff --git a/lib/includes/DiffView.php b/lib/includes/DiffView.php
index b7b241b..71a97f7 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' );
}
@@ -133,7 +133,7 @@
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-marker' ), '+' );
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-addedline' ),
Html::rawElement( 'div', array(),
- Html::rawElement( 'ins', array( 'class' =>
'diffchange diffchange-inline' ),
+ Html::element( 'ins', array( 'class' =>
'diffchange diffchange-inline' ),
$value ) ) );
$html .= Html::closeElement( 'tr' );
@@ -154,7 +154,7 @@
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-marker' ), '-' );
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-deletedline' ),
Html::rawElement( 'div', array(),
- Html::rawElement( 'del', array( 'class' =>
'diffchange diffchange-inline' ),
+ Html::element( 'del', array( 'class' =>
'diffchange diffchange-inline' ),
$value ) ) );
$html .= Html::rawElement( 'td', array( 'colspan'=>'2' ),
' ' );
$html .= Html::closeElement( 'tr' );
@@ -178,12 +178,12 @@
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-marker' ), '-' );
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-deletedline' ),
Html::rawElement( 'div', array(),
- Html::rawElement( 'del', array( 'class' =>
'diffchange diffchange-inline' ),
+ Html::element( 'del', array( 'class' => 'diffchange
diffchange-inline' ),
$oldValue ) ) );
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-marker' ), '+' );
$html .= Html::rawElement( 'td', array( 'class' =>
'diff-addedline' ),
Html::rawElement( 'div', array(),
- Html::rawElement( 'ins', array( 'class' =>
'diffchange diffchange-inline' ),
+ Html::element( 'ins', array( 'class' =>
'diffchange diffchange-inline' ),
$newValue ) ) );
$html .= Html::closeElement( 'tr' );
$html .= Html::closeElement( 'tr' );
@@ -202,8 +202,8 @@
*/
protected function generateDiffHeaderHtml( $name ) {
$html = Html::openElement( 'tr' );
- $html .= Html::rawElement( 'td', array( 'colspan'=>'2', 'class'
=> 'diff-lineno' ), $name );
- $html .= Html::rawElement( 'td', array( 'colspan'=>'2', 'class'
=> 'diff-lineno' ), $name );
+ $html .= Html::element( 'td', array( 'colspan'=>'2', 'class' =>
'diff-lineno' ), $name );
+ $html .= Html::element( 'td', array( 'colspan'=>'2', 'class' =>
'diff-lineno' ), $name );
$html .= Html::closeElement( 'tr' );
return $html;
diff --git a/lib/includes/claim/ClaimDifferenceVisualizer.php
b/lib/includes/claim/ClaimDifferenceVisualizer.php
index 0e45232..68a8553 100644
--- a/lib/includes/claim/ClaimDifferenceVisualizer.php
+++ b/lib/includes/claim/ClaimDifferenceVisualizer.php
@@ -2,9 +2,12 @@
namespace Wikibase;
use Html;
+use Diff\Diff;
/**
* 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
@@ -53,72 +56,117 @@
}
/**
+ * Generates HTML of a claim change.
* @since 0.4
*
* @param ClaimDifference $claimDifference
+ * @param Claim $baseClaim
*
* @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();
}
/**
@@ -128,7 +176,7 @@
*
* @param Snak|null $oldSnak
* @param Snak|null $newSnak
- * @param string $prependHeader
+ * @param string|null $prependHeader
*
* @return string
*/
@@ -198,13 +246,8 @@
*/
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();
-
- $headerText = wfMessage( 'wikibase-entity-property' ) . ' / ' .
$dataTypeLabel . ' / ' . $label;
+ $propertyLabel = $this->getEntityLabel( $propertyId );
+ $headerText = wfMessage( 'wikibase-entity-property' ) . ' / ' .
$propertyLabel;
return $headerText;
}
@@ -224,7 +267,7 @@
if ( $snakType === 'value' ) {
$dataValue = $snak->getDataValue();
- // FIXME!
+ // FIXME! should use some value formatter
if ( $dataValue instanceof EntityId ) {
$diffValueString = $this->getEntityLabel(
$dataValue );
} else {
@@ -232,10 +275,10 @@
}
return $diffValueString;
+ } else {
+ // TODO: should be differenced visually (e.g. italic)
+ return $snakType;
}
-
- // fixme, error handling for unknown snak value types
- return '';
}
/**
@@ -250,13 +293,70 @@
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 Diff $referenceChanges
+ * @param Claim $claim
+ *
+ * @return string
+ */
+ protected function visualizeReferenceChanges( Diff $referenceChanges,
Claim $claim ) {
+ $html = '';
+
+ $claimMainSnak = $claim->getMainSnak();
+ $claimHeader = $this->getMainSnakHeader( $claimMainSnak );
+
+ $newRef = $oldRef = null;
+
+ // Because references only have hashes and no ids,
+ // 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 +364,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..dcf6538 100644
--- a/lib/includes/entity/EntityDiffVisualizer.php
+++ b/lib/includes/entity/EntityDiffVisualizer.php
@@ -137,31 +137,19 @@
$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';
- } else {
- $claim = $claimDiffOp->getOldValue();
- $opType = 'remove';
- }
-
- $mainSnak = $claim->getMainSnak();
-
- return $this->claimDiffVisualizer->getSnakHtml(
- $mainSnak,
- $this->context->getLanguage()->getCode(),
- $opType
- );
+ if ( $claimDiffOp instanceof DiffOpAdd ) {
+ return $this->claimDiffVisualizer->visualizeNewClaim(
$claimDiffOp->getNewValue() );
+ } elseif ( $claimDiffOp instanceof DiffOpRemove ) {
+ return
$this->claimDiffVisualizer->visualizeRemovedClaim( $claimDiffOp->getOldValue()
);
+ } else {
+ throw new MWException( 'Encountered an unexpected diff
operation type for a claim' );
}
-
- 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..2100de7 100644
--- a/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
+++ b/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
@@ -82,12 +82,13 @@
*
* @param ClaimDifference $claimDifference
*/
- public function testVisualizeDiff( ClaimDifference $claimDifference ) {
- $differenceVisualizer = new ClaimDifferenceVisualizer( new
\Wikibase\CachingEntityLoader() );
+// @todo provide a second parameter for the function
+/* public function testVisualizeDiff( ClaimDifference $claimDifference ) {
+ $differenceVisualizer = new ClaimDifferenceVisualizer( new
\Wikibase\CachingEntityLoader(), 'en' );
$visualization = $differenceVisualizer->visualizeDiff(
$claimDifference );
$this->assertInternalType( 'string', $visualization );
}
-
+*/
}
--
To view, visit https://gerrit.wikimedia.org/r/51654
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-wmf11
Gerrit-Owner: Anja Jentzsch <anja.jentz...@wikimedia.de>
Gerrit-Reviewer: Aude <aude.w...@gmail.com>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
