jenkins-bot has submitted this change and it was merged.
Change subject: Followup I59dfcfb25c, language change work with out event
logging
......................................................................
Followup I59dfcfb25c, language change work with out event logging
In I59dfcfb25c, for logging events when page is navigating away, we
used callbacks with mw.hook. That is wrong approach. If event logging
is disabled those callbacks will never called: it broke language change
and all use cases which navigates away from current page.
Event logging should not interfere with any ULS functionality. If ULS
functionality depends on callbacks from event logging, it is wrong.
In this patch, we give a small time window to make sure event logging is
fired, but we won't wait for its success or failure.
If eventlogging is disabled, this time window does not exist.
Change-Id: I0b7d9d8b9d1d01b99422010596ebfa80b2589d04
---
M resources/js/ext.uls.displaysettings.js
M resources/js/ext.uls.eventlogger.js
M resources/js/ext.uls.init.js
M resources/js/ext.uls.interface.js
4 files changed, 50 insertions(+), 36 deletions(-)
Approvals:
Amire80: Looks good to me, approved
jenkins-bot: Verified
Objections:
Aude: There's a problem with this change, please improve
diff --git a/resources/js/ext.uls.displaysettings.js
b/resources/js/ext.uls.displaysettings.js
index 51940ba..cf764e6 100644
--- a/resources/js/ext.uls.displaysettings.js
+++ b/resources/js/ext.uls.displaysettings.js
@@ -158,18 +158,30 @@
new mw.Api().parse( $.i18n(
'ext-uls-display-settings-anon-log-in-cta' ) )
.done( function ( parsedCta ) {
+ var deferred = new $.Deferred();
+
$loginCta.html( parsedCta );
$loginCta.find( 'a' ).click(
function ( event ) {
event.preventDefault();
// Because browsers
navigate away when clicking a link,
// we are are
overriding the normal click behavior to
// allow the event be
logged first - currently there is no
- // local queue for
events. The timeout is there to make sure
- // the user gets to the
new page even if event logging is slow
- // or fails.
- mw.hook(
'mw.uls.login.click' ).fire( function () {
+ // local queue for
events. Since the hook system does not
+ // allow returning
values, we have this ugly event logging
+ // specific hack to
delay the page load if event logging
+ // is enabled. The
promise is passed to the hook, so that
+ // if event logging is
enabled, in can resole the promise
+ // immediately to avoid
extra delays.
+ deferred.done( function
() {
window.location.href = event.target.href;
} );
+
+ mw.hook(
'mw.uls.login.click' ).fire( deferred );
+
+ // Delay is zero if
event logging is not enabled
+ window.setTimeout(
function () {
+
deferred.resolve();
+ }, mw.config.get(
'wgULSEventLogging' ) * 500 );
} );
} );
diff --git a/resources/js/ext.uls.eventlogger.js
b/resources/js/ext.uls.eventlogger.js
index abb0afb..20ad827 100644
--- a/resources/js/ext.uls.eventlogger.js
+++ b/resources/js/ext.uls.eventlogger.js
@@ -48,21 +48,16 @@
},
/**
- * Local wrapper for 'mw.eventLog.logEvent' which handles
default params
- * and ensures the correct schema is loaded.
+ * Local wrapper for 'mw.eventLog.logEvent'
*
* @param {Object} event Event action and optional fields
- * @param {int} [timeout] Fail the request if it is not
completed within
- * the specified timeout. Can be use for example to log
actions that
- * cause the browser to navigate to other pages.
* @return {jQuery.Promise} jQuery Promise object for the
logging call
*/
- log: function ( event, timeout ) {
+ log: function ( event ) {
// We need to create our own deferred for two reasons:
// - logEvent might not be executed immediately
// - we cannot reject a promise returned by it
- // So we proxy the original promises status updates
- // and register our timeout if requested (for links).
+ // So we proxy the original promises status updates.
var deferred = $.Deferred();
this.logEventQueue.add( function () {
@@ -70,10 +65,6 @@
.done( deferred.resolve )
.fail( deferred.reject );
} );
-
- if ( timeout !== undefined ) {
- window.setTimeout( deferred.reject, timeout );
- }
return deferred.promise();
},
@@ -110,11 +101,10 @@
/**
* Log language revert
- *
- * @param {Function} callback
+ * @param {jQuery.Deferred} deferred
*/
- ulsLanguageRevert: function ( callback ) {
- this.log( { action: 'ui-lang-revert' }, 500 ).always(
callback );
+ ulsLanguageRevert: function ( deferred ) {
+ this.log( { action: 'ui-lang-revert' } ).always(
deferred.resolve() );
},
/**
@@ -132,17 +122,11 @@
},
/**
- * Log login link click in display settings. Since this will
navigate
- * away from the current page, provide a callback option.
- *
- * If page is navigating away, event logging will fail because
it is not
- * yet completed. With the callback, the navigation can be
executed in
- * callback. The waiting for callback is not indefinite, but
with a timeout.
- *
- * @param {Function} callback
+ * Log login link click in display settings.
+ * @param {jQuery.Deferred} deferred
*/
- loginClick: function ( callback ) {
- this.log( { action: 'login-click' }, 500 ).always(
callback );
+ loginClick: function ( deferred ) {
+ this.log( { action: 'login-click' } ).always(
deferred.resolve );
},
/**
@@ -159,14 +143,14 @@
* Log interface language change
*
* @param {string} language language code
- * @param {Function} callback
+ * @param {jQuery.Deferred} deferred
*/
- interfaceLanguageChange: function ( language, callback ) {
+ interfaceLanguageChange: function ( language, deferred ) {
this.log( {
action: 'language-change',
context: 'interface',
interfaceLanguage: language
- }, 500 ).always( callback );
+ } ).always( deferred.resolve );
},
/**
diff --git a/resources/js/ext.uls.init.js b/resources/js/ext.uls.init.js
index 6bfe299..947b3b2 100644
--- a/resources/js/ext.uls.init.js
+++ b/resources/js/ext.uls.init.js
@@ -44,14 +44,23 @@
* @param {string} language Language code.
*/
mw.uls.changeLanguage = function ( language ) {
- var uri = new mw.Uri( window.location.href );
+ var uri = new mw.Uri( window.location.href ),
+ deferred = new $.Deferred();
- mw.hook( 'mw.uls.interface.language.change' ).fire( language,
function () {
+ deferred.done( function () {
uri.extend( {
setlang: language
} );
window.location.href = uri.toString();
} );
+
+ mw.hook( 'mw.uls.interface.language.change' ).fire( language,
deferred );
+
+ // Delay is zero if event logging is not enabled
+ window.setTimeout( function () {
+ deferred.resolve();
+ }, mw.config.get( 'wgULSEventLogging' ) * 500 );
+
};
mw.uls.setPreviousLanguages = function ( previousLanguages ) {
diff --git a/resources/js/ext.uls.interface.js
b/resources/js/ext.uls.interface.js
index 28f74a1..c5553b6 100644
--- a/resources/js/ext.uls.interface.js
+++ b/resources/js/ext.uls.interface.js
@@ -247,10 +247,19 @@
// It looks like the tipsy is always created from
scratch so that
// there wont be multiple event handlers bound to same
click.
$( 'a.uls-prevlang-link' ).on( 'click.ulstipsy',
function ( event ) {
+ var deferred = $.Deferred();
+
event.preventDefault();
- mw.hook( 'mw.uls.language.revert' ).fire(
function () {
+ deferred.done( function () {
mw.uls.changeLanguage(
event.target.lang );
} );
+
+ mw.hook( 'mw.uls.language.revert' ).fire(
deferred );
+
+ // Delay is zero if event logging is not enabled
+ window.setTimeout( function () {
+ deferred.resolve();
+ }, mw.config.get( 'wgULSEventLogging' ) * 500 );
} );
tipsyTimer = window.setTimeout( function () {
hideTipsy();
--
To view, visit https://gerrit.wikimedia.org/r/79452
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I0b7d9d8b9d1d01b99422010596ebfa80b2589d04
Gerrit-PatchSet: 6
Gerrit-Project: mediawiki/extensions/UniversalLanguageSelector
Gerrit-Branch: master
Gerrit-Owner: Santhosh <[email protected]>
Gerrit-Reviewer: Amire80 <[email protected]>
Gerrit-Reviewer: Aude <[email protected]>
Gerrit-Reviewer: Nikerabbit <[email protected]>
Gerrit-Reviewer: Santhosh <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
