New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ctrl-f/g support for notebooks and text files #5795
Merged
Merged
Changes from 18 commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
48a8fed
wip
aschlaep a1a84d2
remove files
aschlaep 891ae5d
partially implemented search
aschlaep 142166c
forward and reverse search mostly works
aschlaep d9408ea
improved search, need to reexamine indexing
aschlaep 01804c0
mostly working, needs cleanup
aschlaep 8aea812
wip: mainly feature complete, needs testing, UI, code cleanup
aschlaep 7d3989e
live update index
aschlaep 4f746d0
new UI, styling
aschlaep d94d150
minor cleanup
aschlaep 544ae5e
styling updates, small bugfixes
aschlaep bf0ccbd
remove lastquery, change command category
aschlaep 181bd04
manual initial codemirror scan to avoid issue with lazy loading searc…
aschlaep 4d978f1
code review updates
aschlaep 667a4e9
cleanup, comments
aschlaep 744cefd
guard against non-searchable widgets, update overlay toggle logic
aschlaep 58a81f9
cleanup per review
aschlaep 42f4d73
add styling, resources
aschlaep 14a318e
changes per jasons comments
aschlaep 525b1a8
Change search registry to use a Map.
jasongrout 272af34
move canSearchOn to static
aschlaep ad1396f
update licenses
aschlaep d76f1a8
private underscores
aschlaep 5f206c3
Simplify search provider registry find function.
jasongrout e4ed607
bind -> arrow functions
jasongrout b1d95bb
Change the functions to take only widgets instead of shell.
jasongrout 7b3bef3
Formatting
jasongrout 184074e
Change signal to have a ‘this’ sender, move offset to SearchInstance
jasongrout d75e5c5
Move initializing steps into constructor.
jasongrout d5ac6fb
Break SearchInstance into its own file
aschlaep fb7f998
Make SearchInstance implement IDisposable
aschlaep 53aea0f
async await SearchInstance
aschlaep 046c830
switch to async/await in NBSearchProvider, fix off by one issues
aschlaep 3478507
async await, update license, and minor refactor for CMSearchProvier, …
aschlaep 9a0b5dc
remove private namespace in NBSearchProvider
aschlaep 38224e7
escape to close, address focus issue, add docs to searchproviders
aschlaep f052f5c
Merge branch 'master' into pr/aschlaep/5795
jasongrout 9e9eab5
Fix the search extension to use the new frontend objects.
jasongrout 6955cb0
switch to start/endQuery and endSearch, made search lifetime tweaks, …
aschlaep 0d048a5
Merge branch 'ctrl-f-init' of github.com:aschlaep/jupyterlab into ctr…
aschlaep 52c75fb
fix toggle styling, fix bug on deleted md cell containing match
aschlaep de093b1
Take care of a few more lifecycle and enabling issues; add commands t…
jasongrout 337a628
A bit more cleanup.
jasongrout 8bef9a2
Fix dependencies.
jasongrout ea774b3
Fix codemirror typings for editor test.
jasongrout 31e0484
Fix lint error.
jasongrout File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason that
anchor
andhead
can use the function invocation way, butfrom
andto
need the bind? Seems like these four should be the same, or could use a comment explaining why they are different.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, CodeMirror's Range type takes
anchor
andhead
as values andfrom
andto
as functions. I'll have to double check my assumptions here about the values desired here. There's a bit of annoying translation needed in some places between the CodeMirror Range type and the CodeEditor IRange.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from
andto
are functions that basically pick off the min and max of the anchor/head. See https://github.com/codemirror/CodeMirror/blob/5b949b2dbe92a0153c2bd32b75341e7fe3cf6ce0/src/model/selection.js#L51-L59There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should just be using the CodeMirror Range object directly here? Can we import and use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I was reading it wrong. I think you are eventually going to call this function: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/89592f7ec313174363136fe91749cedff272a70e/types/codemirror/index.d.ts#L294
That function needs a
{from: CodeMirror.Position, to: CodeMirror.Position}
range, not aCodeMirror.Range
object. So I think we perhaps don't need this private function after all?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or at least, if we do want this private function, it is not returning a
CodeMirror.Range
, just a{from: position, to: position}
object.I can understand the confusion. The CodeMirror docs say it takes a range, but looking at the codemirror source, it's clear it wasn't talking about a
CodeMirror.Range
object, but rather a simple object withfrom
andto
positions (not functions).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a patch like this should make this bit of code work:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aschlaep - I think this should work for now, and we can delete the extra argument to the scrollIntoView if we submit a change to the codemirror typings to make the margin argument optional in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/ee0aad13bc3dd0f0176ca6794aacc0227d88c0fe/types/codemirror/index.d.ts#L286 and https://github.com/DefinitelyTyped/DefinitelyTyped/blob/ee0aad13bc3dd0f0176ca6794aacc0227d88c0fe/types/codemirror/index.d.ts#L294