Created attachment 83348 [details] [review]
gda-mdb-provider.c patch

In addition, providers/mdb/gda-mdb-provider.c still uses gda_handler_bin_new_with_prov() instead of gda_handler_bin_new().  Patch attached.

Thanks. I have committed that patch.

The problem with the use of the non-existant gda_data_model_get_command_text() remains.

I see that the latest tarball really is that old. Ubuntu (or maybe Debian) must be packaging code from the mdbtools CVS. If mdbtools is not being maintained then it's probably not a big priority for us.

But maybe some #ifdefing or comments in the code would be useful. 

> The problem with the use of the non-existant gda_data_model_get_command_text()
> remains.

I'm not sure how to fix this.

Please just move the necessary declarations into the relevant public header, adding a comment in the private header about where they are now declared, and documenting them (in the gtk-doc) comments as only for use by provider implementations. I remember seeing an email from Vivien saying that he planned to do this, but it does not seem to have happened.

Please try not to make more functions public than necessary. Some of these private functions do not seem to be used at all.

Thanks.

A grep doesn't show any corresponding code for these declarations.

True. I see no implementation for gda_data_model_get_command_text(). I guess we need to revive it from a previous version.

Relevant diffs:

http://svn.gnome.org/viewcvs/libgda/trunk/libgda/gda-data-model.c?r1=2578&r2=2593
http://svn.gnome.org/viewcvs/libgda/trunk/libgda/gda-data-model.h?r1=2478&r2=2560

So, all we need is this implementation:

const gchar * 	 
gda_data_model_get_command_text (GdaDataModel *model) 	 
{ 	 
	g_return_val_if_fail (GDA_IS_DATA_MODEL (model), NULL); 	 
	  	 
	if (GDA_DATA_MODEL_GET_CLASS (model)->i_get_command) 	 
		return (GDA_DATA_MODEL_GET_CLASS (model)->i_get_command) (model, NULL); 	 
	else 	 
		return NULL; 	 
}

But the i_get_command vfunc no longer exists, so this no longer makes sense.


I am now inclined to just disable the ldap provider, because this is the only one that uses it. I will comment the code to point out where it no longer builds.

Hmm, unfortunately it's used in the MySQL and Oracle providers too.

i_get_command was removed in 
http://svn.gnome.org/viewcvs/libgda/trunk/libgda/gda-data-model.h?r1=2578&r2=2593

This function call (gda_data_model_get_command_text) was removed because it does not make sense to have such a call for all the GdaDataModel implementations (for example for the data model importing data from a CSV file). 

Moreover this function call is used exclusively for code which update a data model returned from a SELECT query, and one of the post 3.0 task is to remove that (non correctly working) feature and replace it with something working. I'll disable that code for the time being and for the 3.0 release.

So what does that mean for the three affected providers?

It just means you can't update a GdaDataModel object you got from executing a SELECT command. This feature has never worked correctly, so it's better if it's disabled until it works correctly.

So you will patch this?

It's in SVN.

Great.  r2870 builds OOTB on Cygwin with all 9 possible providers.

See you all in bug 419063, hopefully the last blocker for 3.0.

Vivien has changed the code so that it no longer uses the callbacks that use gda_data_model_get_command_text(). I have #ifdef-ed out the definitions of the functions that used gda_data_model_get_command_text(), to prevent linker errors, and added comments pointing back to this bug report.

Interestingly, some of the other providers do have *recordset_append_values() implementations that don't seem to need gda_data_model_get_command_text().

Providers which allow updates to some of the GdaDataModel they created when a SELECT statement was executed need to craft "by hand" the INSERT, DELETE and UPDATE queries they need to run in order to actually update the data model. This feature has been present since the early days of libgda, but its completeness has always varied from provider to provider. I plan to rationalize that to make it available at a low cost to all the providers, but that is post 3.0.

Please don't forget this now that libgda 4 has been started on svn trunk.

> Please don't forget this now that libgda 4 has been started on svn trunk.

Update, anyone?

As of rev#3200, it is functionnal and works with any provider: it's implemented in the GdaDataSelect class which is the base class all database providers are required to use when returning a resultset from the execution of a SELECT statement.

I'm in the process of updating the documentation to make the usage easier.

Closing the bug.