Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiline Search Support: line breaks \n #5675

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

sekedus
Copy link

@sekedus sekedus commented Nov 15, 2024

Issue #2869

Description of changes:

  • highlight line break \n
  • search line break \n
  • replace line break \n
  • replace {search} value with \n or \t

Benefit

Users can now perform search/replace line breaks (\n) with regex.

Test

  1. Open: https://sekedus.github.io/ace/kitchen-sink.html
  2. Settings: Document: JavaScript
  3. Set value:
/**
 * 
 * @param {string[]} items
 * @param nada
 */
function foo(items, nada) {
    for (var i=0; i<items.length; i++) {
        alert(items[i] + "juhu\n");
    }	/* Real Tab */




}


// test search/replace line break with regexp

  			
  1. Open search box

    • Fill in search field with \n or \n+ or \n\s+ or \)\s\{\n\s+
    • Enable RegExp mode

    screenshot-ace-searchbox-2024_11_15-20_18_39

  2. Example:

    • Replace two or more line breaks with one line break:
      • \n{2} with \n
      • \n+ with \n
    • Remove line breaks, replace \n with {empty}
    • Replace {any_string} with \n or\t

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Pull Request Checklist:

* highlight line break
* search line break
* replace line break
@sekedus sekedus marked this pull request as draft November 15, 2024 22:58
@sekedus sekedus marked this pull request as ready for review November 16, 2024 01:53
@sekedus sekedus marked this pull request as draft November 16, 2024 13:29
Replace `.ace_search_form input` value with `\n` and `\t` using regex.
@whazor
Copy link
Contributor

whazor commented Nov 18, 2024

Does this also resolve #5674?

@sekedus
Copy link
Author

sekedus commented Nov 18, 2024

Does this also resolve #5674?


Yes.

screenshot-ace-line-breaks-2024_11_18-18_36_36

@sekedus sekedus marked this pull request as draft November 18, 2024 11:47
- Auto updates "updateCounter" and "ace_nomatch" in the search box when "onDocumentChange"
- If regex mode is enabled, fill the "searchInput" field with "escapeRegExp"
@sekedus sekedus marked this pull request as ready for review November 18, 2024 22:20
src/search_highlight.js Outdated Show resolved Hide resolved
src/ext/searchbox.js Show resolved Hide resolved
src/search.js Outdated
for (var matches, i = 0; i < lines.length; i++) {
if (this.$isMultilineSearch(options)) {
matches = this.$multiLineForward(session, re, i, lines.length);
ranges.push(new Range(matches.startRow, matches.startCol, matches.endRow, matches.endCol));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 as the previous case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

src/search.js Outdated
@@ -166,11 +240,15 @@ class Search {
if (!re)
return;

if (this.$isMultilineSearch(options))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are used at 2 places should we create another method as this looks like the reusable code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

src/search.js Outdated Show resolved Hide resolved
src/search.js Outdated
var column = matches[i - 1];
var length = matches[i];
if (callback(row, column, row, column + length))
if (multiline) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we reset regex lastIndex above this line just to reset it?
re.lastIndex = 0;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wasn’t focused earlier.

No need, because it’s already set in multiLineBackwardMatch.

src/search.js Outdated Show resolved Hide resolved
src/search.js Show resolved Hide resolved
@nlujjawal
Copy link
Contributor

Hey @sekedus
Do you have time for writing test cases for the changes as well?

@sekedus
Copy link
Author

sekedus commented Nov 22, 2024

Hey @sekedus Do you have time for writing test cases for the changes as well?

Done.

https://sekedus.github.io/ace/src/test/tests.html?ace/search_test

@sekedus sekedus force-pushed the master branch 2 times, most recently from 3a15da5 to cfb17ca Compare November 24, 2024 06:03
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 76.56250% with 60 lines in your changes missing coverage. Please review.

Project coverage is 86.95%. Comparing base (6dfb903) to head (cab512d).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
src/search.js 69.32% 50 Missing ⚠️
src/search_highlight.js 54.54% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5675      +/-   ##
==========================================
- Coverage   87.01%   86.95%   -0.07%     
==========================================
  Files         598      598              
  Lines       43680    43800     +120     
  Branches     7204     7207       +3     
==========================================
+ Hits        38009    38085      +76     
- Misses       5671     5715      +44     
Flag Coverage Δ
unittests 86.95% <76.56%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nlujjawal
Copy link
Contributor

@sekedus some test case requirement is failing will you be able to resolve this?

@sekedus
Copy link
Author

sekedus commented Nov 27, 2024

@sekedus some test case requirement is failing will you be able to resolve this?

Sorry, I don't know where the error is and how to resolve it.

Any suggestions?

src/editor.js Outdated

// Updates "updateCounter" and "ace_nomatch" in the search box
if (this.searchBox && this.searchBox.active === true)
this.searchBox.find(false, false, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please move this to an input event handler in searchbox, something like editor.on('input', this.$onEditorInput) and remove the listener when deactivating. onDocumentChange can be called multiple times during one edit, and input event debounces that with a timeout.
Also in one of tests add a scenario of changing editor value, e.g.

editor.insert("searchedText") 
setTimeout(function() {
 // verify that search counter have been updated
}, 50)

to prevent coverlay complaining about untested lines

@nlujjawal
Copy link
Contributor

@sekedus The failure of checks is due to some lines of code not having test cases.
You can view in this link about those lines https://github.com/ajaxorg/ace/pull/5675/checks?check_run_id=33534340733.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants