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 support for syncing TODOs #218

Merged
merged 17 commits into from
Dec 21, 2022
Merged

Add support for syncing TODOs #218

merged 17 commits into from
Dec 21, 2022

Conversation

whirm
Copy link

@whirm whirm commented Sep 3, 2020

This PR brings in some commits from @grauschnabel's fork I've adapted to your master plus some fixes on top of it.

Edit: Original PR from the author of most of the work here: #118

@lhindir
Copy link

lhindir commented Sep 8, 2020

This is sweet! Just here to mention #118 for future lurkers.

Would love to see this merged @dengste

@whirm
Copy link
Author

whirm commented Sep 9, 2020

This is sweet! Just here to mention #118 for future lurkers.

Would love to see this merged @dengste

I've added the reference on the PR comment, thanks for noting it!

@whirm
Copy link
Author

whirm commented Sep 28, 2020

@dengste are you planning on reviewing this? Thanks!

@ethancedwards8
Copy link

Ah man, this would be great!

@olekenneth
Copy link

I really would like to see this merged.

I don't remember what the issues where though.
@grauschnabel
Copy link
Contributor

What happend to @dengste? That's a bit confusing that he is not even answering here.. .

@florhizome
Copy link

thanks for all your effort :) this is a great and important feature since nextcloud is spreading :)
next up would be async support ... ;P

sorry for the bad joke, but is there a selfoperable branch at one of yours one could already use until all is merged ? when I try to use "sync-todos" of @whirm all the usual variables are not available anymore.

f.e. "Symbol's function definition is void: org-caldav-calendars"

It would be nice since I think @dengste shouldn't need to rush the commit but still could really use the features right now ^^^

@whirm
Copy link
Author

whirm commented Jun 30, 2021

sorry for the bad joke, but is there a selfoperable branch at one of yours one could already use until all is merged ? when I try to use "sync-todos" of @whirm all the usual variables are not available anymore.

This branch is in a working condition. (that's the version I use myself)

f.e. "Symbol's function definition is void: org-caldav-calendars"

This defcustom is at org-caldav.el:120 so my guess is that you haven't (yet) loaded org-caldav at the time you are trying to access it.

@florhizome
Copy link

hmm ok yeah it's maybe something I do wrong when trying to declare the fork with straight.el...

@florhizome
Copy link

florhizome commented Aug 1, 2021

I wanted to bring up the proposal of designing a plugin/extension package for this. @dengste still doesn't have to found the time to handle merge requests for now. Again, not here to pressure ... But this functionality is pretty important for those who want to use it, and since nextcloud is only going to expand further IMO, deserves a space for itself to be fostered. It could further be discussed if org-caldav could be designed more modular in the future, since it's a complex package and matter which I think is hard to maintain as a sole person, etc.
On another thought the whole matter - Proper taskmanagement with calendar integration on the base of caldav - seems to be complicated and important enough (There doesn't seem to be much (FOSS) Software around being able to help with this) to say it could be worth to dedicate a separate base (a package defining a major mode) to this. For example, my Orgmode has been buggy for some time, and I don't want to rely on it to be fully functional to have my task management working. Of course, this is quite the separate issue on a different timeline, also needing dedicated ppl, but I think it's worthy to have written down somewhere for ppl to consider.

@hny-gd
Copy link

hny-gd commented Aug 1, 2021

Maybe it still might be an idea to look at @girzel 's suggestion to maybe move towards core url-dav.el?

@girzel
Copy link

girzel commented Aug 1, 2021

Just to be clear, that was a pretty hand-wavy suggestion! And something that, while it could potentially simplify this package, wouldn't remove the need for it altogether. But it could theoretically get more maintainer eyes on the basic caldav functionality.

@florhizome
Copy link

where was that mentioned? Yeah, sure, org caldav will remain, maybe it could be possible to reappoint some core functionality in the future, though.

@hny-gd
Copy link

hny-gd commented Aug 8, 2021

It was part of the discussion in #233.

FWIW, this came through on the orgmode mailing list just now: https://github.com/theophilusx/icsorg

@florhizome
Copy link

i thought org-icalendar was already existent and working?

@dschwilk
Copy link

Just chiming in as someone who depends on org-caldav / nextcloud and cares about this. Not a great elisp programmer but am keen to see this survive/grow and can help.

@jackkamm
Copy link
Collaborator

jackkamm commented Jul 31, 2022

I'm also interested in VTODO support, but had some problems with this branch that I found difficult to debug. Debugging was made harder by the fact that this branch has accumulated many changes over the years, some of which are unneeded for core VTODO support.

So, I created a smaller, barebones version of this work here:
https://github.com/jackkamm/org-caldav/tree/sync-todos-clean

I think the barebones version may be easier to review and eventually merge. For example, it has a diff about 5x smaller than the current PR:

# PR #218
git diff --stat master org-caldav.el
 org-caldav.el | 1844 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------
 1 file changed, 1190 insertions(+), 654 deletions(-)

# smaller barebones version
git diff --stat master org-caldav.el
 org-caldav.el | 377 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 310 insertions(+), 67 deletions(-)

To create my new branch, I started by diff'ing this PR with master, and deleted all changes that weren't necessary for basic VTODO functionality. Deleted changes include:

  • A lot of whitespace changes.
  • New features unrelated to VTODO (e.g. days-in-past functionality)
  • Some obsolete fixes for old bugs that were fixed in master.
  • Some "extra" VTODO features that diverge from upstream org-mode's icalendar export, e.g.:
    • I removed PERCENT-COMPLETE functionality, instead using the STATUS field to update todo-keyword.
    • I replaced the PRIORITY handling with something closer to what upstream org-icalendar--vtodo uses.

Then, I squashed all the remaining changes down into a single commit (listing @whirm and @grauschnabel as coauthors).

It seems to be working now, but there may still be some bugs I missed. I'm testing it out using Radicale and Thunderbird currently.

EDIT 2022-08-01: I also created an "unsquashed" version of this branch here, to more easily see how it diverges from the current PR:
https://github.com/jackkamm/org-caldav/tree/sync-todos-clean-unsquashed

@grauschnabel
Copy link
Contributor

Great work, thanks for that @jackkamm .

For me there i no reason to keep things like days-in-past, the idea was just that we do not sync a lot of done todos. So I support to merge your work.

I couldn't see any problems with your version, but I didn't all the tests. I will use it from now and see if it works..

@jackkamm
Copy link
Collaborator

jackkamm commented Aug 8, 2022

Thanks @grauschnabel -- my work is very minor compared to what you have already done.

Also, I was likely too aggressive in removing features; for example, I think I should add back a way to have multiple keywords besides TODO and DONE.

I'm continuing to push some changes to the unsquashed branch [1]. In particular, I'm cleaning up some of the code duplication, and also may revert some of the deletions I made. When it's ready, I'll re-squash and comment here, or perhaps submit an updated PR. Would be interested to hear any feedback in the meantime.

[1] https://github.com/jackkamm/org-caldav/tree/sync-todos-clean-unsquashed

@jackkamm
Copy link
Collaborator

jackkamm commented Aug 28, 2022

Update: I created a branch that preserves the current functionality of this PR, while still reducing the divergence from upstream. The branch also includes several bugfixes and a unit test for the sync-todos functionality.

For now, I've opened a PR against @whirm's branch, in case he wants to include the changes into the current PR: whirm#2

Note this PR is not as small as my previous "barebones" branch, as it includes the functionality I had previously removed -- there were too many important features that I threw out before.

However the PR still achieves substantial reduction in the divergence from upstream, by removing whitespace changes, reducing code duplication, and other refactoring.

Getting closer to upstream also made it easier for me to merge in some other bugfixes (#252). And after updating the unit tests, I found a couple additional bugs which I also added fixes for.

@Thaodan
Copy link

Thaodan commented Oct 17, 2022

Update: I created a branch that preserves the current functionality of this PR, while still reducing the divergence from upstream. The branch also includes several bugfixes and a unit test for the sync-todos functionality.

For now, I've opened a PR against @whirm's branch, in case he wants to include the changes into the current PR: whirm#2

Note this PR is not as small as my previous "barebones" branch, as it includes the functionality I had previously removed -- there were too many important features that I threw out before.

However the PR still achieves substantial reduction in the divergence from upstream, by removing whitespace changes, reducing code duplication, and other refactoring.

Getting closer to upstream also made it easier for me to merge in some other bugfixes (#252). And after updating the unit tests, I found a couple additional bugs which I also added fixes for.

Hey I'm testing the refactored/your master branch right now. How do you map the blocked state to to vtodo?

I also saw that you remove support for rrule is this intended?

@Thaodan
Copy link

Thaodan commented Oct 17, 2022

This RFC mentions the needs-action state which could map quite nicely to a Blocked or waiting state.
https://www.rfc-editor.org/rfc/rfc5545

@Thaodan
Copy link

Thaodan commented Oct 18, 2022

I also saw that you remove support for rrule is this intended?

Restored here:
Thaodan@21313b7

@Thaodan
Copy link

Thaodan commented Oct 18, 2022

Hey I'm testing the refactored/your master branch right now. How do you map the blocked state to to vtodo?

Answering myself: adding WAITING toorg-caldav-todo-percent-states did the trick.
I'm still wondering the status field could be employed for the blocked state, however I don't know if any ui uses the needs-action state.

@Thaodan
Copy link

Thaodan commented Oct 18, 2022

Answering myself: adding WAITING toorg-caldav-todo-percent-states
did the trick. I'm still wondering the status field could be employed
for the blocked state, however I don't know if any ui uses the
needs-action state.

See also: https://www.kanzaki.com/docs/ical/partstat.html

@edgar-vincent
Copy link
Contributor

@whirm @jackkamm @Thaodan @grauschnabel These changes look amazing. Since this repo is pretty much abandoned, would any of you be willing to take up maintainership? It doesn't have to the particularly active (if at all), just merging all of your great work will be good enough and would allow users to be able to benefit from these much-awaited features.

Thanks a lot!

@hny-gd
Copy link

hny-gd commented Oct 19, 2022

Yes please. 🙂

@jackkamm
Copy link
Collaborator

I also saw that you remove support for rrule is this intended?

Don't remember if intentional, but rrule wasn't used so it had no effect. However, I saw in your branch you merged in some more work that makes use of rrule. Does it work well? I'm interested to merge it into my master, but haven't tried it yet.

I'm still wondering the status field could be employed for the blocked state, however I don't know if any ui uses the needs-action state.

I'm sympathetic to this, intuitively I'd also rather map STATUS rather than PERCENT-COMPLETE to the todo state. But I'm also hesitant to break compatibility with the older sync-todo branches, which have been battle-tested for several years now.

@Thaodan
Copy link

Thaodan commented Oct 22, 2022 via email

@Thaodan
Copy link

Thaodan commented Oct 22, 2022 via email

@dakra dakra mentioned this pull request Dec 1, 2022
@jackkamm jackkamm merged commit 9381d4c into dengste:master Dec 21, 2022
@jackkamm
Copy link
Collaborator

jackkamm commented Dec 21, 2022

I've merged this into master now.

I made one backwards-incompatible change: the default value of org-caldav-todo-percent-states was changed to only use the standard built-in keywords (TODO & DONE). So you will need to set that variable if you use additional todo-keywords.

Otherwise, the functionality should be back-compatible with previous versions of this branch. Please, let me know if anything breaks for you.

Many thanks to @grauschnabel and @whirm for their work on this.

@Thaodan
Copy link

Thaodan commented Dec 21, 2022 via email

@jackkamm
Copy link
Collaborator

Could you keep this optionally in? I think there's some use case for that to translate statuses like HOLD.

You can still accomplish this by setting org-caldav-todo-percent-states, see the docstring for an example.

I'm hesitant to include non-standard keywords in the default value -- we use org-todo to update the state after sync'ing, and I'm not sure the behavior is well defined if the state isn't present in org-todo-keywords. And everyone has their own todo keywords, so it seems better to keep the default as vanilla.

@Thaodan
Copy link

Thaodan commented Dec 21, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet