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

Recurrence with threshold date and priority adds unwanted artefacts #565

Closed
stephprobst opened this issue Nov 2, 2023 · 6 comments
Closed
Labels
bug Something isn't working

Comments

@stephprobst
Copy link
Contributor

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:

  1. Create a new task: (A) Do something rec:d t:2023-11-03 @SomeContext , but replace 2023-11-03 with the current date.
  2. 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...

@stephprobst stephprobst added the bug Something isn't working label Nov 2, 2023
@ransome1
Copy link
Owner

ransome1 commented Nov 2, 2023

@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: #271

@stephprobst
Copy link
Contributor Author

stephprobst commented Nov 3, 2023

@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
Copy link
Owner

ransome1 commented Nov 3, 2023

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
Copy link
Contributor Author

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 ransome1 moved this from Backlog to Bugs in sleek 2.x Dec 14, 2023
@ransome1
Copy link
Owner

@stephprobst I assume we can close here, right?

@stephprobst
Copy link
Contributor Author

Yes. All good from my side.

@github-project-automation github-project-automation bot moved this from Bugs to Done in sleek 2.x Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

No branches or pull requests

2 participants