Re: Some notes on pgAdmin

On Sat, Feb 28, 2009 at 12:19 AM, Chuck McDevitt
<cmcdevitt(at)greenplum(dot)com> wrote:
> 1.       Going through the code, I noticed that quite a few files use 4
> spaces for indent on most of their lines, but have some lines using tab
> characters. Usually it’s newer patches that introduced the tabs, but not
> always.  Mixing both kinds of indents is confusing.

Yeah - we generally use 4 spaces, but tabs keep creeping in. I'm
beginning to this we should use tabs instead, and see if we can't run
something akin to pgindent over the code to clean it up nicely.

What do people prefer - tabs or spaces?

> 2.       Pgadmin3.wxs should not have hard-coded paths referring to
> “C:\Program Files”.  Some people have their program files directory on
> another drive, and anyone who is running 64-bit windows will find this
> doesn’t work, because the directory is “C:\Program Files (x86)”.  Wixs has
> variables that point to the right one.  Or use the environment variable
> “VCINSTALLDIR”, but I think the former is easier.

Agreed - I see that's in the Greenplum patch though, so we'll pickup
the fix from that.

> 3.       For the same reason, pgAdmin code that searches for client software
> (pgDump) shouldn’t assume “C:\Program Files”.  This doesn’t work at all on
> 64-bit windows.  The environment variables “%ProgramFiles(x86)%” or
> “%ProgramFiles%” points to the right directory (if they are set).
> “ProgramFiles(x86)” should be preferred if set, because that is where 32-bit
> apps like Postgres get installed.  There may be a better way to do this.

That's fixed in SVN - thanks for pointing out the error.

> 4.       Why do you include the VC 8 redistributable files directly, instead
> of using the standard merge modules:  Microsoft_VC80_CRT_x86.msm and
> policy_8_0_Microsoft_VC80_CRT_x86.msm (usually in “C:\Program Files\Common
> Files\Merge Modules”)?

Hangover from the pre-VC 2005 days that noone ever bothered to fix.
I'll look at it.

> 5.       The current installer files don’t work for Visual Studio 2008, as
> compiles from that need the VC90 redistributables.

I don't know that any of us have tried building with 2K8 yet anyway,
but if/when point 4 is fixed, I'll be happy to accept a patch if
anyone can figure out a nice way to automatically pickup the correct
runtimes regardless of VC++ version.

Thanks!

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com