Wikimedia Bugzilla is closed!

*** Bug 45021 has been marked as a duplicate of this bug. ***

Confirmed and hunt down.

From what I can see the problem may be in ve.dm.Surface not emitting any 'transact' events.

Adding a log() call in ve.init.mw.ViewPageTarget.prototype.onSurfaceModelTransact, shows me the following.

* When entering the first characters on a new page it is called.
  The callback then properly unbinds itself after the first call.
* Go to "Review and save"
  Now it binds the event again, so that it is notified of the first
  change (if ever) when the dialog is closed.
  This happens in ve.init.mw.ViewPageTarget.prototype.onShowChanges
* Close the dialog
* Altering the document again

> the method is not called

Which I assume means ve.dm.Surface for some reason didn't emit any - or mw.ViewPageTarget is binding it wrong, but that looks fine to me.

@Roan, any ideas?

(In reply to comment #2)
> Which I assume means ve.dm.Surface for some reason didn't emit any - or
> mw.ViewPageTarget is binding it wrong, but that looks fine to me.
> 
> @Roan, any ideas?
I'm pretty sure ve.dm.Surface emits the events correctly. You can check this by placing a breakpoint on the line where Surface calls this.emit().

It's possible there's a bug in EventEmitter with detaching and reattaching though.

(In reply to comment #3)
> (In reply to comment #2)
> > Which I assume means ve.dm.Surface for some reason didn't emit any - or
> > mw.ViewPageTarget is binding it wrong, but that looks fine to me.
> > 
> > @Roan, any ideas?
>
> I'm pretty sure ve.dm.Surface emits the events correctly. You can check this
> by
> placing a breakpoint on the line where Surface calls this.emit().
> 
> It's possible there's a bug in EventEmitter with detaching and reattaching
> though.

Hm.. could be. All I know is that the attached callback didn't get fired.

Using the following debug calls:

diff --git a/modules/ve/init/mw/targets/ve.init.mw.ViewPageTarget.js b/modules/ve/init/mw/targets/ve.init.mw.ViewPageTarget.js
index 859d4f8..cee2505 100644
--- a/modules/ve/init/mw/targets/ve.init.mw.ViewPageTarget.js
+++ b/modules/ve/init/mw/targets/ve.init.mw.ViewPageTarget.js
@@ -390,6 +390,7 @@ ve.init.mw.ViewPageTarget.prototype.onSaveError = function ( jqXHR, status ) {
  * @param {string} diffHtml
  */
 ve.init.mw.ViewPageTarget.prototype.onShowChanges = function ( diffHtml ) {
+       mw.log( 'onShowChanges', 'Invalidate the diff (by calling proxiedOnSurfaceModelTransact) on transact event' );
        // Invalidate the diff on next change
        this.surface.getModel().on( 'transact', this.proxiedOnSurfaceModelTransact );
 
@@ -562,6 +563,7 @@ ve.init.mw.ViewPageTarget.prototype.onToolbarFeedbackToolClick = function () {
  * @param {ve.dm.Transaction} tx Processed transaction
  */
 ve.init.mw.ViewPageTarget.prototype.onSurfaceModelTransact = function () {
+       mw.log( 'onSurfaceModelTransact', 'Clear the diff html' );
        // Clear the diff
        this.$saveDialog
                .find( '.ve-init-mw-viewPageTarget-saveDialog-slide-review .ve-init-mw-viewPageTarget-saveDialog-viewer' )
@@ -691,6 +693,9 @@ ve.init.mw.ViewPageTarget.prototype.setUpSurface = function ( doc ) {
        this.surface = new ve.Surface( this, doc, this.surfaceOptions );
        this.surface.getContext().hide();
        this.$document = this.surface.$.find( '.ve-ce-documentNode' );
+       this.surface.getModel().on( 'transact', function () {
+               mw.log( 've.Surface/Model/on/transact' );
+       } );
        this.surface.getModel().on( 'transact', this.proxiedOnSurfaceModelTransact );
        this.surface.getModel().on( 'history', this.proxiedOnSurfaceModelHistory );
        // Transplant the toolbar


I've determined that the "transact" event does indeed continue to be emitted. But onSurfaceModelTransact only gets called once. It is not being rebound:


 # Page load, enter text
 ve.Surface/Model/on/transact load.php:11256
 # Press "Review and save"
 onSurfaceModelTransact Clear the diff html load.php:11256
 # Close dialog
 # Enter more text
 ve.Surface/Model/on/transact load.php:11256
 ve.Surface/Model/on/transact load.php:11256
 # Press "Review and save"
 # Close dialog
 # Enter more lots more text
 (14x) ve.Surface/Model/on/transact  load.php:11256

The comments above are wrong. Actual timeline:

# Page load, enter text
     ve.Surface/Model/on/transact
     onSurfaceModelTransact Clear the diff html
     ve.Surface/Model/on/transact
(2x) ve.Surface/Model/on/transact
# Press "Review and save"
# Close dialog
# Enter more text
(3x) ve.Surface/Model/on/transact
# Press "Review and save"
# Close dialog

Related URL: https://gerrit.wikimedia.org/r/62847 (Gerrit Change I9eddebbdf9294ee3d46286bdf1b157e00252d300)

Code now merged; thanks, Timo! Will go out in next branch.