feat: change names of groups from Priority Field and change the sort of order groups same as task sorting order #1966

ilandikov · 2023-05-21T10:54:02Z

Description

change the naming of groups for the Priority field
change the sort of order of grouping for all fields to sort by task sorting order instead of by group name alphabetical order

Motivation and Context

Fixes #1628 . Priority groups are now displayed without numbers: High, Medium, None, Low, but are sorted in the same order and not in alphabetical order (High, Low, Medium, None).

How has this been tested?

Screenshots (if appropriate)

Types of changes

Changes visible to users:

Bug fix (prefix: fix - non-breaking change which fixes an issue)
New feature (prefix: feat - non-breaking change which adds functionality)
Breaking change (prefix: feat!! or fix!! - fix or feature that would cause existing functionality to not work as expected)
Documentation (prefix: docs - improvements to any documentation content for users)
Sample vault (prefix: vault - improvements to the Tasks-Demo sample vault)
Contributing Guidelines (prefix: contrib - any improvements to documentation content for contributors - see Contributing to Tasks)

Internal changes:

Refactor (prefix: refactor - non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
Tests (prefix: test - additions and improvements to unit tests and the smoke tests)
Infrastructure (prefix: chore - examples include GitHub Actions, issue templates)

Checklist

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 accordingly.
My change has adequate Unit Test coverage.

Terms

My contribution follow this project's contributing guide
I agree to follow this project's Code of Conduct

claremacrae · 2023-05-21T20:45:11Z

How does this relate to PR #1943?
I mean, I think there will be conflicts whichever order this one and that one are merged.

claremacrae · 2023-05-21T20:48:33Z

From the PR description it's not obvious if this is talking about renaming the priorities themselves. Something more specific would be really useful.

claremacrae · 2023-05-21T20:49:31Z

How does this relate to PR #1943?
I mean, I think there will be conflicts whichever order this one and that one are merged.

If this one is going to go ahead, please do update the user docs, so that a complete PR can be reviewed. Many thanks.

ilandikov · 2023-05-22T05:29:24Z

How does this relate to PR #1943?
I mean, I think there will be conflicts whichever order this one and that one are merged.

On the paper it looks like some adjustment would be needed in PriorityField.ts, but the merge seems to be pretty trivial there. However it is the tests that should have the final word here.

From the PR description it's not obvious if this is talking about renaming the priorities themselves. Something more specific would be really useful.

Indeed! Fixed.

If this one is going to go ahead, please do update the user docs, so that a complete PR can be reviewed. Many thanks.

Yup, after I changed the name of the PR, the update of the docs was self evident, thanks =)

claremacrae · 2023-05-22T06:03:45Z

src/Query/Filter/Field.ts

@@ -360,7 +360,7 @@ export abstract class Field {
     * @param reverse - false for normal group order, true for reverse group order.
     */
    public createGrouper(reverse: boolean): Grouper {
-        return new Grouper(this.fieldNameSingular(), this.grouper(), reverse);
+        return new Grouper(this.fieldNameSingular(), this.grouper(), reverse, this.comparator());


I thing this will change (improve) the sort order for urgency grouping.

Which will just require confirmation and then a change the docs of group by urgency please.

I checked the docs of urgency here and here but didn't find it related to the changes in this PR (Priority group names...).

Do you mean that now sorting of groups from urgency field can use this code? I agree with that, but I guess this should be a different PR.

It’s this text:

Currently, the groups run from the lowest urgency to highest.

You can reverse this with group by urgency reverse.

In a future release, the default group order will become from the highest urgency to lowest.

I am reasonably confident that this PR has done point 3 and the above text should be updated.

didn't find it related to the changes in this PR (Priority group names...).

To be more specific, this PR does several things:

change the naming of groups for the Priority field

change the sort of order of grouping for all fields to sort by task sorting order instead of by group name alphabetical order

You are right! I wrote a test and tried it before and after all the changes and it does change the behaviour indeed. Test and docs.

Now I'm thinking that I should've probably split the PR in 2 - one for refactoring with addition of the comparator to the grouper and the other one to fix the Priority group names. May be a third for the Urgency =)

Lmk if you wish to have a separate issue for the reversed urgency order =)

claremacrae · 2023-05-22T07:33:41Z

src/Query/Filter/MultiTextField.ts

@@ -64,7 +64,7 @@ export abstract class MultiTextField extends TextField {
     * This overloads {@link Field.createGrouper} to put a plural field name in the {@link Grouper.property}.
     */
    public createGrouper(reverse: boolean): Grouper {
-        return new Grouper(this.fieldNamePlural(), this.grouper(), reverse);
+        return new Grouper(this.fieldNamePlural(), this.grouper(), reverse, this.comparator());


What happens with this code if the field does not implement sorting?

I expect that calling this.comparator() will then throw some kind of exception.

If you look at the Quick Reference table you will find a few such cases - such as recurrence or recurring (I forget which).

And certainly the new group by function will create a grouper that does not sort.

So... you could write a failing test using recurrence, but if it is taught to sort in future, that test will become useless.

So the testing trick here is to write a new test and have it use a simple implementation for Field that does grouping but not sorting. Perhaps DescriptionLengthGroupingfield. A field that is written purely to make testing easy and self-explanatory.

Then line 67 can be changed to only use this.comparator() if supportsSorting() is true, and if not, fall back on the old behaviour of sorting by group name.

What happens with this code if the field does not implement sorting?

I expect that calling this.comparator() will then throw some kind of exception.

Very good point, thanks.

So the testing trick here is to write a new test and have it use a simple implementation for Field that does grouping but not sorting. Perhaps DescriptionLengthGroupingfield. A field that is written purely to make testing easy and self-explanatory.

Yup, I did that now, this seems to be a good trade-off.

Then line 67 can be changed to only use this.comparator() if supportsSorting() is true, and if not, fall back on the old behaviour of sorting by group name.

On Field level I don't have access to the group names so I can't compare them in the fall back case. I'm not really sure how to solve this.

Ok, I think I got this, but not sure whether it is a good solution. Something smells here, can't tell what exactly. Probably the whole direction of the development ㅠㅠ

claremacrae · 2023-05-24T06:59:05Z

Lmk if you wish to have a separate issue for the reversed urgency order =)

I think what this PR has shown is that:

When group by was first implemented, all the groups were sorted by name. The test requirements were therefore:
- A very few tests to confirm that groups were sorted correctly by name - this could be done in (IIRC) Group.test.ts
- One or two tests of the group names that were generated for each field.
Now that per-field sorting of groups is done, there is an obvious gap in the tests:
- We need a test for each field to show the order in which the group names are sorted...

So I currently see these steps:

PR 1 On the current code, add an expressive test for every field, showing current headings order

It's important that we find a more expressive way to test the group headings order than the current very verbose group testing.

For this PR, this is the sort of thing I mean by expressive (typed from memory without an IDE)

expectGroupHeadingsToBe(
    new PriorityField().grouper(),
    shuffle(SampleTasks.withAllPriorities()),
    [
        'Priority 1: High',
        'Priority 2: Medium',
        'Priority 3: None',
        'Priority 4: Low',
    ]
    );

I am thinking that SampleTasks would have static methods that each return an array of Task objects with a representative set of all possible values for a given property.

Examples would be:

SampleTasks.withAllPriorities() would be straightforward
SampleTasks.withAllDueDateTypes() could return tasks with each of:
- A valid date - today
- A valid date - in the past
- A valid date - in the future
- An invalid date
- No date
- (Maybe SampleTasks.withRepresentativeDueDates() is a better name)

expectGroupHeadingsToBe() could be based on ideas in expectTaskComparesBefore()

The shuffle() call is a nice-to-have, to randomise the order of tasks, so that the tests do not pass by luck

PR 2 Implement the new group sorting mechanism

Do not change the names of any groups, but do use the new sorting approach that you are now using in the current PR...

The new tests will confirm whether I am correct - but I think you will find that only the urgency order will be changed - and it is an improvement.

If I am correct that only urgency changes, then this can actually be labeled as bug-fix, to group most urgent tasks first... and just update the urgency grouping docs.

PR 3 Rename the priority groups

Now the priority group names can be changed easily, and the docs updated, as a small PR.

ilandikov · 2023-05-24T07:57:12Z

Very clear, thanks a lot! I close this PR since we need to split it.

claremacrae · 2023-05-24T10:22:08Z

In my example test above I called grouper() but it maybe should have been createNormalGrouper() or something…

ilandikov · 2023-06-04T08:39:19Z

PR 1 On the current code, add an expressive test for every field, showing current headings order

It's important that we find a more expressive way to test the group headings order than the current very verbose group testing.

test: #1999

PR 2 Implement the new group sorting mechanism

Do not change the names of any groups, but do use the new sorting approach that you are now using in the current PR...

refactor: #2018

PR 3 Rename the priority groups

Now the priority group names can be changed easily, and the docs updated, as a small PR.

claremacrae · 2023-06-04T09:09:52Z

PR 1 On the current code, add an expressive test for every field, showing current headings order

It's important that we find a more expressive way to test the group headings order than the current very verbose group testing.

test: #1999

Yeah, sorry I rushed you to create that PR before tests were added for every field.

claremacrae · 2023-06-04T09:19:10Z

Yeah, sorry I rushed you to create that PR before tests were added for every field.

Also, the 'expressive' bit is still important 😄

ilandikov added 14 commits May 20, 2023 16:00

test: document current group sorting of PriorityField

d7fc0dc

refactor: move localeCompare to groupSorter

1acce88

refactor: move groupSorter to Field

1e7e92c

refactor: rename sorter to comparator

edca28f

refactor: move groupComparator() to the bottom

3f1cb45

refactor: custom groupComparator() in PriorityField

5e827e4

refactor: add Priority.toNumber()

e9355f2

feat: make Priority group names shorter

d0a078f

refactor: rename types

090c195

fix: toNumber() default priority is 3

f61c819

refactor: remove temp variable

1d29d1d

refactor: shorter import type

bf597ea

test: fix test description

fefd7b2

refactor: compare first tasks instead of group names

3180630

claremacrae added scope: grouping Changes to the grouping capabilities status: user docs need work Needs at least some additions, updates or corrections to user documentation labels May 21, 2023

docs: update group names in docs

11c12be

ilandikov changed the title ~~feat: more readable names in priority~~ feat: remove 'Priority X:' from Priority groups May 22, 2023

claremacrae requested changes May 22, 2023

View reviewed changes

claremacrae reviewed May 22, 2023

View reviewed changes

ilandikov added 5 commits May 22, 2023 14:51

test: test priority group sorting

b7f9f20

docs: document updated urgency grouping behaviour

67a5baf

refactor: add DescriptionLengthGroupingfield for testing

65d132c

fix: add defalt group comparator if Field does not support sorting

8c2adc3

test: test default group comparator

75e6cf7

ilandikov changed the title ~~feat: remove 'Priority X:' from Priority groups~~ feat: change names of groups from Priority Field and change the sort of order groups same as task sorting order May 22, 2023

test: fix date to fix urgency calculation

a5c389c

ilandikov closed this May 24, 2023

claremacrae removed the status: user docs need work Needs at least some additions, updates or corrections to user documentation label May 26, 2023

ilandikov mentioned this pull request Jun 4, 2023

refactor: move default comparator to Grouper #2018

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: change names of groups from Priority Field and change the sort of order groups same as task sorting order #1966

feat: change names of groups from Priority Field and change the sort of order groups same as task sorting order #1966

ilandikov commented May 21, 2023 •

edited

Loading

claremacrae commented May 21, 2023

claremacrae commented May 21, 2023

claremacrae commented May 21, 2023

ilandikov commented May 22, 2023 •

edited

Loading

claremacrae May 22, 2023

ilandikov May 22, 2023

claremacrae May 22, 2023

claremacrae May 22, 2023

ilandikov May 22, 2023

ilandikov May 22, 2023

claremacrae May 22, 2023 •

edited

Loading

ilandikov May 22, 2023

ilandikov May 22, 2023

claremacrae commented May 24, 2023

ilandikov commented May 24, 2023

claremacrae commented May 24, 2023

ilandikov commented Jun 4, 2023 •

edited

Loading

PR 1 On the current code, add an expressive test for every field, showing current headings order

PR 2 Implement the new group sorting mechanism

PR 3 Rename the priority groups

claremacrae commented Jun 4, 2023

PR 1 On the current code, add an expressive test for every field, showing current headings order

claremacrae commented Jun 4, 2023

feat: change names of groups from Priority Field and change the sort of order groups same as task sorting order #1966

feat: change names of groups from Priority Field and change the sort of order groups same as task sorting order #1966

Conversation

ilandikov commented May 21, 2023 • edited Loading

Description

Motivation and Context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Terms

claremacrae commented May 21, 2023

claremacrae commented May 21, 2023

claremacrae commented May 21, 2023

ilandikov commented May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claremacrae May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claremacrae commented May 24, 2023

PR 1 On the current code, add an expressive test for every field, showing current headings order

PR 2 Implement the new group sorting mechanism

PR 3 Rename the priority groups

ilandikov commented May 24, 2023

claremacrae commented May 24, 2023

ilandikov commented Jun 4, 2023 • edited Loading

PR 1 On the current code, add an expressive test for every field, showing current headings order

PR 2 Implement the new group sorting mechanism

PR 3 Rename the priority groups

claremacrae commented Jun 4, 2023

PR 1 On the current code, add an expressive test for every field, showing current headings order

claremacrae commented Jun 4, 2023

ilandikov commented May 21, 2023 •

edited

Loading

ilandikov commented May 22, 2023 •

edited

Loading

claremacrae May 22, 2023 •

edited

Loading

ilandikov commented Jun 4, 2023 •

edited

Loading