Thread: Foreign key dialog
Hi, I was looking at a foreign key details. I wonder why the "FK index name" is only available on the dialog. Or is it the same thing that the "Covering index" on the details table ? Last one (and this is why I'm looking at this right now), if I drop this index and display the details of the foreign key with the foreign key dialog, I see that a name is displayed despite the fact that there's no such index. And the SQL tab says nothing has changed because the auto index checkbox is not checked. All this seems a bit disturbing. ISTM that either the "Auto Index" checkbox should be checked or the "FK index name" should be disabled till "Auto Index" checkbox is checked. Comments? Regards. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Guillaume Lelarge wrote: > Hi, > > I was looking at a foreign key details. I wonder why the "FK index name" > is only available on the dialog. Or is it the same thing that the > "Covering index" on the details table ? I believe so, yes. The naming difference could be fixed I guess. > Last one (and this is why I'm looking at this right now), if I drop this > index and display the details of the foreign key with the foreign key > dialog, I see that a name is displayed despite the fact that there's no > such index. And the SQL tab says nothing has changed because the auto > index checkbox is not checked. > > All this seems a bit disturbing. Not surprising - dropping the index doesn't cause the FK object to be refreshed. That would actually be quite difficult I imagine as there is no direct link between the two. > ISTM that either the "Auto Index" checkbox should be checked or the "FK > index name" should be disabled till "Auto Index" checkbox is checked. The checkbox tells pgAdmin to create an index with the specified name to cover the FK columns. When looking at the properties later, it will always be cleared - though I guess it might be better to set it if there is an index, and disable both it and the index name field. Are you going to tweak that with the other stuff you were going to do? Regards, Dave.
Dave Page a écrit : > Guillaume Lelarge wrote: >> I was looking at a foreign key details. I wonder why the "FK index name" >> is only available on the dialog. Or is it the same thing that the >> "Covering index" on the details table ? > > I believe so, yes. The naming difference could be fixed I guess. > I'll check that and fix if necessary. >> Last one (and this is why I'm looking at this right now), if I drop this >> index and display the details of the foreign key with the foreign key >> dialog, I see that a name is displayed despite the fact that there's no >> such index. And the SQL tab says nothing has changed because the auto >> index checkbox is not checked. >> >> All this seems a bit disturbing. > > Not surprising - dropping the index doesn't cause the FK object to be > refreshed. That would actually be quite difficult I imagine as there is > no direct link between the two. > OK. >> ISTM that either the "Auto Index" checkbox should be checked or the "FK >> index name" should be disabled till "Auto Index" checkbox is checked. > > The checkbox tells pgAdmin to create an index with the specified name to > cover the FK columns. When looking at the properties later, it will > always be cleared - though I guess it might be better to set it if there > is an index, and disable both it and the index name field. > > Are you going to tweak that with the other stuff you were going to do? > Which other stuff ? the stuff I told on my "1.8.1 timetable" answer ? Anyways, I'll look at the fk stuff tomorrow morning. I'll take some time to do it. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Guillaume Lelarge wrote: > Which other stuff ? the stuff I told on my "1.8.1 timetable" answer ? Yup. > Anyways, I'll look at the fk stuff tomorrow morning. I'll take some time > to do it. Great, thanks. /D
Guillaume Lelarge wrote: > Dave Page a écrit : >> Guillaume Lelarge wrote: >>> I was looking at a foreign key details. I wonder why the "FK index name" >>> is only available on the dialog. Or is it the same thing that the >>> "Covering index" on the details table ? >> I believe so, yes. The naming difference could be fixed I guess. >> > > I'll check that and fix if necessary. > Done with this patch (needs an embed-xrc run). >>> ISTM that either the "Auto Index" checkbox should be checked or the "FK >>> index name" should be disabled till "Auto Index" checkbox is checked. >> The checkbox tells pgAdmin to create an index with the specified name to >> cover the FK columns. When looking at the properties later, it will >> always be cleared - though I guess it might be better to set it if there >> is an index, and disable both it and the index name field. >> >> Are you going to tweak that with the other stuff you were going to do? >> > [...] > > Anyways, I'll look at the fk stuff tomorrow morning. I'll take some time > to do it. > Done too. I've put the dlgForeignKey::OnCheckDeferrable() code on dlgForeignKey::CheckChange() because we need to have chkDeferred disabled when we open the dlgForeignKey to create a new foreign key. The rest of the code checks chkAutoIndex value to enable/disable stIndexName and txtIndexName. Seems to work as I want. If you think this is enough, I can apply it (1.8 branch and trunk ?). Regards. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com Index: pgadmin/dlg/dlgForeignKey.cpp =================================================================== --- pgadmin/dlg/dlgForeignKey.cpp (révision 6897) +++ pgadmin/dlg/dlgForeignKey.cpp (copie de travail) @@ -43,7 +43,7 @@ BEGIN_EVENT_TABLE(dlgForeignKey, dlgProperty) - EVT_CHECKBOX(XRCID("chkDeferrable"), dlgForeignKey::OnCheckDeferrable) + EVT_CHECKBOX(XRCID("chkDeferrable"), dlgProperty::OnChange) EVT_CHECKBOX(XRCID("chkAutoIndex") , dlgProperty::OnChange) EVT_TEXT(XRCID("txtIndexName"), dlgProperty::OnChange) @@ -80,16 +80,6 @@ } -void dlgForeignKey::OnCheckDeferrable(wxCommandEvent &ev) -{ - bool canDef=chkDeferrable->GetValue(); - stDeferred->Enable(canDef); - if (!canDef) - chkDeferred->SetValue(false); - chkDeferred->Enable(canDef); -} - - wxString dlgForeignKey::DefaultIndexName(const wxString &name) { if (name.Left(3) == wxT("fk_")) @@ -129,7 +119,15 @@ cols += qtIdent(lstColumns->GetText(pos)); } + bool canDef=chkDeferrable->GetValue(); + stDeferred->Enable(canDef); + if (!canDef) + chkDeferred->SetValue(false); + chkDeferred->Enable(canDef); + stIndexName->Enable(chkAutoIndex->GetValue()); + txtIndexName->Enable(chkAutoIndex->GetValue()); + wxString coveringIndex; if (table) coveringIndex = table->GetCoveringIndex(mainForm->GetBrowser(), cols); @@ -143,7 +141,7 @@ txtIndexName->SetValue(savedIndexName); } - wxString idxName=txtIndexName->GetValue().Strip(wxString::both); + wxString idxName = txtIndexName->GetValue().Strip(wxString::both); if (name != savedFKName || idxName == savedIndexName) { @@ -158,7 +156,7 @@ else { if (chkAutoIndex->IsEnabled()) - savedIndexName=txtIndexName->GetValue(); + savedIndexName = txtIndexName->GetValue(); txtIndexName->SetValue(coveringIndex); chkAutoIndex->SetValue(false); @@ -191,6 +189,7 @@ _("Please specify FK index name.")); EnableOK(enable); } + } Index: pgadmin/ui/dlgForeignKey.xrc =================================================================== --- pgadmin/ui/dlgForeignKey.xrc (révision 6897) +++ pgadmin/ui/dlgForeignKey.xrc (copie de travail) @@ -126,7 +126,7 @@ <object class="wxStaticText" name="stIndexName"> - <label>FK index name</label> + <label>Covering index</label> <pos>5,88d</pos>
Guillaume Lelarge wrote: > Seems to work as I want. If you think this is enough, I can apply it > (1.8 branch and trunk ?). Thanks - I've applied it with a couple of changes: - Remove the OnCheckDeferrable() declaration from the header file - Stop disabling the static text labels - we don't normally do that (though I'm aware there are other places that need fixing). - Fix one case where the checkbox got cleared when it shouldn't. Thanks!! /D
Dave Page wrote: > Guillaume Lelarge wrote: >> Seems to work as I want. If you think this is enough, I can apply it >> (1.8 branch and trunk ?). > > Thanks - I've applied it with a couple of changes: > > - Remove the OnCheckDeferrable() declaration from the header file > > - Stop disabling the static text labels - we don't normally do that > (though I'm aware there are other places that need fixing). > I understand why you removed that part (for consistency), but isn't it something that we should do ? it's much simple to know which component is disabled when the static label is disabled too. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Guillaume Lelarge wrote: > Dave Page wrote: >> Guillaume Lelarge wrote: >>> Seems to work as I want. If you think this is enough, I can apply it >>> (1.8 branch and trunk ?). >> Thanks - I've applied it with a couple of changes: >> >> - Remove the OnCheckDeferrable() declaration from the header file >> >> - Stop disabling the static text labels - we don't normally do that >> (though I'm aware there are other places that need fixing). >> > > I understand why you removed that part (for consistency), but isn't it > something that we should do ? it's much simple to know which component > is disabled when the static label is disabled too. > > Perhaps - feel free to go through the rest of the code and make it consistent the other way :-) /D