Обсуждение: new patch for index DESC/NULLS FIRST/NULLS LAST

hi, all

new version Patch for TODO item:
- Add support for DESC/NULLS FIRST/NULLS LAST when creating indexes.

The Go method in dlgIndex seems clumsy. Maybe we need a new
pgIndexColumn class or another coding?

Now, OnAddCol, OnRemoveCol, GetColumns and OnChangeSize methods have
different vesion in dlgIndex and dlgIndexConstraint.

The OnChangeSize for Mac in dlgIndex rewrite to:
    lstColumns->SetSize(wxDefaultCoord, wxDefaultCoord,
        ev.GetSize().GetWidth(), ev.GetSize().GetHeight() - 800);
I don't know whether the size is right.

New method OnDescChange added to dlgIndex.

Please review.

-----------------------------------------------
Quan Zongliang
quanzongliang@gmail.com
CIT Japan:  http://www.cit.co.jp
CIT China:  http://www.citbj.com.cn

Hi,

On Wed, Nov 19, 2008 at 12:38 PM, Quan Zongliang
<quanzongliang@gmail.com> wrote:
> hi, all
>
> new version Patch for TODO item:
> - Add support for DESC/NULLS FIRST/NULLS LAST when creating indexes.
>
> The Go method in dlgIndex seems clumsy. Maybe we need a new
> pgIndexColumn class or another coding?

Hmm, it's getting that way - but another class seems a little
excessive at this point I think.

That code could use some formatting changes though - for example, in
pgAdmin we use:

if (foo == 0)
{
    ...
}

not

if (foo == 0) {
    ...
}

We also try to leave empty lines between distinct blocks of code to
aid readability.


> Now, OnAddCol, OnRemoveCol, GetColumns and OnChangeSize methods have
> different vesion in dlgIndex and dlgIndexConstraint.
>
> The OnChangeSize for Mac in dlgIndex rewrite to:
>        lstColumns->SetSize(wxDefaultCoord, wxDefaultCoord,
>            ev.GetSize().GetWidth(), ev.GetSize().GetHeight() - 800);
> I don't know whether the size is right.

The sizing looks OK on Mac.

> New method OnDescChange added to dlgIndex.

OK - some thoughts:

- lstColumns should always display the order and nulls values, not
just when they are non-default.

- The NULLs radio buttons should not have a shaded box around them.
See the Mode option on dlgFunction for an example.

- I think the dialogue design would look nicer if the DESC and and
NULLs options were on the same horizontal alignment, eg:

DESC []        NULLs    O FIRST    O LAST

What do you think?

- When creating an index with one column, using DESC, I clicked on the
SQL tab and got an assert:

Thread 0 Crashed:
0   libSystem.B.dylib                 0x919bab9e __kill + 10
1   libSystem.B.dylib                 0x91a31ec2 raise + 26
2   ...x_base_carbonud-2.8.0.dylib    0x01608f84 wxTrap() + 18
3   libwx_macud_core-2.8.0.dylib      0x01022912
wxGUIAppTraitsBase::ShowAssertDialog(wxString const&) + 254
4   ...x_base_carbonud-2.8.0.dylib    0x016094f2 ShowAssertDialog(wchar_t
const*, int, wchar_t const*, wchar_t const*, wchar_t const*,
wxAppTraits*) + 412
5   ...x_base_carbonud-2.8.0.dylib    0x016097aa
wxAppConsole::OnAssertFailure(wchar_t const*, int, wchar_t const*,
wchar_t const*, wchar_t const*) + 60
6   ...x_base_carbonud-2.8.0.dylib    0x0160962c wxOnAssert(wchar_t
const*, int, char const*, wchar_t const*, wchar_t const*) + 232
7   libwx_macud_core-2.8.0.dylib      0x00f78149
wxChoice::GetString(unsigned int) const + 97
8   libwx_macud_core-2.8.0.dylib      0x00f7ada4 wxComboBox::GetValue() const + 116
9   pgAdmin3-Debug                    0x000b8fa4 dlgIndex::GetSql() + 664
(dlgIndex.cpp:397)
10  pgAdmin3-Debug                    0x000ce29d
dlgProperty::FillSQLTextfield() + 383 (dlgProperty.cpp:615)
11  pgAdmin3-Debug                    0x000cfb0e
dlgProperty::OnPageSelect(wxNotebookEvent&) + 138
(dlgProperty.cpp:932)

That aside, functionally, it seems to work as it should :-)

Thanks!

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

> That code could use some formatting changes though
> We also try to leave empty lines between distinct blocks of code to
> aid readability.

Done.

> - lstColumns should always display the order and nulls values
> - The NULLs radio buttons should not have a shaded box around them.
> - I think the dialogue design would look nicer if the DESC and and
> NULLs options were on the same horizontal alignment, eg:
>
> DESC []        NULLs    O FIRST    O LAST

Done.

> - When creating an index with one column, using DESC, I clicked on the
> SQL tab and got an assert:

I don't get it in Windows XP.
Maybe, when there is default value, my code don't invoke wxListView's
SetItem() and left original value. Is it NULL in Mac?


-----------------------------------------------
Quan Zongliang
quanzongliang@gmail.com
CIT Japan:  http://www.cit.co.jp
CIT China:  http://www.citbj.com.cn

On Sat, Nov 22, 2008 at 10:43 AM, Quan Zongliang
<quanzongliang@gmail.com> wrote:
>> That code could use some formatting changes though
>> We also try to leave empty lines between distinct blocks of code to
>> aid readability.
>
> Done.
>
>> - lstColumns should always display the order and nulls values
>> - The NULLs radio buttons should not have a shaded box around them.
>> - I think the dialogue design would look nicer if the DESC and and
>> NULLs options were on the same horizontal alignment, eg:
>>
>> DESC []        NULLs    O FIRST    O LAST
>
> Done.

Thanks - patch applied with minor tweaks to the horizontal layout, and
to ensure we always have a direction selected, so it's clear what the
user will get.

>> - When creating an index with one column, using DESC, I clicked on the
>> SQL tab and got an assert:
>
> I don't get it in Windows XP.
> Maybe, when there is default value, my code don't invoke wxListView's
> SetItem() and left original value. Is it NULL in Mac?

Looks like it was an existing bug that I hadn't spotted before - I've
fixed it in SVN. Sorry for the noise.

Thanks again for the patch!

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