r100925 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r100924‎ | r100925 | r100926 >
Date:01:02, 27 October 2011
Author:raindrift
Status:ok (Comments)
Tags:
Comment:
new thumbnail behavior for thumbnailing with multi-file uploads. Multi-file uploads don't get thumbnails, but have a control for
enabling them per-file in case they're needed. Previous behavior is unchanged.
fixed a bug where filename warnings from the API would break FileAPI (chunked, multi-file) uploads.
Needs testing in older IE and firefox, but checking it in now so it can get started w/ review for deploy next week.
Modified paths:
  • /trunk/extensions/UploadWizard/resources/jquery/jquery.showThumbCtrl.css (added) (history)
  • /trunk/extensions/UploadWizard/resources/jquery/jquery.showThumbCtrl.js (added) (history)
  • /trunk/extensions/UploadWizard/resources/mw.FormDataTransport.js (modified) (history)
  • /trunk/extensions/UploadWizard/resources/mw.UploadWizard.js (modified) (history)
  • /trunk/extensions/UploadWizard/resources/mw.UploadWizardDetails.js (modified) (history)
  • /trunk/extensions/UploadWizard/resources/mw.UploadWizardUpload.js (modified) (history)
  • /trunk/extensions/UploadWizard/resources/mw.UploadWizardUploadInterface.js (modified) (history)
  • /trunk/extensions/UploadWizard/resources/uploadWizard.css (modified) (history)

Diff [purge]

Index: trunk/extensions/UploadWizard/resources/jquery/jquery.showThumbCtrl.js
@@ -0,0 +1,14 @@
 2+/**
 3+ * Create 'show thumbnail' control, with optional tooltips. works like the 'remove' control.
 4+ */
 5+( function ( $j ) {
 6+ $j.fn.showThumbCtrl = function( msgKey, tooltipMsgKey, callback ) {
 7+ var msg = (msgKey === null) ? '' : gM( msgKey );
 8+ return $j( '<div class="mwe-upwiz-show-thumb-ctrl ui-corner-all" />' )
 9+ .attr( 'title', gM( tooltipMsgKey ) )
 10+ .click( function() { $j( this ).removeClass( 'hover' ).addClass( 'disabled' ).unbind( 'mouseenter mouseover mouseleave mouseout mouseup mousedown' ); callback(); } )
 11+ .hover( function() { $j( this ).addClass( 'hover' ); },
 12+ function() { $j( this ).removeClass( 'hover' ); } )
 13+ .append( $j( '<div class="ui-icon" /><div class="mwe-upwiz-show-thumb-ctrl-msg">' + msg + '</div>' ) );
 14+ };
 15+} )( jQuery );
Property changes on: trunk/extensions/UploadWizard/resources/jquery/jquery.showThumbCtrl.js
___________________________________________________________________
Added: svn:eol-style
116 + native
Index: trunk/extensions/UploadWizard/resources/jquery/jquery.showThumbCtrl.css
@@ -0,0 +1,22 @@
 2+.mwe-upwiz-show-thumb-ctrl {
 3+ outline: none;
 4+ cursor: pointer;
 5+}
 6+
 7+.mwe-upwiz-show-thumb-ctrl.hover .mwe-upwiz-show-thumb-ctrl-msg {
 8+ text-decoration: underline;
 9+}
 10+
 11+.mwe-upwiz-show-thumb-ctrl.disabled .mwe-upwiz-show-thumb-ctrl-msg {
 12+ text-decoration: none;
 13+ color: #cccccc;
 14+ cursor: default;
 15+}
 16+
 17+.mwe-upwiz-show-thumb-ctrl div {
 18+ color: #0645ad;
 19+ font-variant: bold;
 20+ float: left;
 21+}
 22+
 23+
Property changes on: trunk/extensions/UploadWizard/resources/jquery/jquery.showThumbCtrl.css
___________________________________________________________________
Added: svn:eol-style
124 + native
Index: trunk/extensions/UploadWizard/resources/mw.UploadWizardUploadInterface.js
@@ -16,6 +16,8 @@
1717 _this.div = $j('<div class="mwe-upwiz-file"></div>').get(0);
1818 _this.isFilled = false;
1919
 20+ _this.previewLoaded = false;
 21+
2022 _this.$fileInputCtrl = $j( '<input size="1" class="mwe-upwiz-file-input" name="file" type="file"/>' );
2123 if( mw.UploadWizard.config['enableMultiFileSelect'] ) {
2224 _this.$fileInputCtrl.attr( 'multiple', '1' );
@@ -25,7 +27,7 @@
2628
2729 _this.$indicator = $j( '<div class="mwe-upwiz-file-indicator"></div>' );
2830
29 - var visibleFilenameDiv = $j('<div class="mwe-upwiz-visible-file"></div>')
 31+ _this.visibleFilenameDiv = $j('<div class="mwe-upwiz-visible-file"></div>')
3032 .append( _this.$indicator )
3133 .append( '<div class="mwe-upwiz-visible-file-filename">'
3234 + '<div class="mwe-upwiz-file-preview"/>'
@@ -44,8 +46,10 @@
4547 function() { _this.upload.remove(); }
4648 ).addClass( "mwe-upwiz-file-status-line-item" );
4749
48 - visibleFilenameDiv.find( '.mwe-upwiz-file-status-line' )
 50+ _this.visibleFilenameDiv.find( '.mwe-upwiz-file-status-line' )
4951 .append( _this.$removeCtrl );
 52+
 53+ // Add show thumbnail control
5054
5155 //_this.errorDiv = $j('<div class="mwe-upwiz-upload-error mwe-upwiz-file-indicator" style="display: none;"></div>').get(0);
5256
@@ -69,7 +73,7 @@
7074 // XXX caution -- if the add file input changes size we won't match, unless we add some sort of event to catch this.
7175 _this.form = $j( '<form method="POST" encType="multipart/form-data" class="mwe-upwiz-form"></form>' )
7276 .attr( { action: _this.upload.api.url } )
73 - .append( visibleFilenameDiv )
 77+ .append( _this.visibleFilenameDiv )
7478 .append( _this.fileCtrlContainer
7579 .append( _this.$fileInputCtrl )
7680 )
@@ -116,10 +120,16 @@
117121 .unbind( 'mouseenter mouseover mouseleave mouseout' );
118122
119123 // remove delete control
120 - $j( _this.div )
 124+ $j( _this.visibleFilenameDiv )
121125 .find( '.mwe-upwiz-remove-ctrl' )
122126 .unbind( 'mouseenter mouseover mouseleave mouseout' )
123127 .remove();
 128+
 129+ // remove thumb control
 130+ $j( _this.visibleFilenameDiv )
 131+ .find( '.mwe-upwiz-show-thumb-ctrl' )
 132+ .unbind( 'mouseenter mouseover mouseleave mouseout' )
 133+ .remove();
124134 },
125135
126136 /**
@@ -204,6 +214,10 @@
205215 showStashed: function() {
206216 this.$removeCtrl.detach();
207217 this.$fileInputCtrl.detach();
 218+ if( this.$showThumbCtrl ) {
 219+ this.$showThumbCtrl.detach();
 220+ }
 221+
208222 this.showIndicator( 'stashed' );
209223 this.setStatus( 'mwe-upwiz-stashed-upload' ); // this is just "OK", say something more.
210224 },
@@ -260,6 +274,7 @@
261275 } );
262276 }
263277 }
 278+
264279 return files;
265280 },
266281
@@ -296,6 +311,35 @@
297312 this.clearStatus();
298313 this.setStatusString( statusItems.join( ' \u00b7 ' ) );
299314
 315+ // Only do this for images. Other things get no thumbnail.
 316+ // TODO: a more complete check for thumbnail-ability might be needed here.
 317+ if ( this.upload.imageinfo && this.upload.imageinfo.width && this.upload.imageinfo.height ) {
 318+ if( this.upload.wizard.makePreviewsFlag ) {
 319+ // make the preview now.
 320+ this.makePreview();
 321+ } else {
 322+ // add a control for showing the preview if the user needs it
 323+ this.$showThumbCtrl = $j.fn.showThumbCtrl(
 324+ 'mwe-upwiz-show-thumb',
 325+ 'mwe-upwiz-show-thumb-tip',
 326+ function() { _this.makePreview(); }
 327+ ).addClass( "mwe-upwiz-file-status-line-item" );
 328+
 329+ this.visibleFilenameDiv.find( '.mwe-upwiz-file-status-line' )
 330+ .append( '<br/>' ).append( _this.$showThumbCtrl );
 331+
 332+ }
 333+ }
 334+ },
 335+
 336+ makePreview: function() {
 337+ var _this = this;
 338+
 339+ // don't run this repeatedly.
 340+ if( _this.previewLoaded ) {
 341+ return;
 342+ }
 343+
300344 // do preview if we can
301345 if ( mw.fileApi.isAvailable() && _this.upload.file && mw.fileApi.isPreviewableFile( _this.upload.file ) ) {
302346 var dataUrlReader = new FileReader();
@@ -303,13 +347,16 @@
304348 var image = document.createElement( 'img' );
305349 image.onload = function() {
306350 $.publishReady( 'thumbnails.' + _this.upload.index, image );
 351+ _this.previewLoaded = true;
307352 };
 353+
 354+ // this step (inserting image-as-dataurl into image object) is slow for large images, which
 355+ // is why this is optional and has a control attached to it to load the preview.
308356 image.src = dataUrlReader.result;
309357 _this.upload.thumbnails['*'] = image;
310358 };
311359 dataUrlReader.readAsDataURL( _this.upload.file );
312 - }
313 -
 360+ }
314361 },
315362
316363 fileChangedError: function( code, info ) {
Index: trunk/extensions/UploadWizard/resources/uploadWizard.css
@@ -106,8 +106,8 @@
107107 background: #f5f5f5;
108108 }
109109
110 -.mwe-upwiz-visible-file .mwe-upwiz-remove-ctrl {
111 - visibility: hidden;
 110+.mwe-upwiz-visible-file .mwe-upwiz-remove-ctrl .mwe-upwiz-show-thumb-ctrl {
 111+ visibility: visible;
112112 }
113113
114114 .mwe-upwiz-file-indicator, .mwe-upwiz-count {
@@ -207,7 +207,7 @@
208208 background: #e0f0ff !important;
209209 }
210210
211 -.mwe-upwiz-file.hover .mwe-upwiz-remove-ctrl {
 211+.mwe-upwiz-file.hover .mwe-upwiz-remove-ctrl .mwe-upwiz-show-thumb-ctrl {
212212 visibility: visible;
213213 }
214214
Index: trunk/extensions/UploadWizard/resources/mw.UploadWizard.js
@@ -17,6 +17,8 @@
1818 this.maxUploads = mw.UploadWizard.config[ 'maxUploads' ] || 10;
1919 this.maxSimultaneousConnections = mw.UploadWizard.config[ 'maxSimultaneousConnections' ] || 2;
2020
 21+ this.makePreviewsFlag = true;
 22+
2123 };
2224
2325 mw.UploadWizard.DEBUG = true;
Index: trunk/extensions/UploadWizard/resources/mw.FormDataTransport.js
@@ -58,6 +58,7 @@
5959 });
6060 formData.append('filename', file.name);
6161 formData.append('file', file);
 62+ formData.append( 'ignorewarnings', true );
6263
6364 this.xhr.open("POST", _this.postUrl, true);
6465 this.xhr.send(formData);
@@ -99,9 +100,11 @@
100101 //failed to upload, try again in 3 second
101102 _this.retries++;
102103 if (_this.maxRetries > 0 && _this.retries >= _this.maxRetries) {
 104+ mw.log( 'max retries exceeded on unknown response' );
103105 //upload failed, raise response
104106 _this.transportedCb(response);
105107 } else {
 108+ mw.log( 'retry #' + _this.retries + ' on unknown response' );
106109 setTimeout(function() {
107110 _this.uploadChunk(offset);
108111 }, 3000);
@@ -113,8 +116,10 @@
114117 //failed to upload, try again in 3 second
115118 _this.retries++;
116119 if (_this.maxRetries > 0 && _this.retries >= _this.maxRetries) {
 120+ mw.log( 'max retries exceeded on error event' );
117121 _this.parseResponse(evt, _this.transportedCb);
118122 } else {
 123+ mw.log( 'retry #' + _this.retries + ' on error event' );
119124 setTimeout(function() {
120125 _this.uploadChunk(offset);
121126 }, 3000);
@@ -141,6 +146,7 @@
142147 });
143148 formData.append('offset', offset);
144149 formData.append('filename', file.name);
 150+ formData.append( 'ignorewarnings', true );
145151
146152 if (_this.filekey) {
147153 formData.append('filekey', _this.filekey);
Index: trunk/extensions/UploadWizard/resources/mw.UploadWizardUpload.js
@@ -41,6 +41,7 @@
4242 this.filename = undefined;
4343 this.providedFile = providedFile;
4444 this.file = undefined;
 45+ this.fileCount = undefined;
4546
4647 this.fileKey = undefined;
4748
@@ -291,7 +292,10 @@
292293 // TODO sanitize filename
293294 var basename = mw.UploadWizardUtil.getBasename( filename );
294295
295 -
 296+ if( files.length > 1 ) {
 297+ this.wizard.makePreviewsFlag = false;
 298+ }
 299+
296300 // check to see if the file has already been selected for upload.
297301 var duplicate = false;
298302 $j.each( this.wizard.uploads, function ( i, upload ) {
Index: trunk/extensions/UploadWizard/resources/mw.UploadWizardDetails.js
@@ -236,7 +236,6 @@
237237 _this.submittingDiv.find( '.mwe-upwiz-file-status-line' )
238238 .append( _this.$removeCtrl );
239239
240 -
241240 $j( _this.dataDiv ).append(
242241 _this.$form,
243242 _this.submittingDiv

Follow-up revisions

RevisionCommit summaryAuthorDate
r101105somehow missed the hooks in r100925...raindrift00:57, 28 October 2011
r101171We don't use title case (thanks Siebrand!). Followup to r100925raindrift19:01, 28 October 2011
r101425removed unused variable, added informative comments, changed thumbnail icon....raindrift23:00, 31 October 2011

Comments

#Comment by NeilK (talk | contribs)   21:37, 31 October 2011

UploadWizardUpload.js -- fileCount is unused, I presume you realized you could use files.length

ignorewarnings set to true, why? Was this for debugging?

I think I need a walkthrough here, how this all interacts with setThumbnail().

#Comment by NeilK (talk | contribs)   21:55, 31 October 2011

Ian explains (in person) - fileCount, whoops! - ignorewarnings must be on for chunks, and we will get the appropriate warnings when recombining chunks into a file

#Comment by Brion VIBBER (talk | contribs)   21:40, 31 October 2011

Before you get too far into refactoring this, I also highly recommend merging common code with core's mediawiki.special.upload.js -- that code has for instance been updated to skip the base-64 data: URI step on browsers supporting window.createObjectURL() (which seems to be a bit faster, and also works around a hanging bug in Firefox 7 with some SVG files).

#Comment by NeilK (talk | contribs)   22:13, 31 October 2011

TODO - css change for "add preview" icon.

#Comment by Raindrift (talk | contribs)   23:05, 31 October 2011

Neil, fixed all this stuff, r101425

Brion, I like the idea of merging these two, but mediawiki.special.upload.js seems to be pretty specific to Special:Upload. It might be possible to use parts of it, though, so if any more major refactoring is slated to happen here I'll see about consolidating at the same time. The createObjectURL() support would be really nice... I'd been avoiding it since browser support is still pretty sparse, but it's the way I imagine this working in the future anyway.

#Comment by Brion VIBBER (talk | contribs)   23:08, 31 October 2011

Yeah, the thumbnailing code should get hoisted out and used as a common module that's independent.

(If you feel like really going whole-hog on it, another change should be made to discard the object URLs after they've been used to draw onto the scaled canvas. They auto-dispose when the page closes, but in UploadWizard we're potentially working with more files over some time, all in the same page.)

Status & tagging log