fix: bulk complete and delete were hitting the wrong tasks
task IDs are just line numbers from ReadTasks, and the bulk handlers looped over the selection calling CompleteTask/DeleteTask once per ID. each of those re-reads and rewrites todo.txt, so the moment the first task came out every ID below it shifted down and the rest of the loop clobbered whatever landed on those numbers. selecting 1, 2 and 4 in a four-task file archived "first" and "third", left "second" and "fourth" behind, and then failed on ID 4 with "not found". map iteration order is random, so it changed every run. folded it into CompleteTasks/DeleteTasks, which resolve every ID against a single snapshot and rewrite once. the single-task functions just parse an ID and delegate now, so cli output is unchanged. closes 407feb5
diff --git a/actions.go b/actions.go
@@ -76,66 +76,89 @@ func CompleteTask(cfg *Config, args []string) (string, error) {
return "", fmt.Errorf("invalid task ID: %s", args[0])
}
- tasks, err := ReadTasks(cfg.TodoFile)
+ completed, err := CompleteTasks(cfg, []int{taskID})
if err != nil {
return "", err
}
+ if len(completed) == 0 {
+ return "", fmt.Errorf("task %d not found", taskID)
+ }
- var targetTask *Task
- for i, t := range tasks {
- if t.ID == taskID && t.IsValid {
- targetTask = &tasks[i]
- break
- }
+ recurMsg := ""
+ if completed[0].Recurrence != "" {
+ recurMsg = fmt.Sprintf("\ngenerated next recurring task for %s", completed[0].Recurrence)
}
+ return fmt.Sprintf("marked task %d as done and archived to done.txt!%s", taskID, recurMsg), nil
+}
- if targetTask == nil {
- return "", fmt.Errorf("task %d not found", taskID)
+// CompleteTasks marks every task in ids as done in a single read/rewrite.
+// IDs are line numbers, so completing tasks one at a time makes each removal
+// shift the IDs of everything below it.
+func CompleteTasks(cfg *Config, ids []int) ([]Task, error) {
+ tasks, err := ReadTasks(cfg.TodoFile)
+ if err != nil {
+ return nil, err
+ }
+
+ selected := make(map[int]bool, len(ids))
+ for _, id := range ids {
+ selected[id] = true
}
now := time.Now()
dateString := now.Format("2006-01-02")
- var newRecurLine string
- var recurMsg string
- if targetTask.Recurrence != "" {
- newRecurLine, err = generateRecurringTask(*targetTask, now)
- if err != nil {
- return "", fmt.Errorf("error generating recurring task: %v", err)
+ var completed []Task
+ var archived []string
+ var recurrences []string
+
+ for _, t := range tasks {
+ if !t.IsValid || !selected[t.ID] {
+ continue
}
- recurMsg = fmt.Sprintf("\ngenerated next recurring task for %s", targetTask.Recurrence)
+ completed = append(completed, t)
+ archived = append(archived, fmt.Sprintf("x %s %s", dateString, t.RawLine))
+ if t.Recurrence != "" {
+ newRecurLine, err := generateRecurringTask(t, now)
+ if err != nil {
+ return nil, fmt.Errorf("error generating recurring task: %v", err)
+ }
+ recurrences = append(recurrences, newRecurLine)
+ }
+ }
+
+ if len(completed) == 0 {
+ return nil, nil
}
- doneLine := fmt.Sprintf("x %s %s\n", dateString, targetTask.RawLine)
doneFile, err := os.OpenFile(cfg.DoneFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
if err != nil {
- return "", fmt.Errorf("could not open done file: %v", err)
+ return nil, fmt.Errorf("could not open done file: %v", err)
}
defer doneFile.Close()
- _, err = doneFile.WriteString(doneLine)
- if err != nil {
- return "", fmt.Errorf("could not write to done file: %v", err)
+ for _, line := range archived {
+ if _, err := doneFile.WriteString(line + "\n"); err != nil {
+ return nil, fmt.Errorf("could not write to done file: %v", err)
+ }
}
todoFile, err := os.Create(cfg.TodoFile)
if err != nil {
- return "", fmt.Errorf("could not rewrite todo file: %v", err)
+ return nil, fmt.Errorf("could not rewrite todo file: %v", err)
}
defer todoFile.Close()
for _, t := range tasks {
- if t.ID != taskID && t.IsValid {
+ if t.IsValid && !selected[t.ID] {
todoFile.WriteString(t.RawLine + "\n")
}
}
-
- if newRecurLine != "" {
- todoFile.WriteString(newRecurLine + "\n")
+ for _, line := range recurrences {
+ todoFile.WriteString(line + "\n")
}
- finalMsg := fmt.Sprintf("marked task %d as done and archived to done.txt!%s", taskID, recurMsg)
- return finalMsg, nil
+ return completed, nil
}
func generateRecurringTask(t Task, now time.Time) (string, error) {
@@ -194,32 +217,49 @@ func DeleteTask(cfg *Config, args []string) (string, error) {
return "", fmt.Errorf("invalid task ID: %s", args[0])
}
- tasks, err := ReadTasks(cfg.TodoFile)
+ deleted, err := DeleteTasks(cfg, []int{taskID})
if err != nil {
return "", err
}
+ if len(deleted) == 0 {
+ return "", fmt.Errorf("task %d not found", taskID)
+ }
+ return fmt.Sprintf("deleted task %d completely.", taskID), nil
+}
+
+// DeleteTasks removes every task in ids in a single read/rewrite, for the same
+// reason CompleteTasks does: removing one task shifts the IDs of the rest.
+func DeleteTasks(cfg *Config, ids []int) ([]Task, error) {
+ tasks, err := ReadTasks(cfg.TodoFile)
+ if err != nil {
+ return nil, err
+ }
+
+ selected := make(map[int]bool, len(ids))
+ for _, id := range ids {
+ selected[id] = true
+ }
+
+ var deleted []Task
- found := false
todoFile, err := os.Create(cfg.TodoFile)
if err != nil {
- return "", fmt.Errorf("could not rewrite todo file: %v", err)
+ return nil, fmt.Errorf("could not rewrite todo file: %v", err)
}
defer todoFile.Close()
for _, t := range tasks {
- if t.ID == taskID && t.IsValid {
- found = true
+ if !t.IsValid {
continue
}
- if t.IsValid {
- todoFile.WriteString(t.RawLine + "\n")
+ if selected[t.ID] {
+ deleted = append(deleted, t)
+ continue
}
+ todoFile.WriteString(t.RawLine + "\n")
}
- if !found {
- return "", fmt.Errorf("task %d not found", taskID)
- }
- return fmt.Sprintf("deleted task %d completely.", taskID), nil
+ return deleted, nil
}
func ToggleModifier(cfg *Config, args []string, mod string) (string, error) {
diff --git a/tui.go b/tui.go
@@ -175,6 +175,16 @@ func (m *model) saveUndoState() {
}
}
+// selectedIDs pulls the selected task IDs out of the model's set so they can
+// be handed to the batch actions in one go.
+func selectedIDs(selected map[int]bool) []int {
+ ids := make([]int, 0, len(selected))
+ for id := range selected {
+ ids = append(ids, id)
+ }
+ return ids
+}
+
func (m model) Init() tea.Cmd {
return textinput.Blink
}
@@ -628,12 +638,9 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
}
if len(m.selected) > 0 {
m.saveUndoState()
- for id := range m.selected {
- CompleteTask(m.cfg, []string{strconv.Itoa(id)})
- }
- count := len(m.selected)
+ completed, _ := CompleteTasks(m.cfg, selectedIDs(m.selected))
m.selected = make(map[int]bool)
- m.status = fmt.Sprintf("marked %d tasks as done", count)
+ m.status = fmt.Sprintf("marked %d tasks as done", len(completed))
refreshTasks(&m)
} else if len(m.tasks) > 0 {
m.saveUndoState()
@@ -688,12 +695,9 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
}
if len(m.selected) > 0 {
m.saveUndoState()
- for id := range m.selected {
- DeleteTask(m.cfg, []string{strconv.Itoa(id)})
- }
- count := len(m.selected)
+ deleted, _ := DeleteTasks(m.cfg, selectedIDs(m.selected))
m.selected = make(map[int]bool)
- m.status = fmt.Sprintf("deleted %d tasks", count)
+ m.status = fmt.Sprintf("deleted %d tasks", len(deleted))
refreshTasks(&m)
} else if len(m.tasks) > 0 {
m.saveUndoState()