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

Desktop: Resolves #9927: Beta editor: Fix search results not highlighted #9928

3 changes: 2 additions & 1 deletion .eslintignore
Expand Up @@ -251,7 +251,8 @@ packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/types.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useContextMenu.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useCursorUtils.test.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useCursorUtils.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useEditorSearch.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useEditorSearchExtension.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useEditorSearchHandler.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useExternalPlugins.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useJoplinCommands.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useJoplinMode.js
Expand Down
3 changes: 2 additions & 1 deletion .gitignore
Expand Up @@ -231,7 +231,8 @@ packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/types.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useContextMenu.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useCursorUtils.test.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useCursorUtils.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useEditorSearch.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useEditorSearchExtension.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useEditorSearchHandler.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useExternalPlugins.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useJoplinCommands.js
packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useJoplinMode.js
Expand Down
@@ -1,10 +1,14 @@
import { useEffect, useRef, useState } from 'react';
import shim from '@joplin/lib/shim';
import Logger from '@joplin/utils/Logger';
import CodeMirror5Emulation from '@joplin/editor/CodeMirror/CodeMirror5Emulation/CodeMirror5Emulation';

const logger = Logger.create('useEditorSearch');

export default function useEditorSearch(CodeMirror: any) {
// Registers a helper CodeMirror extension to be used with
// useEditorSearchHandler.

export default function useEditorSearchExtension(CodeMirror: CodeMirror5Emulation) {

const [markers, setMarkers] = useState([]);
const [overlay, setOverlay] = useState(null);
Expand Down Expand Up @@ -73,15 +77,15 @@ export default function useEditorSearch(CodeMirror: any) {
// If we run out of matches then just highlight the final match
break;
}
match = cursor.pos;
match = { from: cursor.from(), to: cursor.to() };
}

if (match) {
if (scrollTo) {
if (withSelection) {
cm.setSelection(match.from, match.to);
} else {
cm.scrollTo(match);
cm.scrollIntoView(match);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

According to the CodeMirror documentation, .scrollTo takes two arguments, while scrollIntoView can take an object with the same type as match.

As such, I suspect we were calling scrollTo incorrectly.

}
}
return cm.markText(match.from, match.to, { className: 'cm-search-marker-selected' });
Expand All @@ -107,7 +111,7 @@ export default function useEditorSearch(CodeMirror: any) {
};
}, []);

CodeMirror.defineExtension('setMarkers', function(keywords: any, options: any) {
CodeMirror?.defineExtension('setMarkers', function(keywords: any, options: any) {
if (!options) {
options = { selectedIndex: 0, searchTimestamp: 0 };
}
Expand Down Expand Up @@ -172,7 +176,7 @@ export default function useEditorSearch(CodeMirror: any) {
// These operations are pretty slow, so we won't add use them until the user
// has finished typing, 500ms is probably enough time
const timeout = shim.setTimeout(() => {
const scrollMarks = this.showMatchesOnScrollbar(searchTerm, true, 'cm-search-marker-scrollbar');
const scrollMarks = this.showMatchesOnScrollbar?.(searchTerm, true, 'cm-search-marker-scrollbar');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At present, showMatchesOnScrollbar is only implemented for CodeMirror 5. As per this discussion thread, we'll likely need to implement this ourselves.

const overlay = searchOverlay(searchTerm);
this.addOverlay(overlay);
setOverlay(overlay);
Expand Down
@@ -0,0 +1,66 @@
import { RefObject, useEffect } from 'react';
import usePrevious from '../../../../hooks/usePrevious';
import { RenderedBody } from './types';
const debounce = require('debounce');

interface Props {
setLocalSearchResultCount(count: number): void;
searchMarkers: any;
webviewRef: RefObject<any>;
editorRef: RefObject<any>;

noteContent: string;
renderedBody: RenderedBody;
}

const useEditorSearchHandler = (props: Props) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

useEditorSearchHandler was originally part of v5/CodeMirror.tsx.

const { webviewRef, editorRef, renderedBody, noteContent, searchMarkers } = props;

const previousContent = usePrevious(noteContent);
const previousRenderedBody = usePrevious(renderedBody);
const previousSearchMarkers = usePrevious(searchMarkers);

useEffect(() => {
if (!searchMarkers) return () => {};

// If there is a currently active search, it's important to re-search the text as the user
// types. However this is slow for performance so we ONLY want it to happen when there is
// a search

// Note that since the CodeMirror component also needs to handle the viewer pane, we need
// to check if the rendered body has changed too (it will be changed with a delay after
// props.content has been updated).
const textChanged = searchMarkers.keywords.length > 0 && (noteContent !== previousContent || renderedBody !== previousRenderedBody);

if (webviewRef.current && (searchMarkers !== previousSearchMarkers || textChanged)) {
webviewRef.current.send('setMarkers', searchMarkers.keywords, searchMarkers.options);

if (editorRef.current) {
// Fixes https://github.com/laurent22/joplin/issues/7565
const debouncedMarkers = debounce(() => {
const matches = editorRef.current.setMarkers(searchMarkers.keywords, searchMarkers.options);

props.setLocalSearchResultCount(matches);
}, 50);
debouncedMarkers();
return () => {
debouncedMarkers.clear();
};
}
}
return () => {};
}, [
editorRef,
webviewRef,
searchMarkers,
previousSearchMarkers,
props.setLocalSearchResultCount,
noteContent,
previousContent,
previousRenderedBody,
renderedBody,
]);

};

export default useEditorSearchHandler;
Expand Up @@ -6,7 +6,7 @@ import { EditorCommand, MarkupToHtmlOptions, NoteBodyEditorProps, NoteBodyEditor
import { commandAttachFileToBody, getResourcesFromPasteEvent } from '../../../utils/resourceHandling';
import { ScrollOptions, ScrollOptionTypes } from '../../../utils/types';
import { CommandValue } from '../../../utils/types';
import { usePrevious, cursorPositionToTextOffset } from '../utils';
import { cursorPositionToTextOffset } from '../utils';
import useScrollHandler from '../utils/useScrollHandler';
import useElementSize from '@joplin/lib/hooks/useElementSize';
import Toolbar from '../Toolbar';
Expand All @@ -25,13 +25,13 @@ import { ThemeAppearance } from '@joplin/lib/themes/type';
import dialogs from '../../../../dialogs';
import { MarkupToHtml } from '@joplin/renderer';
const { clipboard } = require('electron');
const debounce = require('debounce');

import { reg } from '@joplin/lib/registry';
import ErrorBoundary from '../../../../ErrorBoundary';
import useStyles from '../utils/useStyles';
import useContextMenu from '../utils/useContextMenu';
import useWebviewIpcMessage from '../utils/useWebviewIpcMessage';
import useEditorSearchHandler from '../utils/useEditorSearchHandler';

function markupRenderOptions(override: MarkupToHtmlOptions = null): MarkupToHtmlOptions {
return { ...override };
Expand All @@ -45,10 +45,6 @@ function CodeMirror(props: NoteBodyEditorProps, ref: ForwardedRef<NoteBodyEditor

const [webviewReady, setWebviewReady] = useState(false);

const previousContent = usePrevious(props.content);
const previousRenderedBody = usePrevious(renderedBody);
const previousSearchMarkers = usePrevious(props.searchMarkers);

const editorRef = useRef(null);
const rootRef = useRef(null);
const webviewRef = useRef(null);
Expand Down Expand Up @@ -675,37 +671,14 @@ function CodeMirror(props: NoteBodyEditorProps, ref: ForwardedRef<NoteBodyEditor
// eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Old code before rule was applied
}, [renderedBody, webviewReady]);

useEffect(() => {
if (!props.searchMarkers) return () => {};

// If there is a currently active search, it's important to re-search the text as the user
// types. However this is slow for performance so we ONLY want it to happen when there is
// a search

// Note that since the CodeMirror component also needs to handle the viewer pane, we need
// to check if the rendered body has changed too (it will be changed with a delay after
// props.content has been updated).
const textChanged = props.searchMarkers.keywords.length > 0 && (props.content !== previousContent || renderedBody !== previousRenderedBody);

if (webviewRef.current && (props.searchMarkers !== previousSearchMarkers || textChanged)) {
webviewRef.current.send('setMarkers', props.searchMarkers.keywords, props.searchMarkers.options);

if (editorRef.current) {
// Fixes https://github.com/laurent22/joplin/issues/7565
const debouncedMarkers = debounce(() => {
const matches = editorRef.current.setMarkers(props.searchMarkers.keywords, props.searchMarkers.options);

props.setLocalSearchResultCount(matches);
}, 50);
debouncedMarkers();
return () => {
debouncedMarkers.clear();
};
}
}
return () => {};
// eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Old code before rule was applied
}, [props.searchMarkers, previousSearchMarkers, props.setLocalSearchResultCount, props.content, previousContent, renderedBody, previousRenderedBody, renderedBody]);
useEditorSearchHandler({
setLocalSearchResultCount: props.setLocalSearchResultCount,
searchMarkers: props.searchMarkers,
webviewRef,
editorRef,
noteContent: props.content,
renderedBody,
});

const cellEditorStyle = useMemo(() => {
const output = { ...styles.cellEditor };
Expand Down
Expand Up @@ -16,7 +16,7 @@ import useListIdent from '../utils/useListIdent';
import useScrollUtils from '../utils/useScrollUtils';
import useCursorUtils from '../utils/useCursorUtils';
import useLineSorting from '../utils/useLineSorting';
import useEditorSearch from '../utils/useEditorSearch';
import useEditorSearch from '../utils/useEditorSearchExtension';
import useJoplinMode from '../utils/useJoplinMode';
import useKeymap from '../utils/useKeymap';
import useExternalPlugins from '../utils/useExternalPlugins';
Expand Down
Expand Up @@ -26,6 +26,7 @@ import CodeMirrorControl from '@joplin/editor/CodeMirror/CodeMirrorControl';
import useContextMenu from '../utils/useContextMenu';
import useWebviewIpcMessage from '../utils/useWebviewIpcMessage';
import Toolbar from '../Toolbar';
import useEditorSearchHandler from '../utils/useEditorSearchHandler';

const logger = Logger.create('CodeMirror6');
const logDebug = (message: string) => logger.debug(message);
Expand Down Expand Up @@ -338,6 +339,15 @@ const CodeMirror = (props: NoteBodyEditorProps, ref: ForwardedRef<NoteBodyEditor
// }
// }, [editorPaneVisible]);

useEditorSearchHandler({
setLocalSearchResultCount: props.setLocalSearchResultCount,
searchMarkers: props.searchMarkers,
webviewRef,
editorRef,
noteContent: props.content,
renderedBody,
});

useContextMenu({
plugins: props.plugins,
editorCutText, editorCopyText, editorPaste,
Expand Down
Expand Up @@ -10,6 +10,7 @@ import shim from '@joplin/lib/shim';
import PluginService from '@joplin/lib/services/plugins/PluginService';
import setupVim from '@joplin/editor/CodeMirror/util/setupVim';
import { dirname } from 'path';
import useEditorSearch from '../utils/useEditorSearchExtension';

interface Props extends EditorProps {
style: React.CSSProperties;
Expand All @@ -32,6 +33,8 @@ const Editor = (props: Props, ref: ForwardedRef<CodeMirrorControl>) => {
onLogMessageRef.current = props.onLogMessage;
}, [props.onEvent, props.onLogMessage]);

useEditorSearch(editor);

useEffect(() => {
if (!editor) {
return () => {};
Expand Down Expand Up @@ -104,6 +107,26 @@ const Editor = (props: Props, ref: ForwardedRef<CodeMirrorControl>) => {
// eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Should run just once
}, []);

const theme = props.settings.themeData;
useEffect(() => {
if (!editor) return () => {};

const styles = editor.addStyles({
'& .cm-search-marker *, & .cm-search-marker': {
color: theme.searchMarkerColor,
backgroundColor: theme.searchMarkerBackgroundColor,
},
'& .cm-search-marker-selected *, & .cm-search-marker-selected': {
background: `${theme.selectedColor2} !important`,
color: `${theme.color2} !important`,
},
});

return () => {
styles.remove();
};
}, [editor, theme]);

useEffect(() => {
editor?.updateSettings(props.settings);
}, [props.settings, editor]);
Expand Down
Expand Up @@ -108,4 +108,39 @@ describe('CodeMirror5Emulation', () => {
// additional times if its option hasn't updated.
expect(onOtherOptionUpdate).toHaveBeenCalledTimes(1);
});

it('markText decorations should be removable', () => {
const codeMirror = makeCodeMirrorEmulation('Test 1\nTest 2');

const markDecoration = codeMirror.markText(
{ line: 0, ch: 0 },
{ line: 0, ch: 6 },
{ className: 'test-mark-decoration' },
);

const markDecoration2 = codeMirror.markText(
{ line: 1, ch: 0 },
{ line: 1, ch: 1 },
{ className: 'test-decoration-2' },
);

const editorDom = codeMirror.cm6.dom;
expect(editorDom.querySelectorAll('.test-mark-decoration')).toHaveLength(1);
expect(editorDom.querySelectorAll('.test-decoration-2')).toHaveLength(1);

codeMirror.setCursor(0, 2);
codeMirror.replaceSelection('Test');

// Editing the document shouldn't remove the mark
expect(codeMirror.editor.state.doc.toString()).toBe('TeTestst 1\nTest 2');
expect(editorDom.querySelectorAll('.test-mark-decoration')).toHaveLength(1);

// Clearing should remove only the decoration that was cleared.
markDecoration.clear();
expect(editorDom.querySelectorAll('.test-mark-decoration')).toHaveLength(0);
expect(editorDom.querySelectorAll('.test-decoration-2')).toHaveLength(1);

markDecoration2.clear();
expect(editorDom.querySelectorAll('.test-decoration-2')).toHaveLength(0);
});
});