Addshore has uploaded a new change for review.
https://gerrit.wikimedia.org/r/90876
Change subject: Adding and removing claims now shows a full diff
......................................................................
Adding and removing claims now shows a full diff
Until now if you added a new claim or removed a claim
the diff would only show the mainsnak.
This change means the whole claim appears in the diff
when it is removed.
This also adds test for the ClaimDifferenceVisualizer
There were no tests actually running in this class
before as they were commented out...
We also remove a functions that is no longer used
Bug: 53142
Change-Id: Ib8709a7b5a1cc3e2ba32bc9057154ced1ff2d334
---
M lib/includes/ClaimDiffer.php
M lib/includes/ClaimDifferenceVisualizer.php
M lib/tests/phpunit/claim/ClaimDifferTest.php
M lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
4 files changed, 223 insertions(+), 138 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/76/90876/1
diff --git a/lib/includes/ClaimDiffer.php b/lib/includes/ClaimDiffer.php
index 9a23c38..4d1718c 100644
--- a/lib/includes/ClaimDiffer.php
+++ b/lib/includes/ClaimDiffer.php
@@ -39,38 +39,42 @@
*
* @since 0.4
*
- * @param Claim $oldClaim
- * @param Claim $newClaim
+ * @param Claim|null $oldClaim
+ * @param Claim|null $newClaim
*
* @return ClaimDifference
*/
- public function diffClaims( Claim $oldClaim, Claim $newClaim ) {
+ public function diffClaims( $oldClaim, $newClaim ) {
$mainSnakChange = null;
+ $qualifierChanges = null;
$rankChange = null;
$referenceChanges = null;
- if ( !$oldClaim->getMainSnak()->equals(
$newClaim->getMainSnak() ) ) {
- $mainSnakChange = new DiffOpChange(
$oldClaim->getMainSnak(), $newClaim->getMainSnak() );
+ $oldClaimMainSnak = $oldClaim === null ? null :
$oldClaim->getMainSnak();
+ $newClaimMainSnak = $newClaim === null ? null :
$newClaim->getMainSnak();
+ if( $oldClaimMainSnak !== $newClaimMainSnak ){
+ $mainSnakChange = new DiffOpChange( $oldClaimMainSnak,
$newClaimMainSnak );
}
- $oldQualifiers = iterator_to_array( $oldClaim->getQualifiers()
);
- $newQualifiers = iterator_to_array( $newClaim->getQualifiers()
);
- $qualifierChanges = new Diff( $this->listDiffer->doDiff(
- $oldQualifiers,
- $newQualifiers
- ), false );
+ $oldQualifiers = $oldClaim === null ? array() :
iterator_to_array( $oldClaim->getQualifiers() );
+ $newQualifiers = $newClaim === null ? array() :
iterator_to_array( $newClaim->getQualifiers() );
+ if( $oldQualifiers !== $newQualifiers ){
+ $qualifierChanges = new Diff(
$this->listDiffer->doDiff( $oldQualifiers, $newQualifiers ), false );
+ }
- if ( $oldClaim instanceof Statement && $newClaim instanceof
Statement ) {
-
- if ( $oldClaim->getRank() !== $newClaim->getRank() ) {
- $rankChange = new DiffOpChange(
$oldClaim->getRank(), $newClaim->getRank() );
+ if ( $oldClaim instanceof Statement || $newClaim instanceof
Statement ) {
+ $oldRank = $oldClaim === null ? null :
$oldClaim->getRank();
+ $newRank = $newClaim === null ? null :
$newClaim->getRank();
+ if( $oldRank !== $newRank ){
+ $rankChange = new DiffOpChange( $oldRank,
$newRank );
}
- $referenceChanges = new Diff( $this->listDiffer->doDiff(
- iterator_to_array( $oldClaim->getReferences() ),
- iterator_to_array( $newClaim->getReferences() )
- ), false );
+ $oldReferences = $oldClaim === null ? array() :
iterator_to_array( $oldClaim->getReferences() );
+ $newReferences = $newClaim === null ? array() :
iterator_to_array( $newClaim->getReferences() );
+ if( $oldReferences !== $newReferences ){
+ $referenceChanges = new Diff(
$this->listDiffer->doDiff( $oldReferences, $newReferences ), false );
+ }
}
return new ClaimDifference( $mainSnakChange, $qualifierChanges,
$referenceChanges, $rankChange );
diff --git a/lib/includes/ClaimDifferenceVisualizer.php
b/lib/includes/ClaimDifferenceVisualizer.php
index f37a0da..fb6e8b1 100644
--- a/lib/includes/ClaimDifferenceVisualizer.php
+++ b/lib/includes/ClaimDifferenceVisualizer.php
@@ -5,8 +5,11 @@
use Diff\DiffOpAdd;
use Diff\DiffOpChange;
use Diff\DiffOpRemove;
+use Diff\ListDiffer;
use Html;
use Diff\Diff;
+use InvalidArgumentException;
+use MWException;
use RuntimeException;
use ValueParsers\FormattingException;
use Wikibase\Lib\EntityIdLabelFormatter;
@@ -17,12 +20,10 @@
*
* @since 0.4
*
- * @file
- * @ingroup WikibaseLib
- *
* @licence GNU GPL v2+
* @author Tobias Gritschacher < [email protected] >
* @author Katie Filbert < [email protected] >
+ * @author Adam Shorland
*/
class ClaimDifferenceVisualizer {
@@ -48,11 +49,11 @@
* @param EntityIdLabelFormatter $propertyIdFormatter
* @param SnakFormatter $snakFormatter
*
- * @throws \InvalidArgumentException
+ * @throws InvalidArgumentException
*/
public function __construct( EntityIdLabelFormatter
$propertyIdFormatter, SnakFormatter $snakFormatter ) {
if ( $snakFormatter->getFormat() !==
SnakFormatter::FORMAT_PLAIN ) {
- throw new \InvalidArgumentException(
+ throw new InvalidArgumentException(
'Expected $snakFormatter to generate plain
text, not '
. $snakFormatter->getFormat() );
}
@@ -109,16 +110,9 @@
* @return string
*/
public function visualizeNewClaim( Claim $claim ) {
- $mainSnak = $claim->getMainSnak();
-
- $html = '';
-
- $html .= $this->getSnakHtml(
- null,
- $mainSnak
- );
-
- return $html;
+ $claimDiffer = new ClaimDiffer( new ListDiffer() );
+ $claimDifference = $claimDiffer->diffClaims( null, $claim );
+ return $this->visualizeClaimChange( $claimDifference, $claim );
}
/**
@@ -131,16 +125,9 @@
* @return string
*/
public function visualizeRemovedClaim( Claim $claim ) {
- $mainSnak = $claim->getMainSnak();
-
- $html = '';
-
- $html .= $this->getSnakHtml(
- $mainSnak,
- null
- );
-
- return $html;
+ $claimDiffer = new ClaimDiffer( new ListDiffer() );
+ $claimDifference = $claimDiffer->diffClaims( $claim, null );
+ return $this->visualizeClaimChange( $claimDifference, $claim );
}
/**
@@ -153,9 +140,15 @@
* @return string
*/
protected function visualizeMainSnakChange( DiffOpChange
$mainSnakChange ) {
+ if( $mainSnakChange->getNewValue() === null ){
+ $snakHeader = $this->getSnakHeader(
$mainSnakChange->getOldValue() );
+ } else {
+ $snakHeader = $this->getSnakHeader(
$mainSnakChange->getNewValue() );
+ }
+
$valueFormatter = new DiffOpValueFormatter(
// todo: should show specific headers for both columns
- $this->getSnakHeader( $mainSnakChange->getNewValue() ),
+ $snakHeader,
$this->formatSnak( $mainSnakChange->getOldValue() ),
$this->formatSnak( $mainSnakChange->getNewValue() )
);
@@ -182,54 +175,14 @@
}
/**
- * Format a snak
- *
- * @since 0.4
- *
- * @param Snak|null $oldSnak
- * @param Snak|null $newSnak
- * @param string|null $prependHeader
- *
- * @throws \MWException
- * @return string
- */
- public function getSnakHtml( $oldSnak, $newSnak, $prependHeader = null
) {
- $snakHeader = '';
- // @todo fix ugly cruft!
- if ( $prependHeader !== null ) {
- $snakHeader = $prependHeader;
- }
-
- if ( $newSnak instanceof Snak || $oldSnak instanceof Snak ) {
- $headerSnak = $newSnak instanceof Snak ? $newSnak :
$oldSnak;
- $snakHeader .= $this->getSnakHeader( $headerSnak );
- } else {
- // something went wrong
- throw new \MWException( 'Snak parameters not provided.'
);
- }
-
- $oldValue = null;
- $newValue = null;
-
- if ( $oldSnak instanceof Snak ) {
- $oldValue = $this->formatSnak( $oldSnak );
- }
-
- if ( $newSnak instanceof Snak ) {
- $newValue = $this->formatSnak( $newSnak );
- }
-
- $valueFormatter = new DiffOpValueFormatter( $snakHeader,
$oldValue, $newValue );
-
- return $valueFormatter->generateHtml();
- }
-
- /**
- * @param Snak $snak
+ * @param Snak|null $snak
*
* @return string
*/
- protected function formatSnak( Snak $snak ) {
+ protected function formatSnak( $snak ) {
+ if( $snak === null ){
+ return null;
+ }
try {
return $this->snakFormatter->formatSnak( $snak );
} catch ( FormattingException $ex ) {
@@ -263,6 +216,7 @@
$values = array();
foreach ( $snakList as $snak ) {
+ /** @var $snak Snak */
// TODO: change hardcoded ": " so something like
wfMessage( 'colon-separator' ),
// but this will require further refactoring as it
would add HTML which gets escaped
$values[] =
@@ -296,7 +250,7 @@
*
* @since 0.4
*
- * @param Diff[] $changes
+ * @param Diff $changes
* @param Claim $claim
* @param string $breadCrumb
*
diff --git a/lib/tests/phpunit/claim/ClaimDifferTest.php
b/lib/tests/phpunit/claim/ClaimDifferTest.php
index 2bb71dd..ba42032 100644
--- a/lib/tests/phpunit/claim/ClaimDifferTest.php
+++ b/lib/tests/phpunit/claim/ClaimDifferTest.php
@@ -122,7 +122,7 @@
$this->assertInstanceOf( 'Wikibase\ClaimDifference', $actual );
if ( !$expected->equals( $actual ) ) {
- q($expected, $actual);
+ $this->assertEquals($expected, $actual);
}
$this->assertTrue(
diff --git a/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
b/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
index ef7aabb..fd98435 100644
--- a/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
+++ b/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php
@@ -7,79 +7,206 @@
use Diff\DiffOpAdd;
use Diff\DiffOpChange;
use Diff\DiffOpRemove;
+use Wikibase\Claim;
use Wikibase\ClaimDifference;
+use Wikibase\ClaimDifferenceVisualizer;
+use Wikibase\DataModel\Entity\PropertyId;
+use Wikibase\Lib\EntityIdLabelFormatter;
+use Wikibase\Lib\SnakFormatter;
use Wikibase\PropertyNoValueSnak;
use Wikibase\PropertySomeValueSnak;
use Wikibase\PropertyValueSnak;
use Wikibase\Reference;
+use Wikibase\ReferenceList;
+use Wikibase\Repo\WikibaseRepo;
use Wikibase\SnakList;
use Wikibase\Statement;
/**
* @covers Wikibase\ClaimDifferenceVisualizer
*
- * @since 0.4
- *
* @group Wikibase
* @group WikibaseLib
* @group WikibaseClaim
*
* @licence GNU GPL v2+
- * @author Jeroen De Dauw < [email protected] >
+ * @author Adam Shorland
*/
class ClaimDifferenceVisualizerTest extends \MediaWikiTestCase {
- public function visualizeDiffProvider() {
- $differences = array();
+ public function newSnakFormatter( $format = SnakFormatter::FORMAT_PLAIN
){
+ $instance = $this->getMock( 'Wikibase\Lib\SnakFormatter' );
+ $instance->expects( $this->atLeastOnce() )
+ ->method( 'getFormat' )
+ ->will( $this->returnValue( $format ) );
+ $instance->expects( $this->any() )
+ ->method( 'canFormatSnak' )
+ ->will( $this->returnValue( true ) );
+ $instance->expects( $this->any() )
+ ->method( 'formatSnak' )
+ ->will( $this->returnValue( 'SNAK' ) );
+ return $instance;
+ }
- $differences[] = new ClaimDifference();
+ public function newEntityIdLabelFormatter(){
+ $instance = $this
+ ->getMockBuilder( 'Wikibase\Lib\EntityIdLabelFormatter'
)
+ ->disableOriginalConstructor()
+ ->getMock();
- $differences[] = new ClaimDifference(
- new DiffOpChange(
- new PropertyNoValueSnak( 42 ),
- new PropertyNoValueSnak( 43 )
+ $instance->expects( $this->any() )
+ ->method( 'format' )
+ ->will( $this->returnValue( 'PID' ) );
+
+ return $instance;
+ }
+
+ public function newClaimDifferenceVisualizer(){
+ return new ClaimDifferenceVisualizer(
+ $this->newEntityIdLabelFormatter(),
+ $this->newSnakFormatter()
+ );
+ }
+
+ public function testConstruction(){
+ $instance = $this->newClaimDifferenceVisualizer();
+ $this->assertInstanceOf( 'Wikibase\ClaimDifferenceVisualizer',
$instance );
+ }
+
+ public function testConstructionWithBadFormatter(){
+ $this->setExpectedException( 'InvalidArgumentException' );
+ new ClaimDifferenceVisualizer(
+ $this->newEntityIdLabelFormatter(),
+ $this->newSnakFormatter( 'qwertyuiop' )
+ );
+ }
+
+ //TODO come up with a better way of testing this.... EWW at all the
html...
+ public function provideDifferenceAndClaim(){
+ return array(
+ //0 no change
+ array(
+ new ClaimDifference(),
+ new Claim( new PropertyValueSnak( new
PropertyId( 'P1' ), new StringValue( 'foo' ) ) ),
+ ''
),
- new Diff( array(
- new DiffOpAdd( new PropertySomeValueSnak( 44 )
),
- new DiffOpRemove( new PropertyValueSnak( 45,
new StringValue( 'foo' ) ) ),
- new DiffOpChange( new PropertySomeValueSnak( 46
), new PropertySomeValueSnak( 47 ) ),
- ) )
- );
-
- $differences[] = new ClaimDifference(
- new DiffOpChange(
- new PropertyNoValueSnak( 42 ),
- new PropertyNoValueSnak( 43 )
+ //1 mainsnak
+ array(
+ new ClaimDifference(
+ new DiffOpChange(
+ new PropertyValueSnak( new
PropertyId( 'P1' ), new StringValue( 'bar' ) ),
+ new PropertyValueSnak( new
PropertyId( 'P1' ), new StringValue( 'foo' ) )
+ )
+ ),
+ new Claim( new PropertyValueSnak( new
PropertyId( 'P1' ), new StringValue( 'foo' ) ) ),
+ '<tr><td colspan="2"
class="diff-lineno">property / PID</td><td colspan="2"
class="diff-lineno">property / PID</td></tr>'.
+ '<tr><td class="diff-marker">-</td><td
class="diff-deletedline">'.
+ '<div><del class="diffchange
diffchange-inline"><span>SNAK</span></del></div></td>'.
+ '<td class="diff-marker">+</td><td
class="diff-addedline">'.
+ '<div><ins class="diffchange
diffchange-inline"><span>SNAK</span></ins></div></td></tr></tr>'
),
- null,
- new Diff( array(
- new DiffOpAdd( new Reference() ),
- new DiffOpRemove( new Reference( new SnakList(
array( new PropertyNoValueSnak( 50 ) ) ) ) ),
- ) )
+ //2 +qualifiers
+ array(
+ new ClaimDifference(
+ null,
+ new Diff( array(
+ new DiffOpAdd( new
PropertySomeValueSnak( 44 ) ),
+ ) )
+ ),
+ new Claim( new PropertyValueSnak( new
PropertyId( 'P1' ), new StringValue( 'foo' ) ) ),
+ '<tr><td colspan="2"
class="diff-lineno"></td><td colspan="2" class="diff-lineno">property / PID /
qualifier</td></tr>'.
+ '<tr><td colspan="2"> </td><td
class="diff-marker">+</td><td class="diff-addedline">'.
+ '<div><ins class="diffchange
diffchange-inline"><span>PID: SNAK</span></ins></div></td></tr>'
+ ),
+ //3 +references
+ array(
+ new ClaimDifference(
+ null,
+ null,
+ new Diff( array(
+ new DiffOpRemove( new
Reference( new SnakList( array( new PropertyNoValueSnak( 50 ) ) ) ) ),
+ ) )
+ ),
+ new Claim( new PropertyValueSnak( new
PropertyId( 'P1' ), new StringValue( 'foo' ) ) ),
+ '<tr><td colspan="2"
class="diff-lineno">property / PID / reference</td><td colspan="2"
class="diff-lineno"></td></tr>'.
+ '<tr><td class="diff-marker">-</td><td
class="diff-deletedline">'.
+ '<div><del class="diffchange
diffchange-inline"><span>PID: SNAK</span></del></div></td><td
colspan="2"> </td></tr>'
+ ),
+ //4 ranks
+ //TODO no diff is currently created for RANKS,
Implement this!
+ array(
+ new ClaimDifference(
+ null,
+ null,
+ null,
+ new DiffOpChange(
Statement::RANK_NORMAL, Statement::RANK_PREFERRED )
+ ),
+ new Statement( new PropertyValueSnak( new
PropertyId( 'P1' ), new StringValue( 'foo' ) ) ),
+ ''
+ ),
);
-
- $differences[] = new ClaimDifference(
- null,
- null,
- null,
- new DiffOpChange( Statement::RANK_DEPRECATED,
Statement::RANK_PREFERRED )
- );
-
- return $this->arrayWrap( $differences );
}
/**
- * @dataProvider visualizeDiffProvider
- *
- * @param ClaimDifference $claimDifference
+ * @dataProvider provideDifferenceAndClaim
*/
-// @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 );
+ public function testVisualizeClaimChange( $difference, $baseClaim,
$expectedHtml ){
+ $visualizer = $this->newClaimDifferenceVisualizer();
+ $html = $visualizer->visualizeClaimChange( $difference,
$baseClaim );
+ $this->assertEquals( $expectedHtml, $html );
}
-*/
+
+ public function testVisualizeNewClaim(){
+ $expect = '<tr><td colspan="2" class="diff-lineno"></td><td
colspan="2" class="diff-lineno">property / PID</td></tr>'.
+ '<tr><td colspan="2"> </td><td
class="diff-marker">+</td><td class="diff-addedline">'.
+ '<div><ins class="diffchange
diffchange-inline"><span>SNAK</span></ins></div>'.
+ '</td></tr><tr><td colspan="2"
class="diff-lineno"></td><td colspan="2" class="diff-lineno">property / PID /
qualifier</td></tr>'.
+ '<tr><td colspan="2"> </td><td
class="diff-marker">+</td><td class="diff-addedline">'.
+ '<div><ins class="diffchange
diffchange-inline"><span>PID: SNAK</span></ins></div>'.
+ '</td></tr><tr><td colspan="2"
class="diff-lineno"></td><td colspan="2" class="diff-lineno">property / PID /
reference</td></tr>'.
+ '<tr><td colspan="2"> </td><td
class="diff-marker">+</td><td class="diff-addedline">'.
+ '<div><ins class="diffchange
diffchange-inline"><span>PID: SNAK</span></ins></div></td></tr>';
+
+ $visualizer = $this->newClaimDifferenceVisualizer();
+ $claim = new Statement(
+ new PropertyValueSnak( new PropertyId( 'P12' ), new
StringValue( 'foo' ) ),
+ new SnakList( array( new PropertyNoValueSnak( 50 ) ) ),
+ new ReferenceList( array(
+ new Reference(
+ new SnakList( array(
+ new PropertyValueSnak( new
PropertyId( 'P44' ), new StringValue( 'referencevalue' ) )
+ ) ) ) ) ) );
+ $html = $visualizer->visualizeNewClaim( $claim );
+
+ $this->assertEquals( $expect, $html );
+ }
+
+ public function testVisualizeRemovedClaim(){
+ $expect = '<tr><td colspan="2" class="diff-lineno">property /
PID</td><td colspan="2" class="diff-lineno"></td></tr>'.
+ '<tr><td class="diff-marker">-</td><td
class="diff-deletedline">'.
+ '<div><del class="diffchange
diffchange-inline"><span>SNAK</span></del></div>'.
+ '</td><td colspan="2"> </td></tr><tr><td
colspan="2" class="diff-lineno">property / PID / qualifier</td>'.
+ '<td colspan="2" class="diff-lineno"></td></tr><tr><td
class="diff-marker">-</td><td class="diff-deletedline">'.
+ '<div><del class="diffchange
diffchange-inline"><span>PID: SNAK</span></del></div>'.
+ '</td><td colspan="2"> </td></tr><tr><td
colspan="2" class="diff-lineno">property / PID / reference</td>'.
+ '<td colspan="2" class="diff-lineno"></td></tr><tr><td
class="diff-marker">-</td><td class="diff-deletedline">'.
+ '<div><del class="diffchange
diffchange-inline"><span>PID: SNAK</span></del></div></td><td
colspan="2"> </td></tr>';
+
+ $visualizer = $this->newClaimDifferenceVisualizer();
+ $claim = new Statement(
+ new PropertyValueSnak( new PropertyId( 'P12' ), new
StringValue( 'foo' ) ),
+ new SnakList( array( new PropertyNoValueSnak( 50 ) ) ),
+ new ReferenceList( array(
+ new Reference(
+ new SnakList( array(
+ new PropertyValueSnak( new
PropertyId( 'P44' ), new StringValue( 'referencevalue' ) )
+ ) ) ) ) ) );
+ $html = $visualizer->visualizeRemovedClaim( $claim );
+
+ $this->assertEquals( $expect, $html );
+ }
+
+
+
+
}
--
To view, visit https://gerrit.wikimedia.org/r/90876
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib8709a7b5a1cc3e2ba32bc9057154ced1ff2d334
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Addshore <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
