Broken non-strict recurrence

amariusz commented 6 months ago

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.

ransome1 commented 6 months ago

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

amariusz commented 6 months ago

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 commented 6 months ago

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

stephprobst commented 6 months ago

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 commented 6 months ago

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

ransome1 commented 6 months ago

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

amariusz commented 6 months ago

Thanks!

ransome1 commented 6 months ago

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

amariusz commented 6 months ago

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

ransome1 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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.

https://github.com/mpcjanssen/simpletask-android/issues/312#issuecomment-767365840

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

Thanks

ransome1 commented 6 months ago

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 commented 6 months ago

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 🤣: https://github.com/ransome1/sleek/issues/193#issuecomment-878933001 https://github.com/ransome1/sleek/issues/193#issuecomment-878007268

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 commented 6 months ago

@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 commented 6 months ago

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: https://github.com/ransome1/sleek/issues/193#issuecomment-875774526 It boils down to 6 cases.

Thanks,

ransome1 commented 6 months ago

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 commented 6 months ago

Fantastic!

amariusz commented 6 months ago

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 https://github.com/ransome1/sleek/issues/193#issuecomment-877862672.

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

ransome1 commented 6 months ago

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

amariusz commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

ransome1 / sleek