Repository: obsidian-tasks-group/obsidian-tasks (ts)

Issue:
The 'sort by description' order is incorrect after stripping formatting
### Please check that this issue hasn't been reported before.

- [X] I searched previous [Bug Reports](https://github.com/obsidian-tasks-group/obsidian-tasks/issues?q=is%3Aopen+is%3Aissue+label%3A%22type%3A+bug%22+sort%3Acomments-desc) didn't find any similar reports.

### Expected Behavior

Given these tasks - in a file called `description sorting`:

```
- [ ] *un-format* me 4
- [ ] **un-format** me 1
- [ ] _un-format_ me 5
- [ ] [[un-format]] me 7
- [ ] [[un-format|un-format]] me 6
- [ ] [un-format] me 8
- [ ] =un-format= me 3
- [ ] ==un-format== me 2
```

And this tasks block:


```tasks
filename includes description sorting
sort by description
```

The documentation says that:

> Sorting by description should take into account [[Links]] and [Links with an|Alias] (note pipe). It should also take into account *italics* and ==highlights==. It sorts by the text that’s visible in preview mode.

And so I would expect the sorting to successfully strip out all formatting characters - so that the 'un-format' strings are all identical during sorting, and the tasks **should be displayed in the order of the digit at the end of their line.**

So the results should be in this order:

![image](https://user-images.githubusercontent.com/4840096/209588222-385736e5-c514-4544-a978-91af0f7fa22e.png)


#### Edit 2022-12-27

It turns out that `=un-format= me 3` was not valid formatting, so should not have been stripped.

However, `__bold__` should have been recognised.

### Current behaviour

The actual result is this:

![image](https://user-images.githubusercontent.com/4840096/209564985-eb864814-84c1-4f72-aa17-baa05ec2ebad.png)

The fact that the tasks are not sorted purely in the order their trailing digit shows that not all the the formatting characters have been removed.

### Steps to reproduce

See the tasks and query block in 'Expected behavior' above.

### Which Operating Systems are you using?

- [ ] Android
- [ ] iPhone/iPad
- [ ] Linux
- [X] macOS
- [ ] Windows

### Obsidian Version

1.1.9

### Tasks Plugin Version

1.20.0

### Checks

- [ ] I have tried it with all other plugins disabled and the error still occurs

### Possible solution

### Tests that reveal what's going on

I discovered this when writing tests of the current behaviour of sorting tasks by description. 

These tests capture the current behaviour:

https://github.com/obsidian-tasks-group/obsidian-tasks/blob/b20a44c863a93135ca32b80f4b7d522490b59ab5/tests/Query/Filter/DescriptionField.test.ts#L299-L343

There are obvious errors here like `**bold**` being converted to `bold**bold**` instead of to `bond`.

And stray `]` being left.

### Location of the issue

https://github.com/obsidian-tasks-group/obsidian-tasks/blob/b20a44c863a93135ca32b80f4b7d522490b59ab5/src/Query/Filter/DescriptionField.ts#L58-L87

Putting `{}` around the 3 blocks shows the error on the new line 79 - where the middle block has a remnant of the first block - see `startsWithLinkRegex` shown in red in this image:

![image](https://user-images.githubusercontent.com/4840096/209565461-8a6ade7a-8e81-424b-b153-4be69cac537a.png)




Pull request:
fix: Improve stripping of format chars in 'sort by description'
<!--- Provide a general summary of your changes in the Title above -->

# Description

- fix: Prevent 'sort by description' from duplicating text
    - This fixes the incorrect regex used when attempting to remove bold and italic formatting.
- fix: The removal of ** for bold no longer leaves a stray *
- fix: The removal of [[ for links no longer leaves a stray ]
- fix: Recognise `_italic_` and `__bold__` formatting
- fix: Don't allow formatting of empty strings

Also added multiple extra tests.

## Limitations

Note that this is not a perfect parsing of markdown.

For example with this:

`**hello * world**`

It would be nice for that to become:

`hello * world`

But it remains unchanged, because the search pattern does not allow any occurrences of the formatting character inside the formatted string.

## Motivation and Context

This fixes #1431.

## How has this been tested?

By adding new tests and running existing ones.

I will test manually once this PR has been built.

## Screenshots (if appropriate)

With these tasks:

```
- [ ] #task *un-format* me 4
- [ ] #task **un-format** me 1
- [ ] #task _un-format_ me 5
- [ ] #task __un-format__ me 3
- [ ] #task [[un-format]] me 7
- [ ] #task [[un-format|un-format]] me 6
- [ ] #task [un-format] me 8
- [ ] #task ==un-format== me 2
```

**Before**

![image](https://user-images.githubusercontent.com/4840096/209726136-a202aeba-951f-402d-92c9-724e9a3f5877.png)

**After**

![image](https://user-images.githubusercontent.com/4840096/209726220-63cf899e-99d9-4a9f-9208-65352b13fef5.png)

Note: After creating the screenshots, I realised that the `[un-format]` case was intended to be for things like `[un-format](https://github.com/)`. I'm not going to bother redoing them, but I confirmed that this case worked too.

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

Changes visible to users:

- [x] **Bug fix** (prefix: `fix` - non-breaking change which fixes an issue)

Internal changes:

- [x] **Refactor** (prefix: `refactor` - non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
- [x] **Tests** (prefix: `test` - additions and improvements to unit tests and the smoke tests)

## Checklist

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] My code follows the code style of this project and passes `yarn run lint`.
- [ ] My change requires a change to the documentation.
- [ ] I have [updated the documentation](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md#updating-documentation) accordingly.
- [x] My change has adequate [Unit Test coverage](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md#maintaining-the-tests).

## Terms

<!--
By submitting this pull request, you must agree to follow our
[contributing guide](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md) and
[Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md).
Put an x in the boxes to confirm you agree.
-->

- [x] My contribution follow this project's [contributing guide](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md)
- [x] I agree to follow this project's [Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md)


Patch:
diff --git a/src/Query/Filter/DescriptionField.ts b/src/Query/Filter/DescriptionField.ts
index 6e6933bb..bb6c1294 100644
--- a/src/Query/Filter/DescriptionField.ts
+++ b/src/Query/Filter/DescriptionField.ts
@@ -55,11 +55,11 @@ export class DescriptionField extends TextField {
      * Will remove them only if they are closing.
      * Properly reads links [[like this|one]] (note pipe).
      */
-    private static cleanDescription(description: string): string {
+    public static cleanDescription(description: string): string {
         const globalFilter = getSettings().globalFilter;
         description = description.replace(globalFilter, '').trim();
 
-        const startsWithLinkRegex = /^\[\[?([^\]]*)\]/;
+        const startsWithLinkRegex = /^\[\[?([^\]]*)]]?/;
         const linkRegexMatch = description.match(startsWithLinkRegex);
         if (linkRegexMatch !== null) {
             const innerLinkText = linkRegexMatch[1];
@@ -69,20 +69,26 @@ export class DescriptionField extends TextField {
                 innerLinkText.substring(innerLinkText.indexOf('|') + 1) + description.replace(startsWithLinkRegex, '');
         }
 
-        const startsWithItalicOrBoldRegex = /^\*\*?([^*]*)\*/;
-        const italicBoldRegexMatch = description.match(startsWithItalicOrBoldRegex);
+        description = this.replaceFormatting(description, /^\*\*([^*]+)\*\*/);
+        description = this.replaceFormatting(description, /^\*([^*]+)\*/);
+        description = this.replaceFormatting(description, /^==([^=]+)==/);
+        description = this.replaceFormatting(description, /^__([^_]+)__/);
+        description = this.replaceFormatting(description, /^_([^_]+)_/);
+
+        return description;
+    }
+
+    /**
+     * Remove some formatting from text
+     * @param description
+     * @param regExp A regular expression - all matching text is discarded except the first group
+     */
+    private static replaceFormatting(description: string, regExp: RegExp) {
+        const italicBoldRegexMatch = description.match(regExp);
         if (italicBoldRegexMatch !== null) {
             const innerItalicBoldText = italicBoldRegexMatch[1];
-            description = innerItalicBoldText + description.replace(startsWithLinkRegex, '');
-        }
-
-        const startsWithHighlightRegex = /^==?([^=]*)==/;
-        const highlightRegexMatch = description.match(startsWithHighlightRegex);
-        if (highlightRegexMatch !== null) {
-            const innerHighlightsText = highlightRegexMatch[1];
-            description = innerHighlightsText + description.replace(startsWithHighlightRegex, '');
+            description = innerItalicBoldText + description.replace(regExp, '');
         }
-
         return description;
     }
 }


Test patch:
diff --git a/tests/Query/Filter/DescriptionField.test.ts b/tests/Query/Filter/DescriptionField.test.ts
index 997c255c..4c5f3a08 100644
--- a/tests/Query/Filter/DescriptionField.test.ts
+++ b/tests/Query/Filter/DescriptionField.test.ts
@@ -300,21 +300,35 @@ describe('sorting by description', () => {
             );
         });
 
+        // All the strings should be treated as though they contain 'un-format'
+        it.each([
+            '*un-format*',
+            '**un-format**',
+            '_un-format_',
+            '__un-format__',
+            '[[un-format]]',
+            '[[some-other-file-name|un-format]]',
+            '[un-format]',
+            '==un-format==',
+        ])('simple description "%s" is cleaned to "un-format"', (originalDescription: string) => {
+            expect(DescriptionField.cleanDescription(originalDescription)).toStrictEqual('un-format');
+        });
+
         // Each of these pairs of strings is:
         // 1. A task description
         // 2. The result of running that description through the description-cleaning code.
         it.each([
             [
                 '[[Better be second]] most [] removed so these sort equal',
-                'Better be second] most [] removed so these sort equal',
+                'Better be second most [] removed so these sort equal',
             ],
             [
                 '[[Another|Third it should be]] alias is used from 1st link but not 2nd [last|ZZZ]',
-                'Third it should be] alias is used from 1st link but not 2nd [last|ZZZ]',
+                'Third it should be alias is used from 1st link but not 2nd [last|ZZZ]',
             ],
             [
-                '*Very italic text* - this looks completely wrong',
-                'Very italic text*Very italic text* - this looks completely wrong',
+                '*Very italic text*', // (comment to override formatting)
+                'Very italic text',
             ],
             [
                 '[@Zebra|Zebra] alias is used single []*', // (comment to override formatting)
@@ -331,13 +345,14 @@ describe('sorting by description', () => {
             ],
             [
                 '**bold** then ordinary text', // (comment to override formatting)
-                'bold**bold** then ordinary text',
+                'bold then ordinary text',
             ],
             [
                 '*italic* then ordinary text', // (comment to override formatting)
-                'italic*italic* then ordinary text',
+                'italic then ordinary text',
             ],
         ])('description "%s" is cleaned to "%s"', (originalDescription: string, cleanedDescription: string) => {
+            expect(DescriptionField.cleanDescription(originalDescription)).toStrictEqual(cleanedDescription);
             expectTaskComparesEqual(
                 sorter,
                 new TaskBuilder().description(originalDescription).build(),
@@ -346,6 +361,18 @@ describe('sorting by description', () => {
         });
     });
 
+    // All the strings are expected to be treated as unchanged.
+    // They typically have unbalanced numbers of formatting characters,
+    // or other behaviours not yet supported by Description.
+    it.each([
+        '**originalDescription* following text',
+        '__originalDescription_ following text',
+        '**hello * world** - formatting character inside formatting', // it would be nice for this to become 'hello * world'
+        '=un-format= single hyphen is not a valid formatting, so do not remove it',
+    ])('description "%s" is unchanged when cleaned"', (originalDescription: string) => {
+        expect(DescriptionField.cleanDescription(originalDescription)).toStrictEqual(originalDescription);
+    });
+
     it('sorts correctly by the link name and not the markdown', () => {
         const one = fromLine({
             line: '- [ ] *ZZZ An early task that starts with an A; actually not italic since only one asterisk',