Thread: pgAdmin III commit: Revert the previous change that introduced sysSetti
Revert the previous change that introduced sysSettings::WriteBool(), and restore the previous Write() overload. Whilst the change was, in theory, good, it failed to take into account that users of previous versions of pgAdmin would have true/false values stored in the Windows registry as REG_SZ values of "true" or "false", not REG_DWORDs as the new code would write. Needs more thought, and thorough testing on Windows when we re-visit this. Branch ------ master Details ------- http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=aedd49e2b7bfd3104b5a9f30a4e70c0ea2892170 Modified Files -------------- pgadmin/include/utils/sysSettings.h | 6 +++++- pgadmin/utils/sysSettings.cpp | 11 ++++++----- 2 files changed, 11 insertions(+), 6 deletions(-)
Hi Peter; Just so you're aware of this... On Thu, Feb 17, 2011 at 11:31 AM, Dave Page <dpage@pgadmin.org> wrote: > Revert the previous change that introduced sysSettings::WriteBool(), > and restore the previous Write() overload. Whilst the change was, in > theory, good, it failed to take into account that users of previous > versions of pgAdmin would have true/false values stored in the > Windows registry as REG_SZ values of "true" or "false", not REG_DWORDs > as the new code would write. Needs more thought, and thorough testing > on Windows when we re-visit this. > > Branch > ------ > master > > Details > ------- > http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=aedd49e2b7bfd3104b5a9f30a4e70c0ea2892170 > > Modified Files > -------------- > pgadmin/include/utils/sysSettings.h | 6 +++++- > pgadmin/utils/sysSettings.cpp | 11 ++++++----- > 2 files changed, 11 insertions(+), 6 deletions(-) > > > -- > Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgadmin-hackers > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pgAdmin III commit: Revert the previous change that introduced sysSetti
From
Peter Geoghegan
Date:
On 17 February 2011 11:33, Dave Page <dpage@pgadmin.org> wrote: > Hi Peter; > > Just so you're aware of this... I don't mind, but I also don't understand. How could changing the name of the function possibly affect the sort of datatype stored in the registry? -- Regards, Peter Geoghegan
On Thu, Feb 17, 2011 at 1:14 PM, Peter Geoghegan <peter.geoghegan86@gmail.com> wrote: > On 17 February 2011 11:33, Dave Page <dpage@pgadmin.org> wrote: >> Hi Peter; >> >> Just so you're aware of this... > > I don't mind, but I also don't understand. How could changing the name > of the function possibly affect the sort of datatype stored in the > registry? Well, you changed the name of the function, but not any of the call sites, so that wasn't really the issue as much as the removal of the overloaded Write(string, bool) I believe, which left one of the other overloads writing DWORDS instead of the original SZs. The reason it's not a simple fix though (ie. by updating all the WriteBool() call sites to actually call WriteBool() - itself, probably non-trivial), is that users that have existing settings will run into problems with the switch of datatypes. They'll get an error loading old settings into the new version of pgAdmin, and when it writes the new settings back, they'll get a similar error starting an older version of pgAdmin. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pgAdmin III commit: Revert the previous change that introduced sysSetti
From
Peter Geoghegan
Date:
Whoops. I had a false sense of security from ctlListView, where the ambiguity between which is the preferred cast from the double and long overloads prevents AppendItem(true) type calls from compiling at all. I thought that there were no such calls to Write() overloads here - however, there was an implicit cast to int that didn't occur with ctlListView, which is what caused the problem. I would like to produce a new patch where we deal with the problem correctly, and create multiple, non-overloaded variants plus the vanilla string function that is simply called write(), ala wx perl/python bindings. That's probably not that hard to do, and is a better, safer interface for us to use, considering that these calls are fairly prevalent. Objections? -- Regards, Peter Geoghegan
On Thu, Feb 17, 2011 at 2:59 PM, Peter Geoghegan <peter.geoghegan86@gmail.com> wrote: > Whoops. I had a false sense of security from ctlListView, where the > ambiguity between which is the preferred cast from the double and long > overloads prevents AppendItem(true) type calls from compiling at all. > I thought that there were no such calls to Write() overloads here - > however, there was an implicit cast to int that didn't occur with > ctlListView, which is what caused the problem. Yup :-) > I would like to produce a new patch where we deal with the problem > correctly, and create multiple, non-overloaded variants plus the > vanilla string function that is simply called write(), ala wx > perl/python bindings. That's probably not that hard to do, and is a > better, safer interface for us to use, considering that these calls > are fairly prevalent. Objections? No objections to that, but it does need to handle interoperability with older versions too. IOW, it needs to read/write booleans as REG_SZ values of "true" or "false", not REG_DWORDS. There may be other gotchas too - that's the one I was hitting though. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pgAdmin III commit: Revert the previous change that introduced sysSetti
From
Peter Geoghegan
Date:
On 17 February 2011 19:28, Dave Page <dpage@pgadmin.org> wrote: > No objections to that, but it does need to handle interoperability > with older versions too. IOW, it needs to read/write booleans as > REG_SZ values of "true" or "false", not REG_DWORDS. There may be other > gotchas too - that's the one I was hitting though. What about the Read() overloads? They currently work by passing a variable by reference (in the C sense, by pointer), to an overload of read. ISTM that it would be preferable to have variants with different names that return variables by value. What do you think? By the way, this isn't that much trouble. I've already finished with the write() overloads. We're mostly using the inline wrapper functions in sysSettings. -- Regards, Peter Geoghegan
Re: pgAdmin III commit: Revert the previous change that introduced sysSetti
From
Peter Geoghegan
Date:
Uh, it just occurred to me that without exceptions, there is no compelling way to report a failure from Read() variants. Qt does things like returning default constructed variables, that have values of 0 in the event of a failure. What do you think of that? Failures presumably more or less never occur with Read() overloads as things stand. -- Regards, Peter Geoghegan
On Fri, Feb 18, 2011 at 2:47 PM, Peter Geoghegan <peter.geoghegan86@gmail.com> wrote: > On 17 February 2011 19:28, Dave Page <dpage@pgadmin.org> wrote: >> No objections to that, but it does need to handle interoperability >> with older versions too. IOW, it needs to read/write booleans as >> REG_SZ values of "true" or "false", not REG_DWORDS. There may be other >> gotchas too - that's the one I was hitting though. > > What about the Read() overloads? They currently work by passing a > variable by reference (in the C sense, by pointer), to an overload of > read. ISTM that it would be preferable to have variants with different > names that return variables by value. What do you think? I'm not sure that helps - the issue is that the current code reads the string "true" and treats it as a boolean. What we need to do is ensure we always write booleans as strings, otherwise it won't be backwards compatible. Then, ReadBool() just reads a string value, and converts it. > By the way, this isn't that much trouble. I've already finished with > the write() overloads. We're mostly using the inline wrapper functions > in sysSettings. OK. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Feb 18, 2011 at 2:50 PM, Peter Geoghegan <peter.geoghegan86@gmail.com> wrote: > Uh, it just occurred to me that without exceptions, there is no > compelling way to report a failure from Read() variants. Qt does > things like returning default constructed variables, that have values > of 0 in the event of a failure. What do you think of that? Failures > presumably more or less never occur with Read() overloads as things > stand. We generally do the same (the Read() functions tend to take a third parameter containing the default value to return). But, returning an error isn't a problem - we pass the value we read back in a reference passed as a parameter, and can return true or false to indicate error or success. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: pgAdmin III commit: Revert the previous change that introduced sysSetti
From
Peter Geoghegan
Date:
On 18 February 2011 16:46, Dave Page <dpage@pgadmin.org> wrote: > I'm not sure that helps - the issue is that the current code reads the > string "true" and treats it as a boolean. What we need to do is ensure > we always write booleans as strings, otherwise it won't be backwards > compatible. Then, ReadBool() just reads a string value, and converts > it. Sure, we should be calling exactly the same function, but with a slightly different interface (at least in the case of write() variants). I thought that the interface inconsistency between read() and write() variants might be a problem for you. -- Regards, Peter Geoghegan
Re: pgAdmin III commit: Revert the previous change that introduced sysSetti
From
Peter Geoghegan
Date:
Attached patch makes changes described to write() variants. What do you think? I conducted a little test to verify that I correctly changed all call sites - implicit casts can be tricky. Once I had finished changing the function names and their call sites, I provided an overload of write with const wxChar*, complete with implementation/definition. I then provided just a declaration of all the old overloads of write(). I wanted to see if I got any linker errors. The idea of the wxChar* variant (the one with the implementation) is to prevent any write(const wxString&, const wxChar*) calls from calling my write(bool) declaration, as they prefer that to write(const wxString&, const wxString&) . Anyway, none of the old declarations were called, and I saw no linker errors. -- Regards, Peter Geoghegan
Attachment
On Sat, Feb 19, 2011 at 2:45 AM, Peter Geoghegan <peter.geoghegan86@gmail.com> wrote: > Attached patch makes changes described to write() variants. What do you think? > > I conducted a little test to verify that I correctly changed all call > sites - implicit casts can be tricky. Once I had finished changing the > function names and their call sites, I provided an overload of write > with const wxChar*, complete with implementation/definition. I then > provided just a declaration of all the old overloads of write(). I > wanted to see if I got any linker errors. The idea of the wxChar* > variant (the one with the implementation) is to prevent any > write(const wxString&, const wxChar*) calls from calling my > write(bool) declaration, as they prefer that to write(const wxString&, > const wxString&) . Anyway, none of the old declarations were called, > and I saw no linker errors. > > -- > Regards, > Peter Geoghegan > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, Feb 19, 2011 at 2:45 AM, Peter Geoghegan <peter.geoghegan86@gmail.com> wrote: > Attached patch makes changes described to write() variants. What do you think? > > I conducted a little test to verify that I correctly changed all call > sites - implicit casts can be tricky. Once I had finished changing the > function names and their call sites, I provided an overload of write > with const wxChar*, complete with implementation/definition. I then > provided just a declaration of all the old overloads of write(). I > wanted to see if I got any linker errors. The idea of the wxChar* > variant (the one with the implementation) is to prevent any > write(const wxString&, const wxChar*) calls from calling my > write(bool) declaration, as they prefer that to write(const wxString&, > const wxString&) . Anyway, none of the old declarations were called, > and I saw no linker errors. Looks good, and seems to work correctly on Windows (which is where I was expecting problems). Patch applied - thanks! -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company