Skip to content
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 46 commits into from Feb 1, 2019

Conversation

@aschlaep
Copy link
Member

@aschlaep aschlaep commented Dec 22, 2018

Fixes #1074

Goals

This is the start of a document search framework. The goal is to provide default search functionality for notebooks and text documents, but provide the option to override the default behavior or add custom functionality for custom widgets.

What this does

This PR creates the document search extension as a starting point for these search goals. It provides a default implementation for searching notebook inputs (code and markdown) as well as text documents (opened in codemirror). There are two search providers in this PR, one for codemirror and one for notebooks (which uses the codemirror search provider). The default functionality provided includes searching forward and backward, counting matches, tracking match index, case sensitive search, and regex search. I was able to make use of codemirror's searchcursor and overlay functionality, but had to mostly rewrite its search.js to provide my own hooks and match tracking.

Current behavior is that when starting a search, it'll look through all of your code and markdown cells for matches. It'll unhide any hidden inputs containing matches and unrender any markdown containing matches. It'll highlight all matches and put the cursor on the next match after the last cursor position in the active cell. Once done searching, it'll re-render the markdown cells that were temporarily un-rendered (I chose not to re-hide code cells, but that could easily change). The search can then toggle through matches across cells and loop at the beginning/end of the notebook. If you move your cursor mid-search, it'll start toggling through matches from the new cursor position. Thanks to codemirror's awesome overlay, the highlighting will update live as you type.

Also in this PR, I removed the custom types living in the codemirror package in favor of those that exist in @types/codemirror. I added back the types that were used by JupyterLab and do not exist in DefinitelyTyped's set of types. I also had to expose several additional functions on codemirror to perform the search, get the cursor, etc.

Future work

Potential (hopefully near-term) future enhancements to this would include search & replace, searching outputs, and searching across multiple files.

To do:

  • replace UI used for testing with a proper overlay above active document
  • discuss interfaces
  • reorganize the commands
  • address case of adding/deleting cells mid-search
  • ensure search works with multiple documents open
  • more testing/bug squashing

Here's a gif demo with the UI I've been using for testing.
ctrl-f-demo

@jasongrout jasongrout added this to the 1.0 milestone Jan 4, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jan 7, 2019

@aschlaep - here is the recent work on React widgets: #5479

That PR is a bit overwhelming, but when you get a chance to push some of your react code up here, we can take a look at how to use this new react infrastructure.

Copy link
Member

@ivanov ivanov left a comment

some initial comments, have to wrap my head around the larger design of this...

anchor: this._toCodeMirrorPosition(range.start),
head: this._toCodeMirrorPosition(range.end),
from: this._toCodeMirrorPosition.bind(this, range.start),
to: this._toCodeMirrorPosition.bind(this, range.end)
Copy link
Member

@ivanov ivanov Jan 8, 2019

is there a reason that anchor and head can use the function invocation way, but from and to need the bind? Seems like these four should be the same, or could use a comment explaining why they are different.

Copy link
Member Author

@aschlaep aschlaep Jan 8, 2019

For some reason, CodeMirror's Range type takes anchor and head as values and from and to 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.

Copy link
Contributor

@jasongrout jasongrout Jan 26, 2019

from and to 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-L59

Copy link
Contributor

@jasongrout jasongrout Jan 26, 2019

Perhaps we should just be using the CodeMirror Range object directly here? Can we import and use it?

Copy link
Contributor

@jasongrout jasongrout Jan 26, 2019

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 a CodeMirror.Range object. So I think we perhaps don't need this private function after all?

Copy link
Contributor

@jasongrout jasongrout Jan 26, 2019

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 with from and to positions (not functions).

Copy link
Contributor

@jasongrout jasongrout Jan 28, 2019

I think a patch like this should make this bit of code work:

diff --git a/packages/codemirror/src/editor.ts b/packages/codemirror/src/editor.ts
index 9f3cf38c7..fb10dec89 100644
--- a/packages/codemirror/src/editor.ts
+++ b/packages/codemirror/src/editor.ts
@@ -469,7 +469,8 @@ export class CodeMirrorEditor implements CodeEditor.IEditor {
    */
   revealSelection(selection: CodeEditor.IRange): void {
     const range = this._toCodeMirrorRange(selection);
-    this._editor.scrollIntoView(range);
+    // Explicitly pass in undefined to select the right function typing overload.
+    this._editor.scrollIntoView(range, undefined);
   }
 
   /**
@@ -819,12 +820,12 @@ export class CodeMirrorEditor implements CodeEditor.IEditor {
   /**
    * Converts an editor selection to a code mirror selection.
    */
-  private _toCodeMirrorRange(range: CodeEditor.IRange): CodeMirror.Range {
+  private _toCodeMirrorRange(
+    range: CodeEditor.IRange
+  ): { from: CodeMirror.Position; to: CodeMirror.Position } {
     return {
-      anchor: this._toCodeMirrorPosition(range.start),
-      head: this._toCodeMirrorPosition(range.end),
-      from: this._toCodeMirrorPosition.bind(this, range.start),
-      to: this._toCodeMirrorPosition.bind(this, range.end)
+      from: this._toCodeMirrorPosition(range.start),
+      to: this._toCodeMirrorPosition(range.end)
     };
   }

Copy link
Contributor

@jasongrout jasongrout Jan 28, 2019

@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

packages/documentsearch-extension/package.json Outdated Show resolved Hide resolved
packages/documentsearch-extension/src/searchbox.ts Outdated Show resolved Hide resolved
packages/documentsearch-extension/src/searchbox.ts Outdated Show resolved Hide resolved
packages/documentsearch-extension/src/searchbox.ts Outdated Show resolved Hide resolved
packages/documentsearch-extension/src/searchbox.ts Outdated Show resolved Hide resolved
@aschlaep
Copy link
Member Author

@aschlaep aschlaep commented Jan 12, 2019

Put together a UI that's closer to the final thing (need some proper up/down arrows, some minor alignment tweaks), here's a quick demo.
ctrl-f-demo-2

@aschlaep
Copy link
Member Author

@aschlaep aschlaep commented Jan 17, 2019

This is now ready for review. I'm still missing the resources for the buttons, but that should be a minor update (hopefully tomorrow). Near-term future work for the document search extension include writing tests, focus tweaks (focus the document after closing search), perhaps switching the search box to a toolbar widget, and beginning work on search/replace and whole workspace search. @jasongrout

@aschlaep aschlaep changed the title [WIP] Add ctrl-f/g support for notebooks and text files Add ctrl-f/g support for notebooks and text files Jan 17, 2019
@jasongrout jasongrout self-requested a review Jan 26, 2019
packages/codemirror/package.json Outdated Show resolved Hide resolved
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jan 31, 2019

Andrew, I just merged in master and fixed the search extension to work with master. It was indeed a simple renaming.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

@aschlaep - ready for final review?

@aschlaep
Copy link
Member Author

@aschlaep aschlaep commented Feb 1, 2019

Ready now! Caught one more bug and made a styling update, it looks like something I merged in caused the button toggles to get box-sizing: unset, which caused them to get bigger and repeat their background, so I set the buttons to no background repeat as well as setting the input toggles to box-sizing: border-box, which seemed to fix the issue in both chrome and firefox. The bug was an error that occurred when it tried to re-render a markdown cell containing a match after that cell had been deleted.

@gnestor
Copy link
Contributor

@gnestor gnestor commented Feb 1, 2019

I'd be happy to user test when it's ready 👍

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

Grant, if you want to pull this pr and test, that would be awesome

@gnestor
Copy link
Contributor

@gnestor gnestor commented Feb 1, 2019

I tried checking out the branch and jlpming and running lab in dev-mode watch mode, but I don't see the find left sidebar panel...

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

Oh, it's better now! Ctrl f or use the command palette

@gnestor
Copy link
Contributor

@gnestor gnestor commented Feb 1, 2019

Oh ya! First impression: the UX is nice!

jasongrout added 3 commits Feb 1, 2019
…o menu.

Also, I refactored the code a bit, which ended up moving logic into the actual command execute functions.

Now find next and previous are only enabled when there is an active search open.
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

Okay, I think this is ready to go in. Two follow-up issues I noticed for the next iteration:

(a) I noticed when I had flux running, I could no longer see the yellow highlight. This was really confusing. It would be great to see if the highlight disappears in other night-mode blue-light-disabled schemes. Actually, it'd be nice if the highlight color was customizable, or at least if it used the standard jlab theme colors so that it worked well with different themes.
(b) I noticed in vs code and sublime, the search query was kept around even when the search box was not shown, so that find next/previous still worked with the last query. I played a bit with having the search instance stick around, and just hide/show the search box as needed, but didn't get very far before deciding that was for the next iteration.

@jasongrout jasongrout merged commit 937a3d8 into jupyterlab:master Feb 1, 2019
8 of 11 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 1, 2019

@aschlaep - thanks very much again! Merging this, let's iterate from here.

@cquah
Copy link
Contributor

@cquah cquah commented Feb 1, 2019

Thank you @aschlaep @jasongrout !!

@aschlaep
Copy link
Member Author

@aschlaep aschlaep commented Feb 1, 2019

Awesome, thanks @jasongrout!

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants