jenkins-bot has submitted this change and it was merged. Change subject: (bug 46366) Use SnakFormatter for diffs and summaries ......................................................................
(bug 46366) Use SnakFormatter for diffs and summaries This uses the new SnakFormatterFactory facility to allow ClaimDifferenceVisualizer and ClaimSummaryBuilder to operate on snaks in a generic way, without any knowledge about data types or data values. Change-Id: Ide1e947d9073f4648c4b97fd979c68c7ff48a984 --- M lib/includes/ClaimDifferenceVisualizer.php M repo/includes/ClaimSummaryBuilder.php M repo/includes/EntityContentDiffView.php M repo/includes/actions/EditEntityAction.php M repo/includes/api/SetClaim.php M repo/tests/phpunit/includes/ClaimSummaryBuilderTest.php 6 files changed, 116 insertions(+), 123 deletions(-) Approvals: Tobias Gritschacher: Looks good to me, approved jenkins-bot: Verified diff --git a/lib/includes/ClaimDifferenceVisualizer.php b/lib/includes/ClaimDifferenceVisualizer.php index cd35dc5..c99e98f 100644 --- a/lib/includes/ClaimDifferenceVisualizer.php +++ b/lib/includes/ClaimDifferenceVisualizer.php @@ -8,7 +8,8 @@ use Html; use Diff\Diff; use RuntimeException; -use Wikibase\Lib\EntityIdFormatter; +use Wikibase\Lib\EntityIdLabelFormatter; +use Wikibase\Lib\SnakFormatter; /** * Class for generating HTML for Claim Diffs. @@ -27,37 +28,43 @@ /** * @since 0.4 * - * @var EntityLookup - */ - private $entityLookup; - - /** - * @since 0.4 - * * @var string */ private $langCode; /** - * @since 0.4 + * @since 0.5 * - * @var EntityIdFormatter + * @var EntityIdLabelFormatter */ - private $idFormatter; + private $propertyFormatter; + + /** + * @since 0.5 + * + * @var SnakFormatter + */ + private $snakFormatter; /** * Constructor. * * @since 0.4 * - * @param EntityLookup $entityLookup - * @param string $langCode - * @param EntityIdFormatter $idFormatter + * @param EntityIdLabelFormatter $propertyFormatter + * @param SnakFormatter $snakFormatter + * + * @throws \InvalidArgumentException */ - public function __construct( $entityLookup, $langCode, EntityIdFormatter $idFormatter ) { - $this->entityLookup = $entityLookup; - $this->langCode = $langCode; - $this->idFormatter = $idFormatter; + public function __construct( EntityIdLabelFormatter $propertyFormatter, SnakFormatter $snakFormatter ) { + if ( $snakFormatter->getFormat() !== SnakFormatter::FORMAT_PLAIN ) { + throw new \InvalidArgumentException( + 'Expected $snakFormatter to generate plain text, not ' + . $snakFormatter->getFormat() ); + } + + $this->propertyFormatter = $propertyFormatter; + $this->snakFormatter = $snakFormatter; } /** @@ -155,8 +162,8 @@ $valueFormatter = new DiffOpValueFormatter( // todo: should show specific headers for both columns $this->getSnakHeader( $mainSnakChange->getNewValue() ), - $this->getSnakValue( $mainSnakChange->getOldValue() ), - $this->getSnakValue( $mainSnakChange->getNewValue() ) + $this->snakFormatter->formatSnak( $mainSnakChange->getOldValue() ), + $this->snakFormatter->formatSnak( $mainSnakChange->getNewValue() ) ); return $valueFormatter->generateHtml(); @@ -210,11 +217,11 @@ $newValue = null; if ( $oldSnak instanceof Snak ) { - $oldValue = $this->getSnakValue( $oldSnak ); + $oldValue = $this->snakFormatter->formatSnak( $oldSnak ); } if ( $newSnak instanceof Snak ) { - $newValue = $this->getSnakValue( $newSnak ); + $newValue = $this->snakFormatter->formatSnak( $newSnak ); } $valueFormatter = new DiffOpValueFormatter( $snakHeader, $oldValue, $newValue ); @@ -238,9 +245,9 @@ // TODO: change hardcoded ": " so something like wfMessage( 'colon-separator' ), // but this will require further refactoring as it would add HTML which gets escaped $values[] = - $this->getEntityLabel( $snak->getPropertyId() ) . + $this->propertyFormatter->format( $snak->getPropertyId() ) . ': '. - $this->getSnakValue( $snak ); + $this->snakFormatter->formatSnak( $snak ); } return $values; @@ -257,65 +264,10 @@ */ protected function getSnakHeader( Snak $snak ) { $propertyId = $snak->getPropertyId(); - $propertyLabel = $this->getEntityLabel( $propertyId ); + $propertyLabel = $this->propertyFormatter->format( $propertyId ); $headerText = wfMessage( 'wikibase-entity-property' ) . ' / ' . $propertyLabel; return $headerText; - } - - /** - * Get snak value in string form - * - * @since 0.4 - * - * @param Snak $snak - * - * @return string - */ - protected function getSnakValue( Snak $snak ) { - $snakType = $snak->getType(); - - if ( $snakType === 'value' ) { - $dataValue = $snak->getDataValue(); - - // FIXME! should use some value formatter - if ( $dataValue instanceof EntityId ) { - $diffValueString = $this->getEntityLabel( $dataValue ); - } else if ( $dataValue instanceof TimeValue ) { - // TODO: this will just display the plain ISO8601-string, - // we should instead use a decent formatter - $diffValueString = $dataValue->getTime(); - } else { - $diffValueString = $dataValue->getValue(); - } - - return $diffValueString; - } else { - return $snakType; - } - } - - /** - * Get an entity label - * - * @since 0.4 - * - * @param EntityId $entityId - * - * @return string - */ - protected function getEntityLabel( EntityId $entityId ) { - $entity = $this->entityLookup->getEntity( $entityId ); - - if ( $entity instanceof Entity ) { - $lookedUpLabel = $this->entityLookup->getEntity( $entityId )->getLabel( $this->langCode ); - - if ( $lookedUpLabel !== false ) { - return $lookedUpLabel; - } - } - - return $this->idFormatter->format( $entityId ); } /** @@ -387,23 +339,23 @@ // but this will require further refactoring as it would add HTML which gets escaped if ( $change instanceof DiffOpAdd ) { $newVal = - $this->getEntityLabel( $change->getNewValue()->getPropertyId() ) . + $this->propertyFormatter->format( $change->getNewValue()->getPropertyId() ) . ': ' . - $this->getSnakValue( $change->getNewValue() ); + $this->snakFormatter->formatSnak( $change->getNewValue() ); } else if ( $change instanceof DiffOpRemove ) { $oldVal = - $this->getEntityLabel( $change->getOldValue()->getPropertyId() ) . + $this->propertyFormatter->format( $change->getOldValue()->getPropertyId() ) . ': ' . - $this->getSnakValue( $change->getOldValue() ); + $this->snakFormatter->formatSnak( $change->getOldValue() ); } else if ( $change instanceof DiffOpChange ) { $oldVal = - $this->getEntityLabel( $change->getOldValue()->getPropertyId() ) . + $this->propertyFormatter->format( $change->getOldValue()->getPropertyId() ) . ': ' . - $this->getSnakValue( $change->getOldValue() ); + $this->snakFormatter->formatSnak( $change->getOldValue() ); $newVal = - $this->getEntityLabel( $change->getNewValue()->getPropertyId() ) . + $this->propertyFormatter->format( $change->getNewValue()->getPropertyId() ) . ': ' . - $this->getSnakValue( $change->getNewValue() ); + $this->snakFormatter->formatSnak( $change->getNewValue() ); } else { throw new RuntimeException( 'Diff operation of unknown type.' ); } diff --git a/repo/includes/ClaimSummaryBuilder.php b/repo/includes/ClaimSummaryBuilder.php index e338fd0..ee2359b 100644 --- a/repo/includes/ClaimSummaryBuilder.php +++ b/repo/includes/ClaimSummaryBuilder.php @@ -4,7 +4,7 @@ use DataValues\TimeValue; use InvalidArgumentException; -use Wikibase\Lib\EntityIdFormatter; +use Wikibase\Lib\SnakFormatter; /** * EditSummary-Builder for claim operations @@ -16,6 +16,7 @@ * * @licence GNU GPL v2+ * @author Tobias Gritschacher < tobias.gritschac...@wikimedia.de > + * @author Daniel Kinzler */ class ClaimSummaryBuilder { @@ -30,9 +31,9 @@ private $claimDiffer; /** - * @var EntityIdFormatter + * @var Lib\SnakFormatter */ - private $idFormatter; + private $snakValueFormatter; /** * Constructs a new ClaimSummaryBuilder @@ -41,18 +42,24 @@ * * @param string $apiModuleName * @param ClaimDiffer $claimDiffer - * @param EntityIdFormatter $idFormatter + * @param SnakFormatter $snakValueFormatter * * @throws InvalidArgumentException */ - public function __construct( $apiModuleName, ClaimDiffer $claimDiffer, EntityIdFormatter $idFormatter ) { + public function __construct( $apiModuleName, ClaimDiffer $claimDiffer, SnakFormatter $snakValueFormatter ) { if ( !is_string( $apiModuleName ) ) { throw new InvalidArgumentException( '$apiModuleName needs to be a string' ); } + if ( $snakValueFormatter->getFormat() !== SnakFormatter::FORMAT_PLAIN ) { + throw new InvalidArgumentException( + 'Expected $snakValueFormatter to procude plain text output, not ' + . $snakValueFormatter->getFormat() ); + } + $this->apiModuleName = $apiModuleName; $this->claimDiffer = $claimDiffer; - $this->idFormatter = $idFormatter; + $this->snakValueFormatter = $snakValueFormatter; } /** @@ -122,22 +129,13 @@ foreach( $guids as $guid ) { if ( $claims->hasClaimWithGuid( $guid ) ) { $snak = $claims->getClaimWithGuid( $guid )->getMainSnak(); - $key = $this->idFormatter->format( $snak->getPropertyId() ); + $key = $snak->getPropertyId()->getPrefixedId(); if ( !array_key_exists( $key, $pairs ) ) { $pairs[$key] = array(); } - if ( $snak instanceof PropertyValueSnak ) { - $value = $snak->getDataValue(); - // TODO: we should use value formatters here! - if ( $value instanceof TimeValue ) { - $value = $value->getTime(); - } - } else { - $value = $snak->getType(); // todo handle no values in general way (needed elsewhere) - } - + $value = $this->snakValueFormatter->formatSnak( $snak ); $pairs[$key][] = $value; } } diff --git a/repo/includes/EntityContentDiffView.php b/repo/includes/EntityContentDiffView.php index e459a96..252658e 100644 --- a/repo/includes/EntityContentDiffView.php +++ b/repo/includes/EntityContentDiffView.php @@ -5,6 +5,10 @@ use Diff\ListDiffer; use Content, Html; +use ValueFormatters\FormatterOptions; +use ValueFormatters\ValueFormatter; +use Wikibase\Lib\EntityIdLabelFormatter; +use Wikibase\Lib\SnakFormatter; use Wikibase\Repo\WikibaseRepo; /** @@ -49,7 +53,14 @@ public function __construct( $context = null, $old = 0, $new = 0, $rcid = 0, $refreshCache = false, $unhide = false ) { parent::__construct( $context, $old, $new, $rcid, $refreshCache, $unhide ); - $this->mRefreshCache = true; //FIXME: debug only! + //TODO: proper injection + $options = new FormatterOptions( array( + //TODO: fallback chain + ValueFormatter::OPT_LANG => $this->getContext()->getLanguage()->getCode() + ) ); + + $this->propertyNameFormatter = new EntityIdLabelFormatter( $options, WikibaseRepo::getDefaultInstance()->getEntityLookup() ); + $this->snakValueFormatter = WikibaseRepo::getDefaultInstance()->getSnakFormatterFactory()->getFormatter( SnakFormatter::FORMAT_PLAIN, $options ); } /** @@ -134,7 +145,6 @@ * @var EntityContent $new */ $diff = $old->getEntity()->getDiff( $new->getEntity() ); - $langCode = $this->getContext()->getLanguage()->getCode(); $comparer = function( \Comparable $old, \Comparable $new ) { return $old->equals( $new ); @@ -145,9 +155,8 @@ $this->getContext(), new ClaimDiffer( new CallbackListDiffer( $comparer ) ), new ClaimDifferenceVisualizer( - new WikiPageEntityLookup(), - $langCode, - WikibaseRepo::getDefaultInstance()->getIdFormatter() + $this->propertyNameFormatter, + $this->snakValueFormatter ) ); diff --git a/repo/includes/actions/EditEntityAction.php b/repo/includes/actions/EditEntityAction.php index 2db7579..c1f9451 100644 --- a/repo/includes/actions/EditEntityAction.php +++ b/repo/includes/actions/EditEntityAction.php @@ -3,6 +3,10 @@ namespace Wikibase; use Diff\CallbackListDiffer; use Html, Linker, Skin, Status, Revision; +use ValueFormatters\FormatterOptions; +use ValueFormatters\ValueFormatter; +use Wikibase\Lib\EntityIdLabelFormatter; +use Wikibase\Lib\SnakFormatter; use Wikibase\Repo\WikibaseRepo; /** @@ -24,6 +28,21 @@ * @since 0.1 */ abstract class EditEntityAction extends ViewEntityAction { + + public function __construct( \Page $page, \IContextSource $context = null ) { + parent::__construct( $page, $context ); + + //TODO: proper injection + $options = new FormatterOptions( array( + //TODO: fallback chain + ValueFormatter::OPT_LANG => $this->getContext()->getLanguage()->getCode() + ) ); + + $this->propertyNameFormatter = new EntityIdLabelFormatter( $options, WikibaseRepo::getDefaultInstance()->getEntityLookup() ); + $this->snakValueFormatter = WikibaseRepo::getDefaultInstance()->getSnakFormatterFactory()->getFormatter( SnakFormatter::FORMAT_PLAIN, $options ); + + } + /** * @see Action::getName() * @@ -440,9 +459,8 @@ $this->getContext(), new ClaimDiffer( new CallbackListDiffer( $comparer ) ), new ClaimDifferenceVisualizer( - new WikiPageEntityLookup(), - $langCode, - WikibaseRepo::getDefaultInstance()->getIdFormatter() + $this->propertyNameFormatter, + $this->snakValueFormatter ) ); diff --git a/repo/includes/api/SetClaim.php b/repo/includes/api/SetClaim.php index 1789c52..e312f77 100644 --- a/repo/includes/api/SetClaim.php +++ b/repo/includes/api/SetClaim.php @@ -8,12 +8,15 @@ use MWException; use ApiBase; use Diff\ListDiffer; +use ValueFormatters\FormatterOptions; +use ValueFormatters\ValueFormatter; use Wikibase\EntityContent; use Wikibase\Claim; use Wikibase\EntityContentFactory; use Wikibase\ClaimDiffer; use Wikibase\ClaimSaver; use Wikibase\ClaimSummaryBuilder; +use Wikibase\Lib\SnakFormatter; use Wikibase\Repo\WikibaseRepo; use Wikibase\Summary; use Wikibase\Validators\ValidatorErrorLocalizer; @@ -85,10 +88,16 @@ }; $claimDiffer = new ClaimDiffer( new CallbackListDiffer( $comparer ) ); + + $options = new FormatterOptions( array( + //TODO: fallback chain + ValueFormatter::OPT_LANG => $this->getContext()->getLanguage()->getCode() + ) ); + $claimSummaryBuilder = new ClaimSummaryBuilder( $this->getModuleName(), $claimDiffer, - WikibaseRepo::getDefaultInstance()->getIdFormatter() + WikibaseRepo::getDefaultInstance()->getSnakFormatterFactory()->getFormatter( SnakFormatter::FORMAT_PLAIN, $options ) ); $claimSaver = new ClaimSaver(); diff --git a/repo/tests/phpunit/includes/ClaimSummaryBuilderTest.php b/repo/tests/phpunit/includes/ClaimSummaryBuilderTest.php index 7e9a0de..a9f5551 100644 --- a/repo/tests/phpunit/includes/ClaimSummaryBuilderTest.php +++ b/repo/tests/phpunit/includes/ClaimSummaryBuilderTest.php @@ -8,7 +8,7 @@ use Wikibase\Claim; use Wikibase\Claims; use Wikibase\ClaimSummaryBuilder; -use Wikibase\Lib\EntityIdFormatter; +use Wikibase\Lib\SnakFormatter; use Wikibase\Repo\WikibaseRepo; /** @@ -40,6 +40,7 @@ * * @licence GNU GPL v2+ * @author Tobias Gritschacher < tobias.gritschac...@wikimedia.de > + * @author Daniel Kinzler */ class ClaimSummaryBuilderTest extends \MediaWikiTestCase { @@ -138,11 +139,14 @@ } public function testBuildCreateClaimSummary() { - $idFormatter = $this->getMockBuilder( 'Wikibase\Lib\EntityIdFormatter' ) + $snakFormatter = $this->getMockBuilder( 'Wikibase\Lib\SnakFormatter' ) ->disableOriginalConstructor()->getMock(); - $idFormatter->expects( $this->any() ) - ->method( 'format' ) + $snakFormatter->expects( $this->any() ) + ->method( 'formatSnak' ) ->will( $this->returnValue( 'foo' ) ); + $snakFormatter->expects( $this->any() ) + ->method( 'getFormat' ) + ->will( $this->returnValue( SnakFormatter::FORMAT_PLAIN ) ); $comparer = function( \Comparable $old, \Comparable $new ) { return $old->equals( $new ); @@ -151,7 +155,7 @@ $claimSummaryBuilder = new ClaimSummaryBuilder( 'wbsetclaim', new ClaimDiffer( new CallbackListDiffer( $comparer ) ), - $idFormatter + $snakFormatter ); $claims = new Claims(); @@ -173,11 +177,14 @@ * @param string $action */ public function testBuildUpdateClaimSummary( $originalClaim, $modifiedClaim, $action ) { - $idFormatter = $this->getMockBuilder( 'Wikibase\Lib\EntityIdFormatter' ) + $snakFormatter = $this->getMockBuilder( 'Wikibase\Lib\SnakFormatter' ) ->disableOriginalConstructor()->getMock(); - $idFormatter->expects( $this->any() ) - ->method( 'format' ) + $snakFormatter->expects( $this->any() ) + ->method( 'formatSnak' ) ->will( $this->returnValue( 'foo' ) ); + $snakFormatter->expects( $this->any() ) + ->method( 'getFormat' ) + ->will( $this->returnValue( SnakFormatter::FORMAT_PLAIN ) ); $comparer = function( \Comparable $old, \Comparable $new ) { return $old->equals( $new ); @@ -186,7 +193,7 @@ $claimSummaryBuilder = new ClaimSummaryBuilder( 'wbsetclaim', new ClaimDiffer( new CallbackListDiffer( $comparer ) ), - $idFormatter + $snakFormatter ); $claims = new Claims(); -- To view, visit https://gerrit.wikimedia.org/r/83091 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ide1e947d9073f4648c4b97fd979c68c7ff48a984 Gerrit-PatchSet: 10 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Daniel Kinzler <daniel.kinz...@wikimedia.de> Gerrit-Reviewer: Aude <aude.w...@gmail.com> Gerrit-Reviewer: Daniel Kinzler <daniel.kinz...@wikimedia.de> Gerrit-Reviewer: Denny Vrandecic <denny.vrande...@wikimedia.de> Gerrit-Reviewer: Jeroen De Dauw <jeroended...@gmail.com> Gerrit-Reviewer: Tobias Gritschacher <tobias.gritschac...@wikimedia.de> Gerrit-Reviewer: jenkins-bot _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits