[PATCH v4 07/11] lib: added interface notmuch_thread_get_flag_messages
Jani Nikula
jani at nikula.org
Fri Feb 3 00:48:27 PST 2012
On Thu, 02 Feb 2012 23:24:56 +0000, Mark Walters <markwalters1009 at gmail.com> wrote:
> On Fri, 03 Feb 2012 01:07:59 +0200, Jani Nikula <jani at nikula.org> wrote:
> > On Thu, 02 Feb 2012 22:27:36 +0000, Mark Walters <markwalters1009 at gmail.com> wrote:
> > > On Thu, 02 Feb 2012 23:55:33 +0200, Jani Nikula <jani at 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 +0000, Mark Walters <markwalters1009 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.
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);
> > > > > }
> > > > > + 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
> > > > > +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, 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.
> > >
> > > (I assume you mean notmuch_thread_count_messages.)
> >
> > Doh! Yes.
> >
> > > Can I just check this
> > > would return the number of messages which have all the flags in
> > > include_flag_mask and none of the flags in exclude_flag_mask?
>
> Yes I think this works better: these are the flags I want, these are the
> ones I don't want seems natural (versus here are the ones I care about
> and here are the ones of those I want). But I will wait to see if anyone
> else has an opinion.
>
> > Yes, but only if it makes sense to you! :)
> >
> > >
> > > I completely agree about leaving it until we have the API well worked
> > > out. I wrote it in response to Austin's suggestion and then it looked
> > > like it would useful in my attempts to remove the
> > > notmuch_query_set_omit_excluded_messages API. However, those attempts
> > > failed so it doesn't have any users yet.
> > >
> > > Best wishes
> > >
> > > Mark
More information about the notmuch
mailing list