Anja Jentzsch has submitted this change and it was merged. 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(-) Approvals: Anja Jentzsch: Verified; Looks good to me, approved 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: merged 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: Anja Jentzsch <anja.jentz...@wikimedia.de> Gerrit-Reviewer: Aude <aude.w...@gmail.com> Gerrit-Reviewer: jenkins-bot _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits