search

adobe / brackets

An open source code editor for the web, written in JavaScript, HTML and CSS.
http://brackets.io
MIT License
33.26k stars 7.63k forks source link

Incorrect updating of Find in Files Panel after Move Line Down #8086

Open redmunds opened 10 years ago

redmunds commented 10 years ago

I noticed this in master. I have not yet tried in @njx's refactoring of this panel.

Recipe:

  1. Do Find in Files of a known string
  2. Click on any item in result panel
  3. Ctrl-Shift-down to move line down

Results: Find in Files panel now shows and extra entry - shows new line but still has old line

Expected: Panel is updated properly. Works correctly for Move Line Up and all other edits I tried.

Workaround: Save document updates the Find in Files panel correctly

njx commented 10 years ago

The behavior is likely to be the same in my branch - I haven't changed the logic around how the panel updates.

redmunds commented 10 years ago

This happens in Sprint 35, so not a recent injection. Still broken in @njx's refactoring, replace across files branch.

njx commented 10 years ago

It's possible that a bug fix in #8137 might fix this but I haven't tested it yet.

redmunds commented 10 years ago

@njx I just checked -- it is not fixed in #8137.

peterflynn commented 10 years ago

I think I figured out the problem here, while code reviewing PR #8137...

Longer comment here. The short version is that FindInFiles._updateResults() doesn't properly handle multi-part change lists, like the ones generated by Move Line. When processing each change, we want to look at the state of the text immediately after that change, but instead it always looks at the state of the text after all changes in the list.

peterflynn commented 10 years ago

Unfortunately though, I'm realizing that it's not as simple to fix as I suggested in that earlier PR comment... My suggested fix gives us access to only the text in the change record, but that text might start or end in the middle of a match -- so when we rerun the search on that text, we could miss some matches.

Currently, the update code basically expands to full lines and re-runs the search on those whole lines, guaranteeing it never splits in the middle of a match. Of course, that only works because matching across newlines isn't implemented yet -- after that, it will be broken too. So we'll need to find a different solution either way.

One option would be to forgo all the fancy incremental updating logic, lengthen the debounce timeout a bit, and just re-search the entire file instead. We do that on every keystroke in the Find bar, after all, and it's plenty fast there...

njx commented 10 years ago

Yup, I think I'd be in favor of that too - it's less fragile/error-prone. The only question is whether doing the DOM update for the panel is noticeably longer than the highlight update in Find, but it seems unlikely. @TomMalbran what do you think?

(That said, I think we should land the replace-across-files branch first with the existing bugs anyway.)

TomMalbran commented 10 years ago

When I was implementing the update code, I initially started with just researching the entire file. But it made writing really laggy on a file like CodeMirror. But when I tested it we didn't had cached files, so right now it can be a lot faster. The lag was in the search and not in updating the DOM, since right now we are already recreating the table on every change. (It will be nice to implement ReactJS here).

I am ok with the change, if it is not laggy and updates fast enough (not too much Debounce). Currently I use this feature a lot when refactoring and I sometimes notice the debounce. Replace in File will be great, but there are some things that can't be done with it.