Skip to content

Commit b3bd4e0

Browse files
personalizedrefrigeratorpersonalizedrefrigerator
committed
Desktop: Resolves #13399: Partially fix search highlighting by using editor-specific state
Fixes an issue where old search highlighting would persist when a new query was set. However, with this change, moving focus to a secondary window results in nothing being highlighted. Moving focus back to the primary window restores the highlights. The remaining issue may be related to window-specific state that should be global (in app.reducer.ts or in reducer.ts).
1 parent 1cacc3f commit b3bd4e0

2 files changed

Lines changed: 175 additions & 114 deletions

File tree

‎packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useEditorSearchExtension.ts‎

Lines changed: 154 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,139 @@ const logger = Logger.create('useEditorSearch');
88
// Registers a helper CodeMirror extension to be used with
99
// useEditorSearchHandler.
1010

11+
type Mark = { clear: ()=> void };
12+
interface SearchHighlightState {
13+
previousKeywordValue: string;
14+
previousIndex: number;
15+
previousSearchTimestamp: number;
16+
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Partial refactor of old code before rule was applied
17+
overlayTimeoutRef: { current: any };
18+
clearMarkers(): void;
19+
clearOverlay(editor: CodeMirror5Emulation): void;
20+
getSearchTerm(keyword: string): RegExp;
21+
highlightSearch(cm: CodeMirror5Emulation, searchTerm: RegExp, index: number, scrollTo: boolean, withSelection: boolean): Mark;
22+
setMarkers(marker: Mark[]): void;
23+
setPreviousIndex(index: number): void;
24+
setPreviousSearchTimestamp(timestamp: number): void;
25+
setPreviousKeywordValue(value: string): void;
26+
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Partial refactor of old code before rule was applied
27+
setScrollbarMarks(marks: any): void;
28+
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Partial refactor of old code before rule was applied
29+
setOverlay(overlay: any): void;
30+
setOverlayTimeout(timeout: number): void;
31+
}
32+
33+
34+
// Modified from codemirror/addons/search/search.js
35+
const searchOverlay = (query: RegExp) => {
36+
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
37+
return { token: function(stream: any) {
38+
query.lastIndex = stream.pos;
39+
const match = query.exec(stream.string);
40+
if (match && match.index === stream.pos) {
41+
stream.pos += match[0].length || 1;
42+
return 'search-marker';
43+
} else if (match) {
44+
stream.pos = match.index;
45+
} else {
46+
stream.skipToEnd();
47+
}
48+
return null;
49+
} };
50+
};
51+
52+
const addCodeMirrorExtension = (CodeMirror: CodeMirror5Emulation) => {
53+
CodeMirror.defineOption('joplin.search-highlight-state', null, ()=>{});
54+
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
55+
CodeMirror?.defineExtension('setMarkers', function(keywords: any, options: any) {
56+
// Pass arguments in via options to allow the extension to work if multiple editors are open simultaneously
57+
// See https://github.com/laurent22/joplin/issues/13399.
58+
const state: SearchHighlightState = this.getOption('joplin.search-highlight-state');
59+
if (!options) {
60+
options = { selectedIndex: 0, searchTimestamp: 0 };
61+
}
62+
63+
if (options.showEditorMarkers === false) {
64+
state.clearMarkers();
65+
state.clearOverlay(this);
66+
return;
67+
}
68+
69+
state.clearMarkers();
70+
71+
// HIGHLIGHT KEYWORDS
72+
// When doing a global search it's possible to have multiple keywords
73+
// This means we need to highlight each one
74+
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
75+
const marks: any = [];
76+
for (let i = 0; i < keywords.length; i++) {
77+
const keyword = keywords[i];
78+
79+
if (keyword.value === '') continue;
80+
81+
const searchTerm = state.getSearchTerm(keyword);
82+
83+
// We only want to scroll the first keyword into view in the case of a multi keyword search
84+
const scrollTo = i === 0 && (state.previousKeywordValue !== keyword.value || state.previousIndex !== options.selectedIndex || options.searchTimestamp !== state.previousSearchTimestamp);
85+
86+
try {
87+
const match = state.highlightSearch(this, searchTerm, options.selectedIndex, scrollTo, !!options.withSelection);
88+
if (match) marks.push(match);
89+
} catch (error) {
90+
if (error.name !== 'SyntaxError') {
91+
throw error;
92+
}
93+
// An error of 'Regular expression too large' might occur in the markJs library
94+
// when the input is really big, this catch is here to avoid the application crashing
95+
// https://github.com/laurent22/joplin/issues/7634
96+
console.error('Error while trying to highlight words from search: ', error);
97+
}
98+
}
99+
100+
state.setMarkers(marks);
101+
state.setPreviousIndex(options.selectedIndex);
102+
state.setPreviousSearchTimestamp(options.searchTimestamp);
103+
104+
// SEARCHOVERLAY
105+
// We only want to highlight all matches when there is only 1 search term
106+
if (keywords.length !== 1 || keywords[0].value === '') {
107+
state.clearOverlay(this);
108+
const prev = keywords.length > 1 ? keywords[0].value : '';
109+
state.setPreviousKeywordValue(prev);
110+
return 0;
111+
}
112+
113+
const searchTerm = state.getSearchTerm(keywords[0]);
114+
115+
// Determine the number of matches in the source, this is passed on
116+
// to the NoteEditor component
117+
const regexMatches = this.getValue().match(searchTerm);
118+
const nMatches = regexMatches ? regexMatches.length : 0;
119+
120+
// Don't bother clearing and re-calculating the overlay if the search term
121+
// hasn't changed
122+
if (keywords[0].value === state.previousKeywordValue) return nMatches;
123+
124+
state.clearOverlay(this);
125+
state.setPreviousKeywordValue(keywords[0].value);
126+
127+
// These operations are pretty slow, so we won't add use them until the user
128+
// has finished typing, 500ms is probably enough time
129+
const timeout = shim.setTimeout(() => {
130+
const scrollMarks = this.showMatchesOnScrollbar?.(searchTerm, true, 'cm-search-marker-scrollbar');
131+
const overlay = searchOverlay(searchTerm);
132+
this.addOverlay(overlay);
133+
state.setOverlay(overlay);
134+
state.setScrollbarMarks(scrollMarks);
135+
}, 500);
136+
137+
state.setOverlayTimeout(timeout);
138+
state.overlayTimeoutRef.current = timeout;
139+
140+
return nMatches;
141+
});
142+
};
143+
11144
export default function useEditorSearchExtension(CodeMirror: CodeMirror5Emulation) {
12145

13146
const [markers, setMarkers] = useState([]);
@@ -48,23 +181,6 @@ export default function useEditorSearchExtension(CodeMirror: CodeMirror5Emulatio
48181
setOverlayTimeout(null);
49182
}, [scrollbarMarks, overlay, overlayTimeout]);
50183

51-
// Modified from codemirror/addons/search/search.js
52-
const searchOverlay = useCallback((query: RegExp) => {
53-
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
54-
return { token: function(stream: any) {
55-
query.lastIndex = stream.pos;
56-
const match = query.exec(stream.string);
57-
if (match && match.index === stream.pos) {
58-
stream.pos += match[0].length || 1;
59-
return 'search-marker';
60-
} else if (match) {
61-
stream.pos = match.index;
62-
} else {
63-
stream.skipToEnd();
64-
}
65-
return null;
66-
} };
67-
}, []);
68184

69185
// Highlights the currently active found work
70186
// It's possible to get tricky with this functions and just use findNext/findPrev
@@ -115,89 +231,25 @@ export default function useEditorSearchExtension(CodeMirror: CodeMirror5Emulatio
115231
};
116232
}, []);
117233

118-
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
119-
CodeMirror?.defineExtension('setMarkers', function(keywords: any, options: any) {
120-
if (!options) {
121-
options = { selectedIndex: 0, searchTimestamp: 0 };
122-
}
123-
124-
if (options.showEditorMarkers === false) {
125-
clearMarkers();
126-
clearOverlay(this);
127-
return;
128-
}
129-
130-
clearMarkers();
131-
132-
// HIGHLIGHT KEYWORDS
133-
// When doing a global search it's possible to have multiple keywords
134-
// This means we need to highlight each one
135-
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
136-
const marks: any = [];
137-
for (let i = 0; i < keywords.length; i++) {
138-
const keyword = keywords[i];
139-
140-
if (keyword.value === '') continue;
141-
142-
const searchTerm = getSearchTerm(keyword);
143-
144-
// We only want to scroll the first keyword into view in the case of a multi keyword search
145-
const scrollTo = i === 0 && (previousKeywordValue !== keyword.value || previousIndex !== options.selectedIndex || options.searchTimestamp !== previousSearchTimestamp);
146-
147-
try {
148-
const match = highlightSearch(this, searchTerm, options.selectedIndex, scrollTo, !!options.withSelection);
149-
if (match) marks.push(match);
150-
} catch (error) {
151-
if (error.name !== 'SyntaxError') {
152-
throw error;
153-
}
154-
// An error of 'Regular expression too large' might occur in the markJs library
155-
// when the input is really big, this catch is here to avoid the application crashing
156-
// https://github.com/laurent22/joplin/issues/7634
157-
console.error('Error while trying to highlight words from search: ', error);
158-
}
159-
}
160-
161-
setMarkers(marks);
162-
setPreviousIndex(options.selectedIndex);
163-
setPreviousSearchTimestamp(options.searchTimestamp);
164-
165-
// SEARCHOVERLAY
166-
// We only want to highlight all matches when there is only 1 search term
167-
if (keywords.length !== 1 || keywords[0].value === '') {
168-
clearOverlay(this);
169-
const prev = keywords.length > 1 ? keywords[0].value : '';
170-
setPreviousKeywordValue(prev);
171-
return 0;
172-
}
173-
174-
const searchTerm = getSearchTerm(keywords[0]);
175-
176-
// Determine the number of matches in the source, this is passed on
177-
// to the NoteEditor component
178-
const regexMatches = this.getValue().match(searchTerm);
179-
const nMatches = regexMatches ? regexMatches.length : 0;
180-
181-
// Don't bother clearing and re-calculating the overlay if the search term
182-
// hasn't changed
183-
if (keywords[0].value === previousKeywordValue) return nMatches;
184-
185-
clearOverlay(this);
186-
setPreviousKeywordValue(keywords[0].value);
187-
188-
// These operations are pretty slow, so we won't add use them until the user
189-
// has finished typing, 500ms is probably enough time
190-
const timeout = shim.setTimeout(() => {
191-
const scrollMarks = this.showMatchesOnScrollbar?.(searchTerm, true, 'cm-search-marker-scrollbar');
192-
const overlay = searchOverlay(searchTerm);
193-
this.addOverlay(overlay);
194-
setOverlay(overlay);
195-
setScrollbarMarks(scrollMarks);
196-
}, 500);
197-
198-
setOverlayTimeout(timeout);
199-
overlayTimeoutRef.current = timeout;
200-
201-
return nMatches;
202-
});
234+
if (CodeMirror) {
235+
const state: SearchHighlightState = {
236+
previousKeywordValue,
237+
previousIndex,
238+
previousSearchTimestamp,
239+
overlayTimeoutRef,
240+
clearMarkers,
241+
clearOverlay,
242+
getSearchTerm,
243+
highlightSearch,
244+
setMarkers,
245+
setPreviousIndex,
246+
setPreviousSearchTimestamp,
247+
setPreviousKeywordValue,
248+
setScrollbarMarks,
249+
setOverlay,
250+
setOverlayTimeout,
251+
};
252+
addCodeMirrorExtension(CodeMirror);
253+
CodeMirror.setOption('joplin.search-highlight-state', state);
254+
}
203255
}

‎packages/editor/CodeMirror/CodeMirror5Emulation/Decorator.ts‎

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,14 @@ interface CssDecorationSpec extends DecorationRange {
2929
id?: number;
3030
}
3131

32+
interface RemoveMarkDecorationSpec {
33+
id: number;
34+
}
35+
3236
const addLineDecorationEffect = StateEffect.define<CssDecorationSpec>(mapRangeConfig);
3337
const removeLineDecorationEffect = StateEffect.define<CssDecorationSpec>(mapRangeConfig);
3438
const addMarkDecorationEffect = StateEffect.define<CssDecorationSpec>(mapRangeConfig);
35-
const removeMarkDecorationEffect = StateEffect.define<CssDecorationSpec>(mapRangeConfig);
39+
const removeMarkDecorationEffect = StateEffect.define<RemoveMarkDecorationSpec>();
3640
const refreshOverlaysEffect = StateEffect.define();
3741

3842
export interface LineWidgetOptions {
@@ -190,25 +194,29 @@ export default class Decorator {
190194
decorations = decorations.update({
191195
add: [decoration.range(from, to)],
192196
});
193-
} else if (effect.is(removeLineDecorationEffect) || effect.is(removeMarkDecorationEffect)) {
197+
} else if (effect.is(removeLineDecorationEffect)) {
194198
const doc = transaction.state.doc;
195-
const targetFrom = doc.lineAt(effect.value.from).from;
196-
const targetTo = doc.lineAt(effect.value.to).to;
199+
const { from, to } = effect.value;
200+
// Handle the case where { from, to } point to an outdated document
201+
const targetFrom = doc.lineAt(from).from;
202+
const targetTo = doc.lineAt(to).to;
197203

198-
const targetId = effect.value.id;
199204
const targetDecoration = this.classNameToCssDecoration(
200205
effect.value.cssClass, effect.is(removeLineDecorationEffect),
201206
);
202207

203208
decorations = decorations.update({
204209
// Returns true only for decorations that should be kept.
205210
filter: (from, to, value) => {
206-
if (targetId !== undefined) {
207-
return value.spec.id !== effect.value.id;
208-
}
209-
210211
const isInRange = from >= targetFrom && to <= targetTo;
211-
return isInRange && value.eq(targetDecoration);
212+
return !isInRange || !value.eq(targetDecoration);
213+
},
214+
});
215+
} else if (effect.is(removeMarkDecorationEffect)) {
216+
decorations = decorations.update({
217+
// Returns true only for decorations that should be kept.
218+
filter: (_from, _to, value) => {
219+
return value.spec.id !== effect.value.id;
212220
},
213221
});
214222
} else if (effect.is(addLineWidgetEffect)) {
@@ -384,9 +392,10 @@ export default class Decorator {
384392
}
385393

386394
public markText(from: number, to: number, options?: MarkTextOptions) {
395+
const id = this._nextLineWidgetId++;
387396
const effectOptions: CssDecorationSpec = {
388397
cssClass: options.className ?? '',
389-
id: this._nextLineWidgetId++,
398+
id,
390399
from,
391400
to,
392401
};
@@ -398,7 +407,7 @@ export default class Decorator {
398407
return {
399408
clear: () => {
400409
this.editor.dispatch({
401-
effects: removeMarkDecorationEffect.of(effectOptions),
410+
effects: removeMarkDecorationEffect.of({ id }),
402411
});
403412
},
404413
};

0 commit comments

Comments
 (0)