[PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages
On Fri, 03 Feb 2012 09:36:29 +, Mark Walters
wrote:
> On Fri, 03 Feb 2012 08:48:27 +, Jani Nikula wrote:
> > On Thu, 02 Feb 2012 23:24:56 +, Mark Walters > gmail.com> wrote:
> > > On Fri, 03 Feb 2012 01:07:59 +0200, Jani Nikula
> > > wrote:
> > > > On Thu, 02 Feb 2012 22:27:36 +, Mark Walters > > > gmail.com> wrote:
> > > > > On Thu, 02 Feb 2012 23:55:33 +0200, Jani Nikula
> > > > > wrote:
> > > > > >
> > > > > > 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 +, Mark Walters > > > > > at gmail.com> 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?
> > > > >
> > > > > Yes: I was trying to follow Austin's suggestion from
> > > > > id:"20120124025331.GZ16740 at mit.edu" (although stupidly I didn't
> > > > > follow his suggestion of a function name).
> > > > >
> > > > > > 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).
> > > > >
> > > > > Yes I like your name; before I change it do you (and others) prefer it
> > > > > to Austin's suggestion of notmuch_thread_count_flags. Or we could even
> > > > > be more verbose with something like
> > > > > notmuch_thread_count_messages_with_flags
> > > >
> > > > I'd like to make it clear that it's about message count. Not about
> > > > getting flags, not about flag counts. _with_flags is a matter of taste,
> > > > no strong opinions there.
> > >
> > > I think I will go with notmuch_thread_count_messages as you suggest.
> > >
> > > > > > > /* 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?
> > >
> > > I will just comment on this: the *only* reason I put in
> > > NOTMUCH_MESSAGE_FLAG_MAX was as a way of keeping track of the size of
> > > the bitfield. If there is a better way do say!
> >
> > At least one improvement would be to make it NOTMUCH_MESSAGE_FLAG_ALL
> > (or similar) which would be the OR of all the other flags. Above, it
> > should be equal to (1 << 2) - 1. Not only is this something usable to
> > the library users, but also more accurate - if I'm not mistaken, the
> > flagset array currently has one element too many.
> >
> > If documented properly, the users should not be surprised that in the
> > future more flags might be added to NOTMUCH_MESSAGE_FLAG_ALL, and
> > depending on the case they may or may not want to use that.
>
> I think the current array is the correct size; I do need to keep
> track of the number of messages matching no flags, for example to
> calculate the total number of messages.
My bad. Sorry.
> I am not sure of the utility of NOTMUCH_MESSAGE_FLAG_ALL as I think ~0
> would give the same result.
That depends on whether you want to check the flags; ~0 will have more
than there is. But no big issue.
> I am very happy to add it if others see
> some use, and with your earlier suggestions using ARRAY_SIZE etc I would
> only have one use of NOTMUCH_MESSAGE_FLAG_ALL+1.
>
> > Some purists might say that #defines are better suited for defining bit
> > flags than enums, but I'm fine with either.
>
> I am happy either way.
>
> >
> > >
> > > > > The only existing flag is NOTMUCH_MESSAGE_FLAG_MATCH: that is
> > > > > currently
> > > > > zero but in the current code that is the bit offset of the flag; in my
> > > > > version it is the actual bit for the flag (otherwise I think flag
> > > > > masks
> > > > > end up very ugly). I believe all callers use notmuch_message_set_flag
> > > > > and notmuch_message_get_flag so they should not notice the difference.
> > > > >
> > > > > > Please align the assignments.
> > > > >
> > > > > Will do.
> > > > >
> > > > > > > @@ -457,8
[PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages
On Fri, 03 Feb 2012 08:48:27 +, Jani Nikula wrote:
> On Thu, 02 Feb 2012 23:24:56 +, Mark Walters gmail.com> wrote:
> > On Fri, 03 Feb 2012 01:07:59 +0200, Jani Nikula wrote:
> > > On Thu, 02 Feb 2012 22:27:36 +, Mark Walters > > gmail.com> wrote:
> > > > On Thu, 02 Feb 2012 23:55:33 +0200, Jani Nikula
> > > > wrote:
> > > > >
> > > > > 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 +, Mark Walters > > > > gmail.com> 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?
> > > >
> > > > Yes: I was trying to follow Austin's suggestion from
> > > > id:"20120124025331.GZ16740 at mit.edu" (although stupidly I didn't
> > > > follow his suggestion of a function name).
> > > >
> > > > > 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).
> > > >
> > > > Yes I like your name; before I change it do you (and others) prefer it
> > > > to Austin's suggestion of notmuch_thread_count_flags. Or we could even
> > > > be more verbose with something like
> > > > notmuch_thread_count_messages_with_flags
> > >
> > > I'd like to make it clear that it's about message count. Not about
> > > getting flags, not about flag counts. _with_flags is a matter of taste,
> > > no strong opinions there.
> >
> > I think I will go with notmuch_thread_count_messages as you suggest.
> >
> > > > > > /* 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?
> >
> > I will just comment on this: the *only* reason I put in
> > NOTMUCH_MESSAGE_FLAG_MAX was as a way of keeping track of the size of
> > the bitfield. If there is a better way do say!
>
> At least one improvement would be to make it NOTMUCH_MESSAGE_FLAG_ALL
> (or similar) which would be the OR of all the other flags. Above, it
> should be equal to (1 << 2) - 1. Not only is this something usable to
> the library users, but also more accurate - if I'm not mistaken, the
> flagset array currently has one element too many.
>
> If documented properly, the users should not be surprised that in the
> future more flags might be added to NOTMUCH_MESSAGE_FLAG_ALL, and
> depending on the case they may or may not want to use that.
I think the current array is the correct size; I do need to keep
track of the number of messages matching no flags, for example to
calculate the total number of messages.
I am not sure of the utility of NOTMUCH_MESSAGE_FLAG_ALL as I think ~0
would give the same result. I am very happy to add it if others see
some use, and with your earlier suggestions using ARRAY_SIZE etc I would
only have one use of NOTMUCH_MESSAGE_FLAG_ALL+1.
> Some purists might say that #defines are better suited for defining bit
> flags than enums, but I'm fine with either.
I am happy either way.
>
> >
> > > > The only existing flag is NOTMUCH_MESSAGE_FLAG_MATCH: that is currently
> > > > zero but in the current code that is the bit offset of the flag; in my
> > > > version it is the actual bit for the flag (otherwise I think flag masks
> > > > end up very ugly). I believe all callers use notmuch_message_set_flag
> > > > and notmuch_message_get_flag so they should not notice the difference.
> > > >
> > > > > Please align the assignments.
> > > >
> > > > Will do.
> > > >
> > > > > > @@ -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;
> > > > >
> > >
[PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages
On Thu, 02 Feb 2012 23:24:56 +, Mark Walters
wrote:
> On Fri, 03 Feb 2012 01:07:59 +0200, Jani Nikula wrote:
> > On Thu, 02 Feb 2012 22:27:36 +, Mark Walters > gmail.com> wrote:
> > > On Thu, 02 Feb 2012 23:55:33 +0200, Jani Nikula
> > > wrote:
> > > >
> > > > 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 +, Mark Walters > > > gmail.com> 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?
> > >
> > > Yes: I was trying to follow Austin's suggestion from
> > > id:"20120124025331.GZ16740 at mit.edu" (although stupidly I didn't
> > > follow his suggestion of a function name).
> > >
> > > > 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).
> > >
> > > Yes I like your name; before I change it do you (and others) prefer it
> > > to Austin's suggestion of notmuch_thread_count_flags. Or we could even
> > > be more verbose with something like
> > > notmuch_thread_count_messages_with_flags
> >
> > I'd like to make it clear that it's about message count. Not about
> > getting flags, not about flag counts. _with_flags is a matter of taste,
> > no strong opinions there.
>
> I think I will go with notmuch_thread_count_messages as you suggest.
>
> > > > > /* 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?
>
> I will just comment on this: the *only* reason I put in
> NOTMUCH_MESSAGE_FLAG_MAX was as a way of keeping track of the size of
> the bitfield. If there is a better way do say!
At least one improvement would be to make it NOTMUCH_MESSAGE_FLAG_ALL
(or similar) which would be the OR of all the other flags. Above, it
should be equal to (1 << 2) - 1. Not only is this something usable to
the library users, but also more accurate - if I'm not mistaken, the
flagset array currently has one element too many.
If documented properly, the users should not be surprised that in the
future more flags might be added to NOTMUCH_MESSAGE_FLAG_ALL, and
depending on the case they may or may not want to use that.
Some purists might say that #defines are better suited for defining bit
flags than enums, but I'm fine with either.
>
> > > The only existing flag is NOTMUCH_MESSAGE_FLAG_MATCH: that is currently
> > > zero but in the current code that is the bit offset of the flag; in my
> > > version it is the actual bit for the flag (otherwise I think flag masks
> > > end up very ugly). I believe all callers use notmuch_message_set_flag
> > > and notmuch_message_get_flag so they should not notice the difference.
> > >
> > > > Please align the assignments.
> > >
> > > Will do.
> > >
> > > > > @@ -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));
> > >
> > >
> > > Will do
> > >
> > > > > 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);
> >
[PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages
On Thu, 02 Feb 2012 22:27:36 +, Mark Walters
wrote:
> On Thu, 02 Feb 2012 23:55:33 +0200, Jani Nikula wrote:
> >
> > 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 +, Mark Walters > gmail.com> 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?
>
> Yes: I was trying to follow Austin's suggestion from
> id:"20120124025331.GZ16740 at mit.edu" (although stupidly I didn't
> follow his suggestion of a function name).
>
> > 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).
>
> Yes I like your name; before I change it do you (and others) prefer it
> to Austin's suggestion of notmuch_thread_count_flags. Or we could even
> be more verbose with something like
> notmuch_thread_count_messages_with_flags
I'd like to make it clear that it's about message count. Not about
getting flags, not about flag counts. _with_flags is a matter of taste,
no strong opinions there.
>
> > > /* 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?
>
> The only existing flag is NOTMUCH_MESSAGE_FLAG_MATCH: that is currently
> zero but in the current code that is the bit offset of the flag; in my
> version it is the actual bit for the flag (otherwise I think flag masks
> end up very ugly). I believe all callers use notmuch_message_set_flag
> and notmuch_message_get_flag so they should not notice the difference.
>
> > Please align the assignments.
>
> Will do.
>
> > > @@ -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));
>
>
> Will do
>
> > > 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.
>
> I will try and document it better: on first reading I parsed your name
> as flag set (as verb) message counts whereas I assume you mean "flag
> set" as a noun! I will see if I can come up with something though.
Yes, as a noun! :)
>
> > > _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)
>
> ok
>
> >
> > > + if ((i & flag_mask) == (flags & flag_mask))
> > > + count += thread->flag_count_messages[i];
> > > +return count;
> > > +}
> >
> > I
[PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages
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 +, 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,
>
[PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages
On Fri, 03 Feb 2012 01:07:59 +0200, Jani Nikula wrote:
> On Thu, 02 Feb 2012 22:27:36 +, Mark Walters gmail.com> wrote:
> > On Thu, 02 Feb 2012 23:55:33 +0200, Jani Nikula wrote:
> > >
> > > 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 +, Mark Walters > > gmail.com> 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?
> >
> > Yes: I was trying to follow Austin's suggestion from
> > id:"20120124025331.GZ16740 at mit.edu" (although stupidly I didn't
> > follow his suggestion of a function name).
> >
> > > 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).
> >
> > Yes I like your name; before I change it do you (and others) prefer it
> > to Austin's suggestion of notmuch_thread_count_flags. Or we could even
> > be more verbose with something like
> > notmuch_thread_count_messages_with_flags
>
> I'd like to make it clear that it's about message count. Not about
> getting flags, not about flag counts. _with_flags is a matter of taste,
> no strong opinions there.
I think I will go with notmuch_thread_count_messages as you suggest.
> > > > /* 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?
I will just comment on this: the *only* reason I put in
NOTMUCH_MESSAGE_FLAG_MAX was as a way of keeping track of the size of
the bitfield. If there is a better way do say!
> > The only existing flag is NOTMUCH_MESSAGE_FLAG_MATCH: that is currently
> > zero but in the current code that is the bit offset of the flag; in my
> > version it is the actual bit for the flag (otherwise I think flag masks
> > end up very ugly). I believe all callers use notmuch_message_set_flag
> > and notmuch_message_get_flag so they should not notice the difference.
> >
> > > Please align the assignments.
> >
> > Will do.
> >
> > > > @@ -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));
> >
> >
> > Will do
> >
> > > > 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.
> >
> > I will try and document it better: on first reading I parsed your name
> > as flag set (as verb) message counts whereas I assume you mean "flag
> > set" as a noun! I will see if I can come up with something though.
>
> Yes, as a noun! :)
[PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages
On Thu, 02 Feb 2012 23:55:33 +0200, Jani Nikula wrote:
>
> 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 +, Mark Walters gmail.com> 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?
Yes: I was trying to follow Austin's suggestion from
id:"20120124025331.GZ16740 at mit.edu" (although stupidly I didn't
follow his suggestion of a function name).
> 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).
Yes I like your name; before I change it do you (and others) prefer it
to Austin's suggestion of notmuch_thread_count_flags. Or we could even
be more verbose with something like
notmuch_thread_count_messages_with_flags
> > /* 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?
The only existing flag is NOTMUCH_MESSAGE_FLAG_MATCH: that is currently
zero but in the current code that is the bit offset of the flag; in my
version it is the actual bit for the flag (otherwise I think flag masks
end up very ugly). I believe all callers use notmuch_message_set_flag
and notmuch_message_get_flag so they should not notice the difference.
> Please align the assignments.
Will do.
> > @@ -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));
Will do
> > 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.
I will try and document it better: on first reading I parsed your name
as flag set (as verb) message counts whereas I assume you mean "flag
set" as a noun! I will see if I can come up with something though.
> > _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)
ok
>
> > + 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,
[PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages
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.
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)
} 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 **) _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;
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
Re: [PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages
On Fri, 03 Feb 2012 01:07:59 +0200, Jani Nikula j...@nikula.org wrote:
On Thu, 02 Feb 2012 22:27:36 +, Mark Walters markwalters1...@gmail.com
wrote:
On Thu, 02 Feb 2012 23:55:33 +0200, Jani Nikula j...@nikula.org wrote:
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 +, Mark Walters
markwalters1...@gmail.com 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?
Yes: I was trying to follow Austin's suggestion from
id:20120124025331.gz16...@mit.edu (although stupidly I didn't
follow his suggestion of a function name).
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).
Yes I like your name; before I change it do you (and others) prefer it
to Austin's suggestion of notmuch_thread_count_flags. Or we could even
be more verbose with something like
notmuch_thread_count_messages_with_flags
I'd like to make it clear that it's about message count. Not about
getting flags, not about flag counts. _with_flags is a matter of taste,
no strong opinions there.
I think I will go with notmuch_thread_count_messages as you suggest.
/* Message flags */
typedef enum _notmuch_message_flag {
-NOTMUCH_MESSAGE_FLAG_MATCH,
-NOTMUCH_MESSAGE_FLAG_EXCLUDED
+NOTMUCH_MESSAGE_FLAG_MATCH = (10),
+NOTMUCH_MESSAGE_FLAG_EXCLUDED = (11),
+NOTMUCH_MESSAGE_FLAG_MAX = (12)
How are these used by the current lib users at the moment? How will they
break with this change?
I will just comment on this: the *only* reason I put in
NOTMUCH_MESSAGE_FLAG_MAX was as a way of keeping track of the size of
the bitfield. If there is a better way do say!
The only existing flag is NOTMUCH_MESSAGE_FLAG_MATCH: that is currently
zero but in the current code that is the bit offset of the flag; in my
version it is the actual bit for the flag (otherwise I think flag masks
end up very ugly). I believe all callers use notmuch_message_set_flag
and notmuch_message_get_flag so they should not notice the difference.
Please align the assignments.
Will do.
@@ -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));
Will do
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.
I will try and document it better: on first reading I parsed your name
as flag set (as verb) message counts whereas I assume you mean flag
set as a noun! I will see if I can come up with something though.
Yes, as a noun! :)
I haven't come up with a good name: the best I have come up with is
flagset_message_count so if you have any suggestions...
_notmuch_message_close (message);
}
@@ -511,15 +511,28 @@ notmuch_thread_get_thread_id (notmuch_thread_t
*thread)
}
int
