Fix passage suggest to allow spaces. Closes #1176

[hituro]

Description

This change makes the passage suggestion menu (when typing a link) work correctly when typing a link or passage name with a space in it.

Issues Fixed

This change resolves issues: #1176 (and similar)

Credit

Please put an X in one of the squares below only.

[X] I would like to be credited in the application as: Hituro (David Donachie)
[ ] I would not like my name to appear in the application credits.

Presubmission Checklist

Put an X in the squares below to indicate you've done each step.

[X] I have read this project's CONTRIBUTING file and this PR follows the criteria laid out there.
[X] This contribution was created by me and I have the right to submit it under the GPL v3 license. (This is not a rights assignment.)
[X] I have read and agree to abide by this project's Code of Conduct.

[klembot]

It's been a long time since I've touched this code, but the CodeMirror docs suggest that we can use the closeCharacters option to accomplish this. I tried with this code in src/store/use-codemirror-passage-hints.ts:

export function useCodeMirrorPassageHints(story: Story) {
	return React.useCallback(
		(editor: Editor) => {
			editor.showHint({
				closeCharacters: /\]/,
				completeSingle: false,
// and the rest of it as-is

and this seems to work locally for me. But does your approach handle certain scenarios better? I only tried a basic example, but I did notice in your approach that other characters that are defaulted by closeCharacters, like ;, will close the autocomplete, and it seems like they shouldn't.

Only other thing I wonder about is if we go with closeCharacters is exactly what characters should close the hint. ] seems obvious to me, but maybe [ also.

[hituro]

Hi. My approach already uses closeCharacters and closes only on ]] so it allows [, but it looks for ->and|in the selection to see where the passage name should start, we could add[` there as well so that SC's fancy links work.

Most of my code is not about controlling when the suggestion menu closes, but about making sure that the selection doesn't break on a space due to the use of findWordAt(). findWordAt() only ever selects one word, so we have to use getSearchCursor() instead.

Just changing closeCharacters alone doesn't do it — I tried that first. Because the text selection methods need to be entirely different, which is what my pull request does.

If you just change closeCharacters without the rest of the code, then the menu appears after a space, but only suggests based on the final word, and only replaces the final word when you make a selection.

[klembot]

OK, I see. How about this as the hint() function then?

hint() {
  // Get the current cursor position and line content.

  const cursor = editor.getCursor();
  const line = editor.getLine(cursor.line);
  const from = {...cursor};
  const to = {...cursor};

  // Expand the range to the first `[` before the cursor. lastIndexOf() will
  // either give us -1, if there was no match, or the first bracket. In either
  // case, we want to add one so that it either points to the start of the line,
  // or the first character after the match. e.g. `[passage name` becomes
  // `passage name`.

  from.ch = line.lastIndexOf('[', from.ch) + 1;

  const candidate = line.substring(from.ch, to.ch).toLowerCase();
  const comps = {
    from,
    to,
    list: story.passages.reduce<string[]>((result, passage) => {
      if (passage.name.toLowerCase().includes(candidate)) {
        return [...result, passage.name];
      }

      return result;
    }, [])
  };

  CodeMirror.on(comps, 'pick', () => {
    const doc = editor.getDoc();

    doc.replaceRange(']] ', doc.getCursor());
  });

  return comps;
}

It would be nice to not have to take an additional dependency on CodeMirror because it'll complicate the transition to version 6.

Also, did you experiment any with unit tests? In trying this out locally, there seem to be many scenarios to watch out for here (typing links at the start, middle, and end of a paragraph) that it would be good to get test coverage on this.

[hituro]

I did a bunch of tests in various locations in the text, all of which were find, and I also made sure it passed the Jest tests, which I rewrote to mock the extra search methods.

I take it the extra dependency you were trying to avoid is the searchCursor? It's supplied by default in CodeMirror as is, but I haven't looked at v6 to see if that changes.

[klembot]

Yes--it's a separate library, I think, but that's partly because CM 6 is now a lot more modular.

Sorry, by unit tests, I meant Jest tests. it would be good to add tests around the behavior you've added in this PR. I can try to make a pass on that depending on your comfort level.

[hituro]

I'm not very knowledgable about Jest. I did make some changes to the existing test, extending the mocks and the like, but I'm not sure I would know how to modify it for the new functions

[klembot]

No worries. I’ll try that out soon and amend the PR. Thanks for getting the ball rolling on this!