r86622 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r86621‎ | r86622 | r86623 >
Date:13:42, 21 April 2011
Author:catrope
Status:ok (Comments)
Tags:
Comment:
(bug 22209, bug 22574) Make indent, bullet list and numbered list buttons in the WikiEditor toolbar act on each line individually

* Reintroduce support for the splitlines argument in encapsulateSelection(). Was implemented by Adam Miller in r61493 but got lost somehow
* Add 'splitlines': true for the bullet list and numbered list buttons. The indent button already had it, so that part of r61493 did survive
* Copy options.pre and options.post to local pre and post variables instead of using (and modifying!) them directly all over the place
Modified paths:
  • /trunk/extensions/WikiEditor/modules/jquery.wikiEditor.toolbar.config.js (modified) (history)
  • /trunk/phase3/resources/jquery/jquery.textSelection.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/jquery.textSelection.js
——@@ -53,6 +53,8 @@
5454 */
5555 encapsulateSelection: function( options ) {
5656 return this.each( function() {
 57+ var pre = options.pre, post = options.post;
 58+
5759 /**
5860 * Check if the selected text is the same as the insert text
5961 */
——@@ -65,16 +67,34 @@
6668 } else {
6769 while ( selText.charAt( selText.length - 1 ) == ' ' ) {
6870 // Exclude ending space char
69 - selText = selText.substring(0, selText.length - 1);
70 - options.post += ' ';
 71+ selText = selText.substring( 0, selText.length - 1 );
 72+ post += ' ';
7173 }
7274 while ( selText.charAt( 0 ) == ' ' ) {
7375 // Exclude prepending space char
74 - selText = selText.substring(1, selText.length);
75 - options.pre = ' ' + options.pre;
 76+ selText = selText.substring( 1, selText.length );
 77+ pre = ' ' + pre;
7678 }
7779 }
7880 }
 81+
 82+ /**
 83+ * Do the splitlines stuff.
 84+ *
 85+ * Wrap each line of the selected text with pre and post
 86+ */
 87+ function doSplitLines( selText, pre, post ) {
 88+ var insertText = '';
 89+ var selTextArr = selText.split( '\n' );
 90+ for ( var i = 0; i < selTextArr.length; i++ ) {
 91+ insertText += pre + selTextArr[i] + post;
 92+ if ( i != selTextArr.length - 1 ) {
 93+ insertText += '\n';
 94+ }
 95+ }
 96+ return insertText;
 97+ }
 98+
7999 var isSample = false;
80100 if ( this.style.display == 'none' ) {
81101 // Do nothing
——@@ -86,29 +106,34 @@
87107 var endPos = this.selectionEnd;
88108 var scrollTop = this.scrollTop;
89109 checkSelectedText();
 110+
 111+ var insertText = pre + selText + post;
 112+ if ( options.splitlines ) {
 113+ insertText = doSplitLines( selText, pre, post );
 114+ }
90115 if ( options.ownline ) {
91116 if ( startPos != 0 && this.value.charAt( startPos - 1 ) != "\n" ) {
92 - options.pre = "\n" + options.pre;
 117+ insertText = "\n" + insertText;
93118 }
94119 if ( this.value.charAt( endPos ) != "\n" ) {
95 - options.post += "\n";
 120+ insertText += "\n";
96121 }
97122 }
98 - this.value = this.value.substring( 0, startPos ) + options.pre + selText + options.post +
 123+ this.value = this.value.substring( 0, startPos ) + insertText +
99124 this.value.substring( endPos, this.value.length );
100125 // Setting this.value scrolls the textarea to the top, restore the scroll position
101126 this.scrollTop = scrollTop;
102127 if ( window.opera ) {
103 - options.pre = options.pre.replace( /\r?\n/g, "\r\n" );
 128+ pre = pre.replace( /\r?\n/g, "\r\n" );
104129 selText = selText.replace( /\r?\n/g, "\r\n" );
105 - options.post = options.post.replace( /\r?\n/g, "\r\n" );
 130+ post = post.replace( /\r?\n/g, "\r\n" );
106131 }
107132 if ( isSample && options.selectPeri ) {
108 - this.selectionStart = startPos + options.pre.length;
109 - this.selectionEnd = startPos + options.pre.length + selText.length;
 133+ this.selectionStart = startPos + pre.length;
 134+ this.selectionEnd = startPos + pre.length + selText.length;
110135 } else {
111 - this.selectionStart = startPos + options.pre.length + selText.length +
112 - options.post.length;
 136+ this.selectionStart = startPos + pre.length + selText.length +
 137+ post.length;
113138 this.selectionEnd = this.selectionStart;
114139 }
115140 } else if ( document.selection && document.selection.createRange ) {
——@@ -120,33 +145,39 @@
121146 var selText = $(this).textSelection( 'getSelection' );
122147 var scrollTop = this.scrollTop;
123148 var range = document.selection.createRange();
 149+
 150+ checkSelectedText();
 151+ var insertText = pre + selText + post;
 152+ if ( options.splitlines ) {
 153+ insertText = doSplitLines( selText, pre, post );
 154+ }
124155 if ( options.ownline && range.moveStart ) {
125156 var range2 = document.selection.createRange();
126157 range2.collapse();
127158 range2.moveStart( 'character', -1 );
128159 // FIXME: Which check is correct?
129160 if ( range2.text != "\r" && range2.text != "\n" && range2.text != "" ) {
130 - options.pre = "\n" + options.pre;
 161+ insertText = "\n" + insertText;
131162 }
132163 var range3 = document.selection.createRange();
133164 range3.collapse( false );
134165 range3.moveEnd( 'character', 1 );
135166 if ( range3.text != "\r" && range3.text != "\n" && range3.text != "" ) {
136 - options.post += "\n";
 167+ insertText += "\n";
137168 }
138169 }
139 - checkSelectedText();
140 - range.text = options.pre + selText + options.post;
 170+
 171+ range.text = insertText;
141172 if ( isSample && options.selectPeri && range.moveStart ) {
142 - range.moveStart( 'character', - options.post.length - selText.length );
143 - range.moveEnd( 'character', - options.post.length );
 173+ range.moveStart( 'character', - post.length - selText.length );
 174+ range.moveEnd( 'character', - post.length );
144175 }
145176 range.select();
146177 // Restore the scroll position
147178 this.scrollTop = scrollTop;
148179 }
149180 $(this).trigger( 'encapsulateSelection', [ options.pre, options.peri, options.post, options.ownline,
150 - options.replace ] );
 181+ options.replace, options.spitlines ] );
151182 });
152183 },
153184 /**
——@@ -382,7 +413,8 @@
383414 'post': '', // Text to insert after the cursor/selection
384415 'ownline': false, // Put the inserted text on a line of its own
385416 'replace': false, // If there is a selection, replace it with peri instead of leaving it alone
386 - 'selectPeri': true // Select the peri text if it was inserted (but not if there was a selection and replace==false)
 417+ 'selectPeri': true, // Select the peri text if it was inserted (but not if there was a selection and replace==false)
 418+ 'splitlines': true // If multiple lines are selected, encapsulate each line individually
387419 }, options );
388420 break;
389421 case 'getCaretPosition':
Index: trunk/extensions/WikiEditor/modules/jquery.wikiEditor.toolbar.config.js
——@@ -270,7 +270,8 @@
271271 'pre': "* ",
272272 'periMsg': 'wikieditor-toolbar-tool-ulist-example',
273273 'post': "",
274 - 'ownline': true
 274+ 'ownline': true,
 275+ 'splitlines': true
275276 }
276277 }
277278 },
——@@ -285,7 +286,8 @@
286287 'pre': "# ",
287288 'periMsg': 'wikieditor-toolbar-tool-olist-example',
288289 'post': "",
289 - 'ownline': true
 290+ 'ownline': true,
 291+ 'splitlines': true
290292 }
291293 }
292294 },

Follow-up revisions

RevisionCommit summaryAuthorDate
r89402(bug 29104) Fix r86622: make splitlines default to false, makes much more sensecatrope09:56, 3 June 2011
r92923Followup r86622: add initial QUnit test cases for jquery.textSelection module....brion00:44, 23 July 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r61493Adding a split lines option for applying toolbar commands to each line separe...adam17:05, 25 January 2010

Comments

#Comment by Brion VIBBER (talk | contribs)   00:46, 15 June 2011

This really needs some qunit test cases; also need to confirm that behavior for both the core jquery.textSelection (operating on a textarea) and the jquery.wikiEditor.iframe implementation thereof.

#Comment by RobLa-WMF (talk | contribs)   01:13, 22 July 2011

Roan and I discussed this one. Can we make this a "todo" instead of insisting on a test here? After we get caught up, we should raise the bar and this would be a "fixme", but the problem with blocking release on this is that the backlog will continue to build.

#Comment by Brion VIBBER (talk | contribs)   16:57, 22 July 2011

What's the hurry? Release is weeks or months away in either case, and writing a test will give that release higher confidence.

If you can't test it, you can't review it. Is there something in particilar that makes these tests impossible to write?

#Comment by RobLa-WMF (talk | contribs)   18:59, 22 July 2011

The hurry is that we have 673 new and fixme right in the 1.18 branch, and even once we get to zero on that branch, we'll potentially have a large backlog that will have built up since our July 18 branch date. Once we are down to zero and have more frequent releases, then being more stringent about retroactively writing tests will potentially speed us up, since reverting will be easier.

If reverting this revision is fairly simple (relative to writing a test), and if you (Brion) prefer to revert than to make this change, I'd be happy with that resolution. If, however, the only reasonable way forward is writing a test, and we take Roan at his word that he's overloaded (which is pretty obvious from observation), then I'd rather let the test slide than heap on more.

Re: "release is weeks or months away in either case". That's how it becomes "months".

#Comment by Brion VIBBER (talk | contribs)   23:19, 22 July 2011

The simplest thing to do is to write the tests so we can resolve the revision with some measure of confidence. I'll do it, I guess....

#Comment by Brion VIBBER (talk | contribs)   00:45, 23 July 2011

Test cases added in r92923; splitlines looks ok for the bullet lists.

There is one basic failure in IE 6, which may need to be investigated further. No obvious problem with the actual selection, so it might be a problem with the getSelection func. (Not related to this commit, which updated the splitlines support.)

Status & tagging log