New cell creation selects cell with searched word present, not created cell

[mcclaassen]

Description

When a text search is active within a jupyter notebook, creating a new cell will cause a different cell (one containing the word being searched for) to be selected instead of the newly created cell. This also occurs when deleting a cell with dd.

Reproduce

1. Press Ctrl + f
2. Type a word that is in some cell in the notebook
3. Select any cell and press a or b to create a cell above or below the selected cell
4. The cell with the searched word will now be selected instead of the newly created cell

Expected behavior

Normally, the newly created cell will be selected immediately after its creation.

Context

• Operating System and version: Windows 10, version 1903
• Browser and version: Google Chrome desktop app
• JupyterLab version: 1.0.2
• No extensions installed
• a and b keyboard shortcuts not modified

(some) jupyter troubleshoot output:

sys.executable: C:\Users\claassen\AppData\Local\Continuum\anaconda3\python.exe
sys.version: 3.7.3 (default, Apr 24 2019, 15:29:51) [MSC v.1915 64 bit (AMD64)]
platform.platform(): Windows-10-10.0.18362-SP0
where jupyter: C:\Users\claassen\AppData\Local\Continuum\anaconda3\Scripts\jupyter.exe

[adityar-r]

I suppose the issue is caused due to change of the activeCellIndex, under _stepNext.
@jasongrout I had removed these lines, and the issue seems to be resolved. Can you explain me the purpose of these lines?

jupyterlab/packages/documentsearch/src/providers/notebooksearchprovider.ts

Lines 123 to 126 in cd5e119

	this._currentMatch = await this._stepNext(
	this._searchTarget.content.activeCell
	);

[jasongrout]

CC also @aschlaep

[adityar-r]

I suppose the issue is caused due to change of the activeCellIndex, under _stepNext.
@jasongrout I had removed these lines, and the issue seems to be resolved. Can you explain me the purpose of these lines?

jupyterlab/packages/documentsearch/src/providers/notebooksearchprovider.ts

Lines 123 to 126 in cd5e119

	this._currentMatch = await this._stepNext(
	this._searchTarget.content.activeCell
	);

@aschlaep can you help me understand this?

[adityar-r]

@jasongrout shall I remove those lines and create a pull request?

[jasongrout]

Good question. I haven't been in this part of the code in a while, so I'm unsure of the ramifications (presumably that code is there for a reason?). I'm hoping that @aschlaep can chime in...

[telamonian]

This is a duplicate of #6701. @aschlaep didn't seem to think it would be easy to fix, and I never ended up looking into it more deeply. Some of @aschlaep's worries may have been superceeded by the fixes @krassowski made in #6824.

@bw-space Those lines may relate to this bit of the previous conversation:

@aschlaep

The reason it's completely redoing the search when the content changes is because it needs to recount the matches to make sure the current highlighted index number is correct...

Me

I just checked. If the contents of a page change, the search boxes of both Firefox and Chrome will postpone that kind of recalculation until the next search event (eg the user hits ctrl + g). Can we take this same approach?

[adityar-r]

@telamonian I too think, its for updating the index, as without those lines, if I insert a cell with same content, the highlighted search does not seem to change the search position, but if I keep the code, the index does not change untill I create or insert a cell, upon which the index changes to the next nearest search term found. I think if we make amends to this part of the problem, it should fix this issue. What do you think?

[telamonian]

I personally have no problem if the document search updates lazily instead of eagerly. Given the typical set of interactions that add or remove content to/from a notebook, it's hard to see the actual use case for eager-updating search.

If you can make changes such that the search doesn't update until the user interacts with it again (eg presses shift + g), then I'd say that's an improvement and you should make a PR.

Really, the way it works right now represents a very serious usability issue.

[adityar-r]

There is also another issue, if we create a cell and if the search term is present there, then even on using Ctrl + g to reach it then the search halts with an error on the console:

Uncaught (in promise) TypeError: Cannot read property '0' of undefined
at CodeMirrorSearchProvider.highlightNext

[telamonian]

Well it shouldn't do that :)

I just tried reproducing the error you describe, and I cannot (OS X, Chromium).

[mcclaassen]

Hi All,
Should I mark as a duplicate (possible?) and close or can progress be made that wasn't possible before?

[aschlaep]

Sorry folks! I've been fairly busy recently with other work. Yes, that code is necessary to select the first match upon starting a search. As @telamonian mentioned, #6701 does cover this issue and there I explained why this is non-trivial to fix. I haven't had time to look at the issue since @krassowski improved the search performance in #6824. Making search update lazily could be a solution, but would be a pretty significant UX change, so I'd want a bit more buy in from other folks before we committed to that route. I agree this is definitely worth fixing though and is a large UX problem in and of itself. I'll see if I can find some time to take a closer look at this in the next month or so, but I don't have time in the immediate future, unfortunately. If anybody else wants to take a swing at it, by all means go for it!

[krassowski]

This was fixed in #7835 included in JupyterLab 2.0 release. I will close this issue as we are currently at JupyterLab 3.1.