From 9e956156ea49f4b685c3c14e1b762f39d2630520 Mon Sep 17 00:00:00 2001 From: Jani Nikula Date: Thu, 2 Feb 2012 23:55:33 +0200 Subject: [PATCH] Re: [PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages --- f8/47b0125dac3a68f1c5a4f45ba67b5441ae70c3 | 311 ++++++++++++++++++++++ 1 file changed, 311 insertions(+) create mode 100644 f8/47b0125dac3a68f1c5a4f45ba67b5441ae70c3 diff --git a/f8/47b0125dac3a68f1c5a4f45ba67b5441ae70c3 b/f8/47b0125dac3a68f1c5a4f45ba67b5441ae70c3 new file mode 100644 index 000000000..560d795de --- /dev/null +++ b/f8/47b0125dac3a68f1c5a4f45ba67b5441ae70c3 @@ -0,0 +1,311 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 26596431FBD + for ; Thu, 2 Feb 2012 13:55:41 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id ekga1EboEg-f for ; + Thu, 2 Feb 2012 13:55:40 -0800 (PST) +Received: from mail-lpp01m010-f53.google.com (mail-lpp01m010-f53.google.com + [209.85.215.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id CD55A431FAF + for ; Thu, 2 Feb 2012 13:55:39 -0800 (PST) +Received: by lahd3 with SMTP id d3so1670520lah.26 + for ; Thu, 02 Feb 2012 13:55:38 -0800 (PST) +Received: by 10.152.128.163 with SMTP id np3mr2414569lab.51.1328219738195; + Thu, 02 Feb 2012 13:55:38 -0800 (PST) +Received: from localhost (dsl-hkibrasgw4-fe50f800-253.dhcp.inet.fi. + [84.248.80.253]) + by mx.google.com with ESMTPS id i9sm3049561lbz.3.2012.02.02.13.55.35 + (version=SSLv3 cipher=OTHER); Thu, 02 Feb 2012 13:55:36 -0800 (PST) +From: Jani Nikula +To: Mark Walters , notmuch@notmuchmail.org, + amdragon@MIT.EDU +Subject: Re: [PATCH v4 07/11] lib: added interface + notmuch_thread_get_flag_messages +In-Reply-To: <1328204619-25046-7-git-send-email-markwalters1009@gmail.com> +References: <874nv9rv79.fsf@qmul.ac.uk> + <1328204619-25046-7-git-send-email-markwalters1009@gmail.com> +User-Agent: Notmuch/0.11+139~g4340989 (http://notmuchmail.org) Emacs/23.3.1 + (i686-pc-linux-gnu) +Date: Thu, 02 Feb 2012 23:55:33 +0200 +Message-ID: <87k444yk6i.fsf@nikula.org> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Thu, 02 Feb 2012 21:55:41 -0000 + + +Hi Mark - + +This is my first look at any version of the series; apologies if I'm +clueless about some details... Please find some comments below. + +BR, +Jani. + + +On Thu, 2 Feb 2012 17:43:35 +0000, Mark Walters wrote: +> The function is +> notmuch_thread_get_flag_messages +> (notmuch_thread_t *thread, unsigned int flag_mask, unsigned int flags) +> +> and returns the number of messages with the specified flags on flag_mask. + +Is the purpose of this function to get the count of messages that have +certain flags set, certain flags not set, and certain flags don't-care? + +At the very least, I think the documentation of the function should be +greatly improved. + +I think the name of the function should be notmuch_thread_count_messages +which is like notmuch_query_count_messages, but for messages in threads +(and with some extra restrictions). + +> +> This generalises the existing function +> notmuch_thread_get_total_messages and +> notmuch_thread_get_matched_messages which are retained to maintain +> compatibility. +> --- +> lib/message.cc | 6 +++--- +> lib/notmuch.h | 15 +++++++++++++-- +> lib/thread.cc | 39 ++++++++++++++++++++++++++------------- +> 3 files changed, 42 insertions(+), 18 deletions(-) +> +> diff --git a/lib/message.cc b/lib/message.cc +> index 0075425..d60da83 100644 +> --- a/lib/message.cc +> +++ b/lib/message.cc +> @@ -746,7 +746,7 @@ notmuch_bool_t +> notmuch_message_get_flag (notmuch_message_t *message, +> notmuch_message_flag_t flag) +> { +> - return message->flags & (1 << flag); +> + return message->flags & flag; +> } +> +> void +> @@ -754,9 +754,9 @@ notmuch_message_set_flag (notmuch_message_t *message, +> notmuch_message_flag_t flag, notmuch_bool_t enable) +> { +> if (enable) +> - message->flags |= (1 << flag); +> + message->flags |= flag; +> else +> - message->flags &= ~(1 << flag); +> + message->flags &= ~flag; +> } +> +> time_t +> diff --git a/lib/notmuch.h b/lib/notmuch.h +> index f75afae..c02e7f4 100644 +> --- a/lib/notmuch.h +> +++ b/lib/notmuch.h +> @@ -654,6 +654,16 @@ notmuch_thread_get_thread_id (notmuch_thread_t *thread); +> int +> notmuch_thread_get_total_messages (notmuch_thread_t *thread); +> +> +/* Get the number of messages in 'thread' which have the specified +> + * flags on flag_mask. +> + * +> + * This is a more general interface than +> + * notmuch_thread_get_total_messages or +> + * notmuch_thread_get_matched_messages +> + */ +> +int +> +notmuch_thread_get_flag_messages (notmuch_thread_t *thread, unsigned int flag_mask, unsigned int flags); +> + +> /* Get a notmuch_messages_t iterator for the top-level messages in +> * 'thread'. +> * +> @@ -902,8 +912,9 @@ notmuch_message_get_filenames (notmuch_message_t *message); +> +> /* Message flags */ +> typedef enum _notmuch_message_flag { +> - NOTMUCH_MESSAGE_FLAG_MATCH, +> - NOTMUCH_MESSAGE_FLAG_EXCLUDED +> + NOTMUCH_MESSAGE_FLAG_MATCH = (1<<0), +> + NOTMUCH_MESSAGE_FLAG_EXCLUDED = (1<<1), +> + NOTMUCH_MESSAGE_FLAG_MAX = (1<<2) + +How are these used by the current lib users at the moment? How will they +break with this change? + +Please align the assignments. + +> } notmuch_message_flag_t; +> +> /* Get a value of a flag for the email corresponding to 'message'. */ +> diff --git a/lib/thread.cc b/lib/thread.cc +> index e976d64..542f7f4 100644 +> --- a/lib/thread.cc +> +++ b/lib/thread.cc +> @@ -37,8 +37,7 @@ struct visible _notmuch_thread { +> +> notmuch_message_list_t *message_list; +> GHashTable *message_hash; +> - int total_messages; +> - int matched_messages; +> + int flag_count_messages[NOTMUCH_MESSAGE_FLAG_MAX]; +> time_t oldest; +> time_t newest; +> }; +> @@ -226,7 +225,6 @@ _thread_add_message (notmuch_thread_t *thread, +> +> _notmuch_message_list_add_message (thread->message_list, +> talloc_steal (thread, message)); +> - thread->total_messages++; +> +> g_hash_table_insert (thread->message_hash, +> xstrdup (notmuch_message_get_message_id (message)), +> @@ -319,21 +317,18 @@ _thread_add_matched_message (notmuch_thread_t *thread, +> +> date = notmuch_message_get_date (message); +> +> - if (date < thread->oldest || ! thread->matched_messages) { +> + if (date < thread->oldest || ! notmuch_thread_get_matched_messages (thread)) { +> thread->oldest = date; +> if (sort == NOTMUCH_SORT_OLDEST_FIRST) +> _thread_set_subject_from_message (thread, message); +> } +> +> - if (date > thread->newest || ! thread->matched_messages) { +> + if (date > thread->newest || ! notmuch_thread_get_matched_messages (thread)) { +> thread->newest = date; +> if (sort != NOTMUCH_SORT_OLDEST_FIRST) +> _thread_set_subject_from_message (thread, message); +> } +> +> - if (!notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED)) +> - thread->matched_messages++; +> - +> if (g_hash_table_lookup_extended (thread->message_hash, +> notmuch_message_get_message_id (message), NULL, +> (void **) &hashed_message)) { +> @@ -411,7 +406,7 @@ _notmuch_thread_create (void *ctx, +> const char *thread_id; +> char *thread_id_query_string; +> notmuch_query_t *thread_id_query; +> - +> + int i; +> notmuch_messages_t *messages; +> notmuch_message_t *message; +> +> @@ -457,8 +452,8 @@ _notmuch_thread_create (void *ctx, +> thread->message_hash = g_hash_table_new_full (g_str_hash, g_str_equal, +> free, NULL); +> +> - thread->total_messages = 0; +> - thread->matched_messages = 0; +> + for (i = 0; i < NOTMUCH_MESSAGE_FLAG_MAX; i++) +> + thread->flag_count_messages[i] = 0; + +memset (thread->flag_count_messages, 0, sizeof(thread->flag_count_messages)); + +> thread->oldest = 0; +> thread->newest = 0; +> +> @@ -473,6 +468,7 @@ _notmuch_thread_create (void *ctx, +> notmuch_messages_move_to_next (messages)) +> { +> unsigned int doc_id; +> + unsigned int message_flags; +> +> message = notmuch_messages_get (messages); +> doc_id = _notmuch_message_get_doc_id (message); +> @@ -485,6 +481,10 @@ _notmuch_thread_create (void *ctx, +> _notmuch_doc_id_set_remove (match_set, doc_id); +> _thread_add_matched_message (thread, message, sort); +> } +> + message_flags = +> + notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_MATCH) | +> + notmuch_message_get_flag (message, NOTMUCH_MESSAGE_FLAG_EXCLUDED); +> + thread->flag_count_messages[message_flags]++; + +The first impression of using a set of flags as index is that there's a +bug. But this is to keep count of messages with certain flag sets rather +than total for each flag, right? I think this needs more comments, more +documentation. Already naming the field flag_set_message_counts or +similar would help greatly. + +> +> _notmuch_message_close (message); +> } +> @@ -511,15 +511,28 @@ notmuch_thread_get_thread_id (notmuch_thread_t *thread) +> } +> +> int +> +notmuch_thread_get_flag_messages (notmuch_thread_t *thread, unsigned int flag_mask, unsigned int flags) +> +{ +> + unsigned int i; +> + int count = 0; +> + for (i = 0; i < NOTMUCH_MESSAGE_FLAG_MAX; i++) + +ARRAY_SIZE (thread->flag_count_messages) + +> + if ((i & flag_mask) == (flags & flag_mask)) +> + count += thread->flag_count_messages[i]; +> + return count; +> +} + +I wonder if the same could be accomplished by using two flag mask +parameters, include_flag_mask and exclude_flag_mask. I'm thinking of the +usage, would it be easier to use: + +notmuch_query_count_messages (thread, NOTMUCH_MESSAGE_FLAG_MATCH, NOTMUCH_MESSAGE_FLAG_EXCLUDED); + +to get number of messages that have MATCH but not EXCLUDED? 0 as +include_flag_mask could still be special for "all", and you could use: + +notmuch_query_count_messages (thread, 0, NOTMUCH_MESSAGE_FLAG_EXCLUDED); + +Note the name change according to my earlier suggestion. It might be +wise to not export the function before the API is chrystal clear if +there is no pressing need to do so. + +> + +> +int +> notmuch_thread_get_total_messages (notmuch_thread_t *thread) +> { +> - return thread->total_messages; +> + return notmuch_thread_get_flag_messages (thread, 0, 0); +> } +> +> int +> notmuch_thread_get_matched_messages (notmuch_thread_t *thread) +> { +> - return thread->matched_messages; +> + return notmuch_thread_get_flag_messages (thread, +> + NOTMUCH_MESSAGE_FLAG_MATCH | NOTMUCH_MESSAGE_FLAG_EXCLUDED, +> + NOTMUCH_MESSAGE_FLAG_MATCH); +> } +> +> const char * +> -- +> 1.7.2.3 +> +> _______________________________________________ +> notmuch mailing list +> notmuch@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch -- 2.26.2