Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Code cleanups for Find/Replace in Files #8138

Open
njx opened this issue Jun 17, 2014 · 4 comments
Open

Code cleanups for Find/Replace in Files #8138

njx opened this issue Jun 17, 2014 · 4 comments

Comments

@njx
Copy link

njx commented Jun 17, 2014

See #7705, #7809 and #8137.

  • 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()
@njx njx self-assigned this Jun 17, 2014
@peterflynn
Copy link
Member

Remove error-showing functionality from FindReplace.parseQuery()

That one could also be folded into any work we do to make the search APIs headless (I believe someone was working on a PR for that). There are a couple places, including parseQuery(), where we assume if a UI is present we should touch it -- and only operate truly headlessly if the search bar is currently closed. We'd probably need to fix all those cases before exposing a true headless API.

@peterflynn
Copy link
Member

Clean up showError()/showNoResults() (combine them and/or make showNoResults() less confusing)

  • Small thread about code cleanup here: [replace-across-files] Unify Find Bars across FindReplace/FindInFiles #7705 (comment)
  • @TomMalbran suggested a UI cleanup: eliminate .no-results-message (the "No results" label that's prepended to the "in ..." scope label when Find/Replace in Files finds 0 hits -- and instead present the info in the same red dropdown that's used for other error cases (0 files to search / invalid regexp)
  • I noted we might want to wait in case changes in the Find result-count UI (which shares code with this) might affect what we'd do here. There's a near-future user story that changes it into an index indicator (updated during result navigation), and a PR proposal to make it an interactive element.

@peterflynn
Copy link
Member

Move FindUtils.performReplacements() back into FindInFiles.doReplace()

Alternative suggestion: leave it in FindUtils and eliminate the wrapper FindInFiles.doReplace() -- just call it directly. If we moved performReplacements() into FindInFiles, it brings basically all of FindUtils with it... and it's sort of nice having that code separated out as it is currently.

@njx
Copy link
Author

njx commented Jun 19, 2014

@peterflynn Regarding making search headless, the existing PR from @zaggino is obsolete - for what he wants, using FindInFilesUI.searchAndShowResults() is probably good enough. But it would be nice to factor things out further.

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

No branches or pull requests

4 participants