alot icon indicating copy to clipboard operation
alot copied to clipboard

Add move next / previous thread command

Open Nicop06 opened this issue 6 years ago • 4 comments

Add a move next thread and move previous thread command allowing the user to move between threads without going back to the search result. The default binding for these commands are J/K.

Nicop06 avatar Sep 01 '19 00:09 Nicop06

Very quickly as I haven't tested this: The code looks neat and clean to me, you'd need to fix the failing tests but it seems that this is an issue of re-generating it's sources: PYTHON="python3" make -C docs cleanall (python if not on debian)..

The bigger issue is if such a command should be included at all. I've deliberately not included it in the past because

  • I wanted to decouple thread from search buffers as much as possible
  • thread buffers do have one "parent search buffer" but that doesn't have to be the case in future, for instance one could imagine opening a thread directly from CLI calls just as the notmuch CLI can. At the moment though, thread buffer do have a reference to "their" search, I can't recall why that was.
  • search buffers are snapshots at the time they are opened and therefore may be outdated later. Some people consider this a feature (because they do not dynamically change), others really want continuously updated search buffers, which would make a feature like this one (semantically) unstable.

pazz avatar Sep 02 '19 07:09 pazz

I updated the PR. This might not be the most elegant solution to the design but it should solve a few cases:

  • ThreadBuffer does not need a SearchBuffer to be initialized.
  • The _search_buffer variable is now a weak reference which means if the thread buffer will not hold reference to the search buffer after it's destroyed. This is not the most elegant code as we cannot get a weak reference to None. But at least it should work. This is inspired by what notmuch implementation in Emacs is doing. Although, in Emacs, hitting the next thread shortcut results in the buffer being destroyed. I am not sure this is the best behavior and it might be an implementation issue.
  • I rebuilt the documentation and the tests are now passing.
  • I think the snapshot issue with search buffers is not just an issue of this PR but a larger issue. I might be a good idea to have an auto-refresh option if some users want that. But in that case I understand that this CL makes things trickier considering the auto-refresh might only be triggered once the search buffer is active. I see two solutions to that: background threads or

Nicop06 avatar Sep 02 '19 23:09 Nicop06

I updated the PR. There is slight issue when 2 threads are opened from the same search buffer. If you, then the focused thread is shared between the 2, so you have the following behavior:

  1. Open tab 1 on thread 1
  2. Open tab 2 on thread 1
  3. Go to next thread on tab 2: tab 2 now displays thread 2
  4. Go to next thread on tab 1: tab 1 now displays thread 3

I checked and notmuch-emacs has the same issue. This can be solved by having each thread keeping track of which item they are focusing. But I don't think this is such an issue anyway so I kept it as it is.

Nicop06 avatar Nov 03 '19 16:11 Nicop06

oh yes of course that must happen! i haven thought of this 😄. The fix you propose should be easy really, as the list walker has getter for the current position and afaik, the next/prev position getter take those positions as optional parameter..

pazz avatar Nov 03 '19 21:11 pazz