[replace-across-files] Code review fixes by njx · Pull Request #8137 · adobe/brackets

njx · 2014-06-17T01:09:45Z

This addresses most of the code review items from #7705 and #7809. Note that this is a pull into nj/replace-across-files - after this is merged, we'll need to put up another PR to actually merge it into master.

All the small cleanups are in c09eaef and 0f3cb18. In addition, I did two bigger cleanups:

Moved more of the keyboard handling controller logic from FindInFilesUI into FindBar, making the events dispatched from FindBar much more clear.
Made it so modifications to SearchModel results are done via APIs on SearchModel rather than manipulating searchModel.results directly, so that it can keep an accurate count of the number of matches without having to re-count every time results are added.

As part of the latter work I added a number of unit tests for the updating code (there was only one before), and in the process I found and fixed a bug in the line number update calculation (which has probably always been there).

There are a few code review issues that I didn't address, but I think we can land the feature without them:

Clean up showError()/showNoResults() (combine them and/or make showNoResults() less confusing)
Remove error-showing functionality from FindReplace.parseQuery()
Don't pass replaceText to doSearchInScope() - have caller store it separately
Move Replace tests to separate file
Break out event handlers in SearchResultsView.addPanelListeners() (see Tom's comment in [replace-across-files] Replace in Files #7809)
Move FindUtils.performReplacements() back into FindInFiles.doReplace()

I'll file a bug for these - I don't think they should impede shipping the feature this release.

…oResults() and parseQuery() cleanup

* Got rid of unnecessary partial in search-summary-paging.html * Refactored change notifications in Document to _notifyDocumentChange() method, and fix up similar logic in SpecRunnerUtils.createMockActiveDocument() * Moved makeDialogFileList from StringUtils to FileUtils * Renamed "file/item/index" to "fileIndex/itemIndex/matchIndex" for clarity in SearchResultsView * Return empty array from _getSearchMatches instead of null * Properly debounce document changes when updating search results view * Fix up file rename handling in search results view to use indexOf() instead of match() * Rename "doReplaceAll" event on FindBar to "replaceAll" * Return a promise from FindInFilesUI.searchAndShowResults() * Replace calls to _.filter() and _.map() with native JS calls * Lots of comment/JSDoc improvements

…nto FindBar

… every time Also fixes a long-standing bug in updating line numbers for existing result matches.

njx · 2014-06-17T01:19:47Z

/cc @peterflynn @TomMalbran

The last two commits are reasonably semantic, so it might be worth reviewing them separately.

njx · 2014-06-17T01:23:50Z

(Also, I added some comments in the last commit, not in the main PR diff.)

TomMalbran · 2014-06-17T04:46:11Z

The new update code looks nice after a fast check. But it doesn't look like it is setting or keeping a good state of the collapsed property. I am thinking that it might be easier to have a second Object.<path, collapsed> where we keep the collapsed state since we don't need to change it when the results change and we would probably don't even need to remove them. We might only need to update it when the path changes.

njx · 2014-06-17T04:51:27Z

Makes sense...in fact, the collapsed state really shouldn't be in the model anyway, since it's a transient view property. I don't think this refactoring introduces any bugs that weren't already there regarding the collapsed state though (since it's still reusing the original info, including the collapsed state, in the same cases it was before, I think), so I'd be inclined to do that fix separately later.

TomMalbran · 2014-06-17T05:38:50Z

The only new issue I can see is that the collapsed property is never initialized, since it was initialized when adding the results matches. That should result in a bug when trying to access it.

njx · 2014-06-17T10:26:56Z

It should just evaluate to undefined, which is falsy, so there shouldn't be a problem. I just realized there aren't any tests for the collapse behavior and I didn't manually test it, so I'll double check that, but the panel did come up with no issues when I tested it (and with the collapsed value undefined)

redmunds · 2014-06-17T15:04:38Z

@njx I'm seeing 9 FileFilters unit tests fail in this branch.

njx · 2014-06-17T16:26:41Z

Ah, I forgot to run those. I'll run all the tests and see if there are other issues.

njx · 2014-06-17T16:38:41Z

Looks like those might have been tests that were added in master that I got in my last merge. Will fix.

…Clean up duplicate code.

njx · 2014-06-17T18:39:42Z

Unit tests fixed in 2b65a5c. There were some unit test helper functions that got cut-and-pasted, and one copy went stale - refactored it to share those functions as much as possible. (Both pieces of that suite still seem to need separate test windows since they make assumptions about the initial state.)

njx · 2014-06-17T18:41:35Z

@peterflynn Should be ready for review. Note that 2b65a5c is easiest to review with ?w=1.

peterflynn · 2014-06-18T00:10:20Z

I wonder if we should have this auto-remove the last results for fullpath instead of expecting the client to remember to do it every time. It's guaranteed that we'll totally overwrite anything that was previously at this.results[fullpath], so it seems safe to deduct it from the count automatically. And in fact, maybe the API should be called something like setResultsForFile(), since it's never additive when results for the file were in the model previously.

If we don't make that suggested change, we should probably at least clarify the docs, which currently imply calling this is always purely additive.

You're right - that would be clearer (and is harmless in the case where it's already been removed by a previous call to removeResults()). Fixed, and also removed the return value since no one was using it and it wasn't semantically clear anyway.

peterflynn · 2014-06-18T03:31:43Z

I think this actually breaks the intended functionality of createMockActiveDocument() -- per the comment above in the function body, the Document isn't supposed to get added to the working set. Once we start dispatching the pub-sub version of the event, DocumentManager will try to add it to the working set. I'm actually a little surprised this doesn't break stuff, since there's no working set in the test runner window (jQuery no-opping to the rescue, I guess?). But it seems like we should either change it back or add a flag arg to the pub-sub event to suppress adding to the working set...

Ah, I missed that - just thought it was out of sync. I'll revert it and fix up the comment.

Actually, looking at it closely, it doesn't look to me like the Document module's change event is what causes documents to get added to the working set - it's the _dirtyFlagChange event.

(So I'm not going to change this back - please confirm that's ok :))

Good point -- agreed. I wonder if we should augment the comment to point out that's the important part to leave out

Fixed. I also moved the comment up (both here and in Document) to make it clear that it applies to the whole handler, not just the notifyDocumentChange() part. (There's already another bit that's out of sync - we don't check _refreshInProgress in the mock version. I don't know if that would break anything.)

peterflynn · 2014-06-18T04:44:32Z

@njx Done reviewing -- just a few comments

peterflynn · 2014-06-18T05:22:33Z

p.s. There were a couple other things that came up that I don't think should hold up this PR:

Check/uncheck all matches for a given file -- spun off as [ReplaceInFiles] Way to check/uncheck all matches for one file #8159 (ideally to be fixed later within this cycle)
I'm finding the "Replace in..." placeholder text in the query field pretty weird. I know it just parallels the existing "Find in..." placeholder, but in retrospect that's a little weird too... Since we might have some back & forth on what to do there, we should use a separate PR for the string change.
Edge-case usability issue, maybe not required for this sprint -- Confusing that "replace on disk" sometimes includes files listed in working set #8160
Pre-existing results panel bug I found while testing -- Find in Files 'selected match' highlight shown on wrong match after editing text #8161

* Got rid of "navigator" and "scope" options to FindBar, since they're redundant with "multifile" * Renamed SearchModel.addResults() to setResults() and made it remove the existing results first if any * Comment clarifications

njx · 2014-06-18T17:28:03Z

@peterflynn Ready for re-review. I've added #8160 to the card since I think that's the main one we should fix before shipping. Do you want to file a bug for the placeholder text so we can track that issue as well?

njx · 2014-06-18T17:29:27Z

Also, do you think #8159 is a requirement for this story? Seems like an added feature.

peterflynn · 2014-06-18T20:42:19Z

This and the if wrapper below can be removed in light of the new setResults() behavior

I think this is only used once at this point, so I might even inline it.

Note that I can't remove the if wrapper because we return different values in the two cases.

peterflynn · 2014-06-18T20:58:36Z

@njx Done re-reviewing. Just a handful of nits. Do you have cycles for that today? If not lmk and I can do those changes & then merge.

Re #8159 -- it's in the story criteria, so I don't think it's adding new scope. But if we run low on time and need to punt it, it seems separable.

Bug-wise, I think #8162 is worth looking into for this sprint as well.

njx · 2014-06-18T21:35:23Z

@peterflynn Ready for re-review. You're right about #8159 - I missed it in the criteria. We can leave it on the card for now.

peterflynn · 2014-06-18T21:50:27Z

I think we still need to setResults() even in the !matches.length case -- it looks like the call to this from _addSearchResultsForEntry() could be invoked for files where we have a pre-existing record in the SearchModel, which needs to get updated even if the new result is 0 matches.

Yup, good point. I'll actually just call removeResults() directly in that case.

Actually, it's nicer if I just hoist the setResults() call out.

peterflynn · 2014-06-18T21:52:32Z

@njx Just one more issue!

…ches

njx · 2014-06-18T22:23:14Z

@peterflynn OK, should be ready for another round. In addition to fixing the previous issue (and re-fixing it more nicely :)), I also fixed the filesystem issue I mentioned on IRC.

njx · 2014-06-18T22:40:49Z

Reverted the filesystem "fix" since it was inefficient. I'll file a bug.

njx · 2014-06-18T22:45:41Z

Filed as #8168

peterflynn · 2014-06-18T23:15:12Z

Wohoo! Ready to roll!

[replace-across-files] Code review fixes

Narciso Jaramillo added 4 commits June 12, 2014 22:59

Address all code review items from #7705 except for showError()/showN…

c09eaef

…oResults() and parseQuery() cleanup

Move special-case handling of multifile Replace All UI interactions i…

7b2d805

…nto FindBar

Make SearchModel keep an accurate count of results without recounting…

660164f

… every time Also fixes a long-standing bug in updating line numbers for existing result matches.

njx mentioned this pull request Jun 17, 2014

Code cleanups for Find/Replace in Files #8138

Open

njx assigned peterflynn Jun 17, 2014

njx added the PR Triage Complete label Jun 17, 2014

njx added this to the Release 0.41 milestone Jun 17, 2014

njx mentioned this pull request Jun 17, 2014

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

Open

Fix up File Filters unit tests to work with FindInFiles refactoring. …

2b65a5c

…Clean up duplicate code.

njx mentioned this pull request Jun 17, 2014

[replace-across-files] Replace in Files #7809

Merged

peterflynn reviewed Jun 18, 2014
View reviewed changes

peterflynn mentioned this pull request Jun 18, 2014

[ReplaceInFiles] Way to check/uncheck all matches for one file #8159

Closed

More code review fixes:

7e35280

* Got rid of "navigator" and "scope" options to FindBar, since they're redundant with "multifile" * Renamed SearchModel.addResults() to setResults() and made it remove the existing results first if any * Comment clarifications

peterflynn reviewed Jun 18, 2014
View reviewed changes

Final code cleanups/fixes for replace in files

d8449e2

peterflynn reviewed Jun 18, 2014
View reviewed changes

Narciso Jaramillo added 2 commits June 18, 2014 14:59

Always clear results for previous search even if there are no new mat…

1485227

…ches

Nicer fix for always clearing old search results

e4a50c0

peterflynn added a commit that referenced this pull request Jun 18, 2014

Merge pull request #8137 from adobe/nj/replace-review-fixes

9df0482

[replace-across-files] Code review fixes

peterflynn merged commit 9df0482 into nj/replace-across-files Jun 18, 2014

peterflynn deleted the nj/replace-review-fixes branch June 18, 2014 23:15

This was referenced Aug 30, 2021

[CLOSED] [replace-across-files] Code review fixes brackets-archive/bracketsIssues#7282

Open

Incorrect updating of Find in Files Panel after Move Line Down brackets-archive/bracketsIssues#12700

Open

Conversation

njx commented Jun 17, 2014

Uh oh!

njx commented Jun 17, 2014

Uh oh!

njx commented Jun 17, 2014

Uh oh!

TomMalbran commented Jun 17, 2014

Uh oh!

njx commented Jun 17, 2014

Uh oh!

TomMalbran commented Jun 17, 2014

Uh oh!

njx commented Jun 17, 2014

Uh oh!

redmunds commented Jun 17, 2014

Uh oh!

njx commented Jun 17, 2014

Uh oh!

njx commented Jun 17, 2014

Uh oh!

njx commented Jun 17, 2014

Uh oh!

njx commented Jun 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterflynn commented Jun 18, 2014

Uh oh!

peterflynn commented Jun 18, 2014

Uh oh!

njx commented Jun 18, 2014

Uh oh!

njx commented Jun 18, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterflynn commented Jun 18, 2014

Uh oh!

njx commented Jun 18, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterflynn commented Jun 18, 2014

Uh oh!

njx commented Jun 18, 2014

Uh oh!

njx commented Jun 18, 2014

Uh oh!

njx commented Jun 18, 2014

Uh oh!

peterflynn commented Jun 18, 2014

Uh oh!

Reviewers

Assignees

Labels