Added 'All' button to FindReplace functionality.

[zaggino]

I've been missing 'All' button when replacing too many occurrences of a string in file. I can add tests if this gets considered for merge.

[TomMalbran]

Nice feature!

I was thinking that after doing a replace All, we could show what was replaced, in similar format to find in files. So you could go through every replace and check if the result is what you expected and easily undo a replace if it wasn't.

Any thoughts?

[zaggino]

Yes, it could certainly be useful to see what was replaced. This was just a quick idea because I was stuck with pressing 'Yes' for 50 times in one file :)

Are you thinking of doing it for current file, or global style command "Replace in Files" (Ctrl + Shift + H) ?

[TomMalbran]

Hehe. I did that before, and even worse I ended clicking so fast that clicked in between the closing and opening of the dialog, making it disappear...

It is a nice feature to add, and we really need it. But I think it should be well implemented too.

I was actually thinking it just for the current file, since we don't have a Replace in Files yet. Anyway, it should be useful for both commands.

[zaggino]

Ok, I'll give it a try in spare time, maybe I'll come up with something usable.

[TomMalbran]

Or wait to see what others have to say about it. Maybe we don't need it yet or don't need it for the current Replace command. But if you still want to try it, go ahead.

I was thinking of saving for each replacement, the line, the string, and the column position and length of the new string. Then use this information to fill a bottom panel. Similar to how Find in Files works.

[zaggino]

@TomMalbran can you look at the last commit ? I can't figure out why AppInit.htmlReady gets launched when no #status-bar element is available, which causes PanelManager.createBottomPanel to attach panel to nowhere.

There is no check in PanelManager.createBottomPanel to see if #status-bar exists.

    function createBottomPanel(id, $panel, minSize) {
        $panel.insertBefore("#status-bar");
        $panel.hide();
        updateResizeLimits();  // initialize panel's max size

        return new Panel($panel, minSize);
    }

[TomMalbran]

The status bar is also created on htmlReady. Which means that the html ready for this file could be called before the html ready of the Status bar code.

To solve this, the status bar div (empty) should be included in the main view template, and when creating the status bar, it should just include the content inside the status bar div.

[julianasuh]

Hi @TomMalbran, do you want to review this pull request?

[julianasuh]

@larz0 to review UI

[TomMalbran]

@julianasuh Sure. I can.

[larz0]

@zaggino UI looks good but nothing happens when I click "All". Not sure if it's just me.

[zaggino]

@larz0 I haven't fully submitted it yet, working on it right now. I'll let you know today when I have first reviewable version.

[larz0]

@zaggino ahh cool. Thanks for this BTW.

[zaggino]

Ok, first go at UI, right now it looks like this, probably you can get an idea where it is heading from it.

[zaggino]

Check All - None should probably be replaced by checkbox left of "Search for ..."

[TomMalbran]

Wow nice. I was thinking of something like replace all, and then just show the lines that changed with the the replaced word highlighted. Anyway I like this approach.

[zaggino]

Thanks, lets see if anyone else would like to comment before I go further, I have some other stuff to do and then will take a look at pull-4661 in the meantime. @larz0 what do you think about this?

[larz0]

Damn that's really nice! Can't wait!

[zaggino]

@TomMalbran do you have any idea, why would brackets freeze after calling document.replaceRange ? All my instances will just freeze after this line and there's no breakpoint in developer tools. I have to close brackets with task manager - end process in windows.

[TomMalbran]

@zaggino The line and ch values from the cursor positions need to be numbers, and you are passing strings. It also would be simpler to store the selections (start and end cursor positions) in an array and just store the index in the array in the data values in the elements.

After this change, it works nice. There are other issues, like replacing twice (not sure what to do here, since it might be nice to undo and try to replace again, but without undoing replacing again wont give good results), but the replace works.

[zaggino]

@TomMalbran thanks for that, setSelection was working even with strings and since everything just froze without and error I wasn't able to figure it out ... anyway I was planning to close the panel after replacing selected matches so replacing twice would not be possible. Do you think the "replace twice" feature would be somehow useful ?

There are few other issues for me to solve like switching between files and modifying current file while the panel is opened.

[TomMalbran]

No problem. I saw that, not sure why select did worked, maybe select doesn't care if the param is a string.

Actually, closing the panel after replacing sounds good. You can easily undo and use the same replace to get the panel back and try again.

For switching panels, you can just add a listener for the document change, and close the panel it when it does change. You should also close it when switching projects. I think that you might not get a current document change event if when opening a new project no document opens, but check if you need both.

Changing the results while editing is hard. I submitted a pull request to do this for the find in files. You can check my code in PR #4729. But if this ends up being too hard, maybe you could go with an easier solution of replacing all first and then showing what was replaced with no option to replace again, so you don't really need to update the results.

[zaggino]

I was thinking more of just closing the panel when user starts typing into the document, so he won't get unexpected outcome since results will be outdated after modifying part of the file before matches.

As I understand your code, you refresh results after every key stroke? Doesn't that affect performance when user starts typing rapidly?

[TomMalbran]

Closing the panel could be an option for now. Another option can be to remove the replace button and replace with something like search, so that it updates the panel. We can improve it later with an auto-update if required.

Is not really every keystroke, is every change. CodeMirror joins close updates and sends them as a change. Is like if you type really fast and then undo, you might undo several keystrokes. I haven't tested how fast it is, but is not slow.

[zaggino]

I've done some thinking and refactoring on this, now it refreshes when user modifies current document - you were right, it's not slow at all.

I'll have to test everything and read code again so there are no loose ends but I think it's nearing it's final version.

[TomMalbran]

It looks like you are searching the entire document on every change? I tried that first, but that is actually pretty slow on really big files like CodeMirror. What I did instead is search only over the new lines, remove the the results on the removed lines, and modify the rest of the line numbers. But that is harder to have it done right. So if we want to do that here, we could try to do it using one code only, later when my PR gets accepted.

[zaggino]

Ok @TomMalbran , I've removed automatic refresh on every change for now and replaced it with search button that will appear after user modifes a document when the panel is open. I should probably add some tests now, for me the feature works pretty well right now.

[TomMalbran]

I wasn't able to actually test the re-search the entire file before. Was it actually slow on a file like codemirror.js? I might test it later. Anyway I don't think that researching the entire file is a good idea.

If you are done with the code. I will give it a full review and test it more.

@larz0 I have a few questions about the design/usability:

• Should the buttons on the replace panel have the same style as the buttons on the modal bar? They need some style.
• We haven't used any checkboxes in Brackets yet, so we will need a new style for them here. You can style them after this is merged too, if we will style the checkboxes.
• Is it ok to replace the "Replace" button with a "Search" button when the content of the file changed, or should we just close it. We can later fix this with a search over the new content and update the panel, but I want to have PR Find in Files Improvements (Part 3: Auto-update) #4729 merged first and move the core of that code to an utils files to use here.

[larz0]

@TomMalbran

• Yes you're right, they need to have the same style. I can do it after the merge if that's easier.
• I can style the checkboxes once this is merged.
• We should just close it for now instead of replacing it with the "Search" button.

[TomMalbran]

Done with second review.

[zaggino]

Fixed according to second review.

[TomMalbran]

This feature is working great and the code looks nice, just 2 more thing before merging:

• I just run the FindReplace tests and after the last one, the replace bar isn't dismissed, you will have to trigger a click on the stop button.
• I am thinking that we would need to add some tests for Replace All, or can be added on another PR, not sure about it, but there is time till the end of Sprint 30.

[zaggino]

@TomMalbran I'll take a look at it little bit later and set-up a test for this.

[TomMalbran]

I am not sure if we need the tests done in this PR, since replace doesn't had any test either. But if you can still make them, that would be great.

[zaggino]

@TomMalbran - I fixed that test issue with stop button and merged with current master. I can take a look at the tests later but not right now, so feel free to merge so @larz0 can start working on styling the UI.

[TomMalbran]

Sure. If you don't have time to work on the tests right now, we can merge this and complete them later.

Just 2 last things. There was a merge issue after your other fix was merged, could you fix the conflict here. And second, could you rebase this PR, since it has lots of commits and we try to keep the git source small? Thanks

[zaggino]

Did the rebase @TomMalbran , you can squash commits now?

[zaggino]

Nevermind squash, I studied a thing or two and did that myself, so now it's all in one commit ;-)

[TomMalbran]

Nice job. I was about to merge it, but I went and run the test first and unfortunately your test is failing now because of the modal bar transition, which was merged today and also broke all the other find tests. Can you fix that fast and then I merge?

[zaggino]

Ok @TomMalbran , merged master, fixed testcase and squashed again into one commit. That test is now passing.

[TomMalbran]

Awesome. Merging :)

[TomMalbran]

@larz0 You can now change the style of the replace all panel :)

[larz0]

@TomMalbran excellent ☑️

[larz0]

@TomMalbran @zaggino FYI I did a find on "<p>", replacing with "<p1>" and it did this:

[TomMalbran]

Woops, nice catch. Is not doing a html escape on the replace strings. Is easy to fix thought. @zaggino Want to post a fix? Or should I?

[zaggino]

@TomMalbran can you actually add commits to already merged branch to be merged again? This only came to my mind after creating a new PR :/

[TomMalbran]

No. Is better to have new branches for fixes after a merged branch. I guess you can probably add commits to your branch, but you will need to create a new PR. Anyway use new branches for the fixes after the branch was merged.

[njx]

Wow, this is really cool! Wasn't expecting to get a preview panel to let you tweak what gets replaced. Nice idea.