This repository has been archived by the owner on Dec 15, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 197
Search results as text #933
Open
jchavarri
wants to merge
17
commits into
atom:master
Choose a base branch
from
jchavarri:search-results-as-text
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Disclaimer: Any comments I make do not mean that I will be the one reviewing and merging this PR. Just trying to be helpful.
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi! 👋 I have started using Atom lately, and I love it :) but I was missing this feature so much, so taking a stab at building it here.
Description of the Change
Adds a new class
ResultsTextViewManager
that usesTextEditor
in order to show the results as text.It is heavily influenced by Sublime Text search results. At this point, it features:
searchContextLineCountBefore
andsearchContextLineCountAfter
find-and-replace.findResultsAsText
config keyshouldPromptToSave
to not prompt the user when closing the search results paneAlternate Designs
Disclaimer: I have no experience developing Atom plugins or with the Atom codebase at all 😁 so there is probably plenty of room for improvement.
I tried to not touch the existing
ResultsModel
, and just add any needed modifications on the text output at the "rendering" step (i.e. just before setting the text on theTextEditor
instance). I also tried to not subclassTextEditor
as there seems to be a lot of logic relying on that class that might fail when subclassing.Benefits
The main benefit of implementing the results as text with an "opt in" approach is that users that are used to the existing web based results pane won't see any changes.
The benefits of having results as text are mainly:
Possible Drawbacks
I don't see any 🤔 I was expecting to see performance issues but it seems to be as performant as the web based search results, at least on a first look (would need to test more thoroughly).
Applicable Issues
#133 Search in search results. Searcheption
#765 Select and edit search results as text
Also related: https://discuss.atom.io/t/searchception-missing/1779/3.
Todo
onDidSearchPaths
)onDidClear
,onDidClearReplacementState
,onDidErrorForPath
... it's still unclear to me what they do, will have to take a deeper look.pending
on double clickprojectSearchResultsPaneSplitDirection
when opening files on double click (not sure what this config does).untitled
pane opens on launch if theProject Find Results
pane was being shown before closing.Status / questions
Even if this PR is clearly not ready to merge, I would really appreciate a first review or some pointers from the maintainers about the general strategy. Some other questions I have –and wasn't able to find answers to– are:
find-and-replace
project? There doesn't seem to be any.eslintrc
file orpackage.json
config.Thanks!