Recurrence with threshold date and priority adds unwanted artefacts

ransome1 / sleek

todo.txt manager for Linux, Windows and MacOS, free and open-source (FOSS)

https://github.com/ransome1/sleek/wiki

MIT License

1.29k stars 99 forks source link

Recurrence with threshold date and priority adds unwanted artefacts #565

Closed stephprobst closed 7 months ago

stephprobst commented 8 months ago

Bug Report

App Version: 2.0.1

Platform: Windows 10

Installation Method: App Store

Bug Description: When completing a task with rec:d and t:2023-11-02 (the current date) the newly created task suddenly has a threshold date t:2023-11-03 and a due date due:2023-11-03. The newly added due date is incorrect and shouldn't be there. There is also a new text element pri:A in the task, which shouldn't be there.

Steps to Reproduce:

Create a new task: (A) Do something rec:d t:2023-11-03 @SomeContext , but replace 2023-11-03 with the current date.
Complete the task and inspect the newly created task.

Expected Behavior: The new task should look like this: (A) Do something rec:d t:2023-11-04 @SomeContext

Actual Behavior: The new task looks like this: (A) Do something rec:d t:2023-11-04 @SomeContext pri:A due:2023-11-04

Additional Information: The bug was introduced with the release of version 2.0.0, it worked fine in 1.*

Screenshots: Not applicable...

ransome1 commented 8 months ago

@stephprobst there might be a misconception on what the recurrence feature does.

It is not connected to the threshold dates in any way. It really is about the due date. The behaviour you're describing sounds pretty much like it has been developed: https://github.com/ransome1/sleek/wiki/Recurring-todos-(rec:)

And about the priority; When marking a todo with priority as complete, the priority is being preserved as an extension (pri:). This has been implemented after a user pointed out that sleek would break the todo.txt format otherwise: https://github.com/ransome1/sleek/issues/271

stephprobst commented 8 months ago

@ransome1 the documentation states, that recurrence should also work with threshold dates. And it did work in version 1. Link to documentation: https://github.com/ransome1/sleek/wiki/Deferred-todos-(t:)#recurrence . I'm a big fan of this feature and it was one of the things that made me fall in love with sleek. :-)

On the priority: The linked issue #271 discusses the added pri: information on the completed task. I think this is fine. However, the pri: tag also appears in the newly created task, which I don't think makes any sense, as the newly created task also has the priority attribute (A) at the beginning.

FYI - If you are happy with the proposed changes I can also try to send a PR myself in the next days. But I would wait for your confirmation of the concept.

ransome1 commented 8 months ago

On the priority: The linked issue #271 discusses the added pri: information on the completed task. I think this is fine. However, the pri: tag also appears in the newly created task, which I don't think makes any sense, as the newly created task also has the priority attribute (A) at the beginning.

I just double checked this and confirm it is a bug. There is not suppose to be the pri: extension on a newly created task. It might be due to this issue in jstodotxt, where I couldn't find a feasible way yet, to fully remove extensions.

@ransome1 the documentation states, that recurrence should also work with threshold dates. And it did work in version 1. Link to documentation: https://github.com/ransome1/sleek/wiki/Deferred-todos-(t:)#recurrence . I'm a big fan of this feature and it was one of the things that made me fall in love with sleek. :-)

I checked the implementation and it is actually there. Must have been late yesterday ;) But for some reason it doesn't work as expected.

I'm afk now for the next two weeks. A PR would be highly appreciated. It would be good if you could also create the respective test cases, so that we have this feature a bit more under control. There are quite a couple of scenarios, that need to be tested and my tests obviously did not cover this :/

stephprobst commented 8 months ago

I sent two PRs:

One to fix the unit test setup for Windows environments.
One to fix the bugs as discussed in this issue.

Besides the two already mentioned bugs there was a third one, which goes hand in hand with the others:

When a user completes a task with pri:A, the newly created task should also have pri:A and not (A). My thinking is:

pri:A is a random key value pair without special meaning in todo.txt or sleek as far as I can see. It is only there to archive the priority information upon completion of a task.
Therefore an active task should never have a pri:A attribute and always use (A) instead.
This also means, that a recurrent task with pri:A, that is completed, shouldn't cause the newly created task to suddenly have priority (A). It should just carry forward the key value pair like all other key value pairs.

Would you agree? I hope the reason for afk is a relaxing vacation! :-) If so, enjoy!

ransome1 commented 7 months ago

@stephprobst I assume we can close here, right?

stephprobst commented 7 months ago

Yes. All good from my side.