r96944 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r96943‎ | r96944 | r96945 >
Date:08:53, 13 September 2011
Author:siebrand
Status:ok (Comments)
Tags:post1.18deploy 
Comment:
(bug 20919) Search & Replace: Change "Replace Next" functionality to "Replace" functionality. Patch by Amir E. Aharoni.

Submitter's comment: "Replace next" now replaces the currently selected text instead of finding the next occurrence and replacing it. To keep the location of the current occurrence, i added matchIndex to $(this).data.

I added some comments renamed a couple of variable for readability:
* s to textRemainder
* replace to actualReplacement

This is my first significant jQuery-style patch so it may have embarrassing mistakes - constructive criticism is welcome.
Modified paths:
  • /trunk/extensions/WikiEditor/modules/jquery.wikiEditor.dialogs.config.js (modified) (history)

Diff [purge]

Index: trunk/extensions/WikiEditor/modules/jquery.wikiEditor.dialogs.config.js
——@@ -963,24 +963,31 @@
964964 // TODO: Find a cleaner way to share this function
965965 $(this).data( 'replaceCallback', function( mode ) {
966966 $( '#wikieditor-toolbar-replace-nomatch, #wikieditor-toolbar-replace-success, #wikieditor-toolbar-replace-emptysearch, #wikieditor-toolbar-replace-invalidregex' ).hide();
 967+
 968+ // Search string cannot be empty
967969 var searchStr = $( '#wikieditor-toolbar-replace-search' ).val();
968970 if ( searchStr == '' ) {
969971 $( '#wikieditor-toolbar-replace-emptysearch' ).show();
970972 return;
971973 }
 974+
 975+ // Replace string can be empty
972976 var replaceStr = $( '#wikieditor-toolbar-replace-replace' ).val();
 977+
 978+ // Prepare the regular expression flags
973979 var flags = 'm';
974980 var matchCase = $( '#wikieditor-toolbar-replace-case' ).is( ':checked' );
975 - var isRegex = $( '#wikieditor-toolbar-replace-regex' ).is( ':checked' );
976981 if ( !matchCase ) {
977982 flags += 'i';
978983 }
 984+ var isRegex = $( '#wikieditor-toolbar-replace-regex' ).is( ':checked' );
 985+ if ( !isRegex ) {
 986+ searchStr = $.escapeRE( searchStr );
 987+ }
979988 if ( mode == 'replaceAll' ) {
980989 flags += 'g';
981990 }
982 - if ( !isRegex ) {
983 - searchStr = $.escapeRE( searchStr );
984 - }
 991+
985992 try {
986993 var regex = new RegExp( searchStr, flags );
987994 } catch( e ) {
——@@ -990,20 +997,26 @@
991998 .show();
992999 return;
9931000 }
 1001+
9941002 var $textarea = $(this).data( 'context' ).$textarea;
9951003 var text = $textarea.textSelection( 'getContents' );
9961004 var match = false;
997 - var offset, s;
 1005+ var offset, textRemainder;
9981006 if ( mode != 'replaceAll' ) {
999 - offset = $(this).data( 'offset' );
1000 - s = text.substr( offset );
1001 - match = s.match( regex );
 1007+ if (mode == 'replace') {
 1008+ offset = $(this).data( 'matchIndex' );
 1009+ } else {
 1010+ offset = $(this).data( 'offset' );
 1011+ }
 1012+ textRemainder = text.substr( offset );
 1013+ match = textRemainder.match( regex );
10021014 }
10031015 if ( !match ) {
10041016 // Search hit BOTTOM, continuing at TOP
 1017+ // TODO: Add a "Wrap around" option.
10051018 offset = 0;
1006 - s = text;
1007 - match = s.match( regex );
 1019+ textRemainder = text;
 1020+ match = textRemainder.match( regex );
10081021 }
10091022
10101023 if ( !match ) {
——@@ -1017,13 +1030,13 @@
10181031 // in Firefox/Webkit, but in IE replacing the entire content once is better.
10191032 var index;
10201033 for ( var i = 0; i < match.length; i++ ) {
1021 - index = s.indexOf( match[i] );
 1034+ index = textRemainder.indexOf( match[i] );
10221035 if ( index == -1 ) {
10231036 // This shouldn't happen
10241037 break;
10251038 }
1026 - var matchedText = s.substr( index, match[i].length );
1027 - s = s.substr( index + match[i].length );
 1039+ var matchedText = textRemainder.substr( index, match[i].length );
 1040+ textRemainder = textRemainder.substr( index + match[i].length );
10281041
10291042 var start = index + offset;
10301043 var end = start + match[i].length;
——@@ -1043,27 +1056,63 @@
10441057 .show();
10451058 $(this).data( 'offset', 0 );
10461059 } else {
1047 - // Make regex placeholder substitution ($1) work
1048 - var replace = isRegex ? match[0].replace( regex, replaceStr ): replaceStr;
1049 - var start = match.index + offset;
1050 - var end = start + match[0].length;
1051 - var newEnd = start + replace.length;
1052 - var context = $( this ).data( 'context' );
1053 - $textarea.textSelection( 'setSelection', { 'start': start,
1054 - 'end': end } );
 1060+ var start, end;
 1061+
10551062 if ( mode == 'replace' ) {
1056 - $textarea
1057 - .textSelection( 'encapsulateSelection', {
1058 - 'peri': replace,
1059 - 'replace': true } )
1060 - .textSelection( 'setSelection', {
1061 - 'start': start,
1062 - 'end': newEnd } );
 1063+ var actualReplacement;
 1064+
 1065+ if (isRegex) {
 1066+ // If backreferences (like $1) are used, the actual actual replacement string will be different
 1067+ actualReplacement = match[0].replace( regex, replaceStr );
 1068+ } else {
 1069+ actualReplacement = replaceStr;
 1070+ }
 1071+
 1072+ if (match) {
 1073+ // Do the replacement
 1074+ $textarea.textSelection( 'encapsulateSelection', {
 1075+ 'peri': actualReplacement,
 1076+ 'replace': true } );
 1077+ // Reload the text after replacement
 1078+ text = $textarea.textSelection( 'getContents' );
 1079+ }
 1080+
 1081+ // Find the next instance
 1082+ offset = offset + match[0].length + actualReplacement.length;
 1083+ textRemainder = text.substr( offset );
 1084+ match = textRemainder.match( regex );
 1085+
 1086+ if (match) {
 1087+ start = offset + match.index;
 1088+ end = start + match[0].length;
 1089+ } else {
 1090+ // If no new string was found, try searching from the beginning.
 1091+ // TODO: Add a "Wrap around" option.
 1092+ textRemainder = text;
 1093+ match = textRemainder.match( regex );
 1094+ if (match) {
 1095+ start = match.index;
 1096+ end = start + match[0].length;
 1097+ } else {
 1098+ // Give up
 1099+ start = 0;
 1100+ end = 0;
 1101+ }
 1102+ }
 1103+ } else {
 1104+ start = offset + match.index;
 1105+ end = start + match[0].length;
10631106 }
 1107+
 1108+ $( this ).data( 'matchIndex', start);
 1109+
 1110+ $textarea.textSelection( 'setSelection', {
 1111+ 'start': start,
 1112+ 'end': end
 1113+ } );
10641114 $textarea.textSelection( 'scrollToCaretPosition' );
1065 - $textarea.textSelection( 'setSelection', { 'start': start,
1066 - 'end': mode == 'replace' ? newEnd : end } );
1067 - $( this ).data( 'offset', mode == 'replace' ? newEnd : end );
 1115+ $( this ).data( 'offset', end );
 1116+ var context = $( this ).data( 'context' );
10681117 var textbox = typeof context.$iframe != 'undefined' ?
10691118 context.$iframe[0].contentWindow : $textarea[0];
10701119 textbox.focus();
——@@ -1092,6 +1141,8 @@
10931142 },
10941143 open: function() {
10951144 $(this).data( 'offset', 0 );
 1145+ $(this).data( 'matchIndex', 0 );
 1146+
10961147 $( '#wikieditor-toolbar-replace-search' ).focus();
10971148 $( '#wikieditor-toolbar-replace-nomatch, #wikieditor-toolbar-replace-success, #wikieditor-toolbar-replace-emptysearch, #wikieditor-toolbar-replace-invalidregex' ).hide();
10981149 if ( !( $(this).data( 'onetimeonlystuff' ) ) ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r101339Followup r96944: rename "Replace next" button to "Replace" cause that's what ...catrope08:38, 31 October 2011

Comments

#Comment by Amire80 (talk | contribs)   08:56, 13 September 2011

I already have a comment: the "Replace next" button should be greyed out until the first occurrence is found using "Find next". What's the right way to do it?

#Comment by Catrope (talk | contribs)   09:16, 13 September 2011 
var $replaceButton = $( 'get the replace next button somehow' );
$replaceButton.button( { disabled: true } );
#Comment by Catrope (talk | contribs)   17:46, 19 September 2011 
+						// TODO: Add a "Wrap around" option.

What does this mean? Wrapping around is what that code does, or did you mean to say there should be a checkbox that lets you disable the wrap-around behavior?

#Comment by Amire80 (talk | contribs)   17:55, 19 September 2011

An option to disable.

Status & tagging log