Changeset 24671

Timestamp:

07/12/2013 02:01:39 PM (12 years ago)

Author:

markjaquith

Message:

Revisions: Cleanup, bug fixes, refactoring, polish.

• Hide the tooltip initially.
• Fix a bug with routing.
• Further separate the Slider model and view, refactoring its code.
• More reliance on events than direct calls between areas.
• Smarter background diff loading (single mode). Loads the diffs closest to your position first.
• Removed a bunch of manual templating and render() methods. Now relies more on the WP Backbone Views functionality.
• include the requested id in ensure:load.
• new trigger: ensure, for ensure() attempts, regardless of whether they are already loaded.
• pass along a promise in both ensure and ensure:load.
• in ensure, remove requests for diffs we aready have

See #24425.

Location:

trunk/wp-admin

Files:

• css/wp-admin.css (modified) (1 diff)
• includes/revision.php (modified) (1 diff)
• js/revisions.js (modified) (28 diffs)
• revision.php (modified) (1 diff)

trunk/wp-admin/css/wp-admin.css

@@ -3698 +3698 @@
     min-width: 130px;
     padding: 4px;
+    display: none;
 }
 

trunk/wp-admin/includes/revision.php

@@ -100 +100 @@
 
     // Now, grab the initial diff
-    $compare_two_mode = (bool) $from;
-    if ( ! $from ) // Single mode
+    $compare_two_mode = is_numeric( $from );
+    if ( ! $compare_two_mode ) {
         $from = array_keys( array_slice( $revisions, array_search( $selected_revision_id, array_keys( $revisions ) ) - 1, 1, true ) );
         $from = $from[0];
+    }
+
+    $from = absint( $from );
 
     $diffs = array( array(

trunk/wp-admin/js/revisions.js

@@ -34 +34 @@
     revisions.model.Slider = Backbone.Model.extend({
         defaults: {
-            value: 0,
+            value: null,
+            values: null,
             min: 0,
             max: 1,
             step: 1,
+            range: false,
             compareTwoMode: false
         },
@@ -44 +46 @@
             this.frame = options.frame;
             this.revisions = options.revisions;
-            this.set({
-                max:   this.revisions.length - 1,
-                value: this.revisions.indexOf( this.revisions.get( revisions.settings.to ) ),
-                compareTwoMode: this.frame.get('compareTwoMode')
-            });
 
             // Listen for changes to the revisions or mode from outside
@@ -57 +54 @@
             this.listenTo( this, 'change:from', this.handleLocalChanges );
             this.listenTo( this, 'change:to', this.handleLocalChanges );
+            this.listenTo( this, 'change:compareTwoMode', this.updateSliderSettings );
+            this.listenTo( this, 'update:revisions', this.updateSliderSettings );
 
             // Listen for changes to the hovered revision
             this.listenTo( this, 'change:hoveredRevision', this.hoverRevision );
+
+            this.set({
+                max:   this.revisions.length - 1,
+                compareTwoMode: this.frame.get('compareTwoMode'),
+                from: this.frame.get('from'),
+                to: this.frame.get('to')
+            });
+            this.updateSliderSettings();
+        },
+
+        getSliderValue: function( a, b ) {
+            return isRtl ? this.revisions.length - this.revisions.indexOf( this.get(a) ) - 1 : this.revisions.indexOf( this.get(b) );
+        },
+
+        updateSliderSettings: function() {
+            if ( this.get('compareTwoMode') ) {
+                this.set({
+                    values: [
+                        this.getSliderValue( 'to', 'from' ),
+                        this.getSliderValue( 'from', 'to' )
+                    ],
+                    value: null,
+                    range: true // ensures handles cannot cross
+                });
+            } else {
+                this.set({
+                    value: this.getSliderValue( 'to', 'to' ),
+                    values: null,
+                    range: false
+                });
+            }
+            this.trigger( 'update:slider' );
         },
 
@@ -162 +193 @@
     revisions.model.Diffs = Backbone.Collection.extend({
         initialize: function( models, options ) {
+            _.bindAll( this, 'getClosestUnloaded' );
+            this.loadAll = _.once( this._loadAll );
             this.revisions = options.revisions;
             this.requests  = {};
@@ -173 +206 @@
             var deferred = $.Deferred();
             var ids      = {};
+            var from     = id.split(':')[0];
+            var to       = id.split(':')[1];
+            ids[id] = true;
+
+            wp.revisions.log( 'ensure', id );
+
+            this.trigger( 'ensure', ids, from, to, deferred.promise() );
 
             if ( diff ) {
                 deferred.resolveWith( context, [ diff ] );
             } else {
-                this.trigger( 'ensure:load', ids );
-                _.each( ids, _.bind( function(id) {
+                this.trigger( 'ensure:load', ids, from, to, deferred.promise() );
+                _.each( ids, _.bind( function( id ) {
                     // Remove anything that has an ongoing request
                     if ( this.requests[ id ] )
+                        delete ids[ id ];
+                    // Remove anything we already have
+                    if ( this.get( id ) )
                         delete ids[ id ];
                 }, this ) );
@@ -197 +240 @@
         },
 
-        loadNew: function( comparisons ) {
+        // Returns an array of proximal diffs
+        getClosestUnloaded: function( ids, centerId ) {
             var self = this;
-            _.each( comparisons, function( id, index ) {
-                // Already exists in collection. Don't request it again.
-                if ( self.get( id ) )
-                    delete comparisons[ index ];
-            });
-            wp.revisions.log( 'loadNew', comparisons );
-
-            if ( comparisons.length )
-                return this.load( comparisons );
-            else
-                return $.Deferred().resolve().promise();
+            return _.chain([0].concat( ids )).initial().zip( ids ).sortBy( function( pair ) {
+                return Math.abs( centerId - pair[1] );
+            }).map( function( pair ) {
+                return pair.join(':');
+            }).filter( function( diffId ) {
+                return _.isUndefined( self.get( diffId ) ) && ! self.requests[ diffId ];
+            }).value();
+        },
+
+        _loadAll: function( allRevisionIds, centerId, num ) {
+            var self = this, deferred = $.Deferred();
+            diffs = _.first( this.getClosestUnloaded( allRevisionIds, centerId ), num );
+            if ( _.size( diffs ) > 0 ) {
+                this.load( diffs ).done( function() {
+                    deferred.resolve();
+                    self._loadAll( allRevisionIds, centerId, num );
+                });
+                return deferred.promise();
+            } else {
+                return deferred.reject().promise();
+            }
         },
 
@@ -215 +269 @@
             wp.revisions.log( 'load', comparisons );
             // Our collection should only ever grow, never shrink, so remove: false
-            return this.fetch({ data: { compare: comparisons }, remove: false });
-        },
-
-        loadLast: function( num ) {
-            var ids;
-
-            num = num || 1;
-            ids = _.last( this.getProximalDiffIds(), num );
-
-            if ( ids.length )
-                return this.loadNew( ids );
-            else
-                return $.Deferred().resolve().promise();
-        },
-
-        loadLastUnloaded: function( num ) {
-            var ids;
-
-            num = num || 1;
-            ids = _.last( this.getUnloadedProximalDiffIds(), num );
-
-            if ( ids.length )
-                return this.loadNew( ids );
-            else
-                return $.Deferred().resolve().promise();
-        },
-
-        getProximalDiffIds: function() {
-            var previous = 0, ids = [];
-            this.revisions.each( _.bind( function( revision ) {
-                ids.push( previous + ':' + revision.id );
-                previous = revision.id;
-            }, this ) );
-            return ids;
-        },
-
-        getUnloadedProximalDiffIds: function() {
-            var comparisons = this.getProximalDiffIds();
-            comparisons     = _.object( comparisons, comparisons );
-            _.each( comparisons, _.bind( function( id ) {
-                // Exists
-                if ( this.get( id ) )
-                    delete comparisons[ id ];
-            }, this ) );
-            return _.toArray( comparisons );
-        },
-
-        loadAllBy: function( chunkSize ) {
-            chunkSize    = chunkSize || 20;
-            var unloaded = this.getUnloadedProximalDiffIds();
-            if ( unloaded.length ) {
-                return this.loadLastUnloaded( chunkSize ).always( _.bind( function() {
-                    this.loadAllBy( chunkSize );
-                }, this ) );
-            }
+            return this.fetch({ data: { compare: comparisons }, remove: false }).done( function(){
+                wp.revisions.log( 'load:complete', comparisons );
+            });
         },
 
@@ -330 +332 @@
             this.listenTo( this, 'change:from', this.changeRevisionHandler );
             this.listenTo( this, 'change:to', this.changeRevisionHandler );
-            this.listenTo( this, 'update:revisions', this.loadSurrounding );
-            this.listenTo( this, 'change:compareTwoMode', this.changedMode );
+            this.listenTo( this, 'update:revisions', this.updatedRevisions );
             this.listenTo( this.diffs, 'ensure:load', this.updateLoadingStatus );
             this.listenTo( this, 'update:diff', this.updateLoadingStatus );
@@ -340 +341 @@
             properties.compareTwoMode = revisions.settings.compareTwoMode;
             properties.baseUrl = revisions.settings.baseUrl;
-            this.set( properties, { silent: true } );
+            this.set( properties );
 
             // Start the router
@@ -347 +348 @@
         },
 
-        changedMode: function() {
-            // This isn't passed from/to so we grab them from the model
-            this.loadSurrounding( this.get( 'from' ), this.get( 'to' ) );
-        },
-
         updateLoadingStatus: function() {
             this.set( 'loading', ! this.diff() );
         },
 
-        loadSurrounding: function( from, to ) {
-            // Different strategies for single and compare-two models
+        updatedRevisions: function( from, to ) {
             if ( this.get( 'compareTwoMode' ) ) {
                 // TODO: compare-two loading strategy
             } else {
-                // TODO: clean this up to hook in to the ensure process
-                if ( this.revisions.length ) {
-                    // Load the rest: first 10, then the rest by 50
-                    this.diffs.loadLastUnloaded( 10 ).always( _.bind( function() {
-                        this.diffs.loadAllBy( 50 );
-                    }, this ) );
-                }
+                this.diffs.loadAll( this.revisions.pluck('id'), to.id, 40 );
             }
         },
@@ -394 +383 @@
             this.trigger( 'update:revisions', from, to );
 
+            diff = this.diffs.get( diffId );
+
             // If we already have the diff, then immediately trigger the update.
-            diff = this.diffs.get( diffId );
             if ( diff ) {
                 this.trigger( 'update:diff', diff );
@@ -448 +438 @@
 
         render: function() {
-            console.log( 'diff', this.model.diff() );
-            this.model.updateDiff({ immediate: true }).done( _.bind( function() {
-                wp.Backbone.View.prototype.render.apply( this, arguments );
-
-                $('#wpbody-content .wrap').append( this.el );
-                this.updateCompareTwoMode();
-                this.views.ready();
-            }, this ) );
+            wp.Backbone.View.prototype.render.apply( this, arguments );
+
+            $('#wpbody-content .wrap').append( this.el );
+            this.updateCompareTwoMode();
+            this.renderDiff( this.model.diff() );
+            this.views.ready();
 
             return this;
@@ -524 +512 @@
 
     // The tickmarks view
-    // This contains the slider tickmarks.
     revisions.view.Tickmarks = wp.Backbone.View.extend({
         className: 'revisions-tickmarks',
 
-        render: function() {
+        ready: function() {
             var tickCount, tickWidth;
-
             tickCount = this.model.revisions.length - 1;
             tickWidth = 1 / tickCount;
 
-            this.$el.html('');
             _(tickCount).times( function(){ this.$el.append( '<div></div>' ); }, this );
-
             this.$('div').css( 'width', ( 100 * tickWidth ) + '%' );
-        },
-
-        ready: function() {
-            this.render();
-        }
-    });
-
-    // The meta view.
-    // This contains the revision meta, and the restore button.
+        }
+    });
+
+    // The meta view
     revisions.view.Meta = wp.Backbone.View.extend({
         className: 'revisions-meta',
@@ -556 +535 @@
 
         initialize: function() {
-            this.listenTo( this.model, 'update:revisions', this.updateMeta );
+            this.listenTo( this.model, 'update:revisions', this.ready );
+        },
+
+        prepare: function() {
+            return this.model.toJSON();
+        },
+
+        ready: function() {
+            this.$('.restore-revision').prop( 'disabled', this.model.get('to').get('current') );
         },
 
         restoreRevision: function() {
-            var restoreUrl    = this.model.get('to').attributes.restoreUrl.replace(/&amp;/g, '&');
+            var restoreUrl = this.model.get('to').attributes.restoreUrl.replace(/&amp;/g, '&');
             document.location = restoreUrl;
-        },
-
-        updateMeta: function( from, to ) {
-            this.$el.html( this.template( this.model.toJSON() ) );
-            this.$('.restore-revision').prop( 'disabled', to.attributes.current );
         }
     });
 
     // The checkbox view.
-    // Encapsulates all of the configuration for the compare checkbox.
     revisions.view.Checkbox = wp.Backbone.View.extend({
         className: 'revisions-checkbox',
@@ -581 +562 @@
 
         initialize: function() {
-            this.$el.html( this.template() );
+            this.listenTo( this.model, 'change:compareTwoMode', this.updateCompareTwoMode );
+        },
+
+        ready: function() {
+            if ( this.model.revisions.length < 3 )
+                $('.revision-toggle-compare-mode').hide();
         },
 
@@ -592 +578 @@
             // Activate compare two mode?
             this.model.set({ compareTwoMode: $('.compare-two-revisions').prop('checked') });
-
-            // Update route
-            this.model.router.updateUrl();
-        },
-
-        ready: function() {
-            // Hide compare two mode toggle when fewer than three revisions.
-            if ( this.model.revisions.length < 3 )
-                $('.revision-toggle-compare-mode').hide();
-
-            this.listenTo( this.model, 'change:compareTwoMode', this.updateCompareTwoMode );
-
-            // Update the mode in case route has set it
-            this.updateCompareTwoMode();
-        }
-
+        }
     });
 
@@ -620 +591 @@
             this.listenTo( this.model, 'change:hovering', this.toggleVisibility );
             this.listenTo( this.model, 'change:scrubbing', this.toggleVisibility );
-        },
-
-        ready: function() {
-            this.toggleVisibility({ immediate: true });
         },
 
@@ -651 +618 @@
                 return;
 
-            // Insert revision data.
             this.$el.html( this.template( this.model.get('revision').toJSON() ) );
 
@@ -676 +642 @@
 
         initialize: function() {
-            this.$el.html( this.template() );
             this.listenTo( this.model, 'update:revisions', this.disabledButtonCheck );
         },
@@ -696 +661 @@
 
             this.model.set( attributes );
-
-            // Update route
-            this.model.router.updateUrl();
         },
 
@@ -744 +706 @@
         initialize: function() {
             _.bindAll( this, 'start', 'slide', 'stop', 'mouseMove' );
-            this.listenTo( this.model, 'change:compareTwoMode', this.updateSliderSettings );
-            this.listenTo( this.model, 'update:revisions', this.updateSliderSettings );
+            this.listenTo( this.model, 'update:slider', this.applySliderSettings );
         },
 
@@ -755 +716 @@
             }) );
 
-            this.listenTo( this, 'slide:stop', this.updateSliderSettings );
+            this.applySliderSettings();
         },
 
@@ -790 +751 @@
         },
 
-        updateSliderSettings: function() {
-            var handles, leftValue, rightValue;
+        applySliderSettings: function() {
+            this.$el.slider( _.pick( this.model.toJSON(), 'value', 'values', 'range' ) );
+            var handles = this.$('a.ui-slider-handle');
 
             if ( this.model.get('compareTwoMode') ) {
-                leftValue = isRtl ? this.model.revisions.length - this.model.revisions.indexOf( this.model.get('to') ) - 1 :
-                                            this.model.revisions.indexOf( this.model.get('from') ),
-                rightValue = isRtl ?    this.model.revisions.length - this.model.revisions.indexOf( this.model.get('from') ) - 1 :
-                                            this.model.revisions.indexOf( this.model.get('to') );
-
-                // Set handles to current from / to models.
-                // Reverse order for RTL
-                this.$el.slider( {
-                    values: [
-                        leftValue,
-                        rightValue
-                    ],
-                    value: null,
-                    range: true // Range mode ensures handles can't cross
-                } );
-
-                handles = this.$('a.ui-slider-handle');
                 // in RTL mode the 'left handle' is the second in the slider, 'right' is first
                 handles.first()
@@ -818 +763 @@
                     .toggleClass( 'left-handle', !! isRtl )
                     .toggleClass( 'right-handle', ! isRtl );
-
             } else {
-                this.$el.slider( { // Set handle to current to model
-                    // Reverse order for RTL.
-                    value: isRtl ? this.model.revisions.length - this.model.revisions.indexOf( this.model.get('to') ) - 1 :
-                                    this.model.revisions.indexOf( this.model.get('to') ),
-                    values: null, // Clear existing two handled values
-                    range: false
-                } );
-                this.$('a.ui-slider-handle').removeClass('left-handle right-handle');
+                handles.removeClass('left-handle right-handle');
             }
         },
@@ -851 +788 @@
                 // In two handle mode, ensure handles can't be dragged past each other.
                 // Adjust left/right boundaries and reset points.
-                if ( view.model.frame.get('compareTwoMode') ) {
+                if ( view.model.get('compareTwoMode') ) {
                     var rightHandle = $( ui.handle ).parent().find('.right-handle'),
                         leftHandle  = $( ui.handle ).parent().find('.left-handle');
@@ -893 +830 @@
             var attributes;
             // Compare two revisions mode
-            if ( ! _.isUndefined( ui.values ) && this.model.frame.get('compareTwoMode') ) {
+            if ( ! _.isUndefined( ui.values ) && this.model.get('compareTwoMode') ) {
                 // Prevent sliders from occupying same spot
                 if ( ui.values[1] === ui.values[0] )
@@ -928 +865 @@
         stop: function( event, ui ) {
             $( window ).off('mousemove.wp.revisions');
-            this.updateSliderSettings();
+            this.model.updateSliderSettings(); // To snap us back to a tick mark
             this.model.set({ scrubbing: false });
         }
@@ -952 +889 @@
             this.routes = this.getRoutes();
 
-            // Maintain state history when dragging
+            // Maintain state history when dragging/clicking
             this.listenTo( this.model, 'update:diff', _.debounce( this.updateUrl, 250 ) );
+            this.listenTo( this.model, 'change:compareTwoMode', this.updateUrl );
         },
 

trunk/wp-admin/revision.php

@@ -15 +15 @@
 
 $revision_id = absint( $revision );
-$from = absint( $from );
+
+$from = is_numeric( $from ) ? absint( $from ) : null;
 if ( ! $revision_id )
     $revision_id = absint( $to );