jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/352801 )
Change subject: Do not assume that revIds increase in time
......................................................................
Do not assume that revIds increase in time
This patch fixes issues with the RevisonSlider assuming that the higher
revision id belongs to the newer revision. Min/Max methods to decide what
the diff and what the oldid is are removed and the usage of methods is
adjusted accordingly.
Also the test for switchover pointers is removed since this will not work
with the patch.
Bug: T164455
Change-Id: If5d9cbb8ebd872aee376d249942e6881c8edb984
---
M modules/ext.RevisionSlider.DiffPage.js
M modules/ext.RevisionSlider.SliderView.js
M modules/ext.RevisionSlider.SliderViewTwo.js
M tests/browser/features/pointers.feature
M tests/qunit/RevisionSlider.DiffPage.test.js
5 files changed, 66 insertions(+), 67 deletions(-)
Approvals:
Tobias Gritschacher: Looks good to me, approved
jenkins-bot: Verified
diff --git a/modules/ext.RevisionSlider.DiffPage.js
b/modules/ext.RevisionSlider.DiffPage.js
index 341b194..7145d20 100644
--- a/modules/ext.RevisionSlider.DiffPage.js
+++ b/modules/ext.RevisionSlider.DiffPage.js
@@ -12,16 +12,14 @@
/**
* Refreshes the diff view with two given revision IDs
*
- * @param {number} revId1
- * @param {number} revId2
+ * @param {number} diff
+ * @param {number} oldid
* @param {SliderView} sliderView
* @param {number} [retryAttempt=0]
*/
- refresh: function ( revId1, revId2, sliderView, retryAttempt ) {
+ refresh: function ( diff, oldid, sliderView, retryAttempt ) {
var self = this,
retryLimit = 2,
- diff = Math.max( revId1, revId2 ),
- oldid = Math.min( revId1, revId2 ),
data = {
diff: diff,
oldid: oldid
@@ -86,7 +84,7 @@
this.tryCount++;
mw.track(
'counter.MediaWiki.RevisionSlider.error.refresh' );
if ( retryAttempt <= retryLimit ) {
- self.refresh( revId1, revId2,
sliderView, retryAttempt + 1 );
+ self.refresh( diff, oldid,
sliderView, retryAttempt + 1 );
}
// TODO notify the user that we failed
to update the diff?
// This could also attempt to reload
the page with the correct diff loaded without ajax?
@@ -97,17 +95,17 @@
/**
* Replaces the current state in the history stack
*
- * @param {number} revId1
- * @param {number} revId2
+ * @param {number} diff
+ * @param {number} oldid
* @param {SliderView} sliderView
*/
- replaceState: function ( revId1, revId2, sliderView ) {
+ replaceState: function ( diff, oldid, sliderView ) {
// IE9 does not have history.replaceState()
if ( typeof history.replaceState === 'function' ) {
history.replaceState(
- this.getStateObject( revId1, revId2,
sliderView ),
+ this.getStateObject( diff, oldid,
sliderView ),
$( document ).find( 'title' ).text(),
- this.getStateUrl( revId1, revId2 )
+ this.getStateUrl( diff, oldid )
);
}
},
@@ -115,17 +113,17 @@
/**
* Pushes the current state onto the history stack
*
- * @param {number} revId1
- * @param {number} revId2
+ * @param {number} diff
+ * @param {number} oldid
* @param {SliderView} sliderView
*/
- pushState: function ( revId1, revId2, sliderView ) {
+ pushState: function ( diff, oldid, sliderView ) {
// IE9 does not have history.pushState()
if ( typeof history.pushState === 'function' ) {
history.pushState(
- this.getStateObject( revId1, revId2,
sliderView ),
+ this.getStateObject( diff, oldid,
sliderView ),
$( document ).find( 'title' ).text(),
- this.getStateUrl( revId1, revId2 )
+ this.getStateUrl( diff, oldid )
);
}
},
@@ -133,15 +131,15 @@
/**
* Gets a state object to be used with history.replaceState and
history.pushState
*
- * @param {number} revId1
- * @param {number} revId2
+ * @param {number} diff
+ * @param {number} oldid
* @param {SliderView} sliderView
* @return {Object}
*/
- getStateObject: function ( revId1, revId2, sliderView ) {
+ getStateObject: function ( diff, oldid, sliderView ) {
return {
- revid1: revId1,
- revid2: revId2,
+ diff: diff,
+ oldid: oldid,
pointerOlderPos:
sliderView.pointerOlder.getPosition(),
pointerNewerPos:
sliderView.pointerNewer.getPosition(),
sliderPos:
sliderView.slider.getOldestVisibleRevisionIndex()
@@ -151,12 +149,12 @@
/**
* Gets a URL to be used with history.replaceState and
history.pushState
*
- * @param {number} revId1
- * @param {number} revId2
+ * @param {number} diff
+ * @param {number} oldid
* @return {string}
*/
- getStateUrl: function ( revId1, revId2 ) {
- var url = mw.util.wikiScript( 'index' ) + '?diff=' +
Math.max( revId1, revId2 ) + '&oldid=' + Math.min( revId1, revId2 ),
+ getStateUrl: function ( diff, oldid ) {
+ var url = mw.util.wikiScript( 'index' ) + '?diff=' +
diff + '&oldid=' + oldid,
params = this.getExtraDiffPageParams();
if ( Object.keys( params ).length > 0 ) {
Object.keys( params ).forEach( function ( key )
{
@@ -203,7 +201,7 @@
sliderView.$element.find(
'div.mw-revslider-revisions' )
);
sliderView.updatePointerPositionAttributes();
- self.refresh( event.state.revid1,
event.state.revid2 );
+ self.refresh( event.state.diff,
event.state.oldid );
} );
},
diff --git a/modules/ext.RevisionSlider.SliderView.js
b/modules/ext.RevisionSlider.SliderView.js
index b94b6a8..979e7e2 100644
--- a/modules/ext.RevisionSlider.SliderView.js
+++ b/modules/ext.RevisionSlider.SliderView.js
@@ -132,7 +132,7 @@
this.slide( Math.floor( (
this.pointerNewer.getPosition() - 1 ) / this.slider.getRevisionsPerWindow() ),
0 );
this.diffPage.addHandlersToCoreLinks( this );
- this.diffPage.replaceState( mw.config.get(
'extRevisionSliderOldRev' ), mw.config.get( 'extRevisionSliderNewRev' ), this );
+ this.diffPage.replaceState( mw.config.get(
'extRevisionSliderNewRev' ), mw.config.get( 'extRevisionSliderOldRev' ), this );
this.diffPage.initOnPopState( this );
},
@@ -229,7 +229,7 @@
var $p = $( this ),
relativeIndex =
self.getRelativePointerIndex( $p ),
pointer = self.whichPointer( $p
),
- revId1, revId2;
+ diff, oldid;
self.isDragged = false;
self.removePointerDragCursor();
@@ -246,15 +246,15 @@
self.resetPointerStylesBasedOnPosition();
self.resetRevisionStylesBasedOnPointerPosition( $revisions );
- revId1 = self.getRevElementAtPosition(
- $revisions,
self.pointerOlder.getPosition()
- ).data( 'revid' );
-
- revId2 = self.getRevElementAtPosition(
+ diff = self.getRevElementAtPosition(
$revisions,
self.pointerNewer.getPosition()
).data( 'revid' );
- self.refreshRevisions( revId1, revId2 );
+ oldid = self.getRevElementAtPosition(
+ $revisions,
self.pointerOlder.getPosition()
+ ).data( 'revid' );
+
+ self.refreshRevisions( diff, oldid );
self.redrawPointerLines();
},
@@ -347,10 +347,19 @@
}
pClicked.setPosition( targetPos );
view.updatePointerPositionAttributes();
- view.refreshRevisions(
- +view.getRevElementAtPosition( $revisions,
pOther.getPosition() ).data( 'revid' ),
- +$clickedRev.data( 'revid' )
- );
+
+ if ( hasClickedTop ) {
+ view.refreshRevisions(
+ +$clickedRev.data( 'revid' ),
+ +view.getRevElementAtPosition(
$revisions, pOther.getPosition() ).data( 'revid' )
+ );
+ } else {
+ view.refreshRevisions(
+ +view.getRevElementAtPosition(
$revisions, pOther.getPosition() ).data( 'revid' ),
+ +$clickedRev.data( 'revid' )
+ );
+ }
+
view.resetPointerColorsBasedOnValues(
view.pointerOlder.getPosition(), view.pointerNewer.getPosition() );
view.resetRevisionStylesBasedOnPointerPosition(
$revisions );
view.alignPointers();
@@ -377,14 +386,12 @@
/**
* Refreshes the diff page to show the diff for the specified
revisions
*
- * @param {number} revId1
- * @param {number} revId2
+ * @param {number} diff
+ * @param {number} oldid
*/
- refreshRevisions: function ( revId1, revId2 ) {
- var oldRev = Math.min( revId1, revId2 ),
- newRev = Math.max( revId1, revId2 );
- this.diffPage.refresh( oldRev, newRev, this );
- this.diffPage.pushState( oldRev, newRev, this );
+ refreshRevisions: function ( diff, oldid ) {
+ this.diffPage.refresh( diff, oldid, this );
+ this.diffPage.pushState( diff, oldid, this );
},
showNextDiff: function () {
@@ -407,8 +414,8 @@
);
this.updatePointerPositionAttributes();
this.refreshRevisions(
- $( '.mw-revslider-revision[data-pos="' +
this.pointerOlder.getPosition() + '"]' ).attr( 'data-revid' ),
- $( '.mw-revslider-revision[data-pos="' +
this.pointerNewer.getPosition() + '"]' ).attr( 'data-revid' )
+ $( '.mw-revslider-revision[data-pos="' +
this.pointerNewer.getPosition() + '"]' ).attr( 'data-revid' ),
+ $( '.mw-revslider-revision[data-pos="' +
this.pointerOlder.getPosition() + '"]' ).attr( 'data-revid' )
);
},
@@ -846,7 +853,7 @@
revIdOld = self.getRevElementAtPosition( $revisions,
pOld.getPosition() ).data( 'revid' );
revIdNew = self.getRevElementAtPosition( $revisions,
pNew.getPosition() ).data( 'revid' );
- this.diffPage.replaceState( revIdOld, revIdNew, this );
+ this.diffPage.replaceState( revIdNew, revIdOld, this );
scrollLeft =
this.slider.getOldestVisibleRevisionIndex() * this.revisionWidth;
$revisionContainer.scrollLeft( scrollLeft );
diff --git a/modules/ext.RevisionSlider.SliderViewTwo.js
b/modules/ext.RevisionSlider.SliderViewTwo.js
index e7f1445..a565735 100644
--- a/modules/ext.RevisionSlider.SliderViewTwo.js
+++ b/modules/ext.RevisionSlider.SliderViewTwo.js
@@ -188,10 +188,17 @@
pointerMoved.setPosition( pos );
this.updatePointerPositionAttributes();
- this.refreshRevisions(
- this.getRevElementAtPosition( $revisions,
pointerOther.getPosition() ).attr( 'data-revid' ),
- $clickedRev.attr( 'data-revid' )
- );
+ if ( $line.hasClass(
'mw-revslider-pointer-container-newer' ) ) {
+ this.refreshRevisions(
+ $clickedRev.attr( 'data-revid' ),
+ this.getRevElementAtPosition(
$revisions, pointerOther.getPosition() ).attr( 'data-revid' )
+ );
+ } else {
+ this.refreshRevisions(
+ this.getRevElementAtPosition(
$revisions, pointerOther.getPosition() ).attr( 'data-revid' ),
+ $clickedRev.attr( 'data-revid' )
+ );
+ }
this.resetRevisionStylesBasedOnPointerPosition(
$revisions );
this.alignPointers();
},
diff --git a/tests/browser/features/pointers.feature
b/tests/browser/features/pointers.feature
index 7a1bbb5..616635b 100644
--- a/tests/browser/features/pointers.feature
+++ b/tests/browser/features/pointers.feature
@@ -26,16 +26,3 @@
And the upper pointer should be on revision 4
And revision 3 should be loaded on the left of the diff
And revision 4 should be loaded on the right of the diff
-
- Scenario: RevisionSlider pointers switch when crossed over
- Given I am on the diff page
- When I have loaded the RevisionSlider and dismissed the help dialog
- And I drag the lower pointer to revision 3
- And I wait until the diff has loaded
- And I click on revision 1 to move the upper pointer
- And I wait until the diff has loaded
- And I wait until the pointers stopped moving
- Then the lower pointer should be on revision 1
- And the upper pointer should be on revision 3
- And revision 1 should be loaded on the left of the diff
- And revision 3 should be loaded on the right of the diff
diff --git a/tests/qunit/RevisionSlider.DiffPage.test.js
b/tests/qunit/RevisionSlider.DiffPage.test.js
index b37ffaf..648b777 100644
--- a/tests/qunit/RevisionSlider.DiffPage.test.js
+++ b/tests/qunit/RevisionSlider.DiffPage.test.js
@@ -32,8 +32,8 @@
assert.propEqual(
history.state,
{
- revid1: 3,
- revid2: 37,
+ diff: 3,
+ oldid: 37,
pointerOlderPos: 1,
pointerNewerPos: 3,
sliderPos: NaN
--
To view, visit https://gerrit.wikimedia.org/r/352801
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If5d9cbb8ebd872aee376d249942e6881c8edb984
Gerrit-PatchSet: 7
Gerrit-Project: mediawiki/extensions/RevisionSlider
Gerrit-Branch: master
Gerrit-Owner: WMDE-Fisch <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Andrew-WMDE <[email protected]>
Gerrit-Reviewer: Tobias Gritschacher <[email protected]>
Gerrit-Reviewer: WMDE-Fisch <[email protected]>
Gerrit-Reviewer: WMDE-leszek <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
