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

Broken non-strict recurrence #611

Closed
amariusz opened this issue Dec 11, 2023 · 34 comments
Closed

Broken non-strict recurrence #611

amariusz opened this issue Dec 11, 2023 · 34 comments
Labels
bug Something isn't working

Comments

@amariusz
Copy link
Collaborator

Bug Report

App Version: sleek-2.0.3-rc.5 (was present in sleek-2.0.0-dev15)

Platform: Linux

Installation Method: AppImage

Bug Description:
Sleek should determine between strict recurrence (rec:+X) and non-strict recurrence (rec:X). It seems that all cases are treated as strict recurrnce.
https://github.com/ransome1/sleek/wiki/Recurring-todos-(rec:)

Steps to Reproduce:
Enter such tasks and complete them:

2022-01-01 task1 - non-strict recurrence rec:1d t:2023-01-01
2022-01-01 task2 - strict recurrence rec:+1d t:2023-01-01

Expected Behavior:

x 2023-12-11 2022-01-01 task1 - non-strict recurrence rec:1d t:2023-01-01
x 2023-12-11 2022-01-01 task2 - strict recurrence rec:+1d t:2023-01-01
2023-12-11 task1 - non-strict recurrence rec:1d t:2023-12-12
2023-12-11 task2 - strict recurrence rec:+1d t:2023-01-02

Actual Behavior:

x 2023-12-11 2022-01-01 task1 - non-strict recurrence rec:1d t:2023-01-01
x 2023-12-11 2022-01-01 task2 - strict recurrence rec:+1d t:2023-01-01
2022-01-01 task1 - non-strict recurrence rec:1d t:2023-01-02
2022-01-01 task2 - strict recurrence rec:+1d t:2023-01-02

Additional Information:

New tasks should have creation date set to today, instead old date is reused.

@amariusz amariusz added the bug Something isn't working label Dec 11, 2023
@ransome1
Copy link
Owner

@amariusz I'm not on a computer right now. Does this happen also with due dates?

@amariusz
Copy link
Collaborator Author

I've done some more testing. It seems non-strict recurrence is only broken for todos with t: and without due:
However creation date is incorrect for all recurrence cases.

Some more examples:

t: and due:

input

2022-01-01 task3 - non-strict recurrence rec:1m t:2023-01-01 due:2023-01-10
2022-01-01 task4 - strict recurrence rec:+1m t:2023-01-01 due:2023-01-10

Expected Behavior

x 2023-12-11 2022-01-01 task3 - non-strict recurrence rec:1m t:2023-01-01 due:2023-01-10
x 2023-12-11 2022-01-01 task4 - strict recurrence rec:+1m t:2023-01-01 due:2023-01-10
2023-12-11 task3 - non-strict recurrence rec:1m t:2024-01-02 due:2024-01-11
2023-12-11 task4 - strict recurrence rec:+1m t:2023-02-01 due:2023-02-10

Actual Behavior

x 2023-12-11 2022-01-01 task3 - non-strict recurrence rec:1m t:2023-01-01 due:2023-01-10
x 2023-12-11 2022-01-01 task4 - strict recurrence rec:+1m t:2023-01-01 due:2023-01-10
2022-01-01 task3 - non-strict recurrence rec:1m t:2024-01-02 due:2024-01-11
2022-01-01 task4 - strict recurrence rec:+1m t:2023-02-01 due:2023-02-10

wrong creation date

only due:

input

2022-01-01 task5 - non-strict recurrence rec:1m due:2023-01-10
2022-01-01 task6 - strict recurrence rec:+1m due:2023-01-10

Expected Behavior

x 2023-12-11 2022-01-01 task5 - non-strict recurrence rec:1m due:2023-01-10
x 2023-12-11 2022-01-01 task6 - strict recurrence rec:+1m due:2023-01-10
2023-12-11 task5 - non-strict recurrence rec:1m due:2024-01-11
2023-12-11 task6 - strict recurrence rec:+1m due:2023-02-10

Actual Behavior

x 2023-12-11 2022-01-01 task5 - non-strict recurrence rec:1m due:2023-01-10
x 2023-12-11 2022-01-01 task6 - strict recurrence rec:+1m due:2023-01-10
2022-01-01 task5 - non-strict recurrence rec:1m due:2024-01-11
2022-01-01 task6 - strict recurrence rec:+1m due:2023-02-10

wrong creation date

@ransome1
Copy link
Owner

@stephprobst I am currently afk. Do you have an idea, what could be causing this?

@stephprobst
Copy link
Contributor

stephprobst commented Dec 11, 2023

It was a relativelly simple bug, both in the code and the tests. I created a PR to fix it. (The PR does not contain a fix for the creation dates though. Maybe @ransome1 you can have a look at that once you're back.)

@ransome1
Copy link
Owner

Will check it and give you some feedback. Thanks for taking care of it!

@ransome1
Copy link
Owner

@amariusz I just checked @stephprobst PR and it fixes this issue. I will include it in 2.0.3. Thanks for reporting it.

@amariusz
Copy link
Collaborator Author

Thanks!

@ransome1
Copy link
Owner

@amariusz 2.0.3 has been released and contains the fix. Please feel free to close here, if you can confirm the fix.

@amariusz
Copy link
Collaborator Author

How about fix to "wrong creation date" issue? Is it also included in this build?

@ransome1 ransome1 moved this to Backlog in sleek 2.x Dec 12, 2023
@ransome1 ransome1 moved this from Backlog to 2.0.4 in sleek 2.x Dec 12, 2023
@ransome1
Copy link
Owner

input

2022-01-01 task3 - non-strict recurrence rec:1m t:2023-01-01 due:2023-01-10
2022-01-01 task4 - strict recurrence rec:+1m t:2023-01-01 due:2023-01-10

Expected Behavior

x 2023-12-11 2022-01-01 task3 - non-strict recurrence rec:1m t:2023-01-01 due:2023-01-10
x 2023-12-11 2022-01-01 task4 - strict recurrence rec:+1m t:2023-01-01 due:2023-01-10
2023-12-11 task3 - non-strict recurrence rec:1m t:2024-01-02 due:2024-01-11
2023-12-11 task4 - strict recurrence rec:+1m t:2023-02-01 due:2023-02-10

@amariusz why do you expect the threshold date of the newly created non-strict recurrence of one month to be 2024-01-02? Shouldn't it be equal with the new due date, t:2024-01-11

@ransome1 ransome1 moved this from 2.0.4 to In Progress in sleek 2.x Dec 13, 2023
@ransome1
Copy link
Owner

@amariusz can you also please check if this pre-release solves the issue? https://github.com/ransome1/sleek/releases/tag/v2.0.4-rc.1

@amariusz
Copy link
Collaborator Author

@amariusz why do you expect the threshold date of the newly created non-strict recurrence of one month to be 2024-01-02? Shouldn't it be equal with the new due date, t:2024-01-11

Well, that's how t: with due: works. See:
https://github.com/ransome1/sleek/wiki/Deferred-todos-(t:)#non-strict-mode

It's meant to preserve information about time window between the moment task becomes visible and is due. If you set both to the same date this information would be lost.

@amariusz
Copy link
Collaborator Author

@amariusz can you also please check if this pre-release solves the issue? https://github.com/ransome1/sleek/releases/tag/v2.0.4-rc.1

Works great with the single exception of the case you mentioned which is now broken.
After completing the task, with v2.0.4-rc.1:

2023-12-13 task3 - non-strict recurrence rec:1m t:2024-01-13 due:2024-01-13

should be:

2023-12-13 task3 - non-strict recurrence rec:1m t:2024-01-04 due:2024-01-13

@amariusz
Copy link
Collaborator Author

Well, that's how t: with due: works. See: https://github.com/ransome1/sleek/wiki/Deferred-todos-(t:)#non-strict-mode

It's meant to preserve information about time window between the moment task becomes visible and is due. If you set both to the same date this information would be lost.

After giving it some more thought, I think I know where the confusion might come from.
There's inconsistency in this regard between ToPyDo and Simpletask. Simpletask acts as you described. Sleek behaviour documented in the wiki is modeled after ToPyDo. I've raised this issue in SimpleTask almost 3 years ago but there was no follow-up.

mpcjanssen/simpletask-android#312 (comment)

Simpletask behaviour was always unclear to me. I wonder what reasoning led you to this other solution.

Thanks

@ransome1
Copy link
Owner

I wonder what reasoning led you to this other solution.

You mean the solution where the added days are calculated as the difference of due and t as described in https://github.com/ransome1/sleek/wiki/Deferred-todos-(t:)#non-strict-mode, right?

To be honest, I don't know. This solution was contributed by @zerodat a long time ago. When I rewrote sleek I basically recreated his logic. And probably broke it now ;)

I simply assumed this is how this feature is working in other apps.

I myself have never used the the feature. Not because I don't like it, but because it's not relevant in my workflow. Which probably explains why I'm having issues wrapping my head around how it does work and how it should work.

I am open for a discussion about how the recurrence feature should work.

@amariusz
Copy link
Collaborator Author

I wonder what reasoning led you to this other solution.

You mean the solution where the added days are calculated as the difference of due and t as described in https://github.com/ransome1/sleek/wiki/Deferred-todos-(t:)#non-strict-mode, right?

No, there's a misunderstanding here. Let me explain.

In sleek-2.0.3-rc.5 (and in earlier versions including 1.3.2) completing task in non-strict mode with t: and due: was using logic described in wiki (https://github.com/ransome1/sleek/wiki/Deferred-todos-(t:)#non-strict-mode), the same logic ToPyDo is using. Time window between t: and due was preserved.

In your latest build sleek-2.0.4-rc.1 completing task in non-strict mode with t: and due: is broken. Time window is gone - t: and due: are set to the same value. I've just made a remark that you have introduced behaviour that's present for some reason in SimpleTask (which I consider a bug). My guess was that you were inspired/misled by using SimpletTask app.

To be honest, I don't know. This solution was contributed by @zerodat a long time ago. When I rewrote sleek I basically recreated his logic. And probably broke it now ;)
I simply assumed this is how this feature is working in other apps.

I'm not questioning the logic or wiki page. In fact I've even contributed that very page on your request 🤣:
#193 (comment)
#193 (comment)

I am open for a discussion about how the recurrence feature should work.

Could you just try to fix this case?

After completing the task, with v2.0.4-rc.1:

2023-12-13 task3 - non-strict recurrence rec:1m t:2024-01-13 due:2024-01-13

should be:

2023-12-13 task3 - non-strict recurrence rec:1m t:2024-01-04 due:2024-01-13

That would completely solve this issue 😃

@ransome1
Copy link
Owner

@amariusz can you send me an example for each case we need to cover? I can then share the results without. This might be easier than building another pre-release.

@amariusz
Copy link
Collaborator Author

I've already did that in my first two messages in this thread. Those are all the cases recurrence wise.
For clarity and confirmation I will list them here again before and after completion on 2023-12-14.

Before

2022-01-01 task1 - non-strict recurrence rec:1d t:2023-01-01
2022-01-01 task2 - strict recurrence rec:+1d t:2023-01-01

2022-01-01 task3 - non-strict recurrence rec:1m t:2023-01-01 due:2023-01-10
2022-01-01 task4 - strict recurrence rec:+1m t:2023-01-01 due:2023-01-10

2022-01-01 task5 - non-strict recurrence rec:1m due:2023-01-10
2022-01-01 task6 - strict recurrence rec:+1m due:2023-01-10

After

x 2023-12-14 2022-01-01 task1 - non-strict recurrence rec:1d t:2023-01-01
2023-12-14 task1 - non-strict recurrence rec:1d t:2023-12-15
x 2023-12-14 2022-01-01 task2 - strict recurrence rec:+1d t:2023-01-01
2023-12-14 task2 - strict recurrence rec:+1d t:2023-01-02

x 2023-12-14 2022-01-01 task3 - non-strict recurrence rec:1m t:2023-01-01 due:2023-01-10
2023-12-14 task3 - non-strict recurrence rec:1m t:2024-01-05 due:2024-01-14
x 2023-12-14 2022-01-01 task4 - strict recurrence rec:+1m t:2023-01-01 due:2023-01-10
2023-12-14 task4 - strict recurrence rec:+1m t:2023-02-01 due:2023-02-10

x 2023-12-14 2022-01-01 task5 - non-strict recurrence rec:1m due:2023-01-10
2023-12-14 task5 - non-strict recurrence rec:1m due:2024-01-14
x 2023-12-14 2022-01-01 task6 - strict recurrence rec:+1m due:2023-01-10
2023-12-14 task6 - strict recurrence rec:+1m due:2023-02-10

I've made detailed breakdown of all the cases back here: #193 (comment)
It boils down to 6 cases.

Thanks,

@ransome1
Copy link
Owner

Ok good news I guess. I used your cases and was able to create the expected output, of course in a different order:

x 2023-12-14 2022-01-01 task1 - non-strict recurrence rec:1d t:2023-01-01
x 2023-12-14 2022-01-01 task2 - strict recurrence rec:+1d t:2023-01-01
x 2023-12-14 2022-01-01 task3 - non-strict recurrence rec:1m t:2023-01-01 due:2023-01-10
x 2023-12-14 2022-01-01 task4 - strict recurrence rec:+1m t:2023-01-01 due:2023-01-10
x 2023-12-14 2022-01-01 task5 - non-strict recurrence rec:1m due:2023-01-10
x 2023-12-14 2022-01-01 task6 - strict recurrence rec:+1m due:2023-01-10
2023-12-14 task1 - non-strict recurrence rec:1d t:2023-12-15
2023-12-14 task2 - strict recurrence rec:+1d t:2023-01-02
2023-12-14 task3 - non-strict recurrence rec:1m t:2024-01-05 due:2024-01-14
2023-12-14 task4 - strict recurrence rec:+1m t:2023-02-01 due:2023-02-10
2023-12-14 task5 - non-strict recurrence rec:1m due:2024-01-14
2023-12-14 task6 - strict recurrence rec:+1m due:2023-02-10

It will be included in the next pre release, feel free to give it another proper testing.

@amariusz
Copy link
Collaborator Author

Fantastic!

@amariusz
Copy link
Collaborator Author

I've just realized there's one more specific recurrence case - no t: and no due:.

Before completion

2023-12-20 Task that's always available rec:0d

After completion

x 2023-12-17 2023-12-20 Task that's always available rec:0d
2023-12-17 Task that's always available rec:0d due:2023-12-18

Personally I'd prefer behaviour without adding due: but that's how Sleek 1.3.2 was working and what was proposed by @zerodat here #193 (comment).

I assume this case is useful if you want to have ability to execute the same task multiple times during the day and maintain history of executions. Adding due: makes little sense IMO.

@ransome1
Copy link
Owner

ransome1 commented Dec 18, 2023

Since I have never integrated recurrences into my own personal workflow, I probably don't have a qualified opinion on this. Just by judging it from the distance, it seems quiet like an edge case to me.

What you're proposing, @amariusz , is to simply create an exact copy of the todo?

@zerodat @stephprobst what are your takes on it?

@ransome1
Copy link
Owner

@amariusz https://github.com/ransome1/sleek/releases/tag/v2.0.4-rc.2 doesn't do anything about your last post, but it should have included all prior fixes.

@stephprobst
Copy link
Contributor

I don't have a strong preference, but a slight tendency towards thee proposal of @amariusz to create the new task without a due date. I don't see why we would need to add a due date if there wasn't one before. The scenario of completing a task multiple times a day and keeping track of how many times and when the task was completed seems reasonable.

@amariusz
Copy link
Collaborator Author

What you're proposing, @amariusz , is to simply create an exact copy of the todo?

That's almost right - new task should have creation date set to today (which is current behaviour). rec: value doesn't really matter and should be preserved. However rec:0 seems like a good user convention.

@amariusz https://github.com/ransome1/sleek/releases/tag/v2.0.4-rc.2 doesn't do anything about your last post, but it should have included all prior fixes.

I've just tried it. Works great with 6 main cases!

@amariusz
Copy link
Collaborator Author

I don't have a strong preference, but a slight tendency towards thee proposal of @amariusz to create the new task without a due date.

I've done some comparison testing with simpletask and topydo. Unfortunately the behaviour is different in both apps.
topydo is adding due: like Sleek 1.3, although handles rec:0d case better by adding 0 not 1.
simpletask is not adding due:.

IMO there's no benefit to automatically adding due for recurring tasks - it can be added once explicitly if its presence is needed.

If you agree and it's something easy to fix it would be be great to include this change within this issue. But I'm also OK with closing this issue, as I feel the main work is completed. This can serve as a reference information for future purposes.

I'll let you decide. What do you think? @ransome1 @stephprobst

Before

2023-12-20 task7 - always available recurrence rec0d rec:0d 
2023-12-20 task8 - always available recurrence rec1d rec:1d
2023-12-20 task9 - always available recurrence rec2d rec:2d

After in sleek2

2023-12-19 task7 - always available recurrence rec0d rec:0d  due:2023-12-20
2023-12-19 task8 - always available recurrence rec1d rec:1d due:2023-12-20
2023-12-19 task9 - always available recurrence rec2d rec:2d due:2023-12-21
x 2023-12-19 2023-12-20 task7 - always available recurrence rec0d rec:0d 
x 2023-12-19 2023-12-20 task8 - always available recurrence rec1d rec:1d
x 2023-12-19 2023-12-20 task9 - always available recurrence rec2d rec:2d

After in simpletask

2023-12-19 task7 - always available recurrence rec0d rec:0d 
2023-12-19 task8 - always available recurrence rec1d rec:1d
2023-12-19 task9 - always available recurrence rec2d rec:2d
x 2023-12-19 2023-12-20 task7 - always available recurrence rec0d rec:0d 
x 2023-12-19 2023-12-20 task8 - always available recurrence rec1d rec:1d
x 2023-12-19 2023-12-20 task9 - always available recurrence rec2d rec:2d

After in topydo

2023-12-19 task7 - always available recurrence rec0d rec:0d due:2023-12-19
2023-12-19 task8 - always available recurrence rec1d rec:1d due:2023-12-20
2023-12-19 task9 - always available recurrence rec2d rec:2d due:2023-12-21
x 2023-12-19 2023-12-20 task7 - always available recurrence rec0d rec:0d
x 2023-12-19 2023-12-20 task8 - always available recurrence rec1d rec:1d
x 2023-12-19 2023-12-20 task9 - always available recurrence rec2d rec:2d

@ransome1
Copy link
Owner

I adjusted the logic and this would be the outcome. Can you please review it @amariusz ?

Today is 2023-12-19

Case 1

2023-12-19 task7 - always available recurrence rec0d rec:0d
creates
2023-12-19 task7 - always available recurrence rec0d rec:0d

Case 2

2023-12-19 task7 - always available recurrence rec0d rec:0d due:2024-01-01
creates
2023-12-19 task7 - always available recurrence rec0d rec:0d due:2023-12-19

Case 3

2023-12-19 task7 - always available recurrence rec0d rec:0d due:2024-03-01 t:2024-02-03
creates
2023-12-19 task7 - always available recurrence rec0d rec:0d due:2023-12-19 t:2023-11-22

Case 4

2023-12-19 task7 - always available recurrence rec0d rec:+0d due:2024-03-01 t:2024-02-03
creates
2023-12-19 task7 - always available recurrence rec0d rec:+0d due:2024-03-01 t:2024-02-03

Case 5

task7 - always available recurrence rec0d rec:0d due:2024-03-01
creates
2023-12-19 task7 - always available recurrence rec0d rec:0d due:2023-12-19

@amariusz
Copy link
Collaborator Author

Some other edge test cases I haven't thought of :) Everything looks fine to me.

@ransome1
Copy link
Owner

Those adjustments are available in https://github.com/ransome1/sleek/releases/tag/v2.0.4-rc.3

@amariusz
Copy link
Collaborator Author

That's almost perfect but some issues remain:

Before

2023-12-20 task8 - always available recurrence rec1d rec:1d
2023-12-20 task9 - always available recurrence rec2d rec:2d
task7 - always available recurrence rec0d rec:0d due:2024-03-01

After

x 2023-12-19 2023-12-20 task8 - always available recurrence rec1d rec:1d
x 2023-12-19 2023-12-20 task9 - always available recurrence rec2d rec:2d
x 2023-12-19 2023-12-19 task7 - always available recurrence rec0d rec:0d due:2024-03-01

2023-12-19 task8 - always available recurrence rec1d rec:1d due:2023-12-20
2023-12-19 task9 - always available recurrence rec2d rec:2d due:2023-12-21
2023-12-19 task7 - always available recurrence rec0d rec:0d due:2023-12-19

rec:1d and rec:2d still results in added due date. rec:0d is OK
For task7 case Sleek creates completed version which has creation date set to today, but that may not be true.

Should be

x 2023-12-19 2023-12-20 task8 - always available recurrence rec1d rec:1d
x 2023-12-19 2023-12-20 task9 - always available recurrence rec2d rec:2d
x 2023-12-19 task7 - always available recurrence rec0d rec:0d due:2024-03-01

2023-12-19 task8 - always available recurrence rec1d rec:1d 
2023-12-19 task9 - always available recurrence rec2d rec:2d
2023-12-19 task7 - always available recurrence rec0d rec:0d due:2023-12-19

@ransome1
Copy link
Owner

I was under the impression we were just about the rec:0d edge case. Anyhow, it is just a line of code to adjust in order to avoid sleek from adding a due date, if there was no due date present in the completed todo.

Time to wrap this feature up ;) We must not forget to adjust the wiki, once 2.0.4 is published. It won't be accurate any longer.

@amariusz
Copy link
Collaborator Author

amariusz commented Dec 20, 2023

I see your point now.

I assumed we would choose between simpletask's or topydo's behaviour as described here. I also didn't realize that automatic addition of due date is explicitly documented in the wiki, and actually there may be some users that are accustomed to this behaviour.

Also taking under consideration that such task 2023-12-19 task9 - always available recurrence rec:2d looks somehow inconsistent (because 2d has no meaning) compared to 2023-12-19 task9 - always available recurrence rec:0d, I'm also in favor of just treating rec:0d case specially.

Looks like cherry-picking edge cases from similar todo.txt tools but I hope we will end up with most thoughtful and useful set :) From my perspective Sleek 2.0 already has the best recurrence implementation 💪

So the only issue that remains is that artificial creation date added to completed task:

Before

task7 - always available recurrence rec0d rec:0d due:2024-03-01

After

x 2023-12-19 2023-12-19 task7 - always available recurrence rec0d rec:0d due:2024-03-01
             ^^^^^^^^^
2023-12-19 task7 - always available recurrence rec0d rec:0d due:2023-12-19

@ransome1
Copy link
Owner

So the only issue that remains is that artificial creation date added to completed task:

I agree, unfortunately this can only be solved upstream and there is not much going on recently :/

@amariusz
Copy link
Collaborator Author

amariusz commented Dec 20, 2023

Then it's as good as it gets! 😀
Again, thank you both for your commitment in ironing out all these nuanced cases.
It took some back and forth, but I belive the result was worth it.

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

3 participants