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

Issue:
Multiple 'sort by tag' lines ignore all but last one
### 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

With the following instructions:

````text
```tasks
sort by tag 2
sort by tag 1
sort by description
```
````

I expect that the results will show:

- all tasks sorted by tag 2, initially
- any tags that have the same tag 2 value will then be sorted by tag 1
- and then any tags that have the same tag 2 and tag 1 will be sorted by description



### Current behaviour

With the above query, the tasks are actually sorted like this:

- all tasks sorted by tag 1, initially
- and then any tags that have the same tag 1 are sorted by description

In other words, tag 2 is ignored in the sort, even though it is supposed to be the highest priority.



### Steps to reproduce

1. Create these tasks

```text
- [ ] random 2 #a #a
- [ ] random 9 #a #b
- [ ] random 4 #a #c

- [ ] random 7 #b #a
- [ ] random 1 #b #b
- [ ] random 5 #b #c

- [ ] random 3 #c #a
- [ ] random 8 #c #b
- [ ] random 6 #c #c
```

2. Create this tasks block:

````text
```tasks
path includes [whatever the file name or path is of the file containing the test tasks]
sort by tag 2
sort by tag 1
sort by description

hide edit button
hide backlink

```
````

Actual output: Tags are sorted by tag 1 and then description

```markdown
- [ ] random 2 #a #a
- [ ] random 4 #a #c
- [ ] random 9 #a #b
- [ ] random 1 #b #b
- [ ] random 5 #b #c
- [ ] random 7 #b #a
- [ ] random 3 #c #a
- [ ] random 6 #c #c
- [ ] random 8 #c #b
9 tasks
```

Expected output: Tasks are sorted by tag 2 and then tag 1.


```markdown
- [ ] random 2 #a #a
- [ ] random 7 #b #a
- [ ] random 3 #c #a
- [ ] random 9 #a #b
- [ ] random 1 #b #b
- [ ] random 8 #c #b
- [ ] random 4 #a #c
- [ ] random 5 #b #c
- [ ] random 6 #c #c
9 tasks
```


### Which Operating Systems are you using?

- [ ] Android
- [ ] iPhone/iPad
- [ ] Linux
- [X] macOS
- [ ] Windows

### Obsidian Version

1.1.6

### Tasks Plugin Version

1.19.0

### Checks

- [ ] I have tried it with all other plugins disabled and the error still occurs

### Possible solution

The fundamental problem is the single global value `Sort.tagPropertyInstance`.

This is used in `Sort.by()` like this:

```ts
    public static by(query: Pick<Query, 'sorting'>, tasks: Task[]): Task[] {
        ...

        const userComparators: Comparator[] = [];

        for (const sorting of query.sorting) {
            userComparators.push(sorting.comparator);
            if (sorting.property === 'tag') {
                Sort.tagPropertyInstance = sorting.propertyInstance;
            }
        }

        ...
    }
```

Note the line `Sort.tagPropertyInstance = sorting.propertyInstance;` - the last sorting line wins, and forces its `propertyInstance` for all the sort instructions. And then the eventual code that does the sorting by tag - which in 1.20.0 and earlier was in `Sort` - then uses that last tag number, rather than the tag number on the specific `sort by tag` line.


Pull request description:
fix: Multiple 'sort by tag' lines ignore all but last one
<!--- Provide a general summary of your changes in the Title above -->

# Description

- Added test which proved the bug existed
- Each `sort by tag` comparator stores its own tag number
    - instead of them all using a single global tag number - which was stored in `Sort.tagPropertyInstance`
- Removed the `propertyInstance` field from `Sorting`, as no longer used


## Motivation and Context

This fixes #1407
"Multiple 'sort by tag' lines ignore all but last one"

## How has this been tested?

- By adding a failing test and then showing that it now gives the right answer
- Checking the existing tests still pass

## Screenshots (if appropriate)

## 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] **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/Field.ts b/src/Query/Filter/Field.ts
index ab699efc..c338269f 100644
--- a/src/Query/Filter/Field.ts
+++ b/src/Query/Filter/Field.ts
@@ -101,7 +101,7 @@ export abstract class Field {
      * @param reverse - false for normal sort order, true for reverse sort order.
      */
     public createSorter(reverse: boolean): Sorting {
-        return new Sorting(reverse, 1, this.fieldName(), this.comparator());
+        return new Sorting(reverse, this.fieldName(), this.comparator());
     }
 
     /**
diff --git a/src/Query/Sort.ts b/src/Query/Sort.ts
index a3f6814e..36b8b281 100644
--- a/src/Query/Sort.ts
+++ b/src/Query/Sort.ts
@@ -10,8 +10,6 @@ import { PathField } from './Filter/PathField';
 import { UrgencyField } from './Filter/UrgencyField';
 
 export class Sort {
-    static tagPropertyInstance: number = 1;
-
     public static by(query: Pick<Query, 'sorting'>, tasks: Task[]): Task[] {
         // TODO Move code for creating default comparators to separate file
         const defaultComparators: Comparator[] = [
@@ -26,18 +24,11 @@ export class Sort {
 
         for (const sorting of query.sorting) {
             userComparators.push(sorting.comparator);
-            if (sorting.property === 'tag') {
-                Sort.tagPropertyInstance = sorting.propertyInstance;
-            }
         }
 
         return tasks.sort(Sort.makeCompositeComparator([...userComparators, ...defaultComparators]));
     }
 
-    public static comparators: Record<SortingProperty, Comparator> = {
-        tag: Sort.compareByTag,
-    };
-
     /**
      * Legacy function, for creating a Sorting object for a SortingProperty type.
      *
@@ -49,8 +40,8 @@ export class Sort {
      *                   one of the values in ${@link SortingProperty}.
      */
     public static makeLegacySorting(reverse: boolean, propertyInstance: number, property: SortingProperty): Sorting {
-        const comparator = Sort.makeLegacyComparator(property);
-        return new Sorting(reverse, propertyInstance, property, comparator);
+        const comparator = Sort.makeLegacyComparator(property, propertyInstance);
+        return new Sorting(reverse, property, comparator);
     }
 
     /**
@@ -60,13 +51,14 @@ export class Sort {
      * @param property - the name of the property. This string must match
      *                   one of the values in ${@link SortingProperty}.
      *                   Throws if property not recognised.
+     * @param propertyInstance - for tag sorting, this is a 1-based index for the tag number in the task to sort by.
+     *                           TODO eventually, move this number to the comparator used for sorting by tag.
      */
-    public static makeLegacyComparator(property: SortingProperty): Comparator {
-        const comparator = Sort.comparators[property];
-        if (!comparator) {
+    public static makeLegacyComparator(property: SortingProperty, propertyInstance: number): Comparator {
+        if (property !== 'tag') {
             throw Error('Unrecognised legacy sort keyword: ' + property);
         }
-        return comparator;
+        return Sort.makeCompareByTagComparator(propertyInstance);
     }
 
     private static makeCompositeComparator(comparators: Comparator[]): Comparator {
@@ -81,35 +73,37 @@ export class Sort {
         };
     }
 
-    private static compareByTag(a: Task, b: Task): -1 | 0 | 1 {
-        // If no tags then assume they are equal.
-        if (a.tags.length === 0 && b.tags.length === 0) {
-            return 0;
-        } else if (a.tags.length === 0) {
-            // a is less than b
-            return 1;
-        } else if (b.tags.length === 0) {
-            // b is less than a
-            return -1;
-        }
+    private static makeCompareByTagComparator(propertyInstance: number): Comparator {
+        return (a: Task, b: Task) => {
+            // If no tags then assume they are equal.
+            if (a.tags.length === 0 && b.tags.length === 0) {
+                return 0;
+            } else if (a.tags.length === 0) {
+                // a is less than b
+                return 1;
+            } else if (b.tags.length === 0) {
+                // b is less than a
+                return -1;
+            }
 
-        // Arrays start at 0 but the users specify a tag starting at 1.
-        const tagInstanceToSortBy = Sort.tagPropertyInstance - 1;
+            // Arrays start at 0 but the users specify a tag starting at 1.
+            const tagInstanceToSortBy = propertyInstance - 1;
 
-        if (a.tags.length < Sort.tagPropertyInstance && b.tags.length >= Sort.tagPropertyInstance) {
-            return 1;
-        } else if (b.tags.length < Sort.tagPropertyInstance && a.tags.length >= Sort.tagPropertyInstance) {
-            return -1;
-        } else if (a.tags.length < Sort.tagPropertyInstance && b.tags.length < Sort.tagPropertyInstance) {
-            return 0;
-        }
+            if (a.tags.length < propertyInstance && b.tags.length >= propertyInstance) {
+                return 1;
+            } else if (b.tags.length < propertyInstance && a.tags.length >= propertyInstance) {
+                return -1;
+            } else if (a.tags.length < propertyInstance && b.tags.length < propertyInstance) {
+                return 0;
+            }
 
-        if (a.tags[tagInstanceToSortBy] < b.tags[tagInstanceToSortBy]) {
-            return -1;
-        } else if (a.tags[tagInstanceToSortBy] > b.tags[tagInstanceToSortBy]) {
-            return 1;
-        } else {
-            return 0;
-        }
+            if (a.tags[tagInstanceToSortBy] < b.tags[tagInstanceToSortBy]) {
+                return -1;
+            } else if (a.tags[tagInstanceToSortBy] > b.tags[tagInstanceToSortBy]) {
+                return 1;
+            } else {
+                return 0;
+            }
+        };
     }
 }
diff --git a/src/Query/Sorting.ts b/src/Query/Sorting.ts
index 3cdbbde6..18ce6539 100644
--- a/src/Query/Sorting.ts
+++ b/src/Query/Sorting.ts
@@ -18,7 +18,6 @@ export type Comparator = (a: Task, b: Task) => number;
 export class Sorting {
     public readonly property: string;
     public readonly comparator: Comparator;
-    public readonly propertyInstance: number;
 
     /**
      * Constructor.
@@ -26,15 +25,12 @@ export class Sorting {
      * TODO Once SortingProperty has been removed, re-order the parameters so the comparator comes first.
      *
      * @param reverse - whether the sort order should be reversed.
-     * @param propertyInstance - for tag sorting, this is a 1-based index for the tag number in the task to sort by.
-     *                           TODO eventually, move this number to the comparator used for sorting by tag.
      * @param property - the name of the property. If {@link comparator} is not supplied, this string must match
      *                   one of the values in ${@link SortingProperty}.
      * @param comparator - {@link Comparator} function.
      */
-    constructor(reverse: boolean, propertyInstance: number, property: string, comparator: Comparator) {
+    constructor(reverse: boolean, property: string, comparator: Comparator) {
         this.property = property;
-        this.propertyInstance = propertyInstance;
         this.comparator = Sorting.maybeReverse(reverse, comparator);
     }
 


Test patch:
diff --git a/tests/Query/Filter/TagsField.test.ts b/tests/Query/Filter/TagsField.test.ts
index 234aa81b..f82e0dad 100644
--- a/tests/Query/Filter/TagsField.test.ts
+++ b/tests/Query/Filter/TagsField.test.ts
@@ -3,6 +3,7 @@ import type { FilteringCase } from '../../TestingTools/FilterTestHelpers';
 import { shouldSupportFiltering } from '../../TestingTools/FilterTestHelpers';
 import { toMatchTaskFromLine } from '../../CustomMatchers/CustomMatchersForFilters';
 import { TagsField } from '../../../src/Query/Filter/TagsField';
+import { DescriptionField } from '../../../src/Query/Filter/DescriptionField';
 import { fromLine } from '../../TestHelpers';
 import { Sort } from '../../../src/Query/Sort';
 
@@ -486,4 +487,42 @@ describe('Sort by tags', () => {
         // Cleanup
         resetSettings();
     });
+
+    // Issue #1407 - Multiple 'sort by tag' lines ignored all but last one
+    it('should sort correctly with two sort by tag lines', () => {
+        const input = [
+            fromLine({ line: '- [ ] random 2 #a #a 0' }),
+            fromLine({ line: '- [ ] random 9 #a #b 1' }),
+            fromLine({ line: '- [ ] random 4 #a #c 2' }),
+            fromLine({ line: '- [ ] random 7 #b #a 3' }),
+            fromLine({ line: '- [ ] random 1 #b #b 4' }),
+            fromLine({ line: '- [ ] random 5 #b #c 5' }),
+            fromLine({ line: '- [ ] random 3 #c #a 6' }),
+            fromLine({ line: '- [ ] random 8 #c #b 7' }),
+            fromLine({ line: '- [ ] random 6 #c #c 8' }),
+        ];
+        const correctExpectedOrder = [
+            input[0], // 3 values with tag 2 = '#a' - then sorted by tag 1
+            input[3],
+            input[6],
+            input[1], // 3 values with tag 2 = '#b' - then sorted by tag 1
+            input[4],
+            input[7],
+            input[2], // 3 values with tag 2 = '#c' - then sorted by tag 1
+            input[5],
+            input[8],
+        ];
+        expect(
+            Sort.by(
+                {
+                    sorting: [
+                        Sort.makeLegacySorting(false, 2, 'tag'), // tag 2 - ascending
+                        Sort.makeLegacySorting(false, 1, 'tag'), // tag 1 - ascending
+                        new DescriptionField().createNormalSorter(), // then description - ascending
+                    ],
+                },
+                input,
+            ),
+        ).toEqual(correctExpectedOrder);
+    });
 });
diff --git a/tests/Sort.test.ts b/tests/Sort.test.ts
index 8544a514..e2e4a3a5 100644
--- a/tests/Sort.test.ts
+++ b/tests/Sort.test.ts
@@ -38,7 +38,7 @@ describe('Sort', () => {
 
         // Normal way round
         {
-            const sortByDescriptionLength = new Sorting(false, 1, 'junk', comparator);
+            const sortByDescriptionLength = new Sorting(false, 'junk', comparator);
             expect(sortByDescriptionLength.comparator(short, long)).toEqual(1);
             expect(sortByDescriptionLength.comparator(short, short)).toEqual(0);
             expect(sortByDescriptionLength.comparator(long, short)).toEqual(-1);
@@ -46,7 +46,7 @@ describe('Sort', () => {
 
         // Reversed
         {
-            const sortByDescriptionLength = new Sorting(true, 1, 'junk', comparator);
+            const sortByDescriptionLength = new Sorting(true, 'junk', comparator);
             expect(sortByDescriptionLength.comparator(short, long)).toEqual(-1);
             expect(sortByDescriptionLength.comparator(short, short)).toEqual(-0);
             expect(sortByDescriptionLength.comparator(long, short)).toEqual(1);