Re: [Planner Dev] First patch to database backend -> New version

[Richard Hult <richard imendio com>]

On ons, 2004-07-28 at 19:52 +0200, Alvaro del Castillo wrote:
> Hi guys!

Hi! 

> > Looks good... but why did you change the gpl header? :)

> Reverted!

:)

> > There are missing spaces etc coding style wise that would be nice to get
> > fixed before committing. Other than that:

> I have review all the code another time trying to find style issues and
> can't find them :(

Some of them:

+       g_strfreev(versionv_new);

missing space

+static gboolean
+check_database_tables (GdaConnection *conn)
+{
+       GdaDataModel   *res;

too many spaces before the variables

+               gchar  *sql_file = g_build_path (G_DIR_SEPARATOR_S,
+                                                SQL_DIR,
+                                                name,
+                                                NULL);

I think you are looking for g_build_filename () here.

+       else if (upgradable && !create_tables) {
+               gchar        *contents;

too many spaces

+       if (!GDA_IS_CONNECTION (conn)) {

shouldn't that just be a NULL check?

> >   _("Database %s need to be upgraded to version: %s."
> >   " Please backup the database before upgrading."
> >   " Have you done the backup and want to continue?")
> > 
> > The last sentence should probably be removed (likewise for one more
> > dialog).
> 
> Hmmm, I think the user need to answer a clear question in the dialog.
> Without this last sentence, no question is shown to the user. I have
> left it as: "Do you want to continue?"

Well, the dialog should inform about the problem and then the buttons
should show the choices, instead of yes/no. Perhaps something like:

 The database %s needs to be upgraded from version %s to %s.

                                   [ Cancel ]  [ Upgrade ]


> > _("Can't create tables in database %s. File %s could be corrupted."
> >  "\n\nDatabase error: \n%s"),
> > 
> > I think a corrupt file would be the least probable error here, no reason
> > to make a guess like that, IMO.
> 
> Yes, I have changed that to:
> 
> _("Can't create tables in database %s. File %s is not correct, maybe a
> install problem."
> 								   "\n\nDatabase error: \n%s"),

Should probably be "Could not...". I still think we are trying to make
an impossible guess. We should check if the upgrade file exists and if
not we should say that the database is out of date but can't be upgraded
automatically from version foo to version bar.

If the file exists and the upgrade doesn't work, it's probably not an
installation problem, but more likely a bug in the upgrade file or the C
code or the way that the database is set up. Since we have no idea which
it is, we should just say that the upgrade failed and show the error
message, something like:

 The database upgrade failed:\n\n%s

 
> > Should set the main window as parent window for those dialogs (also
> > something we need to go through and fix in older code).
> 
> Done! I will look to all planner code to solve other places with this
> problem.

We could add a bug for that, sounds like a perfect way to get started on
planner ;)

> > 
> >        _("Tables in database %s doesn't exist. "
> >       "Do you want to create them?"),
> > 
> > Should be "do not exist"... (or a more friendly wording like "The
> > database is not setup for Planner. Do you want to do that?".)
> > 
> 
> Changed it two places to:
> 
> _("The database is not setup for Planner. "
> 						   "Do you want to do that?")

Hm, a question: Couldn't we just go ahead and add create the tables? If
the user wants to use the database in the first place, why ask again?

/Richard

-- 
Imendio HB, http://www.imendio.com/