Skip to content

Support editing arbitrary precisions for GlobeCoordinates #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 11, 2014
Merged

Support editing arbitrary precisions for GlobeCoordinates #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6aae97b
Support editing arbitrary precisions for GlobeCoordinates
adrianheine May 28, 2014
e5a4e95
Update en.json
thiemowmde Jun 4, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion i18n/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"valueview-expert-unsupportedvalue-unsupporteddatatype": "Handling of values for \"$1\" data type is not yet supported.",
"valueview-expert-emptyvalue-empty": "empty",
"valueview-expert-globecoordinateinput-precision": "Precision:",
"valueview-expert-globecoordinateinput-customprecision": "special ($1)",
"valueview-expert-timevalue-calendar-gregorian": "Gregorian",
"valueview-expert-timevalue-calendar-julian": "Julian",
"valueview-expert-timeinput-precision": "Precision:",
Expand All @@ -22,4 +23,4 @@
"valueview-preview-label": "will be displayed as:",
"valueview-preview-novalue": "no valid value recognized",
"valueview-listrotator-auto": "auto"
}
}
32 changes: 18 additions & 14 deletions lib/jquery.ui/jquery.ui.listrotator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,25 +339,29 @@
.appendTo( $( 'body' ) ).hide();

$.each( this.options.values, function( i, v ) {
self.$menu.append(
$( '<li/>' )
.append(
$( '<a/>' )
.attr( 'href', 'javascript:void(0);')
.text( v.label )
.on( 'click', function( event ) {
event.stopPropagation();
self._trigger( 'selected', null, [ self.value( v.value ) ] );
self.$menu.hide();
} )
)
.data( 'value', v.value )
);
self._addMenuItem( v );
} );

this.$menu.menu();
},

_addMenuItem: function( item ) {
var self = this;
return $( '<li/>' )
.append(
$( '<a/>' )
.attr( 'href', 'javascript:void(0);')
.text( item.label )
.on( 'click', function( event ) {
event.stopPropagation();
self._trigger( 'selected', null, [ self.value( item.value ) ] );
self.$menu.hide();
} )
)
.data( 'value', item.value )
.appendTo( this.$menu );
},

/**
* Sets/Gets the widget's value. Setting the value involves setting the rotator to the
* specified value without any animation.
Expand Down
24 changes: 23 additions & 1 deletion src/ExpertExtender/ExpertExtender.Listrotator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
_onValueChange: null,
_getUpstreamValue: null,

_$customItem: null,

rotator: null,

/**
Expand Down Expand Up @@ -59,9 +61,29 @@
*/
draw: function() {
var value = this._getUpstreamValue();
if( value && this.rotator.autoActive() ) {
if( !value ) {
return;
}

if( this._$customItem ) {
this.rotator.options.values.splice(this._customValue, 1);
this._$customItem.remove();
this._$customItem = null;
this._customValue = null;
}
if( value.custom ) {
this._customValue = this.rotator.options.values.push( value ) - 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable name is misleading. Should be _customValueIndex or something like that.

It's also missing a declaration.

this._$customItem = this.rotator._addMenuItem( value );
value = value.value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, this is odd. What happens to the other values that are not custom? Are they supposed to be numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numbers or strings, yes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean "number or object"? I may be wrong but it looks like you are introducing this. Why not always make it an object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-custom values are numbers or strings. It's like this:

Listrotators are initialized with an array of name/value pairs. These are the non-custom values which are always shown. Setting the rotator to one of these values is trivial using .value and ._setValue. On the other hand, our experts need to be able to set the Listrotator to a value which is not in this initial list (a ›custom‹ value). This is done by injecting a name/value pair into the Listrotator and then calling .value and ._setValue.

This module here (the ExpertExtender's Listrotator extension) wraps the ugly injection and handles cleaning up. In order to do that, it has to recognize a given value as either custom or not. It could also just try to find it in the Listrotator's value array, but that would be uglier, slower and more error-prone. So the expert has to explicitly say that a value it passes is not just a pre-configured value from the Listrotator's config, but indeed something new and custom. That's done via the custom property, and it also needs to pass a name and a value, because the Listrotator obviously needs a name and a value.

The only thing I can imagine we could change without rewriting a lot of Listrotator (which I tried to avoid with this patch) is changing the signature of the getUpstreamValue argument of ExpertExtender.Listrotator to always return an object, either { value: 'normal value' } or { value: 'custom value', label: 'label for custom value', custom: true }. I don't see much of an advantage doing that, though.

}

if( this.rotator.autoActive() || this._$customItem ) {
this.rotator.value( value );
this.rotator._setValue( value );
if( this._$customItem ) {
this.rotator.$menu.data( 'menu' ).refresh();
this.rotator.activate(); // disables autoActive state
}
}
},

Expand Down
62 changes: 36 additions & 26 deletions src/experts/GlobeCoordinateInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* @author H. Snater < mediawiki@snater.com >
* @author Daniel Werner < daniel.werner@wikimedia.de >
*/
( function( $, vv, GlobeCoordinate, Formatter ) {
( function( $, vv, Formatter ) {
'use strict';

var PARENT = vv.experts.StringValue;
Expand Down Expand Up @@ -38,7 +38,15 @@
},
function(){
var value = self.viewState().value();
return value && roundPrecision( value.getValue().getPrecision() );
if( !value ) {
return value;
}
value = value.getValue().getPrecision();
return getPrecisionSetting( value ) || {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this really is either a number or an object? Hm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugly, isn't it?

custom: true,
value: value,
label: self._messageProvider.getMessage('valueview-expert-globecoordinateinput-customprecision', [ Formatter.PRECISIONTEXT( value ) ] )
};
}
);

Expand Down Expand Up @@ -89,7 +97,7 @@
}

var options = {},
precision = getPrecisionSetting( this.precisionRotator.getValue() );
precision = this.precisionRotator.getValue();

if( precision !== null ) {
options.precision = precision;
Expand All @@ -111,43 +119,29 @@
/**
* Rounds a given precision for being able to use it as internal "constant".
*
* TODO: Calculated numbers used for the precision (e.g. 1/60) may result in different values
* in front- and back-end. Either use dedicated float numbers in front- and back-end or
* integrate the rounding in GlobeCoordinate.
*
* @since 0.1
*
* @param {number} precision
* @return {number}
*/
function roundPrecision( precision ) {
var precisions = GlobeCoordinate.PRECISIONS,
highestPrecision = precisions[precisions.length - 1],
multiplier = 1;

// To make sure that not too much digits are cut off for the "constant" precisions to be
// distinctive, we round with a multiplier that rounds the most precise precision to a
// number greater than 1:
while( highestPrecision * multiplier < 1 ) {
multiplier *= 10;
}

return Math.round( precision * multiplier ) / multiplier;
return Number( precision.toPrecision(4) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know toPrecision returns a string, right? Wouldn't parseFloat be better then?

}

/**
* Returns the original precision level for a rounded precision.
* Returns the original precision level for an unrounded precision.
*
* @since 0.1
*
* @param {number} roundedPrecision
* @param {number} precision
* @return {number|null}
*/
function getPrecisionSetting( roundedPrecision ) {
function getPrecisionSetting( precision ) {
var rounded,
actualPrecision = null;
actualPrecision = null,
roundedPrecision = roundPrecision( precision );

$.each( GlobeCoordinate.PRECISIONS, function( i, precision ) {
$.each( PRECISIONS, function( i, precision ) {
rounded = roundPrecision( precision );
if( rounded === roundedPrecision ) {
actualPrecision = precision;
Expand All @@ -160,7 +154,7 @@

function getPrecisionValues() {
var precisionValues = [];
$.each( GlobeCoordinate.PRECISIONS, function( i, precision ) {
$.each( PRECISIONS, function( i, precision ) {
var label = Formatter.PRECISIONTEXT( precision );
precisionValues.unshift( {
value: roundPrecision( precision ),
Expand All @@ -170,4 +164,20 @@
return precisionValues;
}

}( jQuery, jQuery.valueview, globeCoordinate.GlobeCoordinate, globeCoordinate.Formatter ) );
var PRECISIONS = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you introducing this duplicate here? Can't you reuse the GlobeCoordinate.PRECISIONS array from globeCoordinate.GlobeCoordinate.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's removed there in wmde/DataValuesJavaScript#40.

10,
1,
0.1,
1 / 60,
0.01,
0.001,
1 / 3600,
0.0001,
1 / 36000,
0.00001,
1 / 360000,
0.000001,
1 / 3600000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: See #60 now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That shouldn't be done in this change, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, right.

];

}( jQuery, jQuery.valueview, globeCoordinate.Formatter ) );
1 change: 1 addition & 0 deletions src/experts/resources.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
),
'messages' => array(
'valueview-expert-globecoordinateinput-precision',
'valueview-expert-globecoordinateinput-customprecision',
),
),

Expand Down
61 changes: 58 additions & 3 deletions tests/src/ExpertExtender/ExpertExtender.Listrotator.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,71 @@

testExpertExtenderExtension.constructor(
ExpertExtender.Listrotator,
new ExpertExtender.Listrotator( '', [ 'value' ] )
new ExpertExtender.Listrotator( '', [ { value: 'value', label: 'label' } ] )
);
testExpertExtenderExtension.destroy(
ExpertExtender.Listrotator,
new ExpertExtender.Listrotator( '', [ 'value' ] )
new ExpertExtender.Listrotator( '', [ { value: 'value', label: 'label' } ] )
);
testExpertExtenderExtension.init(
new ExpertExtender.Listrotator( '', [ 'value' ] )
new ExpertExtender.Listrotator( '', [ { value: 'value', label: 'label' } ] )
);

QUnit.test( 'supports custom values', function( assert ) {
var getUpstreamValue = function() {
return {
custom: true,
value: 'custom value',
label: 'label for custom value'
};
};
var $extender = $( '<div />' );

var listrotator = new ExpertExtender.Listrotator(
'',
[ { value: 'fixed value', label: 'label for fixed value' } ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you are testing here with non-custom values that are objects and not numbers as the code above expects. Shouldn't this test fail then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the constructor of Listrotator. This is the list of non-custom label-value pairs this listrotator has.

null,
getUpstreamValue
);

listrotator.init( $extender );
listrotator.draw();

assert.equal( listrotator.getValue(), 'custom value' );
} );

QUnit.asyncTest( 'supports switching away from custom values', function( assert ) {
var onValueChange = sinon.spy();
var upstreamValue = {
custom: true,
value: 'custom value',
label: 'label for custom value'
};
var getUpstreamValue = function() {
return upstreamValue;
};
var $extender = $( '<div />' );

var listrotator = new ExpertExtender.Listrotator(
'',
[ { value: 'fixed value', label: 'label for fixed value' } ],
onValueChange,
getUpstreamValue
);

listrotator.init( $extender );
listrotator.draw();
listrotator.rotator.prev();

setTimeout( function() {
sinon.assert.calledOnce( onValueChange );
assert.equal( listrotator.getValue(), 'fixed value' );

QUnit.start();
}, 200 );

} );

} )(
jQuery,
jQuery.valueview.ExpertExtender,
Expand Down