> On Jun 20, 2018, at 2:29 PM, Christian Stimming <[email protected]> > wrote: > > Am Mittwoch, 20. Juni 2018, 09:59:52 schrieb John Ralls: >>>> It’s all about file compatibility, remember? As it stands, if you make >>>> something a regular member variable then you have to change the schema to >>>> add the element/column, and write a scrub to update older data. We >>>> strongly >>>> prefer not to do that during a stable release cycle and generally require >>>> that the last version of the previous stable series be able to read both >>>> formats. >>> >>> Yes. Although what I'm concerned with is only the lookup performance, not >>> the serialization place of that option. The serialization may be kept >>> unchanged completely. >> >> Right. So thinking about that some more, persistence (I think that’s a >> better description of this than serialization) is driven by the backends >> rather than the objects, so you can create a member variable and as long as >> you don’t tell the backends about it it won’t be a problem. That seems to >> be what you’ve tried to do in your patch. > > Yes, exactly. This can be applied in this case, and maybe others, if there > are > more data fields that turn out to be a performance bottleneck. > >>>> The less involved approaches would be to cache the value or to make KVP >>>> retrieval more efficient. I suspect in this case that caching will be the >>>> easiest. >>> >>> Yes. I've introduced some caching of this value here, but I still need a >>> little bit of help: >>> ... >>> >>> However, I broke the original feature in that change. To reproduce: Open >>> some register where there is some text in the "Num" field, say "1111". >>> Switch on the "Double Line" view mode. Then open File -> Properties, and >>> on the first tab, activate the option "Use Split Action Field for >>> Number". Press Ok. Before my commit, in the opened register the "1111" >>> now moved from the first line of the txn to the second line, and vice >>> versa after changing that option again. Unfortunately my commit broke >>> that feature. Maybe someone has a good idea why? Thanks for some >>> pointers. >> >> As a first guess I’d look at the hook list execution to make sure that your >> adding qof_book_option_num_field_source_changed_cb isn’t somehow preventing >> gnc_plugin_page_register_sort_book_option_changed (in >> gnucash/gnome/gnc-plugin-page-register.c) from running. > > I've gotten rid of that weird function from engine-helpers.[hc] and used the > plan GObject's "notify" signal [1]. Also, the first implementation broke the > respective unittests. I've got another try, this time with the unittests ok > again: > https://github.com/cstim/gnucash/commit/c915ec0954c49ecdd65af84680354c38fe192614 > Unfortunately, the UI feature still seems to be broken as described above, > which is somewhat weird since this is what the unittests should have caught. > > Changing the option in the UI is done inside the Scheme option system. This > particular option is defined in business-prefs.scm. Maybe the changed value > of > the book's property in this case does not trigger the "notify" signal of the > GObject type system, but all the calls to qof_instance_set in the unittests > do > trigger that signal? > > In any case the pattern for the performance speed-up should be rather clear: > Define a cached value of the KVP, register a callback on each write access, > and have each write access invalidate the previously cached value so that it > is retrieved again and stored as valid cached value. The open question here > seems to be where to find the correct callbacks on write access. >
Christian, Yeah, the Scheme option code calls qof_book_set_option and that sets the slot directly with KvpFrame::set_path instead of calling qof_instance_set (which calls g_object_set) so it won’t emit the notify::spilt_action_num_field signal when you change the setting in File>Options. Are you sure that’s working? The unit test on the other hand uses qof_instance_set which wraps g_obect_set_valist and that does fire the notify signal. qof_book_set_option, being defined in qofbook.cpp, offers a simpler approach: Instead of dealing with callbacks just set the cached value along with the slot. You can fix qof_book_set_property:PROP_OPT_NUM_FIELD_SOURCE to call qof_book_set_option instead of qof_instance_set_path_kvp. You’ll still need qof_book_use_split_action_for_num to read the slot on its first call because qof_book_init can’t set cached_num_field_source from KVP because at least in the SQL backend the book is constructed before the slots are loaded. Regards, John Ralls _______________________________________________ gnucash-devel mailing list [email protected] https://lists.gnucash.org/mailman/listinfo/gnucash-devel
