Regression: Background list length computation not happening in the background any more

[romanofski]

Describe the bug It seems we've regressed when it performing a query resulting in many threads.

To Reproduce Steps to reproduce the behavior:

1. Change the query in the thread view with : to *, hit Enter

Expected behavior Result list should be instant while the length is being computed. Instead it takes a long time to perform the query.

Screenshots None

Additional context None

[frasertweedale]

Should be straightforward to bisect and find the commit that introduced this regression. Hopefully it will be as easy to work out how to fix it.

[romanofski]

Boy, this has been b0rked for a long time. If my bisect foo is right, it's this patch which introduced it:

commit e72afdb6827d4bde10dce97ce8f6fe4c4146d1f9 (HEAD, refs/bisect/bad)
Author: Róman Joost <roman@bromeco.de>
Date:   Thu Feb 6 19:41:07 2020 +1000

    Bulk actions

    This implements the support for bulk actions, currently limited to
    tagging only. The major part of this patch is the support for tagging
    multiple items (either mail or thread) from our Brick list widgets now
    able to hold a toggle state in the list item itself.

    Fixes https://github.com/purebred-mua/purebred/issues/5

[romanofski]

I've looked into this today. Still not a faintest idea what could cause it, but it's definitely the commit in my previous comment. I thought:

@@ -226,30 +229,33 @@ getThreads
   :: (MonadError Error m, MonadIO m)
   => T.Text
   -> NotmuchSettings FilePath
-  -> m (V NotmuchThread)
+  -> m (V (SelectableItem NotmuchThread))
 getThreads s settings =
   withDatabaseReadOnly (view nmDatabase settings) $
     flip Notmuch.query (Notmuch.Bare $ T.unpack s)
     >=> Notmuch.threads
-    >=> liftIO . fmap (fromList 128) . lazyTraverse threadToThread
+    >=> liftIO . fmap (fromList 128) . lazyTraverse (fmap (False,) . threadToThread)

would be prime candidate. Perhaps the additional fmap? Checked this by simply changing threadToThread from:

threadToThread
  :: Notmuch.Thread a
  -> IO NotmuchThread

to

threadToThread
  :: Notmuch.Thread a
  -> IO (Toggleable NotmuchThread)

and it's not it.

Now I wonder whether some changes in purebred/src/UI/Actions.hs is the culprit ...

[romanofski]

Just changed the initial search of threads when Purebred starts up to * in order to isolate whether it's notmuch related or it's one of our actions. The problem already starts there and Purebred takes a while to show the entire list of threads.

So the call stack for loading the threads looks like:

Notmuch.query -- from Storage.Notmuch which is calling `lazyTraverse`
getThreads -- from UI.Actions
applySearch -- from UI.App

and in applySearch in order to display them we go:

      liftIO (zonedTimeToUTC <$> getZonedTime) >>= assign asLocalTime
      notifyNumThreads threads
      modifying (asThreadsView . miThreads . listList) (L.listReplace threads (Just 0))
      assign (asThreadsView . miThreads . listLength) Nothing

[romanofski]

Attached a profiling output when running Purebred with the default config changed to do a * query upon start up purebred-linux-x86_64.prof.txt

[romanofski]

Found the problematic line. It's the status bar renderer showing how many items are marked:

renderToggled :: AppState -> Widget n
renderToggled s =
  let currentL = case focusedViewWidget s of
        ListOfThreads -> length $ toListOf (asThreadsView . miListOfThreads . traversed . filtered fst) s
        ListOfFiles -> length $ view (asFileBrowser . fbEntries . to FB.fileBrowserSelection) s
        _ -> length $ toListOf (asThreadsView . miListOfMails . traversed . filtered fst) s
  in if currentL > 0 then str $ "Marked: " <> show currentL else emptyWidget

In hindsight kind of an obvious thing to look for. Just had expected it to be somewhere else. Not sure how I test it tho, so it doesn't regress again.

[romanofski]

Hm... going back over the code we update the length via a separate thread to keep all this lazy loading from happening. I'm currently inclined to simply not show how many items are marked in the status bar, since the UI gives already feedback via the list item colours. Knowing how many are toggled is perhaps something we can ignore for 1.0 ...

[frasertweedale]

@romanofski now that I understand the problem, I think I know how we can still solve it without forcing the list. We can count up the marked elements while traversing the spine in the background, so there is a just a delay until we report it. Need to be a little more clever in order to handle toggling that happens in the foreground prior to spine traversal completion, but that's the idea. But I'd be keen to solve that when we next get together to hack on purebred, and pair.

[romanofski]

@frasertweedale Yeah I also had a different idea towards keeping the marked state maybe not on the item itself. However at this point in time I think we should concentrate on #284 which is currently the only one left for our first release. Otherwise I fear a 1.0 release is never going to happen ...

[frasertweedale]

@romanofski I agree, we can push the proper solution for show number of marked items post-1.0. I'll review the PR for this issue soon.