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

Searching cell output #6768

Closed
aquirdTurtle opened this issue Jul 5, 2019 · 17 comments · Fixed by #7258
Closed

Searching cell output #6768

aquirdTurtle opened this issue Jul 5, 2019 · 17 comments · Fixed by #7258
Labels
pkg:documentsearch status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Milestone

Comments

@aquirdTurtle
Copy link

The new find/replace functionality looks nice, but I noticed that, as far as I can tell, I can't search the output of a cell. For example:
image
I expect the search to find two instances, the instance in the code cell and the instance in the output cell. Obviously the two are of a different kind - you couldn't do a replace on the output cell, but some way to search outputs would be very welcome. In principle it would seem nice if it was possible to search e.g. the text in a figure as well. Perhaps you could select the scope of the search with an additional combo box in the find tool, or with some check boxes.

@arogozhnikov
Copy link

Also faced this issue. That's quite critical, is there a way to disable custom search to use browser built-in?

@ar-nowaczynski
Copy link

ar-nowaczynski commented Jul 22, 2019

To launch browser search, first click on top menu bar in Jupyter Lab and then Ctrl + F.

@arogozhnikov
Copy link

@ar-nowaczynski soo hacky, thanks :)

@quant5
Copy link

quant5 commented Aug 6, 2019

@ar-nowaczynski your solution worked, but clicking is quite cumbersome... maybe there should be a way to map browser search to Ctrl+F and custom search to something else? Or another solution?

@jasongrout
Copy link
Contributor

To disable or change the current search shortcut, see steps 1 and 2 of #6830 (comment)

(to change it, disable the builtin shortcut and add a new one)

@jasongrout jasongrout added this to the Future milestone Aug 6, 2019
@raulf2012
Copy link

@wkzhu An alternative to clicking the top menu bar is to open up the command pallet (Ctrl+Shift+C) and then Ctrl+F
This seems to work

Alternatively, @jasongrout's idea of changing the key bindings is the best one

Here is a code snippet to place within the keyboard shortcuts user preferences that will turn off the Jupyter Ctrl+F shortcut and remap it to a Ctrl+Shift+F shortcut. This will then free up Ctrl+F to browser shortcut
`
{
"shortcuts": [

    // # Searching ########################################################
    // ####################################################################
    {
        "command": "documentsearch:start",
        "keys": [
            "Ctrl Shift F"
        ],
        "selector": ".jp-mod-searchable"
        // "disabled": true
    },

    {
        "command": "documentsearch:start",
        "keys": [
            "Accel F"
        ],
        "selector": ".jp-mod-searchable",
        "disabled": true
    }
]

}

`

@mlucool
Copy link
Contributor

mlucool commented Sep 9, 2019

What do people think about making ctrl-f have an option to choose where to search (input, output)? If not, would people object to using @raulf2012 suggestion:

  1. If you are in a cell, ctrl-f just searches that cell
  2. If you are not in a cell, ctrl-f uses the browsers search
  3. If you are not in a cell, ctrl-shift-f uses the find in code search

@jasongrout
Copy link
Contributor

I think I would prefer to have a keyboard shortcut consistently invoke the same search. Using the same convention we have for context menus (shift-right-click for default browser), perhaps ctrl-shift-f always invokes browser search?

@mlucool
Copy link
Contributor

mlucool commented Sep 9, 2019

In addition to what you said, do you object to enhancing the custom search to allow for search of output? Similar to the regexp option today, I imagine there would be options for searching in all, input, or output. Obviously, replace won't work for output.

@jasongrout
Copy link
Contributor

Searching all output is significantly harder than searching input, since we can have arbitrary renderers rendering outputs, and who knows what html or graphics they might be putting on the page.

Searching stdout and stderr would be much easier, since we know those are text.

@mlucool
Copy link
Contributor

mlucool commented Sep 9, 2019

I quickly tried something based on this and it seems to mostly work. It was able to find all the text I expected it to:

  1. As the output of a cell
  2. In a SVG rendered by a charting lib
  3. In a react custom grid render
  4. On the left hand panel of the cell (e.g. [10])

Open a notebook in lab, then in console add this (use elements to find a notebook id):

function textNodesUnder(el){
  var n, a=[], walk=document.createTreeWalker(el,NodeFilter.SHOW_TEXT,null,false);
  while(n=walk.nextNode()) a.push(n);
  return a;
}
textToFind = '9'
notebookID = 'id-aaf17893-3c55-40a8-a141-5b067ca97437'
textNodesUnder(document.getElementById(notebookID)).filter(e => e.textContent.indexOf(textToFind ) != -1)

Do you see any reason this won't work?

@jasongrout
Copy link
Contributor

jasongrout commented Sep 10, 2019

CC @aschlaep, who has implemented a find that searches the DOM before.

@aschlaep
Copy link
Member

In my experience, working directly with the rendered DOM is really difficult when you have all sorts of other frameworks messing with it (React and Phosphor). I tried to do something like this in the classic notebook, finding the nodes containing matches, inserting a span around the match, applying a class to the span, and keeping track of them so that I could clean up later. You then run into issues like matches spanning across multiple DOM elements. It gets messy. Someone mostly fixed the main issues with this in findAndReplaceDOMText, which I eventually tried using, but it wasn't perfect, partly due to the framework issues I mentioned above, partly due to the implementation of the notebook (for example, both the rendered and unrendered display of the markdown cells remain on the page, just toggling which is visible).

I do think this could be a good place to start for most basic output types, especially those that aren't interactive and especially if you limit this sort of DOM search to the outputs. I haven't tried anything like this yet in JupyterLab with the new documentsearch framework, so maybe it's gotten easier? I'm not sure what sort of model-like information might be available about cell output, so that might be worth looking into as well, to see if you can do something in the output render step based on actual output information. This is the approach that I initially envisioned taking once we got around to cell outputs, maybe due to some bitter memories of DOM-based search on the classic notebook...

@mlucool
Copy link
Contributor

mlucool commented Sep 11, 2019

so that might be worth looking into as well, to see if you can do something in the output render step based on actual output information.

This may be hard. Imagine you have a number like 1568235248028 then a charting library renders this epoch time as 20190911-20:54:08. Find should work for finding 2019.

I believe its at least worth trying to do something here - even if we can't solve every case. From a UX POV only having search on inputs and not outputs feels like a poor choice. While we can escape to the built in search for the browser one as above, but that is something only advanced users will do; it will hurt novice users.

@aschlaep
Copy link
Member

Yep, I agree, it'll likely be difficult. I don't know enough about how outputs are rendered to know where in that process this sort of logic might belong, but I think having something that works with the rendering step rather than on top of it or after the fact might be more robust and flexible for the myriad different output types that exist now and in the future. If search was built directly into how things are rendered, it'd make it easier to integrate different forms of output in the future.

And yeah I totally agree, searching only inputs is not ideal. We absolutely do want to search the outputs. The reason we decided to only search inputs to start is because searching inputs was a much more tractable problem. CodeMirror already has some searching facilities built in, all we needed was a bit of logic to orchestrate it. We wanted to get some form of searching working before 1.0 that would allow for nicer features than what you'd get from the browser, like regex matching and search & replace. We kind of punted on searching the outputs because it's significantly more difficult for the reasons I mentioned above.

I haven't had time since then to take a swing at output search and I'm not sure I'll have much time in the near future. If you think you've got an approach you'd like to try, please go for it! I'd also be happy to talk with you more about the documentsearch package and the approach we've taken for search so far if you'd like.

@mlucool
Copy link
Contributor

mlucool commented Sep 13, 2019

If search was built directly into how things are rendered, it'd make it easier to integrate different forms of output in the future.

I agree that we should allow renders to be allowed to have some ability to implement search. A good example if a table with virtual rendering for data. The table may want to implement search to include all cells within it.

Still, by default, it would be good to implement anything that searched the DOM for text. Even if something like: <span class='bar'>hello </span> world cannot find hello world, having some search text would be a big step forward.

What's the best way to chat?

@BrenBarn
Copy link

Personally I think this issue is critical enough that we should consider disabling the Jupyterlab-specific search by default --- that is, assigning it to a different shortcut or only to some kind of toolbar button, not to Ctrl-F, and making the user change the config to "opt in" to assigning it to Ctrl-F, rather than the reverse. The ability to search the entire page is a basic function of a web browser, and having to use any workaround whatsoever to get past Jupyterlab's grabbing of that shortcut is a major hit to productivity when using it. It will also confuse new users, especially because it fails silently (i.e., if you search for something in the output you just get no results, and maybe think there are none, without realizing it's not searching the output at all).

mlucool pushed a commit to mlucool/jupyterlab that referenced this issue Sep 23, 2019
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
mlucool pushed a commit to mlucool/jupyterlab that referenced this issue Oct 8, 2019
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
telamonian pushed a commit to mlucool/jupyterlab that referenced this issue Oct 28, 2019
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
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Jan 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:documentsearch status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants