[RFC] Refurbishing Recurring/Repeating tasks

zeddo123 commented 3 years ago

We have received many issues and in hindsight I took short sided decisions about recurring tasks. That's why I opening this issue so that we can discuss how we might proceed.

New features

Ability to repeat every x weeks/month/days #627
Ability to have multiple recurring tasks #614
Ability to select whether the task repeats on due dates or start dates

For these features, we could use dateutil proposed by @jaesivsm and @diegogangl.

History and duplicate tasks

This issue was raised here #636. The best behavior (if we want to preserve the "history") would be that then the task is re-opened, the new tasks gets deleted and replaced by the old one. Otherwise, we just update the due date.

Changing the name

I don't even know why I first chose recurring tasks instead of repeating. That's why I'm proposing to change the naming. The only issue with this is that some users might have gotten used to it.

I had an unexpected busy and stressful week, so I couldn't discuss it on IRC or even label issues. Now hopefully I can.

jaesivsm commented 3 years ago

I haven't had time to touch what I started recently (busy week also) but I can open a draft PR with my work in progress.

I'm more of a backend guy than anything. So, unless there is some amazing designer hidden somewhere I want to underline that the recursion rule RFC (somewhere in there) is very (and I mean very) flexible. I don't think we will (at least not on the first draft) have a UI that'll enable every possible combination. I don't think it's needed also.

What we have :

every day (iCalendar : DAILY)
other day (iCalendar DAILY, INVERTVAL=2)
every week (iCalendar WEEKLY,BYWEEKDAY=(<str>))
every month (iCalendar MONTHLY,BYDAY=(<int>))
every year (iCalendar YEARLY,BYMONTH=(<int>),BYDAY=(<int>))

What's easy to add

Support for whatever interval : turn other-day into every {2} {days} by making interval a parameter (we then could have for example every {3} {month} => quarterly)
Multiple recursion : weekly, multiple day => rrule basics + checkbox instead of select on weekdays
Adding dtstart selection seems overkill and counter intuitive (see my proposal in the last section of this post)

What'll be impossible

fine representation of every combination (for example YEARLY,BYMONTH=(1,2,3),BYDAY=(4,10,31),BYWEEKDAY=(TU,MO) will be processed fine by datetime.rrule (I guess) but I don't even want to imagine how to represent that in the UI)

recursion start and duplication fix

For that part I'd take a rather different approach :

drop task duplication entirely
status is now a property
- returns real status if not done or not recursion rule
- if task.closed_date and datetime.now() > task.next_occurence > task.closed_date => not done
rrule comes with it's own dtstart but :
- on the fly replace it with task start_date if available

diegogangl commented 3 years ago

Changing the name

Yeah I think repeating would be easier to grasp

I don't think we will (at least not on the first draft) have a UI that'll enable every possible combination. I don't think it's needed also.

We don't need to support every combination, just the most common uses. I think the discourse guys called it "user driven design", the issues that were opened are the cases we should work to cover in the UI. The other crazy could be exposed in the Quick Add field.

recursion start and duplication fix

For that part I'd take a rather different approach :

drop task duplication entirely
status is now a property
    returns real status if not done or not recursion rule
    if task.closed_date and datetime.now() > task.next_occurence > task.closed_date => not done
rrule comes with it's own dtstart but :
    on the fly replace it with task start_date if available

By status you mean the repeat status (enabled) or the task status?

jaesivsm commented 3 years ago

By status you mean the repeat status (enabled) or the task status?

The task status. I would also propose dropping all properties but the recursion rule (or repeating) attached to the task. Recursion would be enabled if the rule isn't None, otherwise it wouldn't.

diegogangl commented 3 years ago

The task status

Not sure if this is what you mean but that's sort of what I did in the new core branch: https://github.com/getting-things-gnome/gtg/blob/new_core/GTG/core/tasks2.py#L78

I would also propose dropping all properties but the recursion rule (or repeating) attached to the task. Recursion would be enabled if the rule isn't None, otherwise it wouldn't.

Sounds like a good idea in principle, but depends on whether we can get structured info from that string (can't see it in the dateutils docs). We need to know the different options to show them in the UI

jaesivsm commented 3 years ago

I was thinking of something like that

class Task:
    [...]
    def __init__(self):
        [...]
        self._status = Status.ACTIVE
    [...]
    @property 
    def status(self):
        if self._status is not Status.DONE or not self.recursion_rule:
            return self._status
        if datetime.now() > task.next_occurence > task.closed_date:
            return Status.ACTIVE
        return self._status

where next_occurence is based on dateutil.rrule.rrule.after()

Sounds like a good idea in principle, but depends on whether we can get structured info from that string (can't see it in the dateutils docs). We need to know the different options to show them in the UI

rrule can be serialized as a string but is an object with :

recurrence (DAILY, WEEKLY...)
interval (int)
bymonthday (int)
byweekday (MO, TU, ...)
bymonth (int)

I've started to write a wrapper to interact with it (take a look).

edit: copy paste fury

diegogangl commented 3 years ago

I'm not too keen on the property idea TBH. Imagine running that function (at least) once for every task. In a list with +1K tasks it would add some noticeable overhead. We could add a fourth status (Status.REPEATING). So we treat them differently when closing or re-opening.

That wrapper is exactly what I was thinking about :ok_hand:

jaesivsm commented 3 years ago

We could add a fourth status (Status.REPEATING). So we treat them differently when closing or re-opening.

That'd be complicated since then we couldn't have a DONE AND REPEATING task.

Imagine running that function (at least) once for every task.

I don't think it'll affect the perf that much (from what I saw, it's mostly gtk boiler plate codes that takes most of the lag time). Anyway, implementing a caching mechanism limited in time with some logic to avoid thundering herd over the code above would not be very complicated.

diegogangl commented 3 years ago

That'd be complicated since then we couldn't have a DONE AND REPEATING task.

For the closed pane? I was thinking in that case we could duplicate tasks when they are closed. When re-opening, look for a task with the same name set to repeating and don't re-open.

jaesivsm commented 3 years ago

status represent the actual status of a task like DONE, ACTIVE. When you had the repeating status to the mix it makes the whole logic fuzzy. I'm quite sure it's a technical choice to be regretted down the line (with filtering, displaying or anything) and we better keep things simple and straightforward.

On the case of duplication of the task :

It doesn't conserve the unity of a task, obviously. For a single task in the active panel, we could have hundreds in the closed one.
I see it as very bug prone as reported by #636. Since it's data creation on a user action, a bug likely mean that the internal mechanics of GTG are exposed when something goes wrong, and the user is left facing either multiple open tasks with the same name or stuff like that. It won't look good.
From a performance point of view, it's fills up the db with dead tasks, it can't be good on space complexity, especially with user who never deletes their done tasks. The shortest recursion right now is daily, but consider the volume with some hourly task or anything like that. (Even with daily tasks, big dataset are in the 300-500 tasks let's say, in a month you'd be adding up to 10% of that !)
From my code snippet above, the point was that making a comparison on accessing Task.status might be slow : I know is not None comparison is very quick, and it'll definitely be more interesting to do that each time we ask for status than filling up the db.

Neui commented 3 years ago

... it can't be good on space complexity, especially with user who never deletes their done tasks.

There is a feature (enabled by default AFAIK) to delete done tasks after 30 days, look into the preferences.

diegogangl commented 3 years ago

I do agree that the duplicating solution isn't ideal, but it's the only way with our current model + treeview system. If you want to do things more nicely we can work them on the new core. I didn't port any of the repeating task code

zeddo123 commented 3 years ago

I've been playing a bit with dateutil and rrules, and I was able to decrease the amount of code that was used to deal with repeating tasks. You can have a look at what I've been playing around with here, although a lot could change depending on how we rewrite the core.

getting-things-gnome / gtg