Обсуждение: 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

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

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

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

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

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

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

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