Block search after wrapping (( around selection is buggy

[datokrat]

Problem

Select some text, type ((. This opens the block search panel. Select a block. One would expect that this results in a block reference to the selected block. However, in BETA-80, nothing happens (except that some characters are unselected), while in BETA-82, instead of a block reference, we get just ((.

As an aside (which should probably get a separate bug report), after selecting text and then typing ((, sometimes no item is selected at all, so that pressing ENTER leads to an error message that the selected index is still nil. It seems to be reproducible in the following way: Create a new block and then do the above as the first thing you do. I suspect that there is some problem with the local state of the block. After re-opening the page, this state is flushed and the bug occurs at pre-existing nodes again.

I believe the former bug to be a bit more severe.

Demo

Here is a recording demonstrating the two bugs (first the former, then the latter) in the github.io instance.

https://www.loom.com/share/97e1ab3b9d8c40348b5ae1b661a0aa84

Athens version and environment

Tested mainly in the browser (BETA-80 and BETA-82), but this also occured to me in my native BETA-80 running on Linux.

A side remark

My main use case for "write text, select text, type ((" stems from the performance problems with the block search (see #756). I noticed that searching lots of blocks is only slow when the resulting list of blocks is very long. However, when I write a very specific search query and then surround it with ((, the search is very fast, even in older versions of Athens. This puzzles me a bit, since the #756 performance problems seem to be fixed by optimizing the search queries. I'm puzzled why the very specific search queries have been blazing fast even before this fix.

Fix

I have a fix for the first bug, which also contains some refactoring of auto-complete-inline. Since this isn't an issue yet, I did not create a PR.

[datokrat]

The PR is more of a quick fix. Now I noticed some more problems with the block search + autocomplete. For example, undo is impossible after autocompletion, I cannot query for blocks that contain another reference (since the block query panel is replaced with the query panel for the inner reference) and pressing left or right keys closes the query panel.

Does anyone have an idea how the current implementation may be improved? I'm thinking about showing the query panel whenever the parser says that the cursor is surrounded by (()) or [[]]. However, this would have to be checked after every cursor movement and would probably affect performance if the block contains a lot of text.

We might also let the user re-open the query panel by pressing ctrl+space. Then the parsing would not have to be done after every cursor movement.

[tangjeff0]

For example, undo is impossible after autocompletion

I wonder why undo doesn't work. I thought your fix here would've addressed that: b635da0#diff-6ba8baa251a4638119746942241b0030fa8504dc56c602d27e9a41cac117adbeR582-R585

I cannot query for blocks that contain another reference (since the block query panel is replaced with the query panel for the inner reference)

Can you elaborate on this? I don't fully understand.

pressing left or right keys closes the query panel

Does anyone have an idea how the current implementation may be improved? I'm thinking about showing the query panel whenever the parser says that the cursor is surrounded by (()) or [[]]. However, this would have to be checked after every cursor movement and would probably affect performance if the block contains a lot of text.

A good solution to update-query should address both the above points #607. @ssjoleary was working on this but I don't know current status. TLDR, I think update-query should be pretty much decoupled from textarea-keydown.

We might also let the user re-open the query panel by pressing ctrl+space. Then the parsing would not have to be done after every cursor movement

I think we should just calculate after every cursor movement. I don't think it's too expensive of an operation because block size length are typically quite small, and I don't think users should have to learn that keypress.

I recommend seeing how [[ / and (( are implemented in Notion, LogSeq, and Roam. All slightly different. @shanberg chime in if you have ideas on UX around auto-completion. This isn't a UI question - we have the autocomplete dropdown - it's more of a keyboard input interaction question.

[datokrat]

For example, undo is impossible after autocompletion

I wonder why undo doesn't work. I thought your fix here would've addressed that: b635da0#diff-6ba8baa251a4638119746942241b0030fa8504dc56c602d27e9a41cac117adbeR582-R585

Well, as far as I understand, the previous fix addressed the insertion of closing pair characters. However, undo after doing autocompletion is still not possible. If it helps, I did a demonstration here.

The remaining problem is that we currently autocomplete by swapping (:string/local @state). See this line of auto-complete-inline. This probably also has to be replaced by calling execCommand (or some other solution if execCommand will be removed from JavaScript engines one day).

I cannot query for blocks that contain another reference (since the block query panel is replaced with the query panel for the inner reference)

Can you elaborate on this? I don't fully understand.

When the query string (inside (( and ))) is, for example, [[Hi]], then the block search vanishes irreversibly and is replaced with a page search panel since I typed [[. You can also see this in action in this demo.

pressing left or right keys closes the query panel

Does anyone have an idea how the current implementation may be improved? I'm thinking about showing the query panel whenever the parser says that the cursor is surrounded by (()) or [[]]. However, this would have to be checked after every cursor movement and would probably affect performance if the block contains a lot of text.

A good solution to update-query should address both the above points #607. @ssjoleary was working on this but I don't know current status. TLDR, I think update-query should be pretty much decoupled from textarea-keydown.

We might also let the user re-open the query panel by pressing ctrl+space. Then the parsing would not have to be done after every cursor movement

I think we should just calculate after every cursor movement. I don't think it's too expensive of an operation because block size length are typically quite small, and I don't think users should have to learn that keypress.

Yes, I think that's fine -- if it should become a problem in the future, we can still switch gears.

[datokrat]

I hopefully fixed undo after autocomplete with my new commit for #1219 .