refactor: Simplify BacklinkField & add test for #1927 (#1949)

* test: Add test to show that #1927 is already fixed

It was fixed in f3c6a41.

* test: Fix fromLine() test function behaviour when no heading

The actual behaviour of the Tasks plugin when there is no available
preceding heading is to use null, not an empty string.

This makes the test helper fromLine() in TestHelpers.ts behave the same way.

All tests still pass.

(TestBuilder already used null as the default heading, so no extra work was
needed there.)

* test: Add full test coverage for Task backlinks code

* test: BacklinkField.test.ts heading types now match those in Task

Task allows heading to be string or null, not string or undefined.

* test: BacklinkField.test.ts path type now matches that in Task

Task only allows path to be string, not null or undefined.

* refactor: Simplify BacklinkField.value()

Now the test types for BacklinkField match the types used in
Task, it turns out we can use Task.getLinkText() directly
and all tests pass.

* refactor: Simplify BacklinkField.grouper()

This avoids doing substring calculations, which makes the code
a bit easier to reason about.

* test: Fix names of tests - heading was accidentally labelled path

* test: Format test code to match the start of this branch

To make the pull request easier to review.

* comment: Clarify descriptive comments in grouper()

* comment: Clarify some comments

Author: claremacrae

src/Query/Filter/BacklinkField.ts

@@ -1,7 +1,6 @@
 import type { Task } from '../../Task';
 import type { GrouperFunction } from '../Grouper';
 import { TextField } from './TextField';
-import { HeadingField } from './HeadingField';
 import { FilterOrErrorMessage } from './Filter';
 
 export class BacklinkField extends TextField {
@@ -14,26 +13,7 @@ export class BacklinkField extends TextField {
         if (linkText === null) {
             return 'Unknown Location';
         }
-
-        let filenameComponent = 'Unknown Location';
-
-        if (task.filename !== null) {
-            filenameComponent = task.filename;
-        }
-
-        if (task.precedingHeader === null || task.precedingHeader.length === 0) {
-            return filenameComponent;
-        }
-
-        // Markdown characters in the heading must NOT be escaped.
-        const headingGrouper = new HeadingField().createGrouper().grouper;
-        const headingComponent = headingGrouper(task)[0];
-
-        if (filenameComponent === headingComponent) {
-            return filenameComponent;
-        } else {
-            return `${filenameComponent} > ${headingComponent}`;
-        }
+        return linkText;
     }
 
     createFilterOrErrorMessage(line: string): FilterOrErrorMessage {
@@ -51,15 +31,19 @@ export class BacklinkField extends TextField {
     public grouper(): GrouperFunction {
         return (task: Task) => {
             const filename = task.filename;
-            if (filename !== null) {
-                const backlink = this.value(task);
-                if (filename !== backlink) {
-                    // In case backlink is 'file_name > heading', the filename only shall be escaped
-                    return [TextField.escapeMarkdownCharacters(filename) + backlink.substring(filename.length)];
-                }
+            if (filename === null) {
+                return ['Unknown Location'];
             }
 
-            return [TextField.escapeMarkdownCharacters(this.value(task))];
+            // Always escape the filename, to prevent any underscores being interpreted as markdown:
+            let result = TextField.escapeMarkdownCharacters(filename);
+
+            // Only append the heading if it differs from the un-escaped fileanme:
+            if (task.precedingHeader && task.precedingHeader !== filename) {
+                // We don't escape the heading, as any markdown characters in it really are markdown styling:
+                result += ' > ' + task.precedingHeader;
+            }
+            return [result];
         };
     }
 }

tests/Query/Filter/BacklinkField.test.ts

@@ -35,25 +35,28 @@ describe('grouping by backlink', () => {
 
     it.each([
         // no location supplied
-        [undefined, 'heading', ['Unknown Location']],
+        ['', 'heading', ['Unknown Location']],
 
         // no heading supplied
-        ['a/b/c.md', undefined, ['c']],
+        ['a/b/c.md', null, ['c']],
 
         // File and heading, nominal case
         ['a/b/c.md', 'heading', ['c > heading']],
 
         // If file name and heading are identical, avoid duplication ('c > c')
         ['a/b/c.md', 'c', ['c']],
 
+        // If file name and heading are identical, avoid duplication, even if there are underscores in the file name
+        ['a_b_c.md', 'a_b_c', ['a\\_b\\_c']],
+
         // Underscores in file name component are escaped
-        ['a/b/_c_.md', undefined, ['\\_c\\_']],
+        ['a/b/_c_.md', null, ['\\_c\\_']],
 
         // But underscores in the heading component are not
         ['a/b/_c_.md', 'heading _italic text_', ['\\_c\\_ > heading _italic text_']],
     ])(
-        'task "%s" with path "%s" should have groups: %s',
-        (path: string | undefined, heading: string | undefined, groups: string[]) => {
+        'path "%s" and heading "%s" should have groups: %s',
+        (path: string, heading: string | null, groups: string[]) => {
             // Arrange
             const grouper = new BacklinkField().createGrouper().grouper;
             const t = '- [ ] xyz';

tests/Task.test.ts

@@ -414,6 +414,82 @@ describe('parsing tags', () => {
     );
 });
 
+describe('backlinks', () => {
+    function shouldGiveLinkText(
+        path: string,
+        heading: string | null,
+        filenameUnique: boolean,
+        expected: string | null,
+    ) {
+        expect(
+            new TaskBuilder()
+                .path(path)
+                .precedingHeader(heading)
+                .build()
+                .getLinkText({ isFilenameUnique: filenameUnique }),
+        ).toEqual(expected);
+    }
+
+    describe('valid and unique paths', () => {
+        it('valid, unique path without heading should use filename as backlink', () => {
+            shouldGiveLinkText('a/b/c.md', null, true, 'c');
+        });
+
+        it('valid, unique path with different heading should use filename and heading as backlink', () => {
+            shouldGiveLinkText('a/b/c.md', 'heading', true, 'c > heading');
+        });
+
+        it('valid, unique path with same heading should use just filename as backlink', () => {
+            shouldGiveLinkText('a/b/c.md', 'c', true, 'c');
+        });
+    });
+
+    describe('valid non-unique paths', () => {
+        it('valid, non-unique path without heading should use full path as backlink', () => {
+            shouldGiveLinkText('a/b/c.md', null, false, '/a/b/c.md');
+        });
+
+        it('valid, non-unique path with different heading should use full path and heading as backlink', () => {
+            shouldGiveLinkText('a/b/c.md', 'heading', false, '/a/b/c.md > heading');
+        });
+
+        it('valid, non-unique path with same heading as file name should use full path and heading as backlink', () => {
+            shouldGiveLinkText('a/b/c.md', 'c', false, '/a/b/c.md > c');
+        });
+
+        it('valid, non-unique path with same heading as path name with initial slash should use just the path as backlink', () => {
+            // This is not a realistic use-case, but is included to show the current behaviour of the code
+            shouldGiveLinkText('a/b/c.md', '/a/b/c.md', false, '/a/b/c.md');
+        });
+    });
+
+    describe('invalid unique paths', () => {
+        // use invalid file extension to generate null filename
+        // This is not a realistic use-case, but is included to show the current behaviour of the code
+
+        it('invalid, unique path without heading should give null for backlink', () => {
+            shouldGiveLinkText('a/b/c.markdown', null, true, null);
+        });
+
+        it('invalid, unique path with heading should give null for backlink', () => {
+            shouldGiveLinkText('a/b/c.markdown', 'heading', true, null);
+        });
+    });
+
+    describe('invalid non-unique paths', () => {
+        // use invalid file extension to generate null filename
+        // This is not a realistic use-case, but is included to show the current behaviour of the code
+
+        it('invalid, non-unique path without heading uses path anyway for backlink', () => {
+            shouldGiveLinkText('a/b/c.markdown', null, false, '/a/b/c.markdown');
+        });
+
+        it('invalid, non-unique path with heading uses path and backlink anyway for backlink', () => {
+            shouldGiveLinkText('a/b/c.markdown', 'heading', false, '/a/b/c.markdown > heading');
+        });
+    });
+});
+
 describe('to string', () => {
     it('retains the indentation', () => {
         const line = '> > > - [ ] Task inside a nested blockquote or callout';

tests/TestHelpers.ts

@@ -4,7 +4,7 @@ import { TaskLocation } from '../src/TaskLocation';
 export function fromLine({
     line,
     path = '',
-    precedingHeader = '',
+    precedingHeader = null,
 }: {
     line: string;
     path?: string;