Add thread_select_unread setting to open threads at oldest unread message

[klement]

When enabled, opening a thread selects the oldest unread matching message
instead of the oldest matching message. Falls back to the most recent message
if all messages are read. Also fixes a crash in get_last_msg_idx when called
with an invalid root index.

Co-Authored-By: Claude Opus 4.6 [email protected]

[laarmen]

First off, please don't bundle fixes and features in a single commit, it makes the review harder. As a rule of thumb, if there's an "also does that" in your commit message, it's a solid hint it should have been split off 🙂 .

Secondly, adding a configuration flag for this feels wrong to me. The affected method becomes a lot harder to read and reason about, and I really doubt you'd want that behaviour for all searches. IMHO, there are other avenues to solve this that would be less intrusive and/or more broadly useful:

• In your settings, override the thread opening keybinding to immediatly call next_unread. You can either dig through p.app to get the current panel (oddly enough we don't have any helper for that) or do some minimal scaffolding to propagate the panel, e.g. something like this:

diff --git a/dodo/app.py b/dodo/app.py
index 857d9fd..2e3082b 100644
--- a/dodo/app.py
+++ b/dodo/app.py
@@ -194,7 +194,7 @@ class Dodo(QApplication):
         p = search.SearchPanel(self, query, keep_open=keep_open)
         self.add_panel(p)
 
-    def open_thread(self, thread_id: str, query: str) -> None:
+    def open_thread(self, thread_id: str, query: str) -> thread.ThreadPanel:
         """Open a thread panel with the given thread_id
 
         If a panel with this thread_id is already open, switch to it rather than
@@ -204,10 +204,11 @@ class Dodo(QApplication):
             w = self.tabs.widget(i)
             if isinstance(w, thread.ThreadPanel) and (w.thread_id, w.query) == (thread_id, query):
                 self.tabs.setCurrentIndex(i)
-                return
+                return w
 
         p = thread.ThreadPanel(self, thread_id, query)
         self.add_panel(p)
+        return p
 
     def open_compose(self, mode: str='', msg: Optional[dict]=None) -> None:
         """Open a compose panel
diff --git a/dodo/search.py b/dodo/search.py
index bc69842..cb4df49 100644
--- a/dodo/search.py
+++ b/dodo/search.py
@@ -391,12 +391,12 @@ class SearchPanel(panel.Panel):
         pos = self.tree.rect().topLeft()
         self.tree.setCurrentIndex(self.tree.indexAt(pos))
 
-    def open_current_thread(self) -> None:
+    def open_current_thread(self) -> thread.ThreadPanel|None:
         """Open the selected thread"""
 
         thread_id = self.model.thread_id(self.tree.currentIndex())
         if thread_id:
-            self.app.open_thread(thread_id, self.model.q)
+            return self.app.open_thread(thread_id, self.model.q)
     
     def toggle_thread_tag(self, tag: str) -> None:
         """Toggle the given thread tag"""

Which allowed me to write this binding in my local config:

dodo.keymap.search_keymap['Q'] = ('open first unread', lambda p: p.open_current_thread().next_unread())

• Add the ability to refine searches. If you had a method SearchPanel.refine_search(new_search: str), you could have a binding like this:

dodo.keymap.search_keymap['U'] = ('only unread', lambda p: p.refine_search(p.model.q + " and tag:unread"))

where opening a thread would automatically do what you want it to do.

There are other possible avenues, of course.

[klement]

Thanks for your feedback on this. Using your proposed change and keymap works well for threads containing unread messages, but has this weird behaviour when opening a fully read thread as it seems to land on a message somewhere in the middle.

Adjusting the keymap like this seems to fix that:

dodo.keymap.search_keymap['<enter>'] = ('open first unread', lambda p: (
      (t := p.open_current_thread()),
      t and t._select_index(
          u if (u := t.model.next_unread(t.current_index)).isValid()
          else t.model.get_last_msg_idx()
      )
  ))

I’m not familiar enough with the codebase to tell whether this reveals an underlying issue or works as intended though.

[laarmen]

Assuming you are running my patch on top of master, this shouldn't be possible. App.add_panel() calls p.setFocus(), which will seed the data through a refresh() call, at which point you'll have the model fully fed and the current index set to whatever default_message() says.

[laarmen]

@klement Please see #128, it should be better. Note that the approach for the binding is slightly different.