Change Customer Address failing

[John Ralls]

On Nov 15, 2014, at 4:44 PM, Christoph Holtermann <c.holtermann at gmx.de> wrote:

> 
> 
> Am 16.11.2014 um 00:13 schrieb John Ralls:
>> On Nov 15, 2014, at 12:59 PM, Christoph Holtermann <c.holtermann at gmx.de> wrote:
>> 
>>> Am 15.11.2014 um 18:45 schrieb John Ralls:
>>>>> On Nov 15, 2014, at 7:32 AM, Christoph Holtermann <c.holtermann at gmx.de> wrote:
>>>>> 
>>>>> Hello,
>>>>> 
>>>>> some time ago I found that changes to customer data in the GUI are not being
>>>>> saved. When working on a company object i made qof_instance_set_dirty
>>>>> return a debug message. I found that when changing text in gncAddress
>>>>> this is not being triggered. When in the GUI I change the state of activity
>>>>> the object gets marked dirty (and the text is being saved, too). Same thing
>>>>> when using the python bindings.
>>>>> 
>>>>> There I see that SetActive is a method of gncCustomer, while the other ones
>>>>> are of gncAddress.
>>>>> I guess I shouldn't copy this behavior to my company object.
>>>>> 
>>>>> Is this bug known ?
>>>>> 
>>>>> I'll try to find out where it lives.
>>>> Customer address or Company address? I know that the Company data is written directly to KVP in Scheme and that I missed it when I went through all of the C KVP code last year to make sure that the containing object of KVP was properly marked and committed. If customer addresses are also getting missed, that's new.
>>>> 
>>>> You're using the SQL backend, right? I ask because saving an XML file copies everything in memory to a new file; that's why there were so many instances of writing data without committing it existed. It worked without committing in XML, so laziness was rewarded.
>>> I'm using SQL. Only Customer address change fails. Company address change works.
>>> 
>>> Address goes like
>>> 
>>> G_INLINE_FUNC void mark_address (GncAddress *address);
>>> void mark_address (GncAddress *address)
>>> {  
>>>   address->dirty = TRUE;
>>> 
>>>   qof_event_gen (QOF_INSTANCE(address), QOF_EVENT_MODIFY, address->parent);
>>>   qof_event_gen (address->parent, QOF_EVENT_MODIFY, NULL);
>>> }
>>> 
>>> while in my Company object I use (as in gncBillTerm.c or Commodity or Customer ...)
>>> 
>>> G_INLINE_FUNC void mark_company (GncCompany *company);
>>> void mark_company (GncCompany *company)
>>> {  
>>>   qof_instance_set_dirty(QOF_INSTANCE(company));
>>>   qof_event_gen (QOF_INSTANCE(company), QOF_EVENT_MODIFY, NULL);
>>> }
>>> 
>>> in my case it produces the error
>>> 
>>> CRIT <gnc.backend.sql> [gnc_sql_commit_edit()] gnc_sql_commit_edit(): Unknown object type 'gncCompany'
>>> 
>>> when I change mark_address to also call
>>> qof_instance_set_dirty(QOF_INSTANCE(address));
>>> 
>>> an identical error message is being shown. And the changing of address doesn't get saved.
>>> 
>>> So I guess that
>>> 1) my company object should rather behave as address in generating an event to tell it's parent, that
>>> it's dirty. Which would be book. Because book gets saved.
>>> 1b) There are object that get saved and then there are others that get saved by their parents.
>>> 2) The communication between parent Customer and child Address about dirtiness and saving is not
>>> working.
> Hello John,
>> Christoph,
>> 
>> Since you’re using the SQL backend you probably want to study http://wiki.gnucash.org/wiki/SQL. Once there you’ll notice that there’s no table at all for Company; as you know already, that’s implemented entirely in KVP hanging on the Book object. You’ll also notice that there’s no address table: The schema is denormalized with gncAddress’s fields included directly in customers (twice, once for billing and once for shipping); also once each for employees and vendors, so in those cases the commit code will have to determine which object the address goes to and do the change inside a an edit-mark-commit block for that object. Please file a bug on that. I don’t expect you to fix it yourself!
> well, fixing the bug may be easier than creating a new class ... anyway I'm trying to understand how saving object properties works because I need to do that for the company
> object. And it's somewhat the same pathes the bug at address may be at. But filing a bug report is surely a good idea.
> 
> By following what the address class does I realized the difference you mentioned between address and company. address has SQL rows of it's own, the company values
> are stored in KVPs.
> 
> The question is on what level should the company class let the book class do the work of saving.
> 
> Does it need to go the way through
> 
> qof_commit_edit_part2 (&company->inst, gncCompanyOnError,
>                           gncCompanyOnDone, company_free);  ?
> 
> It gets rejected when getting to the SQL backend because it's not known as said above.
> 
> Actually just calling for example qof_book_set_string_option(company->book, "options/Business/Company Email", email); works fine where the book class does all the work.
> 
> So there's no need to call qof_commit_edit (part 1 or 2).
> 
> But shouldn't all qof-objects behave call these functions ?
> 
> 
>> 
>> For your Company class you can either use gncAddress to handle editing the address or you can just have a set of GObject Properties, the setters and getters for which call the kvp frame functions directly inside an edit-mark-commit block on book, that being the object which contains the company kvp info.
> How could I use use gncAddress for this when as you pointed it has its own sql fields where company relies on kvp. In general it would seem much more consistent to me if company
> and customer would base on the same address object but that would imply changing the structure of the company information which seems like a lot more work.
> I try to avoid that and just write a wrapping object for the way the data exists now. So I would rather stick to the book object doing the work.
> 
> If I understand you right I could just mimick qof_book_set_string_option
> 
> qof_book_begin_edit(book);
> kvp_frame_set_string(qof_instance_get_slots(QOF_INSTANCE (book)),
>                                                opt_name, opt_val);
> qof_instance_set_dirty (QOF_INSTANCE (book));
> qof_book_commit_edit(book);
> 
> which would lead to
> 
> qof_book_begin_edit(company->book);
> kvp_frame_set_string(qof_instance_get_slots(QOF_INSTANCE (company->book)),
>                                                "options/Business/Company Email", email);
> qof_instance_set_dirty (QOF_INSTANCE (book));
> qof_book_commit_edit(company->book);
> 
> instead of
> 
> gncCompanyBeginEdit (company);
> qof_book_set_string_option(company->book, "options/Business/Company Email", email);
> mark_company (company);
> gncCompanyCommitEdit (company);
> 
> or just  go
> qof_book_set_string_option(company->book, "options/Business/Company Email", email);
> 
> Should company be marked dirty at all then ?
> 
> Setting clean is done by qof_commit_edit_part2 as I understand it. There's no return code to check. It's the book object being directly set to dirty or clean. So I see no
> way to use dirty/clean in company. Except for comparing the states of book before and after the call maybe.
> 
> Can I restrain from using dirty/clean in company ?
> 
> 
> In respect to the memory management of gobject that you mentioned. Can you tell me a class that handles this properly ? You pointed a lot of classes that don't.

Christoph,

Please remember to copy the list on all replies. Christian and Geert need to see this discussion in full.

How’s your Scheme? The way Company information is handled now is in the options system, which is written in Scheme. Relevant files are in src/app-utils: business-options.scm declares the options, options.scm has the functions for displaying them, and app-utils.scm has function for retrieving them. option-util.[ch] gets the definition from the Scheme and src/gnome-utils/dialog-option.[ch] has the code for turning that into a dialog. In order for your gncCompany object to work it will have to insert itself into that process somehow. Not a trivial task, and I’m not sure that it’s necessarily the right way to go, at least in the short term.

The fact that the useful parts of the options database are in Scheme explains why they’re invisible to Python. The only way to get at them in C is through Guile, which has no Python bindings.

So I suggest that instead of trying to make gncCompany a proper object you have it provide a read-only interface; that saves you having to deal with the edit-mark-commit stuff altogether. You can either call qof_book_get_slots() and work from there or load the appropriate option database (see src/app-utils/option-util.h) and work with that. Since the latter gets you some abstraction from the implementation it’s preferable IMO.

To answer your direct questions, when changing state in a way that’s persisted you have to find the persistent object (see the SQL wiki article) and wrap your changes in a edit-mark-commit on that object, with one exception: Splits are at present stored in a Transaction commit, so if you’re editing a split you must edit, mark, and commit the parent transaction. That’s so that the transaction balancing code is called. In the case of gncCompany, if it wasn’t all bound up in the options stuff, its containing top-level is the book so you’d edit-mark-commit the book. For top-level slots qof_book_set_string_option() does that all for you, but the Company info is in options/business, so to get at it you’d have to call qof_book_get_slots() and kvp_frame_set_string_path().

Regards,
John Ralls