Automatic mark mail as read #56

romanofski · 2017-09-03T05:08:39Z

No description provided.

romanofski · 2017-09-03T05:11:56Z

src/Storage/Notmuch.hs

+        Nothing -> pure $ Left ("Tags are corrupt")
+        Just nmtags ->
+            bracket
+                (databaseOpen dbpath >>= either (error . show) pure)


I've fiddled with the error here. This would obviously crash, but the Type expected in IO is always (Database RW), but I'm puzzled, since I thought the first function handles the Left value, which is String. What I want is to give an error back, rather than... well just crashing.

We're going to need to start using MonadError monad transformer to be able to better manage IO-with-error scenarios. For now leave it as-is and I'll address in a later commit (just add a FIXME comment)

I've moved it in a backlog item... when we have the time.

romanofski · 2017-09-03T05:15:12Z

Travis died as well with the change. Will have to investigate. Wouldn't be surprised if it has something to do with the old, but stable version of Ubuntu sigh. I'm also thinking of adding one additional acceptance test to check if toggling works. Not sure if it will run in Travis tho, since I've noticed that I dealt with different escape sequences to assert e.g. text is bold or not.

romanofski · 2017-09-04T07:41:05Z

Hm.. I've got trouble building hs-notmuch now (see travis error). I actually ran into the same error locally as well, but I was unable to reproduce it 100%. In case you know what's going on on top of year head please share :))

frasertweedale · 2017-09-04T13:41:14Z

@romanofski there are breaking API changes in notmuch v0.25 :( Might take a little while to sort out.

edit: can probably fix it tomorrow. An old-ish version of libnotmuch (0.17) is used in Ubuntu trusty though. Fortunately there's only one incompatibility there though. I'll have to change the type of databaseDestroy though.

romanofski · 2017-09-05T02:43:33Z

@frasertweedale I'd be happy to add in a .travis and stack.yml file. stack just because most of the infrastructure is now build around it, including ready-made files for testing against several platforms/GHC.

frasertweedale · 2017-09-05T04:23:24Z

@romanofski IMO the "correct" approach for hs-notmuch is to determine which versions of notmuch we want to support, and have CI that checks out every single release, builds libnotmuch, then builds hs-notmuch against it. But that's a task for another day.

edit: created purebred-mua/hs-notmuch#13

frasertweedale · 2017-09-05T14:15:53Z

@romanofski well I didn't quite get it done today. But the changes are well on the way. Hopefully Wednseday.

frasertweedale · 2017-09-06T09:31:55Z

@romanofski new hs-notmuch commit which should hopefully fix CI issues, add support for notmuch-0.25, and introduces the monad transformer for dealing with errors. Lots of API changes, you will have fun chasing this.

purebred-mua/hs-notmuch@5bc75a6

The most important function you'll need to perform the refactor/rebase is https://hackage.haskell.org/package/mtl-2.2.1/docs/Control-Monad-Except.html#v:runExceptT.

romanofski · 2017-09-07T11:32:06Z

Does it build?

romanofski · 2017-09-07T11:34:43Z

Meh now I realize that I've squashed my changes against latest hs-notmuch just with the parent. Should have perhaps fiddled it into the separate patches :/ Could probably do that with a few items I still want to "clean up"

romanofski · 2017-09-07T11:36:00Z

Alright doesn't build... perhaps we should yank hs-notmuch into travis first ...

This is needed to later lookup the message again.

Instead of keeping a persistent attribute, make the decision to draw a mail as "unread" during rendering.

romanofski · 2017-09-08T11:33:39Z

I think I'm getting there to where I want to have this and you can start having a look. I think the intermittend test failures are caused by unstable search results, since the maildir I'm using exposes two mails with different content. That's why I created purebred-mua/hs-notmuch#15

Anyway, have a look and tell me what you think.

frasertweedale

A few nits. One kinda important thing. Haven't tested yet.

frasertweedale · 2017-09-09T05:08:35Z

src/Storage/Notmuch.hs

+              >>= messages
+      mails <- liftIO $ mapM messageToMail msgs
+      return $ Vec.fromList mails
+  ) >>= pure . either (Left . show) pure


instead of

runExceptT (k) >>= pure . either (Left . show) pure

Why not:

first show <$> runExceptT (k)

(Data.Bifunctor.first)

Also, does getMessages run multiple times? Either we should open the database somewhere else and pass it around, or you should use databaseDestroy to close the database after each use (otherwise we leak memory
and probably other resources).

Many thanks for the review. Yes getMessages is used a few times, certainly every time when you changes the search. Would it make sense to open the database with the config?

One other thing which comes to my mind is using (resourceT)[https://hackage.haskell.org/package/resourcet] on the client side, to always close the database ... Currently I'd be leaning towards the resourcet approach and always closing the database after reading.

ResourceT, maybe. Let's not do it in this rebase. Something to look at after perhaps.

frasertweedale · 2017-09-09T05:12:04Z

src/Storage/Notmuch.hs

+    Just nmtags ->
+      bracket (runExceptT (databaseOpen dbpath) >>= either (error . show) pure)
+              (void . runExceptT . databaseDestroy)
+              (\db -> tagsToMessage nmtags m db)


Yeah, bracket is a bit awkward because it forces IO; the transformer is unhelpful here :/
Not much we can do about it right now.

frasertweedale · 2017-09-09T05:14:34Z

src/Storage/Notmuch.hs

+addTag :: NotmuchMail -> T.Text -> NotmuchMail
+addTag m t =
+    let xs = view mailTags m
+    in set mailTags (xs `union` [t]) m


Might be nicer to use over here (also in removeTag)

oh yes.. much, much better!! Cheers for that.

frasertweedale · 2017-09-09T05:14:55Z

src/Storage/Notmuch.hs

-    :: HasTags (Message n RO)
-    => Message n RO
+    :: HasTags (Message n a)
+    => Message n a


frasertweedale · 2017-09-09T05:20:34Z

src/UI/Index/Keybindings.hs

+            in do setNotmuchMailTags dbpath (op m newTag)
+                  >>= either
+                      (\err -> pure $ s & asError ?~ err)
+                      (pure . updateMailInList s)


perhaps the following instead of (>>=)

either (\err -> set asError (Just err) s) (updateMailInList s) <$> setNotmuchMailTags dbpath (op m newTag)

Nice. Cheers.

This adds functionality to automatically set e-mail as `read` as well as allowing to toggle the mail back into the 'unread' state. Currently the Keybinding to toggle unread is set to `t` (for toggle). Issue: #53

This includes a small re-factoring of moving all defined style attributes into the config, which will make it easier to define themes in the future.

Allow easier debugging by running the tests with a "DEBUG=1" environment variable, which prints all captured output to STDERR.

Using the new debug mode, capture all STDERR output into a log file and print it after all tests are run. This will hopefully make it easier to debug failures.

This fixes an intermittently breaking test, which has a timing problem. The test driver waits for a string which is already present on the screen capture which results not in waiting for a re-draw. Since the body is not shown when all headers are visible, we use the string in the mail body to wait for to circumvent the timing problem.

romanofski · 2017-09-10T11:29:25Z

src/Storage/Notmuch.hs

+getMessages s settings =
+  first show <$>
+  bracket (runExceptT (databaseOpenReadOnly (view nmDatabase settings))
+           >>= either (error . show) pure)


This is still not right IMHO... it would currently crash the application if I wouldn't be able the open the database properly. Arguably that's pretty bad in itself, but perhaps the application shouldn't crash. Same for the function which opens the database RW for tagging. Any suggestions or leave it for now and keep a backlog item.

Leave for now.

romanofski · 2017-09-10T11:29:53Z

Alright this should be it. I've added a bunch of fix up patches as well and an acceptance test.

romanofski requested a review from frasertweedale September 3, 2017 05:08

romanofski commented Sep 3, 2017

View reviewed changes

romanofski mentioned this pull request Sep 4, 2017

Proper error propagation: Use MonadError transformer #57

Closed

romanofski force-pushed the automatic_mark_mail_as_read branch from 0eb3ce7 to 8cb6254 Compare September 4, 2017 07:14

romanofski force-pushed the automatic_mark_mail_as_read branch from 8cb6254 to f5639d7 Compare September 7, 2017 11:31

romanofski added 4 commits September 8, 2017 20:47

Use latest HEAD of hs-notmuch

281a25a

Retrieve the messageId

e723258

This is needed to later lookup the message again.

Retrieve the messageId

f4b3556

This is needed to later lookup the message again.

Indicate if mail is unread during rendering

f231cc9

Instead of keeping a persistent attribute, make the decision to draw a mail as "unread" during rendering.

romanofski force-pushed the automatic_mark_mail_as_read branch from f5639d7 to 399c02b Compare September 8, 2017 11:15

frasertweedale reviewed Sep 9, 2017

View reviewed changes

toggle mails as read

7b3a8e8

This adds functionality to automatically set e-mail as `read` as well as allowing to toggle the mail back into the 'unread' state. Currently the Keybinding to toggle unread is set to `t` (for toggle). Issue: #53

romanofski force-pushed the automatic_mark_mail_as_read branch 2 times, most recently from 8538e4e to 774a519 Compare September 10, 2017 10:41

romanofski added 5 commits September 10, 2017 21:13

Properly style selected, unread mail in list

6c766f7

This includes a small re-factoring of moving all defined style attributes into the config, which will make it easier to define themes in the future.

Acceptance test automatic set to read feature

53a72d4

tests: Allow easier debugging

806e51f

Allow easier debugging by running the tests with a "DEBUG=1" environment variable, which prints all captured output to STDERR.

ci: run all tests in debug mode

7601c51

Using the new debug mode, capture all STDERR output into a log file and print it after all tests are run. This will hopefully make it easier to debug failures.

romanofski force-pushed the automatic_mark_mail_as_read branch from 774a519 to 76039d3 Compare September 10, 2017 11:14

romanofski commented Sep 10, 2017

View reviewed changes

frasertweedale merged commit 76039d3 into master Sep 11, 2017

frasertweedale mentioned this pull request Sep 11, 2017

Automatically mark mail is "read" #53

Closed

frasertweedale deleted the automatic_mark_mail_as_read branch September 28, 2017 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatic mark mail as read #56

Automatic mark mail as read #56

romanofski commented Sep 3, 2017

romanofski Sep 3, 2017

frasertweedale Sep 4, 2017

romanofski Sep 4, 2017

romanofski commented Sep 3, 2017

romanofski commented Sep 4, 2017

frasertweedale commented Sep 4, 2017 •

edited

Loading

romanofski commented Sep 5, 2017

frasertweedale commented Sep 5, 2017 •

edited

Loading

frasertweedale commented Sep 5, 2017

frasertweedale commented Sep 6, 2017

romanofski commented Sep 7, 2017

romanofski commented Sep 7, 2017

romanofski commented Sep 7, 2017

romanofski commented Sep 8, 2017

frasertweedale left a comment

frasertweedale Sep 9, 2017

frasertweedale Sep 9, 2017

romanofski Sep 9, 2017

romanofski Sep 9, 2017

frasertweedale Sep 9, 2017

frasertweedale Sep 9, 2017

frasertweedale Sep 9, 2017

romanofski Sep 9, 2017

frasertweedale Sep 9, 2017

frasertweedale Sep 9, 2017

romanofski Sep 9, 2017

romanofski Sep 10, 2017

frasertweedale Sep 11, 2017

romanofski commented Sep 10, 2017

Automatic mark mail as read #56

Automatic mark mail as read #56

Conversation

romanofski commented Sep 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanofski commented Sep 3, 2017

romanofski commented Sep 4, 2017

frasertweedale commented Sep 4, 2017 • edited Loading

romanofski commented Sep 5, 2017

frasertweedale commented Sep 5, 2017 • edited Loading

frasertweedale commented Sep 5, 2017

frasertweedale commented Sep 6, 2017

romanofski commented Sep 7, 2017

romanofski commented Sep 7, 2017

romanofski commented Sep 7, 2017

romanofski commented Sep 8, 2017

frasertweedale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanofski commented Sep 10, 2017

frasertweedale commented Sep 4, 2017 •

edited

Loading

frasertweedale commented Sep 5, 2017 •

edited

Loading