f8/47b0125dac3a68f1c5a4f45ba67b5441ae70c3

   1 Return-Path: <jani@nikula.org>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 26596431FBD\r
   6         for <notmuch@notmuchmail.org>; Thu,  2 Feb 2012 13:55:41 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id ekga1EboEg-f for <notmuch@notmuchmail.org>;\r
  16         Thu,  2 Feb 2012 13:55:40 -0800 (PST)\r
  17 Received: from mail-lpp01m010-f53.google.com (mail-lpp01m010-f53.google.com\r
  18         [209.85.215.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  19         (No client certificate requested)\r
  20         by olra.theworths.org (Postfix) with ESMTPS id CD55A431FAF\r
  21         for <notmuch@notmuchmail.org>; Thu,  2 Feb 2012 13:55:39 -0800 (PST)\r
  22 Received: by lahd3 with SMTP id d3so1670520lah.26\r
  23         for <notmuch@notmuchmail.org>; Thu, 02 Feb 2012 13:55:38 -0800 (PST)\r
  24 Received: by 10.152.128.163 with SMTP id np3mr2414569lab.51.1328219738195;\r
  25         Thu, 02 Feb 2012 13:55:38 -0800 (PST)\r
  26 Received: from localhost (dsl-hkibrasgw4-fe50f800-253.dhcp.inet.fi.\r
  27         [84.248.80.253])\r
  28         by mx.google.com with ESMTPS id i9sm3049561lbz.3.2012.02.02.13.55.35\r
  29         (version=SSLv3 cipher=OTHER); Thu, 02 Feb 2012 13:55:36 -0800 (PST)\r
  30 From: Jani Nikula <jani@nikula.org>\r
  31 To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org,\r
  32         amdragon@MIT.EDU\r
  33 Subject: Re: [PATCH v4 07/11] lib: added interface\r
  34         notmuch_thread_get_flag_messages\r
  35 In-Reply-To: <1328204619-25046-7-git-send-email-markwalters1009@gmail.com>\r
  36 References: <874nv9rv79.fsf@qmul.ac.uk>\r
  37         <1328204619-25046-7-git-send-email-markwalters1009@gmail.com>\r
  38 User-Agent: Notmuch/0.11+139~g4340989 (http://notmuchmail.org) Emacs/23.3.1\r
  39         (i686-pc-linux-gnu)\r
  40 Date: Thu, 02 Feb 2012 23:55:33 +0200\r
  41 Message-ID: <87k444yk6i.fsf@nikula.org>\r
  42 MIME-Version: 1.0\r
  43 Content-Type: text/plain; charset=us-ascii\r
  44 X-BeenThere: notmuch@notmuchmail.org\r
  45 X-Mailman-Version: 2.1.13\r
  46 Precedence: list\r
  47 List-Id: "Use and development of the notmuch mail system."\r
  48         <notmuch.notmuchmail.org>\r
  49 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  50         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  51 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  52 List-Post: <mailto:notmuch@notmuchmail.org>\r
  53 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  54 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  55         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  56 X-List-Received-Date: Thu, 02 Feb 2012 21:55:41 -0000\r
  57 \r
  58 \r
  59 Hi Mark -\r
  60 \r
  61 This is my first look at any version of the series; apologies if I'm\r
  62 clueless about some details... Please find some comments below.\r
  63 \r
  64 BR,\r
  65 Jani.\r
  66 \r
  67 \r
  68 On Thu,  2 Feb 2012 17:43:35 +0000, Mark Walters <markwalters1009@gmail.com> wrote:\r
  69 > The function is\r
  70 > notmuch_thread_get_flag_messages\r
  71 > (notmuch_thread_t *thread, unsigned int flag_mask, unsigned int flags)\r
  72 > \r
  73 > and returns the number of messages with the specified flags on flag_mask.\r
  74 \r
  75 Is the purpose of this function to get the count of messages that have\r
  76 certain flags set, certain flags not set, and certain flags don't-care?\r
  77 \r
  78 At the very least, I think the documentation of the function should be\r
  79 greatly improved.\r
  80 \r
  81 I think the name of the function should be notmuch_thread_count_messages\r
  82 which is like notmuch_query_count_messages, but for messages in threads\r
  83 (and with some extra restrictions).\r
  84 \r
  85 > \r
  86 > This generalises the existing function\r
  87 > notmuch_thread_get_total_messages and\r
  88 > notmuch_thread_get_matched_messages which are retained to maintain\r
  89 > compatibility.\r
  90 > ---\r
  91 >  lib/message.cc |    6 +++---\r
  92 >  lib/notmuch.h  |   15 +++++++++++++--\r
  93 >  lib/thread.cc  |   39 ++++++++++++++++++++++++++-------------\r
  94 >  3 files changed, 42 insertions(+), 18 deletions(-)\r
  95 > \r
  96 > diff --git a/lib/message.cc b/lib/message.cc\r
  97 > index 0075425..d60da83 100644\r
  98 > --- a/lib/message.cc\r
  99 > +++ b/lib/message.cc\r
 100 > @@ -746,7 +746,7 @@ notmuch_bool_t\r
 101 >  notmuch_message_get_flag (notmuch_message_t *message,\r
 102 >                         notmuch_message_flag_t flag)\r
 103 >  {\r
 104 > -    return message->flags & (1 << flag);\r
 105 > +    return message->flags & flag;\r
 106 >  }\r
 107 >  \r
 108 >  void\r
 109 > @@ -754,9 +754,9 @@ notmuch_message_set_flag (notmuch_message_t *message,\r
 110 >                         notmuch_message_flag_t flag, notmuch_bool_t enable)\r
 111 >  {\r
 112 >      if (enable)\r
 113 > -     message->flags |= (1 << flag);\r
 114 > +     message->flags |= flag;\r
 115 >      else\r
 116 > -     message->flags &= ~(1 << flag);\r
 117 > +     message->flags &= ~flag;\r
 118 >  }\r
 119 >  \r
 120 >  time_t\r
 121 > diff --git a/lib/notmuch.h b/lib/notmuch.h\r
 122 > index f75afae..c02e7f4 100644\r
 123 > --- a/lib/notmuch.h\r
 124 > +++ b/lib/notmuch.h\r
 125 > @@ -654,6 +654,16 @@ notmuch_thread_get_thread_id (notmuch_thread_t *thread);\r
 126 >  int\r
 127 >  notmuch_thread_get_total_messages (notmuch_thread_t *thread);\r
 128 >  \r
 129 > +/* Get the number of messages in 'thread' which have the specified\r
 130 > + * flags on flag_mask.\r
 131 > + *\r
 132 > + * This is a more general interface than\r
 133 > + * notmuch_thread_get_total_messages or\r
 134 > + * notmuch_thread_get_matched_messages\r
 135 > + */\r
 136 > +int\r
 137 > +notmuch_thread_get_flag_messages (notmuch_thread_t *thread, unsigned int flag_mask, unsigned int flags);\r
 138 > +\r
 139 >  /* Get a notmuch_messages_t iterator for the top-level messages in\r
 140 >   * 'thread'.\r
 141 >   *\r
 142 > @@ -902,8 +912,9 @@ notmuch_message_get_filenames (notmuch_message_t *message);\r
 143 >  \r
 144 >  /* Message flags */\r
 145 >  typedef enum _notmuch_message_flag {\r
 146 > -    NOTMUCH_MESSAGE_FLAG_MATCH,\r
 147 > -    NOTMUCH_MESSAGE_FLAG_EXCLUDED\r
 148 > +    NOTMUCH_MESSAGE_FLAG_MATCH = (1<<0),\r
 149 > +    NOTMUCH_MESSAGE_FLAG_EXCLUDED = (1<<1),\r
 150 > +    NOTMUCH_MESSAGE_FLAG_MAX  = (1<<2)\r
 151 \r
 152 How are these used by the current lib users at the moment? How will they\r
 153 break with this change?\r
 154 \r
 155 Please align the assignments. \r
 156 \r
 157 >  } notmuch_message_flag_t;\r
 158 >  \r
 159 >  /* Get a value of a flag for the email corresponding to 'message'. */\r
 160 > diff --git a/lib/thread.cc b/lib/thread.cc\r
 161 > index e976d64..542f7f4 100644\r
 162 > --- a/lib/thread.cc\r
 163 > +++ b/lib/thread.cc\r
 164 > @@ -37,8 +37,7 @@ struct visible _notmuch_thread {\r
 165 >  \r
 166 >      notmuch_message_list_t *message_list;\r
 167 >      GHashTable *message_hash;\r
 168 > -    int total_messages;\r
 169 > -    int matched_messages;\r
 170 > +    int flag_count_messages[NOTMUCH_MESSAGE_FLAG_MAX];\r
 171 >      time_t oldest;\r
 172 >      time_t newest;\r
 173 >  };\r
 174 > @@ -226,7 +225,6 @@ _thread_add_message (notmuch_thread_t *thread,\r
 175 >  \r
 176 >      _notmuch_message_list_add_message (thread->message_list,\r
 177 >                                      talloc_steal (thread, message));\r
 178 > -    thread->total_messages++;\r
 179 >  \r
 180 >      g_hash_table_insert (thread->message_hash,\r
 181 >                        xstrdup (notmuch_message_get_message_id (message)),\r
 182 > @@ -319,21 +317,18 @@ _thread_add_matched_message (notmuch_thread_t *thread,\r
 183 >  \r
 184 >      date = notmuch_message_get_date (message);\r
 185 >  \r
 186 > -    if (date < thread->oldest || ! thread->matched_messages) {\r
 187 > +    if (date < thread->oldest || ! notmuch_thread_get_matched_messages (thread)) {\r
 188 >       thread->oldest = date;\r
 189 >       if (sort == NOTMUCH_SORT_OLDEST_FIRST)\r
 190 >           _thread_set_subject_from_message (thread, message);\r
 191 >      }\r
 192 >  \r
 193 > -    if (date > thread->newest || ! thread->matched_messages) {\r
 194 > +    if (date > thread->newest || ! notmuch_thread_get_matched_messages (thread)) {\r
 195 >       thread->newest = date;\r
 196 >       if (sort != NOTMUCH_SORT_OLDEST_FIRST)\r
 197 >           _thread_set_subject_from_message (thread, message);\r
 198 >      }\r
 199 >  \r
 200 > -    if (!notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED))\r
 201 > -     thread->matched_messages++;\r
 202 > -\r
 203 >      if (g_hash_table_lookup_extended (thread->message_hash,\r
 204 >                           notmuch_message_get_message_id (message), NULL,\r
 205 >                           (void **) &hashed_message)) {\r
 206 > @@ -411,7 +406,7 @@ _notmuch_thread_create (void *ctx,\r
 207 >      const char *thread_id;\r
 208 >      char *thread_id_query_string;\r
 209 >      notmuch_query_t *thread_id_query;\r
 210 > -\r
 211 > +    int i;\r
 212 >      notmuch_messages_t *messages;\r
 213 >      notmuch_message_t *message;\r
 214 >  \r
 215 > @@ -457,8 +452,8 @@ _notmuch_thread_create (void *ctx,\r
 216 >      thread->message_hash = g_hash_table_new_full (g_str_hash, g_str_equal,\r
 217 >                                                 free, NULL);\r
 218 >  \r
 219 > -    thread->total_messages = 0;\r
 220 > -    thread->matched_messages = 0;\r
 221 > +    for (i = 0; i < NOTMUCH_MESSAGE_FLAG_MAX; i++)\r
 222 > +     thread->flag_count_messages[i] = 0;\r
 223 \r
 224 memset (thread->flag_count_messages, 0, sizeof(thread->flag_count_messages));\r
 225 \r
 226 >      thread->oldest = 0;\r
 227 >      thread->newest = 0;\r
 228 >  \r
 229 > @@ -473,6 +468,7 @@ _notmuch_thread_create (void *ctx,\r
 230 >        notmuch_messages_move_to_next (messages))\r
 231 >      {\r
 232 >       unsigned int doc_id;\r
 233 > +     unsigned int message_flags;\r
 234 >  \r
 235 >       message = notmuch_messages_get (messages);\r
 236 >       doc_id = _notmuch_message_get_doc_id (message);\r
 237 > @@ -485,6 +481,10 @@ _notmuch_thread_create (void *ctx,\r
 238 >           _notmuch_doc_id_set_remove (match_set, doc_id);\r
 239 >           _thread_add_matched_message (thread, message, sort);\r
 240 >       }\r
 241 > +     message_flags =\r
 242 > +         notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH) |\r
 243 > +         notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED);\r
 244 > +     thread->flag_count_messages[message_flags]++;\r
 245 \r
 246 The first impression of using a set of flags as index is that there's a\r
 247 bug. But this is to keep count of messages with certain flag sets rather\r
 248 than total for each flag, right? I think this needs more comments, more\r
 249 documentation. Already naming the field flag_set_message_counts or\r
 250 similar would help greatly.\r
 251 \r
 252 >  \r
 253 >       _notmuch_message_close (message);\r
 254 >      }\r
 255 > @@ -511,15 +511,28 @@ notmuch_thread_get_thread_id (notmuch_thread_t *thread)\r
 256 >  }\r
 257 >  \r
 258 >  int\r
 259 > +notmuch_thread_get_flag_messages (notmuch_thread_t *thread, unsigned int flag_mask, unsigned int flags)\r
 260 > +{\r
 261 > +    unsigned int i;\r
 262 > +    int count = 0;\r
 263 > +    for (i = 0; i < NOTMUCH_MESSAGE_FLAG_MAX; i++)\r
 264 \r
 265 ARRAY_SIZE (thread->flag_count_messages)\r
 266 \r
 267 > +     if ((i & flag_mask) == (flags & flag_mask))\r
 268 > +         count += thread->flag_count_messages[i];\r
 269 > +    return count;\r
 270 > +}\r
 271 \r
 272 I wonder if the same could be accomplished by using two flag mask\r
 273 parameters, include_flag_mask and exclude_flag_mask. I'm thinking of the\r
 274 usage, would it be easier to use:\r
 275 \r
 276 notmuch_query_count_messages (thread, NOTMUCH_MESSAGE_FLAG_MATCH, NOTMUCH_MESSAGE_FLAG_EXCLUDED);\r
 277 \r
 278 to get number of messages that have MATCH but not EXCLUDED? 0 as\r
 279 include_flag_mask could still be special for "all", and you could use:\r
 280 \r
 281 notmuch_query_count_messages (thread, 0, NOTMUCH_MESSAGE_FLAG_EXCLUDED);\r
 282 \r
 283 Note the name change according to my earlier suggestion. It might be\r
 284 wise to not export the function before the API is chrystal clear if\r
 285 there is no pressing need to do so.\r
 286 \r
 287 > +\r
 288 > +int\r
 289 >  notmuch_thread_get_total_messages (notmuch_thread_t *thread)\r
 290 >  {\r
 291 > -    return thread->total_messages;\r
 292 > +    return notmuch_thread_get_flag_messages (thread, 0, 0);\r
 293 >  }\r
 294 >  \r
 295 >  int\r
 296 >  notmuch_thread_get_matched_messages (notmuch_thread_t *thread)\r
 297 >  {\r
 298 > -    return thread->matched_messages;\r
 299 > +    return notmuch_thread_get_flag_messages (thread,\r
 300 > +                                          NOTMUCH_MESSAGE_FLAG_MATCH | NOTMUCH_MESSAGE_FLAG_EXCLUDED,\r
 301 > +                                          NOTMUCH_MESSAGE_FLAG_MATCH);\r
 302 >  }\r
 303 >  \r
 304 >  const char *\r
 305 > -- \r
 306 > 1.7.2.3\r
 307 > \r
 308 > _______________________________________________\r
 309 > notmuch mailing list\r
 310 > notmuch@notmuchmail.org\r
 311 > http://notmuchmail.org/mailman/listinfo/notmuch\r