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/inter note link searching #2286

Merged
merged 20 commits into from
Oct 31, 2020
Merged

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Aug 21, 2020

This PR adds an auto-complete handler to use the search index for the app to generate a list of inter-note links.

It triggers automatically after typing a [, or if you dismiss the popup via ESC or by typing until there are no matches, you can trigger it with Ctrl + Space.

n.b. even if the key combo is used, it will only open if there is an open bracket! Changed this to allow inserting a note link with Ctrl+Space at any time.

Notes

  • Can use the full note list search query syntax. E.g. to search for notes with tag classes you type [tag:classes and then hit the autocomplete trigger
  • Only searches the note title
  • Autocomplete results will match the sort settings for the note list
  • Limits matches so to find specific note you need to type more search to narrow the list
  • The icon for pinned notes is different. It can look weird in the sort order if they aren't distinguished from each other, but it needs a better icon.
  • We should do a better job adding the singleton autocompleter. It's easy but hacky here.

(Graphic somewhat outdated but gives a general idea)
interLinkAutoComplete mov

@dmsnell dmsnell changed the base branch from develop to rewrite/no-races August 21, 2020 02:35
@dmsnell dmsnell force-pushed the rewrite/no-races branch 2 times, most recently from 957a513 to ca8d3ae Compare September 21, 2020 19:51
Base automatically changed from rewrite/no-races to develop September 21, 2020 20:30
@dmsnell dmsnell force-pushed the add/inter-note-link-searching branch from 2951c4c to 1646fba Compare September 21, 2020 21:42
@codebykat codebykat added this to the Internal Links milestone Oct 8, 2020
@codebykat codebykat removed this from the Internal Links milestone Oct 21, 2020
@codebykat

This comment has been minimized.

@codebykat

This comment has been minimized.

@codebykat

This comment has been minimized.

dmsnell and others added 5 commits October 27, 2020 15:50
Adds an auto-complete provider to use Monaco's built-in functionality for
searching notes when linking. This patch presents an alternative approach
from creating a custom dialog.

Due to the way that the auto-complete always returns results even if none
are wanted it may be best to look for a custom dialog.
@codebykat codebykat force-pushed the add/inter-note-link-searching branch from 49cbb08 to 613b533 Compare October 27, 2020 22:50
@dmsnell

This comment has been minimized.

@codebykat

This comment has been minimized.

@dmsnell
Copy link
Member Author

dmsnell commented Oct 29, 2020

I think we should limit search to the titles only and exclude the current note and this might actually work out.

Since the other platforms do that I agree, though I wish everything were text search, or full-text but with title priority. There are a couple things we could explore without changing a substantial amount of code:

  • wrap whatever is typed inside the autocomplete area in quotes, which will treat it as a phrase and naturally surface things that might be identically written in a title. this is a trick but could end up being a neat one
  • duplicate search functionality temporarily in this component but only search the title - skip sorting and other features of the search indexer
  • supply a very large number of max results from the search indexer and then narrow down the results looking only at the title. in most cases this would get everything we want and do it quickly, I would expect, except when the search query is too general to be of good use otherwise.

long term I guess adding a title-search to the search indexer is probably a sound bet anyway.

@codebykat
Copy link
Member

Found this again, just want to leave it here for posterity. There's no API to check if the completions are bound already, and since they are bound to the model, not the editor instance, they can persist, which is why we need to ensure we only add them once. microsoft/monaco-editor#2084 (comment)

@codebykat
Copy link
Member

This is ready for testing! I hacked it into submission. I probably owe @dmsnell a few cat pictures after he tells me what crimes I have committed.

I added extra parameters to the search function to allow searching titles only and excluding an array of IDs. This gives us:

  • Search titles only
  • Exclude current note

In the process I found the completion provider needed to access the current note ID, so I had to rewire things a little. I ended up removing the singleton tracker and using a state prop and the editor's onDispose method.

I also found that the bracket trigger is not sufficient to get the suggestions to pop up when you would expect, so I added a command that binds to Ctrl+Space and only calls triggerSuggest if we're inside a bracket. (More or less what was suggested here.) Needs refactoring because I copied the code from the completion provider. Also it may need a different keyboard combo for Windows, I need to test that. But I'm going to mark this as ready for review so folks can test it as-is and I can refine it further.

@codebykat codebykat marked this pull request as ready for review October 30, 2020 06:19
@codebykat codebykat requested a review from a team October 30, 2020 06:19
@codebykat codebykat added this to the 2.1.0 milestone Oct 30, 2020
@codebykat
Copy link
Member

Oh I also tested passing in the typed text as a quoted phrase rather than splitting it up, but I like the current behavior (split by word) better personally.

lib/note-content-editor.tsx Outdated Show resolved Hide resolved
additionalTextEdits,
kind: note.isPinned
? languages.CompletionItemKind.Snippet
: languages.CompletionItemKind.File,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we all agree that this is right here? I put it in originally as I was exploring the API but I never had strong feelings about it, or if we should try and do anything better with the icons

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do something better with it in a follow-up 🤷

// detail: note.preview,
documentation: note.content,
insertText: `[${note.title}](simplenote://note/${note.noteId})`,
sortText: index.toString(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this property is used to sort the results. sorting by index should then reflect the sort order in the notes list, as that's how we proceed through notes in the search.

if we want any kind of alphabetical sort we could use note.title and if we wanted some relevance search we could perform a quick search distance calculation on the note title against the search query. that would put closer matches at the top

lib/note-content-editor.tsx Outdated Show resolved Hide resolved
lib/search/index.ts Outdated Show resolved Hide resolved
lib/search/index.ts Outdated Show resolved Hide resolved
@dmsnell
Copy link
Member Author

dmsnell commented Oct 30, 2020

looking good!

Copy link
Contributor

@belcherj belcherj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!!

@codebykat codebykat force-pushed the add/inter-note-link-searching branch from 3e21828 to 8a2ad51 Compare October 30, 2020 22:02
@codebykat
Copy link
Member

This could use some refinement and I'm not sure if the trigger key interaction is going to annoy people (pops up automatically when you type a [), but I have addressed the review comments and fixed the bugs I know about, so let's merge this and we can build on it!

@codebykat codebykat merged commit 1d70830 into develop Oct 31, 2020
@codebykat codebykat deleted the add/inter-note-link-searching branch October 31, 2020 01:02
export let searchNotes: (
args: Partial<SearchState>,
maxResults: number
) => [T.EntityId, T.Note | undefined][] = () => [];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this undefined jumps off the screen - why would we return results for notes that don't exist?

should we not be filtering out those notes before returning them? what is a caller supposed to do if they get a note id without a note?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@belcherj and I were of consensus that it would never return undefined, but TypeScript was unhappy, so we added this. Please do fix in a follow-up PR if you know the fix for it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants