Skip to content

Commit

Permalink
Desktop: Fixes laurent22#10538: Fix wrong text selected when adding a…
Browse files Browse the repository at this point in the history
… link in the beta editor

This pull request migrates `wrapSelections` away from CodeMirror 5 APIs
and makes it instead reuse code written for the same purpose on mobile.
  • Loading branch information
personalizedrefrigerator committed Jun 4, 2024
1 parent f50a279 commit 967cd24
Show file tree
Hide file tree
Showing 7 changed files with 15 additions and 53 deletions.
4 changes: 2 additions & 2 deletions .eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -758,8 +758,6 @@ packages/editor/CodeMirror/markdown/markdownCommands.toggleList.test.js
packages/editor/CodeMirror/markdown/markdownCommands.js
packages/editor/CodeMirror/markdown/markdownMathParser.test.js
packages/editor/CodeMirror/markdown/markdownMathParser.js
packages/editor/CodeMirror/markdown/markdownReformatter.test.js
packages/editor/CodeMirror/markdown/markdownReformatter.js
packages/editor/CodeMirror/pluginApi/PluginLoader.js
packages/editor/CodeMirror/pluginApi/codeMirrorRequire.js
packages/editor/CodeMirror/pluginApi/customEditorCompletion.test.js
Expand All @@ -772,6 +770,8 @@ packages/editor/CodeMirror/testUtil/loadLanguages.js
packages/editor/CodeMirror/testUtil/pressReleaseKey.js
packages/editor/CodeMirror/testUtil/typeText.js
packages/editor/CodeMirror/theme.js
packages/editor/CodeMirror/util/editorStateUtils.test.js
packages/editor/CodeMirror/util/editorStateUtils.js
packages/editor/CodeMirror/util/isInSyntaxNode.js
packages/editor/CodeMirror/util/setupVim.js
packages/editor/SelectionFormatting.js
Expand Down
4 changes: 2 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -737,8 +737,6 @@ packages/editor/CodeMirror/markdown/markdownCommands.toggleList.test.js
packages/editor/CodeMirror/markdown/markdownCommands.js
packages/editor/CodeMirror/markdown/markdownMathParser.test.js
packages/editor/CodeMirror/markdown/markdownMathParser.js
packages/editor/CodeMirror/markdown/markdownReformatter.test.js
packages/editor/CodeMirror/markdown/markdownReformatter.js
packages/editor/CodeMirror/pluginApi/PluginLoader.js
packages/editor/CodeMirror/pluginApi/codeMirrorRequire.js
packages/editor/CodeMirror/pluginApi/customEditorCompletion.test.js
Expand All @@ -751,6 +749,8 @@ packages/editor/CodeMirror/testUtil/loadLanguages.js
packages/editor/CodeMirror/testUtil/pressReleaseKey.js
packages/editor/CodeMirror/testUtil/typeText.js
packages/editor/CodeMirror/theme.js
packages/editor/CodeMirror/util/editorStateUtils.test.js
packages/editor/CodeMirror/util/editorStateUtils.js
packages/editor/CodeMirror/util/isInSyntaxNode.js
packages/editor/CodeMirror/util/setupVim.js
packages/editor/SelectionFormatting.js
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,6 @@ import { focus } from '@joplin/lib/utils/focusHandler';

const logger = Logger.create('CodeMirror 6 commands');

const wrapSelectionWithStrings = (editor: CodeMirrorControl, string1: string, string2 = '', defaultText = '') => {
if (editor.somethingSelected()) {
editor.wrapSelections(string1, string2);
} else {
editor.wrapSelections(string1 + defaultText, string2);

// Now select the default text so the user can replace it
const selections = editor.listSelections();
const newSelections = [];
for (let i = 0; i < selections.length; i++) {
const s = selections[i];
const anchor = { line: s.anchor.line, ch: s.anchor.ch + string1.length };
const head = { line: s.head.line, ch: s.head.ch - string2.length };
newSelections.push({ anchor: anchor, head: head });
}
editor.setSelections(newSelections);
}
};

interface Props {
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
webviewRef: RefObject<any>;
Expand Down Expand Up @@ -92,7 +73,9 @@ const useEditorCommands = (props: Props) => {
textLink: async () => {
const url = await dialogs.prompt(_('Insert Hyperlink'));
focus('useEditorCommands::textLink', editorRef.current);
if (url) wrapSelectionWithStrings(editorRef.current, '[', `](${url})`);
if (url) {
editorRef.current.wrapSelections('[', `](${url})`);
}
},
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
insertText: (value: any) => editorRef.current.insertText(value),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { StreamParser } from '@codemirror/language';
import Decorator, { LineWidgetOptions, MarkTextOptions } from './Decorator';
import insertLineAfter from '../editorCommands/insertLineAfter';
import CodeMirror5BuiltInOptions from './CodeMirror5BuiltInOptions';
import { RegionSpec, toggleInlineFormatGlobally } from '../util/editorStateUtils';
const { pregQuote } = require('@joplin/lib/string-utils-common');


Expand Down Expand Up @@ -426,32 +427,10 @@ export default class CodeMirror5Emulation extends BaseCodeMirror5Emulation {
);
}

// TODO: Currently copied from useCursorUtils.ts.
// TODO: Remove the duplicate code when CodeMirror 5 is eventually removed.
public wrapSelections(string1: string, string2: string) {
const selectedStrings = this.getSelections();

// Batches the insert operations, if this wasn't done the inserts
// could potentially overwrite one another
this.operation(() => {
for (let i = 0; i < selectedStrings.length; i++) {
const selected = selectedStrings[i];

// Remove white space on either side of selection
const start = selected.search(/[^\s]/);
const end = selected.search(/[^\s](?=[\s]*$)/);
const core = selected.substring(start, end - start + 1);

// If selection can be toggled do that
if (core.startsWith(string1) && core.endsWith(string2)) {
const inside = core.substring(string1.length, core.length - string1.length - string2.length);
selectedStrings[i] = selected.substring(0, start) + inside + selected.substring(end + 1);
} else {
selectedStrings[i] = selected.substring(0, start) + string1 + core + string2 + selected.substring(end + 1);
}
}
this.replaceSelections(selectedStrings);
});
public wrapSelections(start: string, end: string) {
this.editor.dispatch(
toggleInlineFormatGlobally(this.editor.state, RegionSpec.of({ template: { start, end } })),
);
}

public static commands = (() => {
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/CodeMirror/markdown/markdownCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
RegionSpec, growSelectionToNode, renumberSelectedLists,
toggleInlineFormatGlobally, toggleRegionFormatGlobally, toggleSelectedLinesStartWith,
isIndentationEquivalent, stripBlockquote, tabsToSpaces,
} from './markdownReformatter';
} from '../util/editorStateUtils';
import intersectsSyntaxNode from '../util/isInSyntaxNode';

const startingSpaceRegex = /^(\s*)/;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {
findInlineMatch, MatchSide, RegionSpec, renumberSelectedLists, tabsToSpaces, toggleRegionFormatGlobally,
} from './markdownReformatter';
} from './editorStateUtils';
import { Text as DocumentText, EditorSelection, EditorState } from '@codemirror/state';
import { indentUnit } from '@codemirror/language';
import createTestEditor from '../testUtil/createTestEditor';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ const toggleInlineRegionSurrounded = (

changes.push({
from: sel.to,
insert: spec.template.start,
insert: spec.template.end,
});

// If not a caret,
Expand Down

2 comments on commit 967cd24

@personalizedrefrigerator
Copy link
Owner Author

@personalizedrefrigerator personalizedrefrigerator commented on 967cd24 Jun 4, 2024

Choose a reason for hiding this comment

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

Most of markdownReformatter isn't specific to markdown. As such, this commit renames it to editorStateUtils and uses one of its utility functions to implement wrapSelections.

This commit isn't ready for a pull request yet. The following needs to be done:

  • Move markdown-related utilities out of editorStateUtils — probably into their own separate files in markdown/utils.
    • It might be best to do this in a separate pull request.
  • Add more automated tests.
    • A bug was fixed in editorStateUtils that needs a regression test.
    • Tests should be added for wrapSelections.

@personalizedrefrigerator
Copy link
Owner Author

Choose a reason for hiding this comment

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

Note that the issue does not seem to be a recent regression — I can reproduce it in v2.14.22.

Please sign in to comment.