Comment 29 for bug 800561

(In reply to comment #10)
> Ok, some comments:
> 1. You did not change VERSION_INFO in configure.ac. I guess it was just
> overlooked, right? The API is changed, even though in the compatible way.

Right, sorry, I overlooked it. I should be bumped for the patches 2 to 5.

> 2. The xkl_config_item_new_with_data - I do not have strong objection, but why
> are just plain setters/getters not sufficient?

They would be sufficient. If you prefer individual setters, I can change the patches accordingly.

> 3. Why is that in Makefile.am?
> -xklavier_headers = xklavier.h xkl_config_registry.h xkl_engine.h \
> - xkl_config_rec.h xkl_config_item.h xkl_engine_marshal.h
> +xklavier_headers = xkl_engine.h xkl_config_item.h xkl_config_registry.h \
> + xkl_config_rec.h xkl_engine_marshal.h xklavier.h

With the original order, g-ir-scanner failed on some unknown symbols. It seems to have some left-to-right ordering. That's not very scientific, I know, I'm not a guru for the g-i implementation.

> 4. set_log_appender is mostly for debugging, so for now let's ignore it

Agreed.

> 5. I will check around get_current_state. Perhaps making it const would be
> affordable...

I'm not sure that const will help, as even with const the interpreter might still need to copy the value. Boxing the struct (i. e. providing a proper constructor and _copy() method) should help. Again, I wasn't terribly concerned about this particular method; once you are happy with the current patches and they can land, we can do the finishing touches.