Re: [GNC-dev] Python-Bindings: issue when C expects GList argument
john <[email protected]>
| Newsgroups | gmane.comp.gnome.apps.gnucash.devel |
|---|---|
| Message-ID | <[email protected]> |
gncOwnerCreatePaymentLotSecs is a bad design because it tries to fit two use cases. One, in gncInvoiceApplyPayment, has a transaction already and passes it to gncOwnerCreatePaymentLotSecs. That's OK. The other, in gnc_payment_ok_cb via gncOwnerApplyPayemtSecs, asks it to create a transaction and apply the payment to it, and passes in a Transaction** so that it can get the new transaction. That's bad. The fix is to remove the transaction-creation code from gncOwnerCreatePaymentLotSecs and change the Transaction** parameter to Transaction* in both gncOwnerCreatePaymentLotSecs and gncOwnerApplyPaymentSecs. gnc_payment_ok_cb (actually gnc_payment_ok_cb's controller function, but that's for a different rant) should create the transaction itself and pass it to gncOwnerApplyPayemtSecs. While that will make gncOwnerCreatePaymentLotSecs wrappable, I think it's debatable that it should be wrapped: It's pretty clearly an implementation detail of applying a payment. Implementation details shouldn't be part of the public API. Regards, John Ralls > On May 10, 2022, at 2:59 PM, Matteo Lasagni <[email protected]> wrote: > > Thank you Derek, I understand what you mean. > The point here is that I was not interested in the value returned through the pointer, > but only in making that function callable from Python. > I was looking for a solution to automatically associate multiple invoices to the same transaction, > as it is possible with the GUI; this seemed to be the right way. > Maybe there is a better solution for this specific goal. > > However, I try to follow your suggestion to unref the return value or return it in a tuple. > Since I kept receiving always the following error, no matter how I change the swig interface, > I was only focusing on the input value (i.e., c input direction) quite convinced the first step was > to fix the conversion of a double pointer. > > TypeError: in method 'gncOwnerCreatePaymentLotSecs', argument 2 of type 'Transaction **' > > > Best, > Matteo > > > > On Tue, 10 May 2022 at 23:37, Derek Atkins <[email protected] <mailto:[email protected]>> wrote: > More specifically... > > The issue here is that there are multiple return values. > gncOwnerCreatePaymentLotSecs() is *RETURNING* both a GNCLot* and a > Transaction*. However, C doesn't allow you to do that, so the function > returns the GNCLot* and assigns the Transaction* to the Transaction** > argument as a second return value. > > Your proposed wrapper here is wrong. > > You can either write a wrapper that obtains the transaction and then > unrefs it (returning only the Lot), or you can write a wrapper that > obtains both values and returns the tuple (Lot, Txn) in Python -- assuming > Python, like Scheme, allows that. > > Most likely this is why the function wasn't wrapped before -- due to the > complexities of the double return values. > > -derek > > On Tue, May 10, 2022 5:23 pm, Matteo Lasagni wrote: > > If I can find a good solution, yes, I will put it in a pull request. > > > > I have tried to encapsulate Transaction* in a list, but of course it > > doesn't work without touching swig definitions. > > I am now trying to create a specific typemap for handling the double > > pointer. I think this would be the cleanest solution. > > > > Alternatively, a quick and dirty workaround is to add a C wrapper in > > gncOwner.c that expects Transaction* and passes Transaction** to > > gncOwnerCreatePaymentLotSecs: > > > > > > GNCLot * > > gncOwnerCreatePaymentLotSecsPyHelper (const GncOwner *owner, Transaction > > *txn, Account *posted_acc, Account *xfer_acc, gnc_numeric amount, > > gnc_numeric exch, time64 date, const char *memo, const char *num) > > { > > return gncOwnerCreatePaymentLotSecs(owner, &txn, posted_acc, xfer_acc, > > amount, exch, date, memo, num); > > } > > > > > > regards, > > Matteo > > > > On Tue, 10 May 2022 at 17:25, john <[email protected] <mailto:[email protected]>> wrote: > > > >> > >> > >> On May 10, 2022, at 1:53 AM, Matteo Lasagni <[email protected] <mailto:[email protected]>> wrote: > >> > >> Thank you, John! > >> > >> I fixed it by adding the following into base-typemaps.i: > >> > >> %typemap(in) GList * { > >> $1 = NULL; > >> /* Check if is a list */ > >> if (PyList_Check($input)) { > >> int i; > >> int size = PyList_Size($input); > >> for (i = size-1; i >= 0; i--) { > >> PyObject *o = PyList_GetItem($input, i); > >> $1 = g_list_prepend($1, o); > >> } > >> } else { > >> PyErr_SetString(PyExc_TypeError, "not a list"); > >> return NULL; > >> } > >> } > >> > >> I had overlooked the swig definition in engine.i, where there is a > >> typemap > >> (in) for GList, but then it is cleared right afterwards... > >> I am not (yet) very familiar with swig :-) > >> > >> I have also noticed that CreatePaymentLotSecs does not work out of the > >> box, as it expects **Transaction while only *Transaction is type mapped. > >> > >> I will test these changes then I will figure out how to share them with > >> the developers. > >> > >> > >> Matteo > >> > >> > >> On Tue, 10 May 2022 at 02:32, John Ralls <[email protected] <mailto:[email protected]>> wrote: > >> > >>> > >>> > >>> > On May 9, 2022, at 2:17 PM, Matteo Lasagni <[email protected] <mailto:[email protected]>> > >>> wrote: > >>> > > >>> > Hi, > >>> > I am writing a python script to automatically associate a set of > >>> > invoices to a single transaction. > >>> > To this end, I am trying to use the function > >>> AutoApplyPaymentsWithLots > >>> > with no success: > >>> > > >>> > > >>> > lots = [lot1, lot2, lotn, ..] > >>> > owner.AutoApplyPaymentsWithLots(lots) > >>> > > >>> > This causes the following error: > >>> > > >>> > TypeError: in method 'gncOwnerAutoApplyPaymentsWithLots', argument 2 > >>> of > >>> > type 'GList *' > >>> > > >>> > I have tried different solutions from passing different types of > >>> python > >>> > objects, to updating the swig typemap and also to modifying the > >>> > auto-generated gnucash_core.c...nothing worked out so far. > >>> > I could write an additional c-wrapper, but I am sure there is a more > >>> > general solution. > >>> > >>> The fundamental problem is that there's only a Python out typemap for > >>> GList, that is one that converts from a GList* to a python list. > >>> Lacking an > >>> in typemap the gncOwner.AutoApplyPaymentsWithLots python wrapper > >>> expects an > >>> already constructed and Swig-wrapped GList*. > >>> > >>> Regards, > >>> John Ralls > >>> > >>> > >> > >> Nice. Would you put that in a pull request? > >> > >> There are two typemap(in) GList* in engine.i, but they're both for Guile > >> (Scheme), not Python. 'scm' is the giveaway. > >> > >> If there's a way to implement an output pointer parameter in Python I > >> don't know about it. You can pass in a container object and put the > >> output > >> Transaction* in the container, but that's both a bit of a hack and > >> doesn't > >> have the same meaning as Transaction** nor is it very pythonic. Nor, for > >> that matter, is gncOwnerCreatePaymentLotSecs a very good design as it > >> mixes > >> responsibilities. Unfortunately there's a lot of that in GnuCash because > >> most of it is written with a very narrow focus, in this case providing > >> for > >> the payments dialog. > >> > >> Regards, > >> John Ralls > >> > >> > > > > -- > > > > ------------- > > Matteo Lasagni > > > > > > *"You can't connect the dots looking forward,you can only connect them > > looking backwards"* [Steve Jobs] > > _______________________________________________ > > gnucash-devel mailing list > > [email protected] <mailto:[email protected]> > > https://lists.gnucash.org/mailman/listinfo/gnucash-devel <https://lists.gnucash.org/mailman/listinfo/gnucash-devel> > > > > > -- > Derek Atkins 617-623-3745 > [email protected] <mailto:[email protected]> www.ihtfp.com <http://www.ihtfp.com/> > Computer and Internet Security Consultant > > > > -- > > ------------- > Matteo Lasagni > > "You can't connect the dots looking forward, > you can only connect them looking backwards" [Steve Jobs]