Wikimedia Bugzilla is closed!

Perhaps it is better to create something like an addOnLivePreviewHook()

Yes, specifically allowing a hook function to be called either before the contents of wpTextbox1 are grabbed or after the preview is updated. A call before would be helpful for adding live preview support to such scripts as [[User:PleaseStand/References segregator]], which temporarily reformats the wikitext when editing starts and changes it back to the correct format when the page is saved.

(In reply to comment #1)
> Perhaps it is better to create something like an addOnLivePreviewHook()

Absolutely right. Would taking this one step further and build a more generic hook system in wikibits make sense:

function addNamedHook( eventName, hookFunct );
function runNamedHook( eventName, autoexecuteFutureHooks = false );

Second argument of runNamedEventHook would determine if any future added hooks for that event are executed immediately (e.g. for onload) or delayed till the next run (e.g. for livepreview). Could possibly pass additional arguments through to the hook functions.
Otherwise I see multiple such hook systems being built into MediaWiki itself in the future, it's easy to predict that it will get more and more javascript functionality down the road. Additional hooks could already be useful in e.g. mwsuggest and ajaxwatch, and I could see use for such generic hooks to facilitate inter-script communication of specialized project scripts as well.

It would probably be more ideal to use jQuery "bind" event system somthing like: 

for the "add hook" you would use something like; 
$j( mw ).bind( eventName , function( event, param1, param2 ){
   // stuff to do at eventName time
});

Then the run hook use : 
$j( mw ).trigger( eventName, [ array, of, params ] );

A bind system is more general and can be applied to interface objects .. rather than just all in a root name space. 

This is already used in the usability extension and mwEmbed makes extensive use of this binding type system in conjunction with the scriptLoader it enables modules to conditionally extend each other while preserving a single server request for all script and css resources. 

For example the timedText module conditionally extends the embedPlayer request with its timedText stuff, the set of dependent libraries is dynamically built out binded to object specific events. 
i.e: http://svn.wikimedia.org/viewvc/mediawiki/trunk/extensions/TimedMediaHandler/TimedText/loader.js?view=markup

Created attachment 7389 [details]
Updated patch using jQuery custom events

Sure, why reinvent the wheel.

Fixed in r66721

I don't think it's a good idea to use jQuery with the object mw as the selector. It currently works, .bind/.trigger match, but I think it's more by luck and it might break in the future.

According to the documentation, jQuery() is designed to be given a selector string, a DOM element, an array of DOM elements, another jQuery object, or even nothing; but certainly not pure JS objects.

The bind trigger system is widely used for pure JS objects, so I don't think jQuery will ever disallow it. For example if you google the concept you see lots of folks recommending it.

But I do agree it can result in some strange gotchas and edge cases that may not be ideal and we should not do it "just because it works and other say to do it". 

A more traditional hook system may be more correct.

While jQuery does not officially "accept pure JS" objects its inherent in the model of loose typing that jQuery is designed around. i.e you can illustrate this with much more evil things like: 

var o = {};
$(o).css('width', 10);
console.log(o.width);

// Output:
10px

If we were to write a hook system it should share the nifty features of the jQuery one. Like namespaced bind add and removal, being able to pass in an array of objects to be binded, some concept of inheritance of same named local methods as the trigger, a convenience function to setup the hook system on-demand so that binds and triggers have no negative consequences if either does not exist etc.

In essence everything the jQuery bind / trigger system does. You can think of it as a plugin. We can always just override jQuery.fn.bind to guarantee pure JS object paths behavior predictably into the future. 

It comes down to semantics and clarity of intent of any given developer. Does it add syntactical clarity to be able to treat the bind trigger concept the same across objects or does it muddle things that it works the same? 

If it can save time and code to share the concept across objects then that’s good, if it will confuse people and take away flexibility then that’s bad. 



Some examples of gotchas for jQuery base bind / trigger

var o = {length: 3};
$(o).bind( 'custom', function(){ console.log('hi') } );
$(o).trigger('custom');

// Output
hi
hi
hi 

Of course this can be a "feature" if you have an array of objects that you want to bind events to. 

Also a "feature" of the binding mechanism includes calling the child method i.e:

var o = {change: function(){ console.log('hi method')} };
$(o).bind( 'change', function(){ console.log('hi bind') } );
$(o).trigger('change');

// Output
hi bind
hi method

Which can be tricky / non-ideal if you don't know about that.

Thank you for this detailed and very interesting explanation.

I took a look at the source of jQuery and ran through some debug sessions, their work using makeArray for loose-typing, and unique identifiers for robust references is really impressive.

I agree it is very, very unlikely that jQuery would disallow non-DOM objects in the future, and I think we can be confident it will continue to work with an object such as mw.

(In reply to comment #8)
> Some examples of gotchas for jQuery base bind / trigger
> 
> var o = {length: 3};
> $(o).bind( 'custom', function(){ console.log('hi') } );
> $(o).trigger('custom');
> 
> // Output
> hi
> hi
> hi 

Throws TypeError as of jQuery 1.6+, presumably because it's looping through 'o' and finds [0], [1] and [2] to be undefined.

Suggest we revert r66721 and possibly other similar implementations and instead create a mediawiki.hook module. ie.

mw.hooks.run( 'key', .., .. );
mw.hooks.add( 'key', fn );

That way different parts of the pages build up can tab into different events and hooks (instead or relying on the over-used "document ready" event. For example:

* 'outputPage.bodyContentReady' - (args: $bodyContent ): Run after <div id="bodyContent"> has been populated. Called once on a normal page, and re-called in special circumstances (ie. after live preview is injected). Scripts like tablesorters and collapsiblemakers would then hook into this, instead of document-ready to do their stuff.

That was an example of what people should not do. Presumably people would know better than to use "length" as an object property, bind trigger won't be the only thing that acts funny. 

I make pretty heavy use of bind trigger model. It certainly has its trade offs but so does a monolithic global hook system. 

Some points for bind / trigger: 

Its kind of standard for building reusable hooks into components, its used a lot in jQuery ui for extending ui stuff for example: http://jqueryui.com/demos/accordion/#event-create  

You mention "different parts of the pages build up can tab into different events
and hooks" thats not exclusive to the mediaWiki object, complex components need extendability as well. 

The bind trigger model integrates nicely when your creating virtual interfaces for components that inter-mix with emerging apis for dom elements. 

For example you create a virtual iframe to handle uploading you can use a fallback to flash on older browser that triggers the same events, as the modern html5 versions. As the native event becomes available you seamlessly pass the event along the same code path. 

The bind trigger model also is good about encapsulating bindings in the object representation. So when you copy or duplicate your element / object representation it keeps all its bindings.  

jQuery based Bind trigger of course plays nice with jQuery so you can bind / trigger on selectors.  

You can namescape your extension bindings so that its easy to unsubscribe a given extending component. For example you have "player" element. A few modules adding bindings to 'onplay', 'onpause', and 'onend'. Each module namespaces its event binding, i.e: onplay.fooModule so when a controller or sequencer decides it needs to unbind a given module it just runs $(player).unbind(.fooModule) then all fooModule bindings will be removed, with the flexibility to manage this in the controller or in the extending module.  


You certainly ~could~ build a hook system that had a lot of these qualities but it would not be as simple as .run and .add 

Maybe the point is simply the master mw object use hook while anything that touches a selector uses bind / trigger. I would be cautions that we don't end up with other components adding events to the core hook system when it would be a better path for the events to be locally bind / triggered.

If the core promotes this bind trigger model it may help people think about doing things that way for their module rather then each module setting up a local hook system or mapping and calling all points of extendability with a globally named hook.

Adding "patch" and "reviewed" keywords here for consistency, since Amalthea's attached patch was reviewed. Thanks for the patch, Amalthea.

Using the jQuery Event binding/triggering model doesn't seem good on performance (kinda slow for something this simple). Also it's not very recommended by jQuery to use it for this.

I'd suggest we create a simple module ourselves, the functionality we want here really is simple and we should be able to rely on this.

A very rough example:

  var register = {};

  mw.hooks = {
    register: function( name, callback ) {

      if(!register[name]) register[name] = [];

      push.call( register[name], callback );

    },
    run: function( name [, data ] ) {

      if(!register[name]) return;

      for ( var i = 0, len = register[name].length; i < len; i++ ) {
        register[name][i].apply( data || [] );
      }

    }
  };

If we update our copy of jQuery to 1.7 we could even use jQuery.Callbacks [1] to make this code a little more robust and shorter. 

--
Krinkle

[1] http://api.jquery.com/jQuery.Callbacks/#pubsub

Rephrasing bug to be more generic.

Example events (most of these currently abuse dom-ready or window-load, by making these mediawiki-custom events many things such as jquery-sortable would then work again for ajax preview).

* 'articleReady'
-- Runs whenever the article section of the page is rendered in html. Normally only once during a page load. Other times it woud run would be during ajax preview. Plugins like jquery.sortable or jquery.makeCollapsible would bind to this instead of $(document).ready

-- data: [ article wrapper element (#bodyContent for Vector) ]

* 'diffHeader' 
-- Runs whenever the diff meta data is shown. Useful for gadgets that want to insert links in here. Normally only once on a page, but gadgets like RTRC that dynamically load in diffs on-demand on Special:RecentChanges would fire these events more often so that stuff like Twinkle can actually work with other gadgets.

-- data: [ table.diff element containing the meta data and diff itself ]

This has some overlap with bug 30713.

This bug should probably focus on the introduction of a mw.hooks for anything we need it for. While the other bug focuses on the hook/event that happens on domready/preview and whether it should be a hook or a dom event.

Next week.

(In reply to comment #16)
> Next week.
....

Krinkle, will you be implementing this or can somebody else step in? Do you already have something that could be used as a start?

I think the API we need has been already pretty well described here and in the discussion under bug 30713.

Other than the code I have proposed here and on other bugs, I don't have any secret local branch that would be wasted if someone else works on it.

However I do have very specific ideas for how it should be implemented and would like to stay involved with it.

Having said that, I have a lot of things I work on and would very much welcome it if someone else steps up and gives this a shot. I'll do my best to be helpful in code review and give you anything you need.

Unassigned per comment 19

(In reply to comment #20)
> Unassigned per comment 19

You changed the assignee field instead of the bug status (which is still ASSIGNED).

Anyway, re-assigning, I'm working on this now and will push (at least a draft version) today.

(In reply to comment #21 by Krinkle)
> Anyway, re-assigning, I'm working on this now and will push (at least a draft
> version) today.

Krinkle: Did that happen? Is there some (public) gerrit change ID to review?

An updated concept, using $.Callbacks:


@@ -290,6 +290,112 @@ var mw = ( function ( $, undefined ) {
 
		hook: ( function () {
			var lists = {};
			return {
				/**
				 * Register a hook handler
				 */
				on: function ( name, handler ) {
					var list = lists[name] || ( lists[name] = $.Callback( 'memory') );
					list.add( handler );
				},
				/**
				 * Unregister a hook handler
				 */
				off: function ( name, handler ) {
					var list = lists[name];
					if ( list ) {
						list.remove( handler );
					}
				},
				/**
				 * Run a hook.
				 * @param {string} name Name of hook.
				 * @param {Mixed...} data
				 */
				emit: function ( name ) {
					var list = lists[name] || ( lists[name] = $.Callback( 'memory') );
					list.fireWith( null, slice.call( arguments, 1 ) );
				}
			};
		}() ),

Change-Id: Ic73a3efe53d6fb731e7f1e531d5f51530cd7e4fe

Merged