Changeset 24732

Timestamp:

07/18/2013 04:35:19 PM (11 years ago)

Author:

markjaquith

Message:

Revisions: Better error handling.

• Shows an error message if the current diff can't be loaded.
• For bulk pre-loading, catches errors, and cuts subsequent requests in half, until eventually giving up.
• Some CSS fixes related to this, and the loading spinner.
• wp.revisions.loadAll() now returns a promise representing whether or not all revisions could be loaded.

Fixes #24758.

Location:

trunk/wp-admin

Files:

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

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

@@ -3555 +3555 @@
 }
 
+.revisions .diff-error {
+    position: absolute;
+    text-align: center;
+    margin: 0 auto;
+    width: 100%;
+    display: none;
+}
+
+.revisions.diff-error .diff-error {
+    display: block;
+}
+
 .revisions .loading-indicator {
     position: absolute;
-    text-align: center;
     vertical-align: middle;
     opacity: 0;
-    margin: 0 auto;
     width: 100%;
-    height: 32px;
     top: 3em;
-    background: #fff url(../images/wpspin_light-2x.gif) no-repeat center top;
     -webkit-transition: opacity 0.5s;
     -moz-transition:    opacity 0.5s;
@@ -3570 +3578 @@
     -o-transition:      opacity 0.5s;
     transition:         opacity 0.5s;
+}
+
+.revisions .loading-indicator span.spinner {
+    display: block;
+    margin: 0 auto;
+    float: none;
 }
 
@@ -3586 +3600 @@
 .revisions.loading .diff {
     opacity: 0.5;
+}
+
+.revisions.diff-error .diff {
+    visibility: hidden;
 }
 

trunk/wp-admin/js/revisions.js

@@ -234 +234 @@
                 request.done( _.bind( function() {
                     deferred.resolveWith( context, [ this.get( id ) ] );
-                }, this ) );
+                }, this ) ).fail( _.bind( function() {
+                    deferred.reject();
+                }) );
             }
 
@@ -257 +259 @@
             if ( _.size( diffs ) > 0 ) {
                 this.load( diffs ).done( function() {
-                    deferred.resolve();
-                    self._loadAll( allRevisionIds, centerId, num );
+                    self._loadAll( allRevisionIds, centerId, num ).done( function() {
+                        deferred.resolve();
+                    });
+                }).fail( function() {
+                    if ( 1 === num ) { // Already tried 1. This just isn't working. Give up.
+                        deferred.reject();
+                    } else { // Request fewer diffs this time
+                        self._loadAll( allRevisionIds, centerId, Math.ceil( num / 2 ) ).done( function() {
+                            deferred.resolve();
+                        });
+                    }
                 });
-                return deferred.promise();
             } else {
-                return deferred.reject().promise();
-            }
+                deferred.resolve();
+            }
+            return deferred;
         },
 
@@ -315 +326 @@
         defaults: {
             loading: false,
+            error: false,
             compareTwoMode: false
         },
@@ -321 +333 @@
             var properties = {};
 
+            _.bindAll( this, 'receiveDiff' );
             this._debouncedEnsureDiff = _.debounce( this._ensureDiff, 200 );
 
@@ -352 +365 @@
 
         updateLoadingStatus: function() {
+            this.set( 'error', false );
             this.set( 'loading', ! this.diff() );
         },
@@ -400 +414 @@
             // If we already have the diff, then immediately trigger the update.
             if ( diff ) {
-                this.trigger( 'update:diff', diff );
+                this.receiveDiff( diff );
                 return $.Deferred().resolve().promise();
             // Otherwise, fetch the diff.
@@ -419 +433 @@
         },
 
+        receiveDiff: function( diff ) {
+            // Did we actually get a diff?
+            if ( _.isUndefined( diff ) || _.isUndefined( diff.id ) ) {
+                this.set({
+                    loading: false,
+                    error: true
+                });
+            } else if ( this._diffId === diff.id ) { // Make sure the current diff didn't change
+                this.trigger( 'update:diff', diff );
+            }
+        },
+
         _ensureDiff: function() {
-            return this.diffs.ensure( this._diffId, this ).done( function( diff ) {
-                // Make sure the current diff didn't change while the request was in flight.
-                if ( this._diffId === diff.id )
-                    this.trigger( 'update:diff', diff );
-            });
+            return this.diffs.ensure( this._diffId, this ).always( this.receiveDiff );
         }
     });
@@ -444 +466 @@
             this.listenTo( this.model, 'change:compareTwoMode', this.updateCompareTwoMode );
             this.listenTo( this.model, 'change:loading', this.updateLoadingStatus );
+            this.listenTo( this.model, 'change:error', this.updateErrorStatus );
 
             this.views.set( '.revisions-control-frame', new revisions.view.Controls({
@@ -469 +492 @@
         updateLoadingStatus: function() {
             this.$el.toggleClass( 'loading', this.model.get('loading') );
+        },
+
+        updateErrorStatus: function() {
+            this.$el.toggleClass( 'diff-error', this.model.get('error') );
         },
 

trunk/wp-admin/revision.php

@@ -187 +187 @@
 
 <script id="tmpl-revisions-diff" type="text/html">
-    <div class="loading-indicator"></div>
+    <div class="loading-indicator"><span class="spinner"></span></div>
+    <div class="diff-error"><?php _e( 'Sorry, something went wrong. The requested comparison could not be loaded.' ); ?></div>
     <div class="diff">
     <# _.each( data.fields, function( field ) { #>