On Mon, Apr 02, 2018 at 03:12:49PM +0200, Gero Treuner wrote: > I felt that it would be nice when mutt instantly notifies about new > mail. Think about mails like "There is on piece of cake left!".
Hi Gero, Sorry it took a bit longer than I expected to get back to your review. I have taken a _quick_ look at the patch. Overall I like the idea and (with some fixes) think this would be worth including. I'll take a more thorough look when I have a bit more time. A few comments: * I greatly appreciate you using a minimally invasive approach. It's tempting to want to rewrite things, but that makes my review job much harder. * Some of the other callers of mutt_getch() may be adversely affected by the patch. e.g., _mutt_enter_fname() may now abort the prompt if a new mail notification happens at the wrong time. Hmmm... it looks like the other callers are okay because they now handle SIGWINCH redraws, but I'll want to take a second look to make sure all the callers properly handle a timeout retval. * I need to test this myself, but I'm assuming the poll() in mutt_monitor_poll() will just return 0 when the timeout() set in km_dokey() fires? * Why not add and remove monitors in the same locations that mutt_sb_notify_mailbox() is called inside mutt_parse_mailboxes()? I think that makes more sense than where you are doing it now. * I'd rather not have private data, e.g. INotifyFd, Monitor, exposed in monitor.h, unless there is a good reason. * I think we should have some kind of configure.ac option to "opt out" of the inotify, even if your system has it. There may be a reason someone doesn't want to use it. I need to release 1.9.5 this week, and would like to release 1.10.0 the beginning of next month. It's a bit late in the cycle to include this for 1.10, so how about if we work toward getting it committed after 1.10.0 is released? -- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
