Daniel Kinzler has uploaded a new change for review.
https://gerrit.wikimedia.org/r/83091
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, 117 insertions(+), 121 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/91/83091/1
diff --git a/lib/includes/ClaimDifferenceVisualizer.php
b/lib/includes/ClaimDifferenceVisualizer.php
index cd35dc5..e06dd7e 100644
--- a/lib/includes/ClaimDifferenceVisualizer.php
+++ b/lib/includes/ClaimDifferenceVisualizer.php
@@ -8,7 +8,9 @@
use Html;
use Diff\Diff;
use RuntimeException;
-use Wikibase\Lib\EntityIdFormatter;
+use Wikibase\Lib\EntityIdLabelFormatter;
+use Wikibase\Lib\SnakFormatter;
+use Wikibase\Lib\SnakFormatterFactory;
/**
* Class for generating HTML for Claim Diffs.
@@ -27,37 +29,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() !==
SnakFormatterFactory::FORMAT_PLAIN ) {
+ throw new \InvalidArgumentException(
+ 'Expected $snakFormatter to generate plain
text, not '
+ . $snakFormatter->getFormat() );
+ }
+
+ $this->propertyFormatter = $propertyFormatter;
+ $this->snakFormatter = $snakFormatter;
}
/**
@@ -155,8 +163,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 +218,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 +246,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 +265,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 +340,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..d88634c 100644
--- a/repo/includes/ClaimSummaryBuilder.php
+++ b/repo/includes/ClaimSummaryBuilder.php
@@ -5,6 +5,8 @@
use DataValues\TimeValue;
use InvalidArgumentException;
use Wikibase\Lib\EntityIdFormatter;
+use Wikibase\Lib\SnakFormatter;
+use Wikibase\Lib\SnakFormatterFactory;
/**
* EditSummary-Builder for claim operations
@@ -30,9 +32,9 @@
private $claimDiffer;
/**
- * @var EntityIdFormatter
+ * @var Lib\SnakFormatter
*/
- private $idFormatter;
+ private $snakValueFormatter;
/**
* Constructs a new ClaimSummaryBuilder
@@ -41,18 +43,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() !==
SnakFormatterFactory::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 +130,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..e4ac45b 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\SnakFormatterFactory;
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(
SnakFormatterFactory::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..772361b 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\SnakFormatterFactory;
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(
SnakFormatterFactory::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..be6c84e 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\SnakFormatterFactory;
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(
SnakFormatterFactory::FORMAT_PLAIN, $options )
);
$claimSaver = new ClaimSaver();
diff --git a/repo/tests/phpunit/includes/ClaimSummaryBuilderTest.php
b/repo/tests/phpunit/includes/ClaimSummaryBuilderTest.php
index 33cc675..e77879e 100644
--- a/repo/tests/phpunit/includes/ClaimSummaryBuilderTest.php
+++ b/repo/tests/phpunit/includes/ClaimSummaryBuilderTest.php
@@ -9,6 +9,7 @@
use Wikibase\Claims;
use Wikibase\ClaimSummaryBuilder;
use Wikibase\Lib\EntityIdFormatter;
+use Wikibase\Lib\SnakFormatterFactory;
use Wikibase\Repo\WikibaseRepo;
/**
@@ -37,6 +38,7 @@
* @group Wikibase
* @group WikibaseRepo
* @group ClaimSummaryBuilder
+ * @group XXX
*
* @licence GNU GPL v2+
* @author Tobias Gritschacher < [email protected] >
@@ -132,11 +134,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(
SnakFormatterFactory::FORMAT_PLAIN ) );
$comparer = function( \Comparable $old, \Comparable $new ) {
return $old->equals( $new );
@@ -145,7 +150,7 @@
$claimSummaryBuilder = new ClaimSummaryBuilder(
'wbsetclaim',
new ClaimDiffer( new CallbackListDiffer( $comparer ) ),
- $idFormatter
+ $snakFormatter
);
$claims = new Claims();
@@ -167,11 +172,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(
SnakFormatterFactory::FORMAT_PLAIN ) );
$comparer = function( \Comparable $old, \Comparable $new ) {
return $old->equals( $new );
@@ -180,7 +188,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: newchange
Gerrit-Change-Id: Ide1e947d9073f4648c4b97fd979c68c7ff48a984
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
