Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add new priorities: highest and lowest #1943

Merged
merged 12 commits into from
May 29, 2023

Conversation

chrabia
Copy link
Contributor

@chrabia chrabia commented May 9, 2023

Description

Two new priorities are added for tasks: "highest" and "lowest"

Motivation and Context

There are too few priorities to choose from, this PR introduces two new ones, which probably should be enough. The names for new priorities were inspired by the names used in jira, but some new names could be introduced for easier suggesting when creating the task. But I'm not sure about what those could be, "asap" seems nice for the "highest", but I don't have a good alternative for the "lowest".
I'm not sure what numbers should really be used for urgency calculation, they might need to be changed a bit.
Also, I'm not really sure about the emoji used for "highest" priority, other propositions for that: ❕or 🔥

How has this been tested?

The change is small, so mostly older unit tests are adapted. But also, a few new tests are added. The change was tested on my local Obsidian too, by creating a few tasks with new priorities and checking that everything works and e.g. the tasks are sorted correctly.

Screenshots (if appropriate)

image

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

Terms

@claremacrae
Copy link
Collaborator

Hi, thank you for the suggestion.

It would have have been helpful to us both to have had a chance to discuss the ideas in this before you did so much work.

There are too few priorities to choose from

Could you please explain your reasoning behind the above statement?

I do not recall any previous requests for more priority levels.

@claremacrae
Copy link
Collaborator

Some of the tests have failed, but I recommend not investigating them whilst the principles of the change are still being considered.

@chrabia
Copy link
Contributor Author

chrabia commented May 9, 2023

oh, yeah, seems that the tests for Suggestor needs to be fixed too. I can do that, but as you said, let's discuss everything first.

Could you please explain your reasoning behind the above statement?
I do not recall any previous requests for more priority levels.

Well then, maybe I'm first? As I'm developer myself, I thought that instead of issuing a request for a feature, I can help 🙂 and implement it myself.
But, now that I'm looking through discussions and and issues, that's not true.
E.g. this particular user created a workaround for priorities, and a higher number of them too in particular:
#1366 (comment)
Also, when reading the initial issue for introducing priorities
#6
I see that everyone there (even the developer who implemented the priorities feature!) were suggesting to have more priorities, in particular the "highest" and "lowest" are mentioned a few times.

So it seems it's not only me, but other users would like this feature too.

Another thing worth considering is that this change has a rather small impact and it's not introducing a lot of complexity to the plugin, so it shouldn't be a problem to add this.

@claremacrae
Copy link
Collaborator

Well then, maybe I'm first? As I'm developer myself, I thought that instead of issuing a request for a feature, I can help 🙂 and implement it myself.

I appreciate that.

As the following in the Tasks Contributing Guide says, it's not mandatory to discuss in advance, but it does help...

https://publish.obsidian.md/tasks-contributing/Contributing/Updating+code

Thank you for the links.

Neither of the examples is a request for more priorities, though - and it's interesting to me that in #6, this plugin's author, even though having mentioned highest and lowest, went ahead and implemented without them.

I read #1366 (comment) as mainly about the visual display of priorities, not the number of them.

I'm still keen to understand, though, what are your specific reasons for needing more priorities?

In the meantime, I'll go ahead and make some initial comments on the changes in the code.

@claremacrae claremacrae self-requested a review May 10, 2023 04:27
@claremacrae
Copy link
Collaborator

The names for new priorities were inspired by the names used in jira,

OK I'll take the fact that these new names already exist in Jira as good enough motivation!!!

Copy link
Collaborator

@claremacrae claremacrae left a comment

Choose a reason for hiding this comment

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

I had a read-through of the code, and tried the new code out - sorting, grouping, searching, auto-suggest, dataview format.

It worked well in all the areas changed.

As well as the changes already made, there are quite a few documentation references that will need updating please. But I suggest holding off doing that until the code is finalised.

src/Task.ts Outdated Show resolved Hide resolved
src/Task.ts Outdated Show resolved Hide resolved
src/Urgency.ts Outdated Show resolved Hide resolved
src/TaskSerializer/DefaultTaskSerializer.ts Outdated Show resolved Hide resolved
tests/Query/Filter/PriorityField.test.ts Outdated Show resolved Hide resolved
@claremacrae claremacrae added the scope: emojis and signifiers Anything to do with emojis and signifiers label May 10, 2023
@chrabia
Copy link
Contributor Author

chrabia commented May 12, 2023

I'm still keen to understand, though, what are your specific reasons for needing more priorities?

Well, I started lately moving from other task management apps to Obsidian, I really like it and this plugin in particular too. But, I have a lot of tasks, and priority feature helps me to, well, prioritize what should I be doing in which order. These tasks are usually not really time-related things, so assigning due date doesn't really make sense. But, when you have only 4 levels of priority there is not really enough granularity. Especially, considering that I'm a bit reluctant of setting many tasks to "high" at the moment, as the highest possible priority is something that makes you consider a task to be done ASAP. And there can't be many things which you can really do ASAP. So that leaves me with 3 priorities really for ~100 tasks.

@claremacrae
Copy link
Collaborator

...These tasks are usually not really time-related things, so assigning due date doesn't really make sense. But, when you have only 4 levels of priority there is not really enough granularity. ...

Perfect. I had not thought of priorities like this, and this explanation really helps! Many thanks.

@claremacrae
Copy link
Collaborator

Whilst waiting until I can review the changes, it would be really helpful if you could have a first go at finding all the references to Priority in the documentation and updating them where appropriate, please.

Edit the docs/ directory as a vault within Obsidian.

@chrabia
Copy link
Contributor Author

chrabia commented May 23, 2023

Whilst waiting until I can review the changes, it would be really helpful if you could have a first go at finding all the references to Priority in the documentation and updating them where appropriate, please.

Edit the docs/ directory as a vault within Obsidian.

Found some time for that - done :).
I wasn't sure what to do with the Styling section, so I left images as they were and added purple color for highest and green for lowest, should be fine I guess.

Copy link
Collaborator

@claremacrae claremacrae left a comment

Choose a reason for hiding this comment

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

This is looking good.

I should be able to actually test it all out in a few days' time, but in the meantime I read through the changes.

docs/Advanced/Styling.md Show resolved Hide resolved
docs/Quick Reference.md Outdated Show resolved Hide resolved
docs/Quick Reference.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@claremacrae claremacrae left a comment

Choose a reason for hiding this comment

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

Thank you again for this.

I've now been able to test it out, and am sure I will make use of the new priorities.

Here are a few comments based on testing.

src/ui/EditTask.svelte Show resolved Hide resolved
src/ui/EditTask.svelte Outdated Show resolved Hide resolved
styles.css Show resolved Hide resolved
@claremacrae
Copy link
Collaborator

Hi, thanks very much for the extra commits... ❤️

I'll try them out as soon as I can, and resolve conversations as I go...

@claremacrae claremacrae changed the title feat: added new priorities: highest and lowest feat: Add new priorities: highest and lowest May 29, 2023
@claremacrae
Copy link
Collaborator

claremacrae commented May 29, 2023

Reminder to self, for after merge:

Copy link
Collaborator

@claremacrae claremacrae left a comment

Choose a reason for hiding this comment

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

Great stuff - thank you very much indeed!

@claremacrae claremacrae merged commit 93ab4dd into obsidian-tasks-group:main May 29, 2023
1 check passed
@claremacrae
Copy link
Collaborator

@chrabia Many thanks for this super useful contribution, that I had no idea I even wanted! ❤️ 😄 🎉

@chrabia
Copy link
Contributor Author

chrabia commented May 30, 2023

@chrabia Many thanks for this super useful contribution, that I had no idea I even wanted! ❤️ 😄 🎉

You're welcome, thanks for the review :) I hope I'll have an opportunity to contribute again! 😄

@claremacrae
Copy link
Collaborator

@chrabia Just to let you know about a minor change I'm proposing to make...

In the auto-suggest, I want to put:

  • High above Highest
  • Low about Lowest

When using the auto-suggest, because Highest comes before High, if I start typing Hi I always get Highest first...

I'm thinking it should default to the shorted word first, so High and Low, and people should have to type more characters, or arrow-down, to get the longer word.

This would also not break people's muscle memory from using the previous High and Low...

This has tripped me up several times today, so I'm expecting someone will log a request for it sometime soon....

Just wanted to let you know in advance...

@chrabia
Copy link
Contributor Author

chrabia commented May 30, 2023

Yes, it makes sense, go for it :)

claremacrae added a commit that referenced this pull request May 30, 2023
Fixes #1943

The order of priorities in auto suggestion is now

- high
- medium
- low
- highest
- lowest

This retains the original order, for people's muscle memory. And it means that the
longer names are after the shorter ones, which also makes it possible to type both,
and it means that typing parts of names selects the same result as before
highest and lowest were added.
claremacrae added a commit that referenced this pull request May 30, 2023
)

Fixes #1943

The order of priorities in auto suggestion is now

- high
- medium
- low
- highest
- lowest

This retains the original order, for people's muscle memory. And it means that the
longer names are after the shorter ones, which also makes it possible to type both,
and it means that typing parts of names selects the same result as before
highest and lowest were added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: emojis and signifiers Anything to do with emojis and signifiers
Projects
Status: 🎉 Released
Development

Successfully merging this pull request may close these issues.

None yet

2 participants