Floris Bruynooghe venit, vidit, dixit 2021-01-11 21:33:16: > Hi Micahel, > > Thanks for adding this feature! > > On Wed 06 Jan 2021 at 10:08 +0100, Michael J. Gruber wrote: > > diff --git a/bindings/python-cffi/notmuch2/_query.py b/bindings/python-cffi/notmuch2/_query.py > > index 1db6ec96..a1310944 100644 > > --- a/bindings/python-cffi/notmuch2/_query.py > > +++ b/bindings/python-cffi/notmuch2/_query.py > > @@ -2,6 +2,7 @@ from notmuch2 import _base as base > > from notmuch2 import _capi as capi > > from notmuch2 import _errors as errors > > from notmuch2 import _message as message > > +from notmuch2 import _tags as tags > > from notmuch2 import _thread as thread > > > > > > @@ -66,6 +67,17 @@ class Query(base.NotmuchObject): > > raise errors.NotmuchError(ret) > > return count_p[0] > > > > + def collect_tags(self): > > + """Return the tags of messages matching this query.""" > > + msgs_pp = capi.ffi.new('notmuch_messages_t**') > > + ret = capi.lib.notmuch_query_search_messages(self._query_p, msgs_pp) > > + if ret != capi.lib.NOTMUCH_STATUS_SUCCESS: > > + raise errors.NotmuchError(ret) > > + self._msgs_p = msgs_pp[0] > > + tagset = tags.ImmutableTagSet( > > + self, '_msgs_p', capi.lib.notmuch_messages_collect_tags) > > + return tags.ImmutableTagSet._from_iterable(tagset) > > + > > def threads(self): > > """Return an iterator over all the threads found by the query.""" > > threads_pp = capi.ffi.new('notmuch_threads_t **') > > How this this look on the C level for the memory allocator? If the > query gets freed the messages also get freed? In that case indeed the > messages do need to be kept alive together with the query indeed. > Keeping them as an attribute on the query object seems reasonable I > think, they'd be freed at the same time and I don't think this creates a > circular reference. I'm afraid I don't have the full picture of the memory model and object lifetimes in the bindings. > There's only a small detail though: to get the correct ObjectDestroyed > exceptions you need Query._msgs_p to be a _base.MemoryPoiniter > descriptor, like Query._query_p is currently. And then you also need to > set it to None in Query._destroy. I was trying to mimick what Query.messages() does: it creates msgs_pp locally. The only reason for self._msgs_p in Thread.collect_tags() is the way in which tags.ImmutableTagSet() expects the pointer: as an attribute of self. But maybe there is a better way to call into capi.lib.notmuch_messages_collect_tags()? If not, and if I understand you correctly, then I should apply diff --git i/bindings/python-cffi/notmuch2/_query.py w/bindings/python-cffi/notmuch2/_query.py index a1310944..fa58825d 100644 --- i/bindings/python-cffi/notmuch2/_query.py +++ w/bindings/python-cffi/notmuch2/_query.py @@ -17,6 +17,7 @@ class Query(base.NotmuchObject): match libnotmuch's memory management. """ _query_p = base.MemoryPointer() + _msgs_p = base.MemoryPointer() def __init__(self, db, query_p): self._db = db @@ -40,6 +41,7 @@ class Query(base.NotmuchObject): if self.alive: capi.lib.notmuch_query_destroy(self._query_p) self._query_p = None + self._msgs_p = None on top of this patch, right? > It would also be really good to add at least one happy-path test for > this, ensuring that you get the tags in the query. You could add some > to TestQuery in test_database.py. The database is created with unread > and inbox tags i think (see conftest.py:75) so the messages should > already have some tags which is sufficient as you're not testing > notmuch's behaviour itself but only that the bindings give you a right > looking tagset (the tagset itself also has tests already, so no need to > test this extensively, just see if you have some tags). Thanks for the pointer. Will do for v2, comparing: set(db.collect_tags('*')) == set(db.tags) Michael _______________________________________________ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-leave@notmuchmail.org