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

Support for output search #7258

Merged
merged 33 commits into from Dec 24, 2019
Merged

Support for output search #7258

merged 33 commits into from Dec 24, 2019

Conversation

mlucool
Copy link
Contributor

@mlucool mlucool commented Sep 23, 2019

References

Fixes: #6768

Code changes

This is the first pass at search which can search output in a generic way. This commit does this on a best effort through walking and manipulating the DOM. There are limitations, including the inability to handle that to a user <span><b>foo</b>bar</span> should be searched as foobar. This also will not handle SVGs and does not expose a way to delegate search to output presenters. Still, this works well on stdout/stderr type messages, tables, etc.

User-facing changes

Search will now include output of code cells, this allows users to look for foo in both input and output. Unlike cod cells, replace will be ignored here.

Before:
image

After:
image

Example showing it working on pandas output:
image

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Sep 23, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Sep 23, 2019

cc @aschlaep @jasongrout

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Oct 1, 2019

Ping @aschlaep @jasongrout

This is the first pass at search which can search output in a generic
way. This commit does this on a best effort through walking and
manipulating the DOM. There are limitations, including the inability to
handle that to a user <span><b>foo</b>bar</span> should be searched as
foobar. This also will not handle SVGs and does not expose a way to
delegate search to output presenters. Still, this works very well on
stdout/stderr type messages, tables, etc.

Fixes: jupyterlab#6768
@afshin
Copy link
Member

@afshin afshin commented Oct 28, 2019

Considering how minimal the use of lodash is in this particular PR, I would suggest not using it at all. All of the uses seem available already in ArrayExt in @phosphor/algorithm https://phosphorjs.github.io/phosphor/api/algorithm/modules/arrayext.html

@telamonian
Copy link
Member

@telamonian telamonian commented Oct 28, 2019

I'm seeing an issue whereby certain search highlighting never gets cleared. For example, if I search for "i" and then "invalid" repeatedly, this happens:

image

@telamonian
Copy link
Member

@telamonian telamonian commented Oct 28, 2019

Following @afshin's suggestion, I replaced the uses of lodash with @phosphor/algorithm. I tested and the PR's behavior is unaffected by the change, including the highlighting bug I mentioned in the previous post.

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Oct 28, 2019

Thanks @telamonian. I seeing if I can figure out a way to fix that bug. Just so you have some understanding:
image

This is what the DOM looks like before we mutate it for highlight. Now when we highlight we wrap some text in a span and save the old node to replace. In this case the i in the last invalid causes problems because it's saving some node state that had already been mutated.

I.e. it stores this partially mutated node instead of a true original node:
image

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Oct 28, 2019

@telamonian This has been fixed. Can you try again?

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Nov 10, 2019

@telamonian

It seems the only issue remaining is that searching more of the dom continues to reduce performance.

What do people think of using this little space to add in the option via a dropdown menu for: In/Out/All
image

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Nov 15, 2019

@telamonian - any more thoughts on this to move it forward? Is there a favorite option that we should do to get this merged?

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Nov 19, 2019

@telamonian I have added the ability to do this in the UI as I think users may want to flip back and forth.

While I think the expanded UI needs a little UX work (I didn't know how to easily turn a button into a dropdown menu), it is totally functional:

Default:
image

Expanded:
image

@telamonian
Copy link
Member

@telamonian telamonian commented Nov 20, 2019

Thank you for continuing to push forward on this.

I agree that the UI definitely needs a bit of work. The two input/four state thing is confusing even me a little.

Since it's tomorrow, I'll bring this up at the weekly dev meeting and see what @tgeorgeux (our UI/UX guy) and the rest of the community thinks. You're welcome to join, if you'd care to; getting any kind of consensus is the fastest way to cut through issues like these. The meeting is at noon ET, and the link is here.

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Nov 20, 2019

@telamonian I've mocked up a simple UI that allows different actions in an options menu.

Mockup

Design Files

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Nov 20, 2019

Would it be possible to have the options open while searching, perhaps as a panel below the find/replace lines? That would allow content-specific widgets, not just a set of options. For example, I can imagine a map search showing a thumbnail of the entire map, and you being able to draw a rough region to filter find results.

For example, in vs code, the sidebar search has three dots to expand a filtering mechanism:
Screen Shot 2019-11-20 at 2 42 42 PM
Screen Shot 2019-11-20 at 2 42 54 PM

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Nov 20, 2019

@jasongrout Something like this?

Option 2 Prototype
Option 2 Design Files

@telamonian
Copy link
Member

@telamonian telamonian commented Nov 20, 2019

Summary of the relevant bits of today's dev meeting:

  • output search is notebook specific, and we want our basic filesearch UI to be simple and content agnostic. Thus, extra options like output search will be hidden behind an ... button
  • the output search option makes most sense as a single binary (eg "Search cell outputs" checkbox). We can expand this later if people come to us with use cases for output only search
  • searching/highlighting output makes sense in terms of "find", but doesn't make much sense in terms of "find + replace". Thus, output search should automatically be disabled when the "replace" form is revealed (ie when the caret on the left of the search UI is clicked)

@tgeorgeux Your option 2 both looks good and satisfies the above-listed reqs, so I think we should move forward with that.

@mlucool Does this sound good to you? If it seems like too much of a lift, I'd be fine with doing the implementation myself for the new UI

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Nov 20, 2019

@tgeorgeux - can you show us what it looks like with the replace line expanded?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Nov 20, 2019

Also, would it make more sense to have the option panel the same width as the find/replace lines?

@telamonian
Copy link
Member

@telamonian telamonian commented Nov 20, 2019

@jasongrout I kind of like that it only takes up as much space as needed (plus a little margin for style). It allows for (at least slightly) more visibility of a user's actual document.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Nov 20, 2019

@jasongrout I kind of like that it only takes up as much space as needed (plus a little margin for style). It allows for (at least slightly) more visibility of a user's actual document.

Yes, but the idea is that the content is contributing that section, so different content will have different sizes for that panel. That can be jarring if different content has different widths, whereas I think it looks much better if the width is uniform and the thing the content has control of is the height.

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Nov 20, 2019

Thanks @telamonian for bringing this up in today's meeting. Sorry I couldn't make it.

Thus, extra options like output search will be hidden behind an ... button

That makes sense.

the output search option makes most sense as a single binary (eg "Search cell outputs" checkbox). We can expand this later if people come to us with use cases for output only search

Is there a strong reason for not letting users search only output? While I agree that option may be the least used one, it's easy to think of a few cases when I would choose that.

searching/highlighting output makes sense in terms of "find", but doesn't make much sense in terms of "find + replace".

While it doesn't make much sense now. We may want this in the future to replace input in an output cell (e.g. jupyter widgets).

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 11, 2019

Also:

  • Delete "Search Options" header

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Dec 11, 2019

@jasongrout, thanks! Yep sounds good.

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Dec 11, 2019

  • Make label clickable to toggle checkbox.

I know this is sort of covered by the 'grey out label' check-box but I'd like to make sure this gets in as well.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Dec 14, 2019

@tgeorgeux any other commits you wanna push before this is ready to go in?

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Dec 15, 2019

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Dec 24, 2019

I was about to merge this in this morning, but last night I had another thought. My understanding (but please correct me if I am wrong here) is that implementing find and replace in JupyterLab is mostly about the replace half. Otherwise, we can mostly use the browser's find and replace. Which seems like it will always be more performant.

So I wonder if your goal, @mlucool, of searching outputs might be better served by just using the native search. You could rebind JupyterLab's find and replace to another hot key, so that you could access both easily. What do you think?

Otherwise, we can merge this in, but I am just trying to exhaust all other options since it does add a non trivial amount of logic we will have to maintain and additional UI complexity.

@afshin
Copy link
Member

@afshin afshin commented Dec 24, 2019

@saulshanabrook:

implementing find and replace in JupyterLab is mostly about the replace half

I think the find part is just as important. An IDE typically has several search options as well, like regular expression match, case sensitivity, word or substring match, etc. In JupyterLab, in particular, we'll likely want things like inputs only, outputs only, etc. on the find side.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 24, 2019

I think the find part is just as important.

Yes. We have more context than the browser, so theoretically can offer more targeted, smarter search. Things like finding a place on a map view, finding a value in a datamodel where only a window is shown in the browser, etc.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Dec 24, 2019

Alright, sounds good @jasongrout and @afshin. If there are no objections then, we can merge this in.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 24, 2019

If there are no objections then, we can merge this in.

(I haven't evaluated this PR, and probably won't be able to soon, so don't wait for my approval. I just wanted to chime in on your comment.)

@saulshanabrook saulshanabrook merged commit 37c7a64 into jupyterlab:master Dec 24, 2019
8 of 10 checks passed
@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Dec 24, 2019

Cool. Please tag me on any bugs or issue about this or future search filtering things.

@mlucool mlucool deleted the output-search branch Dec 24, 2019
@blink1073 blink1073 added this to the 2.0 milestone Jan 3, 2020
@lock lock bot added the status:resolved-locked label Feb 2, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants