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

Add hooks to let other plugins extend the Tasks plugin. #1249

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

alexthewilde
Copy link

@alexthewilde alexthewilde commented Oct 22, 2022

Description

Add hooks to let other plugins extend the Tasks plugin.

Motivation and Context

I needed a way to extend the Tasks plugin with my own functionality (see discussion).

How has this been tested?

Screenshots (if appropriate)

Here's an example of how other plugins can make use of the new hooks to extend the Tasks plugin's functionality:

Use like
image

Renders as
image

Implemented in new plugin

export const extendTask = (listItem: HTMLLIElement, task: any, api: any) => {
  if (api.layoutOptions.showDoTodayButton) {
    addDoTodayButton(listItem, task, api);
  }
}

export const extendParser = (line: string, layoutOptions: any) => {
  if (line === 'show do today button') {
    layoutOptions.showDoTodayButton = true;
    return true;
  }
  
  return false;
}

function addDoTodayButton(listItem: HTMLLIElement, task: any, api: any) {
  const today = window.moment();
  const isToday = task.scheduledDate && task.scheduledDate.format('YYYY-MM-DD') === today.format('YYYY-MM-DD');
  const doTodayBtn = listItem.createEl('a', {
      text: isToday ? 'not today' : 'do today',
      cls: 'tasks-do-today',
  });

  doTodayBtn.onClickEvent((event: MouseEvent) => {
      const updatedTask = new api.Task({
          ...task,
          scheduledDate: isToday ? null : today,
      });

      api.replaceTaskWithTasks({
          originalTask: task,
          newTasks: [updatedTask],
      });
  });
}

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)
  • Sample vault (prefix: vault - improvements to the Tasks-Demo sample vault)

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

claremacrae commented Oct 22, 2022

Wow. It looks amazing. Huge thanks for working on this. It would open up some really exciting avenues with the plugin.

That's a lot to get my head around, in not very much code...
It might also give ideas of how to easily add new button functionality in to the existing Tasks plugin.

How to test it

I notice you haven't filled in the testing section. How are you imagining I might test it?
I don't suppose you could share a link to your test plugin please, so that it could be tested more quickly and easily?

GitHub Actions

I see that the 'verify' GitHub Action failed. Looks like there are some ESLint issues????

Implications for future changes to the Tasks code

What impact would it have on future refactoring of the Tasks code? If this were published, for example, would it prevent changes to the interface of the Task class? Asking because there is a lot of public data that I would want to protect at some point... I would hate to break a load of people's customisations - but would also prefer not to have our hands tied for future refactorings...

Again, very many thanks...

@claremacrae claremacrae added the question Further information is requested label Oct 24, 2022
@claremacrae
Copy link
Collaborator

Hi @alexthewilde - I hope you're OK. I just wanted to make sure you've seen my comments above...

@alexthewilde
Copy link
Author

alexthewilde commented Oct 25, 2022

Here's the very simple obsidian-tasks-extended plugin to show how a Tasks Extension plugin can be written.

I see that the 'verify' GitHub Action failed. Looks like there are some ESLint issues????

I look into this.

What impact would it have on future refactoring of the Tasks code?

This change is orthogonal to the existing code, so it won't break any existing functionality.

@claremacrae
Copy link
Collaborator

claremacrae commented Oct 25, 2022

Test plugin

Here's the very simple obsidian-tasks-extended plugin to show how a Tasks Extension plugin can be written.

Thank you.

I was able to install a build of this PR in to the sample vault in the tasks repo - and to follow your instructions in the above example repo.

It ran with no errors, and will allow me to experiment with the changes, which is great.

This was my task block:

```tasks
group by happens
show do today button
```

Oddly, the new button was displayed on every task. I cannot see the logic error - but it would be good to understand the location of any problem, in case it affects the PR.

image

The feature's impact on future changes to the Tasks code

What impact would it have on future refactoring of the Tasks code?

This change is orthogonal to the existing code, so it won't break any existing functionality.

I meant the reverse. Whether I will be prevented from improving the code in the Tasks repo, such as by adding missing abstractions.

For example, your sample plugin contains this code:

  doTodayBtn.onClickEvent((event: MouseEvent) => {
      const updatedTask = new api.Task({
          ...task,
          scheduledDate: isToday ? null : today,
      });

      api.replaceTaskWithTasks({
          originalTask: task,
          newTasks: [updatedTask],
      });
  });

What if scheduledDate becomes a class, instead of a Moment?

What if api.replaceTaskWithTasks() changes the parameters it takes.

As soon as this feature is published, my hands will be tied from changing any current code that the PR makes accessible to people's extensions.

See Hyrum's Law..

@claremacrae
Copy link
Collaborator

As soon as this feature is published, my hands will be tied from changing any current code that the PR makes accessible to people's extensions.

That isn't intended as a 'no' to this PR.

It's meant to be a request for advice and help on what would need to be done to which bits of Tasks to enable future changes to made safely.

For example, I presume it means removing all public data in classes that users might reasonably want to use in their extensions - replacing them with getters, and where relevant, setters.

And understanding how those settings might work as parameter types change over time.

Fields that store a Moment now will change to storing some TaskDate or similar class in future, for example.

@claremacrae
Copy link
Collaborator

I see that the 'verify' GitHub Action failed. Looks like there are some ESLint issues????

I look into this.

Thanks. All are green now.

@claremacrae
Copy link
Collaborator

(FYI I've ticked the 'My change requires a change to the documentation.' box in the original description... )

@alexthewilde
Copy link
Author

For example, I presume it means removing all public data in classes that users might reasonably want to use in their extensions - replacing them with getters, and where relevant, setters.

That's right. Exposing an API comes with certain responsibilities. That's the trade-off I guess.

It would actually be better if the Tasks plugin exposed its API methods like this. This allows other plugin developers to access them like app.plugins.plugins['obsidian-tasks-plugin'].replaceTaskWithTasks().

Then the hook implementations wouldn't need to get the api object passed in anymore. That's anyway a workaround for the missing plugin API.

@alexthewilde
Copy link
Author

alexthewilde commented Oct 25, 2022

Oddly, the new button was displayed on every task. I cannot see the logic error

There's no logic error. Displaying the button on every task is intended. You can explicitly control this via show do today button for your query.

@claremacrae
Copy link
Collaborator

Oddly, the new button was displayed on every task. I cannot see the logic error

There's no logic error. Displaying the button on every task is intended. You can explicitly control this via show do today button for your query.

What's the point of the button then?

@claremacrae
Copy link
Collaborator

For example, I presume it means removing all public data in classes that users might reasonably want to use in their extensions - replacing them with getters, and where relevant, setters.

That's right. Exposing an API comes with certain responsibilities. That's the trade-off I guess.

Good, we are agreed.

It would actually be better if the Tasks plugin exposed its API methods like this. This allows other plugin developers to access them like app.plugins.plugins['obsidian-tasks-plugin'].replaceTaskWithTasks().

Then the hook implementations wouldn't need to get the api object passed in anymore. That's anyway a workaround for the missing plugin API.

I don't understand this at all. I mean, I don't understand in what way the Tasks code would need to change in order to do what you suggest.

@claremacrae
Copy link
Collaborator

I don't understand this at all. I mean, I don't understand in what way the Tasks code would need to change in order to do what you suggest.

Actually, I now kind of do get it. Does all the use of any again hide the fact that if the interface of a Task changes, it will break client code.

Should we be looking to expose some kind of .d.ts (?) file to publish an interface.

@alexthewilde
Copy link
Author

Should we be looking to expose some kind of .d.ts (?) file to publish an interface.

Exposing types doesn't help with exposing the actual methods. You actually need to make each method that you want to expose a member of the main class.

To be frank, what we're talking about here are developer ergonomics. It's not really that important right now. Either way, if you change the plugin's API methods (regardless of how they're being exposed), then this needs to be communicated to dependent developers, e.g. via "breaking change" in your Changelog.

@alexthewilde
Copy link
Author

alexthewilde commented Oct 25, 2022

Oddly, the new button was displayed on every task. I cannot see the logic error

There's no logic error. Displaying the button on every task is intended. You can explicitly control this via show do today button for your query.

What's the point of the button then?

To toggle the scheduled date to today or null i.e. reset it.

I might be the only person on the planet who needs this. But now I'm able to build it myself, instead of begging you to add this button to the Tasks core plugin :)

@claremacrae
Copy link
Collaborator

claremacrae commented Oct 25, 2022

To be frank, what we're talking about here are developer ergonomics. It's not really that important right now. Either way, if you change the plugin's API methods (regardless of how they're being exposed), then this needs to be communicated to dependent developers, e.g. via "breaking change" in your Changelog.

Indeed.

To clarify, the reason I'm persisting with this is that my very strong philosophy is to work hard to minimise the number of breaking changes.

I do this in all projects that have external users, but especially in Tasks, as it has around 175,000 downloads now, and I know for sure that many people will take advantage of your new feature when it is released.

That focus is mainly for the benefit of users - but it's also for my own benefit. When I took over looking after Tasks, I completely under-estimated how much of my own time would go just on user support. And a breaking change would undoubtedly worsen that.

If someone shares a Tasks extension that a bunch of people use, that is later broken by a Tasks update, my support load is guaranteed to increase.

So I am after a better backwards-compatibility story than "log a breaking change in the changelog".

@alexthewilde
Copy link
Author

@claremacrae that makes absolute sense. Btw thank you for maintaining the plugin! I love the simplicity of letting me create tasks (instead of task notes) while I'm in the flow of writing.

Regarding backwards-compatibility: right now, only replaceTaskWithTasks gets exposed. Unless you plan to change this method frequently, you shouldn't have to worry about too many regressions.

The more API methods you expose however, the more things can break. So you should indeed be careful here.

@claremacrae
Copy link
Collaborator

@alexthewilde Thank you, that helps a lot.

The thing I'm missing, though, is that you say only replaceTaskWithTasks gets exposed.

But it's actually the Task class that I'm concerned about. It is a monster. What am I not understanding here, about why future changes to it would not be a problem for users?

Another example, what if someone wanted to change the recurrence rule for a task. That's another class....

@alexthewilde
Copy link
Author

But it's actually the Task class that I'm concerned about. It is a monster. What am I not understanding here, about why future changes to it would not be a problem for users?

Yes indeed. Changes to the Task model (not necessarily the class though) would affect everyone who uses it in an extension.

I think it depends on how stable the plugin is. If you plan on turning everything upside down, then I'd certainly not build something on top of it!

@claremacrae
Copy link
Collaborator

I think it depends on how stable the plugin is. If you plan on turning everything upside down, then I'd certainly not build something on top of it!

I'm still learning about the code, and nothing in the following is cast in stone or even certain to happen...

One of the main reasons that user support takes time on this plugin is because there are some missing abstractions that make it hard than people assume. So I keep answering the same questions over and over again (taking time away from development)

Some examples - a far from complete list:

  • I want to use custom formats for the due date, etc in my tasks.
    • Sorry no. There is no abstraction for date data (Moment is used directly) so implementing that currently would be a ton of work.
  • I don't like emojis, I want to use [dataview format, or one of a myriad of other formats]...
    • Sorry no. All the parsing of Task lines is in the already massive Task class. There should be abstractions for the various different formats.
  • My task is in an H3, I want to search text in its parent H1, H2, H3
    • Sorry no. Tasks are stored in a flat list, and they only know about their immediate previous heading. We should have an abstraction for sections in markdown files.
  • I want to search nested tasks - or show nested/indented tasks
    • Sorry no. Tasks are stored in a flat list

I am really good at refactoring existing code safely. I can do all these things. I just need some days of uninterrupted spare time to focus on these things, and the irony of so much time going on user support is that it makes that harder...

I'm even considering reworking all the parsing code to base it on the dataview API.

@claremacrae
Copy link
Collaborator

claremacrae commented Oct 25, 2022

OK - on re-reading the above, I think that the answer is that I am happy to commit to keeping working any extensions that are written against Tasks 1.x...

And in the documentation I will say to expect that they will need to be updated when Tasks 2.x is released. That there is no compatibility guarantee that extensions will continue to work across major version changes.

This all means that this PR will need to include a smoke test to allow easy manual testing that an extension still works with the current (v.1.x) code.

Here is the current smoke testing page:
obsidian-tasks/resources/sample_vaults/Tasks-Demo/Manual Testing/Smoke Testing the Tasks Plugin.md

So the smoke test should look something like:

  • Load the page blah
  • Switch to Reading view
  • Confirm that [some customisation is visible]
  • Confirm that nothing no errors were written in the console

As well as being useful for smoke testing, the new page in the sample vault will be useful for people to test out and experiment with the new capability.

@alexthewilde
Copy link
Author

This all means that this PR will need to include a smoke test to allow easy manual testing that an extension still works with the current (v.1.x) code.

Maybe I'll find some time on the weekend.
Do you see yourself capable of writing smoke tests?

@claremacrae
Copy link
Collaborator

Maybe I'll find some time on the weekend. Do you see yourself capable of writing smoke tests?

Yes, of course I can. Time permitting, of course.

This one is a lot more work though, as it involves figuring out how to include a sample plugin inside this vault - and ensuring that when someone downloads the sample vault from the Artifacts list in the GitHub Actions log, it has the uptodate build of the extension plugin, with all its dependencies built.

So the bulk of the work here is not in writing the the 1 or 2 sentences in the smoke test, it is ensuring that it only takes someone a minute or so to run the smoke tests, from the download.

@claremacrae
Copy link
Collaborator

Or maybe the extension demo/test plugin goes in another repo in the obsidian-tasks-group user, so that it can have its own Github Actions - and it's easy for people to fork it and modify it to their heart's content?

And then it is just installed inside the sample vault...

@alexthewilde alexthewilde removed their assignment Oct 29, 2022
@alexthewilde
Copy link
Author

@claremacrae I'm afraid that writing smoke tests will take me too much time figuring things out and I don't have that time really. Sorry that I can't be of help here. I was hoping that you could go forward with this task. If not, maybe somebody else from the community with more experience in that regard?

Btw. feel free to fork Obsidian Tasks Extended into your GitHub account if need be.

I will update the Docs on how to use the new hooks.

@claremacrae
Copy link
Collaborator

Hi @alexthewilde No problem. I am definitely happy to carry on with it and get it to release.

As an aside, the reason I assigned a few people to their PRs was that I've created a personal board to keep track of the many things in flight here, and it helped to see names against the creators of the PRs... Nothing more than that.

@claremacrae
Copy link
Collaborator

Btw. feel free to fork Obsidian Tasks Extended into your GitHub account if need be.

That's great - thank you!

I will update the Docs on how to use the new hooks.

Much appreciated.

@claremacrae claremacrae self-assigned this Oct 29, 2022
@claremacrae claremacrae removed the question Further information is requested label Nov 1, 2022
@claremacrae claremacrae added scope: rendering of tasks How the plugin displays tasks (except CSS issues) scope: for plugin developers Tools for use by plugin developers, including the Tasks API labels Nov 11, 2022
@claremacrae claremacrae mentioned this pull request Dec 22, 2022
14 tasks
@BluBloos
Copy link
Contributor

BluBloos commented Feb 4, 2023

re: #1249 (comment)

specifically the following line

It would actually be better if the Tasks plugin exposed its API methods like this. This allows other plugin developers to access them like app.plugins.plugins['obsidian-tasks-plugin'].replaceTaskWithTasks()

I believe this is related to #1389.

@claremacrae
Copy link
Collaborator

Implications for future changes to the Tasks code

What impact would it have on future refactoring of the Tasks code? If this were published, for example, would it prevent changes to the interface of the Task class? Asking because there is a lot of public data that I would want to protect at some point... I would hate to break a load of people's customisations - but would also prefer not to have our hands tied for future refactorings...

The above is now a solved problem!

As part of the new group by function facility, I've started publishing a documented set of properties that can be accessed on Tasks, and I will guarantee these going forwards. They are mostly getters that are separate from the stored data, so do not prevent future refactorings...

https://publish.obsidian.md/tasks/Scripting/Task+Properties

For now, these properties are read-only, and we would need to define a convention for updating task objects that is independent of the storage.

But with this, in conjunction with the recently added API, I can see that adopting the ideas in this PR could become very feasible in the foreseeable future.
https://publish.obsidian.md/tasks/Advanced/Tasks+Api

@claremacrae
Copy link
Collaborator

Setting to draft status any PRs which are not currently mergable, but will be useful in future.

@claremacrae claremacrae marked this pull request as draft May 7, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: for plugin developers Tools for use by plugin developers, including the Tasks API scope: rendering of tasks How the plugin displays tasks (except CSS issues)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants