Обсуждение: Enabling SQL text field in the SQL tab of object dialog
Hi all, I was looking at the new features of DBVisualizer and found something that please me. I don't know if you remember this old thread : http://www.pgadmin.org/archives/pgadmin-hackers/2007-03/msg00004.php I want to be able to change the generated SQL on object dialog. DBVisualizer seems to do it in a way that feels great. At first, editing the SQL is disabled. You can enable it by clicking on a button. See this screenshot : http://www.minq.se/products/dbvis/imageviewer.jsp?image=versions/6.0/images/6.0-alterTable.png I think this is something we can do. You can modify an object with the GUI and fine tune the queries by enabling the SQL text field. Of course, if you do this, all other components are disabled right away. If you click to disable editing, you get back to "GUI mode" and loose all your editing modifications. With appropriate alerts, I think it is acceptable for users. There's still one problem : > Also, on some dialogs there are placeholders included in the code for cases when we have multi-step queries that are tiedtogether by a generated ID. IF the user mucked about with those, it could break things particularly spectaularly. But we don't need to make the SQL text field editable for every SQL tab. Ideas ? Comments ? (this is obviously not for 1.8) Regards. -- Guillaume. <!-- http://abs.traduc.org/ http://lfs.traduc.org/ http://docs.postgresqlfr.org/ -->
Guillaume Lelarge wrote: > Hi all, > > I was looking at the new features of DBVisualizer and found something > that please me. I don't know if you remember this old thread : > http://www.pgadmin.org/archives/pgadmin-hackers/2007-03/msg00004.php > > I want to be able to change the generated SQL on object dialog. > DBVisualizer seems to do it in a way that feels great. At first, editing > the SQL is disabled. You can enable it by clicking on a button. See this > screenshot : > > http://www.minq.se/products/dbvis/imageviewer.jsp?image=versions/6.0/images/6.0-alterTable.png > > I think this is something we can do. You can modify an object with the > GUI and fine tune the queries by enabling the SQL text field. Of course, > if you do this, all other components are disabled right away. If you > click to disable editing, you get back to "GUI mode" and loose all your > editing modifications. With appropriate alerts, I think it is acceptable > for users. I'm not against the idea in principle. > There's still one problem : > >> Also, on some dialogs there are placeholders included in the code for >> cases when we have multi-step queries that are tied together by a >> generated ID. IF the user mucked about with those, it could break things >> particularly spectaularly. > > But we don't need to make the SQL text field editable for every SQL tab. True. > Ideas ? Comments ? I think I'd like to see a prototype so we can get a feel for how it would work and what might explode. It shouldn't be too hard to do - just add an option to the SQL tab, and when selected, lock the tabset to that tab. That should be doable on the appropriate base class. Regards, Dave
Dave Page a écrit : > Guillaume Lelarge wrote: > [...] >> Ideas ? Comments ? > > I think I'd like to see a prototype so we can get a feel for how it > would work and what might explode. It shouldn't be too hard to do - just > add an option to the SQL tab, and when selected, lock the tabset to that > tab. That should be doable on the appropriate base class. > Finally, here is the prototype. As I first talked about this one year ago, I will summarize the idea : adding a checkbox on the SQL tab of an object's properties to let the user change the SQL query. Checking will disable the contents of the other tabs because we don't want to try to reverse engineer the user's changes. So, here is the patch that does this. I'm sure there's work left to do (most notably some duplicate code) but, at least, it works for me on two different scenarios : changing the SQL query and adding another SQL query. Comments are welcome. Regards. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com Index: pgadmin/include/dlg/dlgProperty.h =================================================================== --- pgadmin/include/dlg/dlgProperty.h (revision 7373) +++ pgadmin/include/dlg/dlgProperty.h (working copy) @@ -86,6 +86,7 @@ void OnChange(wxCommandEvent &ev); void OnChangeOwner(wxCommandEvent &ev); void OnChangeStc(wxStyledTextEvent& event); + void OnChangeReadOnly(wxCommandEvent& event); protected: void AddUsers(ctlComboBoxFix *cb1, ctlComboBoxFix *cb2=0); @@ -97,7 +98,7 @@ pgDatabase *database; frmMain *mainForm; - ctlSQLBox *sqlPane; + wxPanel *sqlPane; wxTextValidator numericValidator; @@ -105,6 +106,8 @@ wxTextCtrl *txtName, *txtOid, *txtComment; ctlComboBox *cbOwner; ctlComboBox *cbClusterSet; + wxCheckBox *chkReadOnly; + ctlSQLBox *sqlTextField; int width, height; wxTreeItemId item, owneritem; Index: pgadmin/dlg/dlgProperty.cpp =================================================================== --- pgadmin/dlg/dlgProperty.cpp (revision 7373) +++ pgadmin/dlg/dlgProperty.cpp (working copy) @@ -59,8 +59,6 @@ #include "schema/pgUser.h" - - class replClientData : public wxClientData { public: @@ -72,6 +70,9 @@ }; +#define CTRLID_CHKSQLTEXTFIELD 1000 + + BEGIN_EVENT_TABLE(dlgProperty, DialogWithHelp) EVT_NOTEBOOK_PAGE_CHANGED(XRCID("nbNotebook"), dlgProperty::OnPageSelect) @@ -80,6 +81,8 @@ EVT_COMBOBOX(XRCID("cbOwner"), dlgProperty::OnChange) EVT_TEXT(XRCID("txtComment"), dlgProperty::OnChange) + EVT_CHECKBOX(CTRLID_CHKSQLTEXTFIELD, dlgProperty::OnChangeReadOnly) + EVT_BUTTON(wxID_HELP, dlgProperty::OnHelp) EVT_BUTTON(wxID_OK, dlgProperty::OnOK) EVT_BUTTON(wxID_APPLY, dlgProperty::OnApply) @@ -90,6 +93,7 @@ { readOnly=false; sqlPane=0; + sqlTextField=0; processing=false; mainForm=frame; database=0; @@ -311,7 +315,34 @@ void dlgProperty::CreateAdditionalPages() { - sqlPane = new ctlSQLBox(nbNotebook, CTL_PROPSQL, wxDefaultPosition, wxDefaultSize, wxTE_MULTILINE | wxSUNKEN_BORDER| wxTE_READONLY | wxTE_RICH2); + int width, height; + + // get a few sizes and widths +#ifdef __WIN32__ + GetClientSize(&width, &height); +#else + nbNotebook->GetClientSize(&width, &height); + height -= ConvertDialogToPixels(wxPoint(0, 20)).y; // sizes of tabs +#endif + wxPoint zeroPos=ConvertDialogToPixels(wxPoint(5, 5)); + wxSize chkSize=ConvertDialogToPixels(wxSize(65,12)); + + // add a panel + sqlPane = new wxPanel(nbNotebook); + + // add checkbox to the panel + chkReadOnly = new wxCheckBox(sqlPane, CTRLID_CHKSQLTEXTFIELD, wxT("Read only"), + wxPoint(zeroPos.x, zeroPos.y), + chkSize); + chkReadOnly->SetValue(true); + + // add ctlSQLBox to the panel + sqlTextField = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxPoint(zeroPos.x, zeroPos.y + chkSize.GetHeight()), + wxSize(width - 2*zeroPos.x, height - 2*zeroPos.y), + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + + // add panel to the notebook nbNotebook->AddPage(sqlPane, wxT("SQL")); } @@ -506,6 +537,42 @@ } +void dlgProperty::OnChangeReadOnly(wxCommandEvent &ev) +{ + size_t pos; + + sqlTextField->SetReadOnly(chkReadOnly->GetValue()); + for (pos = 0; pos < nbNotebook->GetPageCount() - 1; pos++) + { + nbNotebook->GetPage(pos)->Enable(chkReadOnly->GetValue()); + } + + if (chkReadOnly->GetValue()) + { + // create a function because this is a duplicated code + sqlTextField->SetReadOnly(false); + if (btnOK->IsEnabled()) + { + wxString tmp; + if (cbClusterSet && cbClusterSet->GetSelection() > 0) + { + replClientData *data=(replClientData*)cbClusterSet->GetClientData(cbClusterSet->GetSelection()); + tmp.Printf(_("-- Execute replicated using cluster \"%s\", set %ld\n"), data->cluster.c_str(), data->setId); + } + sqlTextField->SetText(tmp + GetSql() + GetSql2()); + } + else + { + if (GetObject()) + sqlTextField->SetText(_("-- nothing to change")); + else + sqlTextField->SetText(_("-- definition incomplete")); + } + sqlTextField->SetReadOnly(true); + } +} + + bool dlgProperty::tryUpdate(wxTreeItemId collectionItem) { ctlTree *browser=mainForm->GetBrowser(); @@ -753,7 +820,16 @@ return; } - wxString sql=GetSql(); + wxString sql; + if (chkReadOnly->GetValue()) + { + sql = GetSql(); + } + else + { + sql = sqlTextField->GetText(); + } + wxString sql2=GetSql2(); if (!apply(sql, sql2)) @@ -768,9 +844,10 @@ void dlgProperty::OnPageSelect(wxNotebookEvent& event) { - if (sqlPane && event.GetSelection() == (int)nbNotebook->GetPageCount()-1) + if (sqlTextField && chkReadOnly->GetValue() && + event.GetSelection() == (int)nbNotebook->GetPageCount()-1) { - sqlPane->SetReadOnly(false); + sqlTextField->SetReadOnly(false); if (btnOK->IsEnabled()) { wxString tmp; @@ -779,16 +856,16 @@ replClientData *data=(replClientData*)cbClusterSet->GetClientData(cbClusterSet->GetSelection()); tmp.Printf(_("-- Execute replicated using cluster \"%s\", set %ld\n"), data->cluster.c_str(), data->setId); } - sqlPane->SetText(tmp + GetSql() + GetSql2()); + sqlTextField->SetText(tmp + GetSql() + GetSql2()); } else { if (GetObject()) - sqlPane->SetText(_("-- nothing to change")); + sqlTextField->SetText(_("-- nothing to change")); else - sqlPane->SetText(_("-- definition incomplete")); + sqlTextField->SetText(_("-- definition incomplete")); } - sqlPane->SetReadOnly(true); + sqlTextField->SetReadOnly(true); } }
On Sat, Jun 14, 2008 at 3:01 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Finally, here is the prototype. As I first talked about this one year ago, I > will summarize the idea : adding a checkbox on the SQL tab of an object's > properties to let the user change the SQL query. Checking will disable the > contents of the other tabs because we don't want to try to reverse engineer > the user's changes. > > So, here is the patch that does this. I'm sure there's work left to do (most > notably some duplicate code) but, at least, it works for me on two different > scenarios : changing the SQL query and adding another SQL query. I haven't tested yet (just reviewed the diff), but a couple of thoughts come to mind: - Can we disable the controls on the form, rather than the tabs so the user can still browse the object details after modfying the SQL? (Or does disabling the tab effectively do that?). The downside of that is that we'd need to keep track of which controls were already disabled when we disable them all, so if re-enabling them we end up in the original state. I realise this is not what I suggested previously. - Before returning to GUI mode, we should warn the user (if he has modified the SQL) that his changes will be lost. Of he accepts the change, the SQL should be regenerated immediately. So, before I test this on my lapdog, I'm sure we all want to know - did anything actually explode? -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Sat, Jun 14, 2008 at 3:01 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >> Finally, here is the prototype. As I first talked about this one year ago, I >> will summarize the idea : adding a checkbox on the SQL tab of an object's >> properties to let the user change the SQL query. Checking will disable the >> contents of the other tabs because we don't want to try to reverse engineer >> the user's changes. >> >> So, here is the patch that does this. I'm sure there's work left to do (most >> notably some duplicate code) but, at least, it works for me on two different >> scenarios : changing the SQL query and adding another SQL query. > > I haven't tested yet (just reviewed the diff), but a couple of > thoughts come to mind: > > - Can we disable the controls on the form, rather than the tabs so the > user can still browse the object details after modfying the SQL? (Or > does disabling the tab effectively do that?). The downside of that is > that we'd need to keep track of which controls were already disabled > when we disable them all, so if re-enabling them we end up in the > original state. I realise this is not what I suggested previously. > You can't really disable a tab. You disable the page of the tab, so you can still browse the different tabs. > - Before returning to GUI mode, we should warn the user (if he has > modified the SQL) that his changes will be lost. Of he accepts the > change, the SQL should be regenerated immediately. > The patch does not warn the user, but I think it would be a good addition. But the SQL is regenerated as soon as the user clicks on the checkbox a second time. > So, before I test this on my lapdog, I'm sure we all want to know - > did anything actually explode? > AFAIK, no. Nothing exploded. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Guillaume Lelarge a écrit :
> Dave Page a écrit :
>> On Sat, Jun 14, 2008 at 3:01 PM, Guillaume Lelarge
>> <guillaume@lelarge.info> wrote:
>>
>>> Finally, here is the prototype. As I first talked about this one year
>>> ago, I
>>> will summarize the idea : adding a checkbox on the SQL tab of an
>>> object's
>>> properties to let the user change the SQL query. Checking will
>>> disable the
>>> contents of the other tabs because we don't want to try to reverse
>>> engineer
>>> the user's changes.
>>>
>>> So, here is the patch that does this. I'm sure there's work left to
>>> do (most
>>> notably some duplicate code) but, at least, it works for me on two
>>> different
>>> scenarios : changing the SQL query and adding another SQL query.
>>
>> I haven't tested yet (just reviewed the diff), but a couple of
>> thoughts come to mind:
>>
>> - Can we disable the controls on the form, rather than the tabs so the
>> user can still browse the object details after modfying the SQL? (Or
>> does disabling the tab effectively do that?). The downside of that is
>> that we'd need to keep track of which controls were already disabled
>> when we disable them all, so if re-enabling them we end up in the
>> original state. I realise this is not what I suggested previously.
>>
>
> You can't really disable a tab. You disable the page of the tab, so you
> can still browse the different tabs.
>
>> - Before returning to GUI mode, we should warn the user (if he has
>> modified the SQL) that his changes will be lost. Of he accepts the
>> change, the SQL should be regenerated immediately.
>>
>
> The patch does not warn the user, but I think it would be a good
> addition. But the SQL is regenerated as soon as the user clicks on the
> checkbox a second time.
>
>> So, before I test this on my lapdog, I'm sure we all want to know -
>> did anything actually explode?
>>
>
> AFAIK, no. Nothing exploded.
>
Grmbl... There's a big flaw in my patch. You're right once again.
Problem is, pgAdmin divide the queries to launch in two groups, only for
some objects : database and tablespace. The SQL textfield contains the
concatenation of the two groups. So we can't let the user change this
field if we aren't able to separate the two groups.
I think there are two possible ways to deal with this:
* the simpler, but confusing, one: add another textfield, put each
group in one textfield;
* the difficult, but less error prone, one: prevent the user to use the
read/write mode when he's on the database's properties (and the same
for tablespace).
I prefer the latter because it's less confusing for the user (why two
SQL textfields?), less error prone (what happens if the user takes SQL
textfield 1's contents and put it in SQL textfield 2?).
Desactivating the read/write mode for database and tablespace doesn't
seem a big issue to me, does it?
Comments?
--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com
On Mon, Jun 16, 2008 at 9:11 PM, Dave Page <dpage@postgresql.org> wrote: > Hmm, I'd forgotten about that. We do it because it allows us to run > non-transaction safe DDL in separate transactions (which is required > in 8.3). Actually, this is not a major issue - all that will happen is > that the server will reject the SQL in 8.3+ if the user includes no > transaction safe DDL with other commands (or blindly accept it in > earlier versions). I say we just split the text control on those two > dialogues, per option 1. Hmm, just to add - we could always include a comment in the GetSQL() output to explain why the first statement is separate from the rest. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Mon, Jun 16, 2008 at 8:58 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Grmbl... There's a big flaw in my patch. You're right once again. Meh. It's happened a couple of times now :-) > Problem is, pgAdmin divide the queries to launch in two groups, only for > some objects : database and tablespace. The SQL textfield contains the > concatenation of the two groups. So we can't let the user change this field > if we aren't able to separate the two groups. > > I think there are two possible ways to deal with this: > > * the simpler, but confusing, one: add another textfield, put each > group in one textfield; > * the difficult, but less error prone, one: prevent the user to use the > read/write mode when he's on the database's properties (and the same > for tablespace). > > I prefer the latter because it's less confusing for the user (why two SQL > textfields?), less error prone (what happens if the user takes SQL textfield > 1's contents and put it in SQL textfield 2?). > > Desactivating the read/write mode for database and tablespace doesn't seem a > big issue to me, does it? Hmm, I'd forgotten about that. We do it because it allows us to run non-transaction safe DDL in separate transactions (which is required in 8.3). Actually, this is not a major issue - all that will happen is that the server will reject the SQL in 8.3+ if the user includes no transaction safe DDL with other commands (or blindly accept it in earlier versions). I say we just split the text control on those two dialogues, per option 1. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Mon, Jun 16, 2008 at 8:58 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >> Grmbl... There's a big flaw in my patch. You're right once again. > > Meh. It's happened a couple of times now :-) > hehe >> Problem is, pgAdmin divide the queries to launch in two groups, only for >> some objects : database and tablespace. The SQL textfield contains the >> concatenation of the two groups. So we can't let the user change this field >> if we aren't able to separate the two groups. >> >> I think there are two possible ways to deal with this: >> >> * the simpler, but confusing, one: add another textfield, put each >> group in one textfield; >> * the difficult, but less error prone, one: prevent the user to use the >> read/write mode when he's on the database's properties (and the same >> for tablespace). >> >> I prefer the latter because it's less confusing for the user (why two SQL >> textfields?), less error prone (what happens if the user takes SQL textfield >> 1's contents and put it in SQL textfield 2?). >> >> Desactivating the read/write mode for database and tablespace doesn't seem a >> big issue to me, does it? > > Hmm, I'd forgotten about that. We do it because it allows us to run > non-transaction safe DDL in separate transactions (which is required > in 8.3). Actually, this is not a major issue - all that will happen is > that the server will reject the SQL in 8.3+ if the user includes no > transaction safe DDL with other commands (or blindly accept it in > earlier versions). I say we just split the text control on those two > dialogues, per option 1. > Option 1 seems not very user friendly to me. Are you against option 2? if yes, why? -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Mon, Jun 16, 2008 at 9:34 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Option 1 seems not very user friendly to me. Are you against option 2? if > yes, why? Yes, because it's inconsistent to offer the feature in all but two dialogues. With option 1, the worst that will happen is the user will get an error if they mix tx-sfe and non tx-safe ddl in the one box. It'll be handled gracefully by pgAdmin though, so they c an easily edit and try again. Plus we can include a short comment in the generated SQL to explain the split; something like: -- This statement is executed on it's own because it is not transaction safe. CREATE DATABASE foo; -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Mon, Jun 16, 2008 at 9:34 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >> Option 1 seems not very user friendly to me. Are you against option 2? if >> yes, why? > > Yes, because it's inconsistent to offer the feature in all but two > dialogues. With option 1, the worst that will happen is the user will > get an error if they mix tx-sfe and non tx-safe ddl in the one box. > It'll be handled gracefully by pgAdmin though, so they c an easily > edit and try again. Plus we can include a short comment in the > generated SQL to explain the split; something like: > > -- This statement is executed on it's own because it is not transaction safe. > CREATE DATABASE foo; > OK, this works on my local branch but I still have one issue with it: if the user change the name of a newly created object in the SQL text field, the treeview will display the name available in the name field, not the real one available in the SQL text field. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Mon, Jun 16, 2008 at 11:07 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > > OK, this works on my local branch but I still have one issue with it: if the > user change the name of a newly created object in the SQL text field, the > treeview will display the name available in the name field, not the real one > available in the SQL text field. The only way I can think to handle that is to refresh the parent collection, not just the object being modified. That can be potentially dangerous if the user has another property dialogue for a sibling object open though (actually that's an issue now iirc - this would just make it far more likely to occur). -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Mon, Jun 16, 2008 at 11:07 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >> OK, this works on my local branch but I still have one issue with it: if the >> user change the name of a newly created object in the SQL text field, the >> treeview will display the name available in the name field, not the real one >> available in the SQL text field. > > The only way I can think to handle that is to refresh the parent > collection, not just the object being modified. That's also the way I wanted to handle this. But we get back to the refresh issue. I will need to fix this one. > That can be > potentially dangerous if the user has another property dialogue for a > sibling object open though (actually that's an issue now iirc - this > would just make it far more likely to occur). > What kind of issue are you talking about? can you give me an example? -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Tue, Jun 17, 2008 at 8:59 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: >> That can be >> potentially dangerous if the user has another property dialogue for a >> sibling object open though (actually that's an issue now iirc - this >> would just make it far more likely to occur). >> > > What kind of issue are you talking about? can you give me an example? When you open a properties dialogue, it gets passed a pointer to the pgObject which it may use right up until it is closed. If you refresh part of the tree, you delete and recreate all the pgObjects under the node you refresh, so the dialogue can end up with a pointer to an object that's been deleted. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Tue, Jun 17, 2008 at 8:59 AM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >>> That can be >>> potentially dangerous if the user has another property dialogue for a >>> sibling object open though (actually that's an issue now iirc - this >>> would just make it far more likely to occur). >>> >> What kind of issue are you talking about? can you give me an example? > > When you open a properties dialogue, it gets passed a pointer to the > pgObject which it may use right up until it is closed. If you refresh > part of the tree, you delete and recreate all the pgObjects under the > node you refresh, so the dialogue can end up with a pointer to an > object that's been deleted. > OK, but this is already an issue. Here is patch revision 2. It creates a new textfield for the second SQL query, and it takes care of the refresh of the tree. This patch seems to resolve all issues I could find (apart from the one above). Comments? -- Guillaume. http://www.postgresqlfr.org http://dalibo.com Index: pgadmin/include/dlg/dlgProperty.h =================================================================== --- pgadmin/include/dlg/dlgProperty.h (revision 7376) +++ pgadmin/include/dlg/dlgProperty.h (working copy) @@ -60,6 +60,8 @@ void EnableOK(bool enable); virtual bool IsUpToDate() { return true; }; void ShowObject(); + + void FillSQLTextfield(); void CheckValid(bool &enable, const bool condition, const wxString &msg); static dlgProperty *CreateDlg(frmMain *frame, pgObject *node, bool asNew, pgaFactory *factory=0); @@ -86,6 +88,7 @@ void OnChange(wxCommandEvent &ev); void OnChangeOwner(wxCommandEvent &ev); void OnChangeStc(wxStyledTextEvent& event); + void OnChangeReadOnly(wxCommandEvent& event); protected: void AddUsers(ctlComboBoxFix *cb1, ctlComboBoxFix *cb2=0); @@ -97,7 +100,7 @@ pgDatabase *database; frmMain *mainForm; - ctlSQLBox *sqlPane; + wxPanel *sqlPane; wxTextValidator numericValidator; @@ -105,6 +108,9 @@ wxTextCtrl *txtName, *txtOid, *txtComment; ctlComboBox *cbOwner; ctlComboBox *cbClusterSet; + wxCheckBox *chkReadOnly; + ctlSQLBox *sqlTextField1; + ctlSQLBox *sqlTextField2; int width, height; wxTreeItemId item, owneritem; Index: pgadmin/dlg/dlgProperty.cpp =================================================================== --- pgadmin/dlg/dlgProperty.cpp (revision 7376) +++ pgadmin/dlg/dlgProperty.cpp (working copy) @@ -59,8 +59,6 @@ #include "schema/pgUser.h" - - class replClientData : public wxClientData { public: @@ -72,6 +70,9 @@ }; +#define CTRLID_CHKSQLTEXTFIELD 1000 + + BEGIN_EVENT_TABLE(dlgProperty, DialogWithHelp) EVT_NOTEBOOK_PAGE_CHANGED(XRCID("nbNotebook"), dlgProperty::OnPageSelect) @@ -80,6 +81,8 @@ EVT_COMBOBOX(XRCID("cbOwner"), dlgProperty::OnChange) EVT_TEXT(XRCID("txtComment"), dlgProperty::OnChange) + EVT_CHECKBOX(CTRLID_CHKSQLTEXTFIELD, dlgProperty::OnChangeReadOnly) + EVT_BUTTON(wxID_HELP, dlgProperty::OnHelp) EVT_BUTTON(wxID_OK, dlgProperty::OnOK) EVT_BUTTON(wxID_APPLY, dlgProperty::OnApply) @@ -90,6 +93,8 @@ { readOnly=false; sqlPane=0; + sqlTextField1=0; + sqlTextField2=0; processing=false; mainForm=frame; database=0; @@ -311,7 +316,39 @@ void dlgProperty::CreateAdditionalPages() { - sqlPane = new ctlSQLBox(nbNotebook, CTL_PROPSQL, wxDefaultPosition, wxDefaultSize, wxTE_MULTILINE | wxSUNKEN_BORDER| wxTE_READONLY | wxTE_RICH2); + int width, height; + + // get a few sizes and widths +#ifdef __WIN32__ + GetClientSize(&width, &height); +#else + nbNotebook->GetClientSize(&width, &height); + height -= ConvertDialogToPixels(wxPoint(0, 20)).y; // sizes of tabs +#endif + wxPoint zeroPos=ConvertDialogToPixels(wxPoint(5, 5)); + wxSize chkSize=ConvertDialogToPixels(wxSize(65,12)); + + // add a panel + sqlPane = new wxPanel(nbNotebook); + + // add checkbox to the panel + chkReadOnly = new wxCheckBox(sqlPane, CTRLID_CHKSQLTEXTFIELD, wxT("Read only"), + wxPoint(zeroPos.x, zeroPos.y), + chkSize); + chkReadOnly->SetValue(true); + + // add ctlSQLBox to the panel + sqlTextField1 = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxPoint(zeroPos.x, zeroPos.y + chkSize.GetHeight()), + wxSize(width - 2 * zeroPos.x, (height - 3 * zeroPos.y) / 2), + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + + sqlTextField2 = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxPoint(zeroPos.x, zeroPos.y + chkSize.GetHeight() + (height - 3 * zeroPos.y) / 2), + wxSize(width - 2 * zeroPos.x, (height - 3 * zeroPos.y) / 2), + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + + // add panel to the notebook nbNotebook->AddPage(sqlPane, wxT("SQL")); } @@ -506,6 +543,54 @@ } +void dlgProperty::OnChangeReadOnly(wxCommandEvent &ev) +{ + size_t pos; + + sqlTextField1->SetReadOnly(chkReadOnly->GetValue()); + sqlTextField2->SetReadOnly(chkReadOnly->GetValue()); + for (pos = 0; pos < nbNotebook->GetPageCount() - 1; pos++) + { + nbNotebook->GetPage(pos)->Enable(chkReadOnly->GetValue()); + } + + if (chkReadOnly->GetValue()) + { + if (wxMessageBox(_("Are you sure you wish to cancel your edit?"), _("SQL editor"), wxYES_NO) == wxNO) + return; + } +} + + +void dlgProperty::FillSQLTextfield() +{ + // create a function because this is a duplicated code + sqlTextField1->SetReadOnly(false); + sqlTextField2->SetReadOnly(false); + if (btnOK->IsEnabled()) + { + wxString tmp; + if (cbClusterSet && cbClusterSet->GetSelection() > 0) + { + replClientData *data=(replClientData*)cbClusterSet->GetClientData(cbClusterSet->GetSelection()); + tmp.Printf(_("-- Execute replicated using cluster \"%s\", set %ld\n"), data->cluster.c_str(), data->setId); + } + sqlTextField1->SetText(tmp + GetSql()); + sqlTextField2->SetText(GetSql2()); + } + else + { + if (GetObject()) + sqlTextField1->SetText(_("-- nothing to change")); + else + sqlTextField1->SetText(_("-- definition incomplete")); + sqlTextField2->SetText(wxT("")); + } + sqlTextField1->SetReadOnly(true); + sqlTextField2->SetReadOnly(true); +} + + bool dlgProperty::tryUpdate(wxTreeItemId collectionItem) { ctlTree *browser=mainForm->GetBrowser(); @@ -608,7 +693,7 @@ mainForm->GetSqlPane()->SetReadOnly(true); } } - else if (item) + else if (item && chkReadOnly->GetValue()) { wxTreeItemId collectionItem=item; @@ -753,8 +838,18 @@ return; } - wxString sql=GetSql(); - wxString sql2=GetSql2(); + wxString sql; + wxString sql2; + if (chkReadOnly->GetValue()) + { + sql = GetSql(); + sql2 = GetSql2(); + } + else + { + sql = sqlTextField1->GetText(); + sql2 = sqlTextField2->GetText(); + } if (!apply(sql, sql2)) { @@ -768,27 +863,10 @@ void dlgProperty::OnPageSelect(wxNotebookEvent& event) { - if (sqlPane && event.GetSelection() == (int)nbNotebook->GetPageCount()-1) + if (sqlTextField1 && chkReadOnly->GetValue() && + event.GetSelection() == (int)nbNotebook->GetPageCount()-1) { - sqlPane->SetReadOnly(false); - if (btnOK->IsEnabled()) - { - wxString tmp; - if (cbClusterSet && cbClusterSet->GetSelection() > 0) - { - replClientData *data=(replClientData*)cbClusterSet->GetClientData(cbClusterSet->GetSelection()); - tmp.Printf(_("-- Execute replicated using cluster \"%s\", set %ld\n"), data->cluster.c_str(), data->setId); - } - sqlPane->SetText(tmp + GetSql() + GetSql2()); - } - else - { - if (GetObject()) - sqlPane->SetText(_("-- nothing to change")); - else - sqlPane->SetText(_("-- definition incomplete")); - } - sqlPane->SetReadOnly(true); + FillSQLTextfield(); } }
On Tue, Jun 17, 2008 at 10:51 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Dave Page a écrit : >> >> When you open a properties dialogue, it gets passed a pointer to the >> pgObject which it may use right up until it is closed. If you refresh >> part of the tree, you delete and recreate all the pgObjects under the >> node you refresh, so the dialogue can end up with a pointer to an >> object that's been deleted. >> > > OK, but this is already an issue. Right, but my point is that this is going to make it more likely to occur. I wonder if we need some reference counting on objects in the tree, and only allow any kind of refresh if all child nodes have a ref count of zero. I'm not entirely sure how we'd implement that. > Here is patch revision 2. It creates a new textfield for the second SQL > query, and it takes care of the refresh of the tree. This patch seems to > resolve all issues I could find (apart from the one above). > > Comments? - Can we only have the dual textboxes on the dialogues that actually need them please? - The 'do you want to lose your changes' prompt should only be shown if changes have actually been made. - There is a drawing artifact on Mac. I may have to look at that after you commit if you have no access to suitable hardware (maybe you can get JPA to spring for a Mac Mini - they're nice and cheap :-p ) Cheers, Dave. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Tue, Jun 17, 2008 at 10:51 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >> Dave Page a écrit : >>> When you open a properties dialogue, it gets passed a pointer to the >>> pgObject which it may use right up until it is closed. If you refresh >>> part of the tree, you delete and recreate all the pgObjects under the >>> node you refresh, so the dialogue can end up with a pointer to an >>> object that's been deleted. >>> >> OK, but this is already an issue. > > Right, but my point is that this is going to make it more likely to occur. > > I wonder if we need some reference counting on objects in the tree, > and only allow any kind of refresh if all child nodes have a ref count > of zero. I'm not entirely sure how we'd implement that. > >> Here is patch revision 2. It creates a new textfield for the second SQL >> query, and it takes care of the refresh of the tree. This patch seems to >> resolve all issues I could find (apart from the one above). >> >> Comments? > > - Can we only have the dual textboxes on the dialogues that actually > need them please? > Done. I thought we didn't hide UI objects, only disable them, to get a more consistant UI. > - The 'do you want to lose your changes' prompt should only be shown > if changes have actually been made. > Done too. > - There is a drawing artifact on Mac. I may have to look at that after > you commit if you have no access to suitable hardware (maybe you can > get JPA to spring for a Mac Mini - they're nice and cheap :-p ) > I forwarded this mail to jpa and damien, just to know what they think about your great idea :) Unfortunately, no answer yet. The button text doesn't please me. Neither do the message box text. If one has a better idea, I'll be glad to know about it. Cheers. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com Index: pgadmin/include/dlg/dlgProperty.h =================================================================== --- pgadmin/include/dlg/dlgProperty.h (revision 7376) +++ pgadmin/include/dlg/dlgProperty.h (working copy) @@ -60,6 +60,8 @@ void EnableOK(bool enable); virtual bool IsUpToDate() { return true; }; void ShowObject(); + + void FillSQLTextfield(); void CheckValid(bool &enable, const bool condition, const wxString &msg); static dlgProperty *CreateDlg(frmMain *frame, pgObject *node, bool asNew, pgaFactory *factory=0); @@ -86,6 +88,7 @@ void OnChange(wxCommandEvent &ev); void OnChangeOwner(wxCommandEvent &ev); void OnChangeStc(wxStyledTextEvent& event); + void OnChangeReadOnly(wxCommandEvent& event); protected: void AddUsers(ctlComboBoxFix *cb1, ctlComboBoxFix *cb2=0); @@ -97,7 +100,7 @@ pgDatabase *database; frmMain *mainForm; - ctlSQLBox *sqlPane; + wxPanel *sqlPane; wxTextValidator numericValidator; @@ -105,6 +108,10 @@ wxTextCtrl *txtName, *txtOid, *txtComment; ctlComboBox *cbOwner; ctlComboBox *cbClusterSet; + wxCheckBox *chkReadOnly; + ctlSQLBox *sqlTextField1; + ctlSQLBox *sqlTextField2; + bool enableSQL2; int width, height; wxTreeItemId item, owneritem; Index: pgadmin/dlg/dlgProperty.cpp =================================================================== --- pgadmin/dlg/dlgProperty.cpp (revision 7376) +++ pgadmin/dlg/dlgProperty.cpp (working copy) @@ -59,8 +59,6 @@ #include "schema/pgUser.h" - - class replClientData : public wxClientData { public: @@ -72,6 +70,9 @@ }; +#define CTRLID_CHKSQLTEXTFIELD 1000 + + BEGIN_EVENT_TABLE(dlgProperty, DialogWithHelp) EVT_NOTEBOOK_PAGE_CHANGED(XRCID("nbNotebook"), dlgProperty::OnPageSelect) @@ -80,6 +81,8 @@ EVT_COMBOBOX(XRCID("cbOwner"), dlgProperty::OnChange) EVT_TEXT(XRCID("txtComment"), dlgProperty::OnChange) + EVT_CHECKBOX(CTRLID_CHKSQLTEXTFIELD, dlgProperty::OnChangeReadOnly) + EVT_BUTTON(wxID_HELP, dlgProperty::OnHelp) EVT_BUTTON(wxID_OK, dlgProperty::OnOK) EVT_BUTTON(wxID_APPLY, dlgProperty::OnApply) @@ -90,6 +93,8 @@ { readOnly=false; sqlPane=0; + sqlTextField1=0; + sqlTextField2=0; processing=false; mainForm=frame; database=0; @@ -117,6 +122,11 @@ txtComment = CTRL_TEXT("txtComment"); cbOwner = CTRL_COMBOBOX2("cbOwner"); cbClusterSet = CTRL_COMBOBOX2("cbClusterSet"); + + wxString db = wxT("Database"); + wxString ts = wxT("Tablespace"); + enableSQL2 = db.Cmp(factory->GetTypeName()) == 0 + || ts.Cmp(factory->GetTypeName()) == 0; wxNotebookPage *page=nbNotebook->GetPage(0); wxASSERT(page != NULL); @@ -311,7 +321,50 @@ void dlgProperty::CreateAdditionalPages() { - sqlPane = new ctlSQLBox(nbNotebook, CTL_PROPSQL, wxDefaultPosition, wxDefaultSize, wxTE_MULTILINE | wxSUNKEN_BORDER| wxTE_READONLY | wxTE_RICH2); + int width, height; + + // get a few sizes and widths +#ifdef __WIN32__ + GetClientSize(&width, &height); +#else + nbNotebook->GetClientSize(&width, &height); + height -= ConvertDialogToPixels(wxPoint(0, 20)).y; // sizes of tabs +#endif + wxPoint zeroPos=ConvertDialogToPixels(wxPoint(5, 5)); + wxSize chkSize=ConvertDialogToPixels(wxSize(65,12)); + + // add a panel + sqlPane = new wxPanel(nbNotebook); + + // add checkbox to the panel + chkReadOnly = new wxCheckBox(sqlPane, CTRLID_CHKSQLTEXTFIELD, wxT("Read only"), + wxPoint(zeroPos.x, zeroPos.y), + chkSize); + chkReadOnly->SetValue(true); + + // add ctlSQLBoxes to the panel + + if (enableSQL2) + { + sqlTextField1 = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxPoint(zeroPos.x, zeroPos.y + chkSize.GetHeight()), + wxSize(width - 2 * zeroPos.x, (height - 3 * zeroPos.y) / 2), + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + + sqlTextField2 = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxPoint(zeroPos.x, zeroPos.y + chkSize.GetHeight() + (height - 3 * zeroPos.y) / 2), + wxSize(width - 2 * zeroPos.x, (height - 3 * zeroPos.y) / 2), + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + } + else + { + sqlTextField1 = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxPoint(zeroPos.x, zeroPos.y + chkSize.GetHeight()), + wxSize(width - 2 * zeroPos.x, (height - 3 * zeroPos.y)), + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + } + + // add panel to the notebook nbNotebook->AddPage(sqlPane, wxT("SQL")); } @@ -506,6 +559,78 @@ } +void dlgProperty::OnChangeReadOnly(wxCommandEvent &ev) +{ + size_t pos; + + if (sqlTextField1->GetText().Cmp(GetSql()) != 0 || + (enableSQL2 && sqlTextField2->GetText().Cmp(GetSql2()) != 0)) + { + if (wxMessageBox(_("Are you sure you wish to cancel your edit?"), _("SQL editor"), wxYES_NO) == wxNO) + { + chkReadOnly->SetValue(false); + return; + } + } + + sqlTextField1->SetReadOnly(chkReadOnly->GetValue()); + if (enableSQL2) + { + sqlTextField2->SetReadOnly(chkReadOnly->GetValue()); + } + for (pos = 0; pos < nbNotebook->GetPageCount() - 1; pos++) + { + nbNotebook->GetPage(pos)->Enable(chkReadOnly->GetValue()); + } + + if (chkReadOnly->GetValue()) + { + FillSQLTextfield(); + } +} + + +void dlgProperty::FillSQLTextfield() +{ + // create a function because this is a duplicated code + sqlTextField1->SetReadOnly(false); + if (enableSQL2) + { + sqlTextField2->SetReadOnly(false); + } + if (btnOK->IsEnabled()) + { + wxString tmp; + if (cbClusterSet && cbClusterSet->GetSelection() > 0) + { + replClientData *data=(replClientData*)cbClusterSet->GetClientData(cbClusterSet->GetSelection()); + tmp.Printf(_("-- Execute replicated using cluster \"%s\", set %ld\n"), data->cluster.c_str(), data->setId); + } + sqlTextField1->SetText(tmp + GetSql()); + if (enableSQL2) + { + sqlTextField2->SetText(GetSql2()); + } + } + else + { + if (GetObject()) + sqlTextField1->SetText(_("-- nothing to change")); + else + sqlTextField1->SetText(_("-- definition incomplete")); + if (enableSQL2) + { + sqlTextField2->SetText(wxT("")); + } + } + sqlTextField1->SetReadOnly(true); + if (enableSQL2) + { + sqlTextField2->SetReadOnly(true); + } +} + + bool dlgProperty::tryUpdate(wxTreeItemId collectionItem) { ctlTree *browser=mainForm->GetBrowser(); @@ -608,7 +733,7 @@ mainForm->GetSqlPane()->SetReadOnly(true); } } - else if (item) + else if (item && chkReadOnly->GetValue()) { wxTreeItemId collectionItem=item; @@ -753,8 +878,25 @@ return; } - wxString sql=GetSql(); - wxString sql2=GetSql2(); + wxString sql; + wxString sql2; + if (chkReadOnly->GetValue()) + { + sql = GetSql(); + sql2 = GetSql2(); + } + else + { + sql = sqlTextField1->GetText(); + if (enableSQL2) + { + sql2 = sqlTextField2->GetText(); + } + else + { + sql2 = wxT(""); + } + } if (!apply(sql, sql2)) { @@ -768,27 +910,10 @@ void dlgProperty::OnPageSelect(wxNotebookEvent& event) { - if (sqlPane && event.GetSelection() == (int)nbNotebook->GetPageCount()-1) + if (sqlTextField1 && chkReadOnly->GetValue() && + event.GetSelection() == (int)nbNotebook->GetPageCount()-1) { - sqlPane->SetReadOnly(false); - if (btnOK->IsEnabled()) - { - wxString tmp; - if (cbClusterSet && cbClusterSet->GetSelection() > 0) - { - replClientData *data=(replClientData*)cbClusterSet->GetClientData(cbClusterSet->GetSelection()); - tmp.Printf(_("-- Execute replicated using cluster \"%s\", set %ld\n"), data->cluster.c_str(), data->setId); - } - sqlPane->SetText(tmp + GetSql() + GetSql2()); - } - else - { - if (GetObject()) - sqlPane->SetText(_("-- nothing to change")); - else - sqlPane->SetText(_("-- definition incomplete")); - } - sqlPane->SetReadOnly(true); + FillSQLTextfield(); } }
On Fri, Jun 20, 2008 at 12:48 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Done. I thought we didn't hide UI objects, only disable them, to get a more > consistant UI. On a single dialogue we disable objects so that dialogue will always have the same layout no matter what server you're connected to. The design from dialogue to dialogue may change as appropriate however. > The button text doesn't please me. Neither do the message box text. If one > has a better idea, I'll be glad to know about it. How about 'Lock DDL' for the checkbox? The message box could be something like: Locking the DDL will replace it with generated SQL. Are your sure you wish to lose the changes you have made? I can't help thinking that 'Lock DDL' is unnecessarily obscure though. Maybe 'Read only' is better. In anycase, there are still some sizing issues to resolve - see the attach screenshots for examples. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Вложения
Dave Page a écrit : > On Fri, Jun 20, 2008 at 12:48 AM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >> Done. I thought we didn't hide UI objects, only disable them, to get a more >> consistant UI. > > On a single dialogue we disable objects so that dialogue will always > have the same layout no matter what server you're connected to. The > design from dialogue to dialogue may change as appropriate however. > >> The button text doesn't please me. Neither do the message box text. If one >> has a better idea, I'll be glad to know about it. > > How about 'Lock DDL' for the checkbox? The message box could be something like: > > Locking the DDL will replace it with generated SQL. Are your sure you > wish to lose the changes you have made? > > I can't help thinking that 'Lock DDL' is unnecessarily obscure though. > Maybe 'Read only' is better. > I like "Lock DDL". Perhaps "Read only DDL" would please you more ? > In anycase, there are still some sizing issues to resolve - see the > attach screenshots for examples. > It is great on GTK. I really should get a windows build system... and a Mac Mini :) -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Fri, Jun 20, 2008 at 1:09 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > I like "Lock DDL". Perhaps "Read only DDL" would please you more ? Nah, I like that even less. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Guillaume Lelarge a écrit : > Dave Page a écrit : >> On Fri, Jun 20, 2008 at 12:48 AM, Guillaume Lelarge >> <guillaume@lelarge.info> wrote: >>> Done. I thought we didn't hide UI objects, only disable them, to get >>> a more >>> consistant UI. >> >> On a single dialogue we disable objects so that dialogue will always >> have the same layout no matter what server you're connected to. The >> design from dialogue to dialogue may change as appropriate however. >> >>> The button text doesn't please me. Neither do the message box text. >>> If one >>> has a better idea, I'll be glad to know about it. >> >> How about 'Lock DDL' for the checkbox? The message box could be >> something like: >> >> Locking the DDL will replace it with generated SQL. Are your sure you >> wish to lose the changes you have made? >> >> I can't help thinking that 'Lock DDL' is unnecessarily obscure though. >> Maybe 'Read only' is better. >> > > I like "Lock DDL". Perhaps "Read only DDL" would please you more ? > >> In anycase, there are still some sizing issues to resolve - see the >> attach screenshots for examples. >> > > It is great on GTK. I really should get a windows build system... and a > Mac Mini :) > I'm at the PostgreSQL booth, for the RMLL in Mont-de-Marsan (http://rmll.info). I'm trying the wifi system :) Here is the patch that fixes your issue on Win32. Works great on Win32 and Linux... I'm not sure for Mac OS X though. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com Index: pgadmin/include/dlg/dlgProperty.h =================================================================== --- pgadmin/include/dlg/dlgProperty.h (revision 7386) +++ pgadmin/include/dlg/dlgProperty.h (working copy) @@ -60,6 +60,8 @@ void EnableOK(bool enable); virtual bool IsUpToDate() { return true; }; void ShowObject(); + + void FillSQLTextfield(); void CheckValid(bool &enable, const bool condition, const wxString &msg); static dlgProperty *CreateDlg(frmMain *frame, pgObject *node, bool asNew, pgaFactory *factory=0); @@ -86,6 +88,7 @@ void OnChange(wxCommandEvent &ev); void OnChangeOwner(wxCommandEvent &ev); void OnChangeStc(wxStyledTextEvent& event); + void OnChangeReadOnly(wxCommandEvent& event); protected: void AddUsers(ctlComboBoxFix *cb1, ctlComboBoxFix *cb2=0); @@ -97,7 +100,7 @@ pgDatabase *database; frmMain *mainForm; - ctlSQLBox *sqlPane; + wxPanel *sqlPane; wxTextValidator numericValidator; @@ -105,6 +108,10 @@ wxTextCtrl *txtName, *txtOid, *txtComment; ctlComboBox *cbOwner; ctlComboBox *cbClusterSet; + wxCheckBox *chkReadOnly; + ctlSQLBox *sqlTextField1; + ctlSQLBox *sqlTextField2; + bool enableSQL2; int width, height; wxTreeItemId item, owneritem; Index: pgadmin/dlg/dlgProperty.cpp =================================================================== --- pgadmin/dlg/dlgProperty.cpp (revision 7386) +++ pgadmin/dlg/dlgProperty.cpp (working copy) @@ -59,8 +59,6 @@ #include "schema/pgUser.h" - - class replClientData : public wxClientData { public: @@ -72,6 +70,9 @@ }; +#define CTRLID_CHKSQLTEXTFIELD 1000 + + BEGIN_EVENT_TABLE(dlgProperty, DialogWithHelp) EVT_NOTEBOOK_PAGE_CHANGED(XRCID("nbNotebook"), dlgProperty::OnPageSelect) @@ -80,6 +81,8 @@ EVT_COMBOBOX(XRCID("cbOwner"), dlgProperty::OnChange) EVT_TEXT(XRCID("txtComment"), dlgProperty::OnChange) + EVT_CHECKBOX(CTRLID_CHKSQLTEXTFIELD, dlgProperty::OnChangeReadOnly) + EVT_BUTTON(wxID_HELP, dlgProperty::OnHelp) EVT_BUTTON(wxID_OK, dlgProperty::OnOK) EVT_BUTTON(wxID_APPLY, dlgProperty::OnApply) @@ -90,6 +93,8 @@ { readOnly=false; sqlPane=0; + sqlTextField1=0; + sqlTextField2=0; processing=false; mainForm=frame; database=0; @@ -117,6 +122,11 @@ txtComment = CTRL_TEXT("txtComment"); cbOwner = CTRL_COMBOBOX2("cbOwner"); cbClusterSet = CTRL_COMBOBOX2("cbClusterSet"); + + wxString db = wxT("Database"); + wxString ts = wxT("Tablespace"); + enableSQL2 = db.Cmp(factory->GetTypeName()) == 0 + || ts.Cmp(factory->GetTypeName()) == 0; wxNotebookPage *page=nbNotebook->GetPage(0); wxASSERT(page != NULL); @@ -311,7 +321,46 @@ void dlgProperty::CreateAdditionalPages() { - sqlPane = new ctlSQLBox(nbNotebook, CTL_PROPSQL, wxDefaultPosition, wxDefaultSize, wxTE_MULTILINE | wxSUNKEN_BORDER| wxTE_READONLY | wxTE_RICH2); + int width, height; + + // get a few sizes and widths + nbNotebook->GetClientSize(&width, &height); + height -= ConvertDialogToPixels(wxPoint(0, 20)).y; // sizes of tabs + wxPoint zeroPos=ConvertDialogToPixels(wxPoint(5, 5)); + wxSize chkSize=ConvertDialogToPixels(wxSize(65,12)); + + // add a panel + sqlPane = new wxPanel(nbNotebook); + + // add checkbox to the panel + chkReadOnly = new wxCheckBox(sqlPane, CTRLID_CHKSQLTEXTFIELD, wxT("Read only"), + wxPoint(zeroPos.x, zeroPos.y), + chkSize); + chkReadOnly->SetValue(true); + + // add ctlSQLBoxes to the panel + + if (enableSQL2) + { + sqlTextField1 = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxPoint(zeroPos.x, zeroPos.y + chkSize.GetHeight()), + wxSize(width - 3 * zeroPos.x, (height - 3 * zeroPos.y) / 2), + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + + sqlTextField2 = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxPoint(zeroPos.x, zeroPos.y + chkSize.GetHeight() + (height - 3 * zeroPos.y) / 2), + wxSize(width - 3 * zeroPos.x, (height - 3 * zeroPos.y) / 2), + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + } + else + { + sqlTextField1 = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxPoint(zeroPos.x, zeroPos.y + chkSize.GetHeight()), + wxSize(width - 3 * zeroPos.x, (height - 3 * zeroPos.y)), + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + } + + // add panel to the notebook nbNotebook->AddPage(sqlPane, wxT("SQL")); } @@ -506,6 +555,78 @@ } +void dlgProperty::OnChangeReadOnly(wxCommandEvent &ev) +{ + size_t pos; + + if (sqlTextField1->GetText().Cmp(GetSql()) != 0 || + (enableSQL2 && sqlTextField2->GetText().Cmp(GetSql2()) != 0)) + { + if (wxMessageBox(_("Are you sure you wish to cancel your edit?"), _("SQL editor"), wxYES_NO) == wxNO) + { + chkReadOnly->SetValue(false); + return; + } + } + + sqlTextField1->SetReadOnly(chkReadOnly->GetValue()); + if (enableSQL2) + { + sqlTextField2->SetReadOnly(chkReadOnly->GetValue()); + } + for (pos = 0; pos < nbNotebook->GetPageCount() - 1; pos++) + { + nbNotebook->GetPage(pos)->Enable(chkReadOnly->GetValue()); + } + + if (chkReadOnly->GetValue()) + { + FillSQLTextfield(); + } +} + + +void dlgProperty::FillSQLTextfield() +{ + // create a function because this is a duplicated code + sqlTextField1->SetReadOnly(false); + if (enableSQL2) + { + sqlTextField2->SetReadOnly(false); + } + if (btnOK->IsEnabled()) + { + wxString tmp; + if (cbClusterSet && cbClusterSet->GetSelection() > 0) + { + replClientData *data=(replClientData*)cbClusterSet->GetClientData(cbClusterSet->GetSelection()); + tmp.Printf(_("-- Execute replicated using cluster \"%s\", set %ld\n"), data->cluster.c_str(), data->setId); + } + sqlTextField1->SetText(tmp + GetSql()); + if (enableSQL2) + { + sqlTextField2->SetText(GetSql2()); + } + } + else + { + if (GetObject()) + sqlTextField1->SetText(_("-- nothing to change")); + else + sqlTextField1->SetText(_("-- definition incomplete")); + if (enableSQL2) + { + sqlTextField2->SetText(wxT("")); + } + } + sqlTextField1->SetReadOnly(true); + if (enableSQL2) + { + sqlTextField2->SetReadOnly(true); + } +} + + bool dlgProperty::tryUpdate(wxTreeItemId collectionItem) { ctlTree *browser=mainForm->GetBrowser(); @@ -608,7 +729,7 @@ mainForm->GetSqlPane()->SetReadOnly(true); } } - else if (item) + else if (item && chkReadOnly->GetValue()) { wxTreeItemId collectionItem=item; @@ -753,8 +874,25 @@ return; } - wxString sql=GetSql(); - wxString sql2=GetSql2(); + wxString sql; + wxString sql2; + if (chkReadOnly->GetValue()) + { + sql = GetSql(); + sql2 = GetSql2(); + } + else + { + sql = sqlTextField1->GetText(); + if (enableSQL2) + { + sql2 = sqlTextField2->GetText(); + } + else + { + sql2 = wxT(""); + } + } if (!apply(sql, sql2)) { @@ -768,27 +906,10 @@ void dlgProperty::OnPageSelect(wxNotebookEvent& event) { - if (sqlPane && event.GetSelection() == (int)nbNotebook->GetPageCount()-1) + if (sqlTextField1 && chkReadOnly->GetValue() && + event.GetSelection() == (int)nbNotebook->GetPageCount()-1) { - sqlPane->SetReadOnly(false); - if (btnOK->IsEnabled()) - { - wxString tmp; - if (cbClusterSet && cbClusterSet->GetSelection() > 0) - { - replClientData *data=(replClientData*)cbClusterSet->GetClientData(cbClusterSet->GetSelection()); - tmp.Printf(_("-- Execute replicated using cluster \"%s\", set %ld\n"), data->cluster.c_str(), data->setId); - } - sqlPane->SetText(tmp + GetSql() + GetSql2()); - } - else - { - if (GetObject()) - sqlPane->SetText(_("-- nothing to change")); - else - sqlPane->SetText(_("-- definition incomplete")); - } - sqlPane->SetReadOnly(true); + FillSQLTextfield(); } }
On Tue, Jul 1, 2008 at 11:52 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > I'm at the PostgreSQL booth, for the RMLL in Mont-de-Marsan > (http://rmll.info). I'm trying the wifi system :) :-) > Here is the patch that fixes your issue on Win32. Works great on Win32 and > Linux... I'm not sure for Mac OS X though. Hmm, this still isn't looking so good I'm afraid: - If I open the properties dialogue on a database, click the SQL tab and then uncheck Read only, having changed nothing, I get asked if I want to cancel my edit. I believe this is because in the edit case, GetSQL() will return nothing, whilst the textbox may contain '-- nothing to change' - The sizing is still wrong on Mac. Worse, the text boxes no longer resize on the resizeable dialogues (dlgFunction, dlgPackage, dlgTrigger and friends). I would suggest avoiding trying to calculate the dimensions in code - in my experience this almost never works well. Instead, I would suggest laying the two textboxes and the checkbox in a sizer and letting that do the work. Not only should that be easier to code (once you fully understand sizers!), but it should work properly on all platforms, and be resize-friendly. The same should probably be done for other standard tabs (eg, privileges, gucs) when you update all the XRC files as you discussed previously. Sorry, I know this isn't what you wanted to hear - but we'll get there :-) -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Tue, Jul 1, 2008 at 11:52 AM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >> I'm at the PostgreSQL booth, for the RMLL in Mont-de-Marsan >> (http://rmll.info). I'm trying the wifi system :) > > :-) > >> Here is the patch that fixes your issue on Win32. Works great on Win32 and >> Linux... I'm not sure for Mac OS X though. > > Hmm, this still isn't looking so good I'm afraid: > :-/ > - If I open the properties dialogue on a database, click the SQL tab > and then uncheck Read only, having changed nothing, I get asked if I > want to cancel my edit. I believe this is because in the edit case, > GetSQL() will return nothing, whilst the textbox may contain '-- > nothing to change' > Fixed. > - The sizing is still wrong on Mac. Worse, the text boxes no longer > resize on the resizeable dialogues (dlgFunction, dlgPackage, > dlgTrigger and friends). > > I would suggest avoiding trying to calculate the dimensions in code - I like this idea. > in my experience this almost never works well. Instead, I would > suggest laying the two textboxes and the checkbox in a sizer and > letting that do the work. Not only should that be easier to code (once > you fully understand sizers!), but it should work properly on all > platforms, and be resize-friendly. > OK, it wasn't easy to do. But it now works. I don't specify any size and the rendering looks great... on Linux. On Windows, it doesn't. I spent part of the last two days to try to understand the Windows behaviour but I failed. Do you have any idea on the Windows behavior? is there something Windows specific when we use the flex grid sizer? > The same should probably be done for other standard tabs (eg, > privileges, gucs) when you update all the XRC files as you discussed > previously. > > Sorry, I know this isn't what you wanted to hear - but we'll get there :-) > Oh no problem with me on this. I'm just worried on the number of bad patches. The patch attached fixed some issues: * The "Are you sure..." message is displayed only when "Read Only" is checked. * No button is the default on the "Are you sure" message (which has a weird behaviour... hitting Esc will validate the dialog and the user will lose his changes... I'm not sure we should keep this). * the first issue you had. * Sizing problems fixed on Linux with a wxFlexGridSizer. Remaining issues: * Objects' size on Windows (and Max OS X ?). -- Guillaume. http://www.postgresqlfr.org http://dalibo.com Index: pgadmin/include/dlg/dlgProperty.h =================================================================== --- pgadmin/include/dlg/dlgProperty.h (revision 7390) +++ pgadmin/include/dlg/dlgProperty.h (working copy) @@ -60,6 +60,8 @@ void EnableOK(bool enable); virtual bool IsUpToDate() { return true; }; void ShowObject(); + + void FillSQLTextfield(); void CheckValid(bool &enable, const bool condition, const wxString &msg); static dlgProperty *CreateDlg(frmMain *frame, pgObject *node, bool asNew, pgaFactory *factory=0); @@ -86,6 +88,7 @@ void OnChange(wxCommandEvent &ev); void OnChangeOwner(wxCommandEvent &ev); void OnChangeStc(wxStyledTextEvent& event); + void OnChangeReadOnly(wxCommandEvent& event); protected: void AddUsers(ctlComboBoxFix *cb1, ctlComboBoxFix *cb2=0); @@ -97,7 +100,7 @@ pgDatabase *database; frmMain *mainForm; - ctlSQLBox *sqlPane; + wxPanel *sqlPane; wxTextValidator numericValidator; @@ -105,6 +108,10 @@ wxTextCtrl *txtName, *txtOid, *txtComment; ctlComboBox *cbOwner; ctlComboBox *cbClusterSet; + wxCheckBox *chkReadOnly; + ctlSQLBox *sqlTextField1; + ctlSQLBox *sqlTextField2; + bool enableSQL2; int width, height; wxTreeItemId item, owneritem; Index: pgadmin/dlg/dlgProperty.cpp =================================================================== --- pgadmin/dlg/dlgProperty.cpp (revision 7390) +++ pgadmin/dlg/dlgProperty.cpp (working copy) @@ -59,8 +59,6 @@ #include "schema/pgUser.h" - - class replClientData : public wxClientData { public: @@ -72,6 +70,9 @@ }; +#define CTRLID_CHKSQLTEXTFIELD 1000 + + BEGIN_EVENT_TABLE(dlgProperty, DialogWithHelp) EVT_NOTEBOOK_PAGE_CHANGED(XRCID("nbNotebook"), dlgProperty::OnPageSelect) @@ -80,6 +81,8 @@ EVT_COMBOBOX(XRCID("cbOwner"), dlgProperty::OnChange) EVT_TEXT(XRCID("txtComment"), dlgProperty::OnChange) + EVT_CHECKBOX(CTRLID_CHKSQLTEXTFIELD, dlgProperty::OnChangeReadOnly) + EVT_BUTTON(wxID_HELP, dlgProperty::OnHelp) EVT_BUTTON(wxID_OK, dlgProperty::OnOK) EVT_BUTTON(wxID_APPLY, dlgProperty::OnApply) @@ -90,6 +93,8 @@ { readOnly=false; sqlPane=0; + sqlTextField1=0; + sqlTextField2=0; processing=false; mainForm=frame; database=0; @@ -117,6 +122,11 @@ txtComment = CTRL_TEXT("txtComment"); cbOwner = CTRL_COMBOBOX2("cbOwner"); cbClusterSet = CTRL_COMBOBOX2("cbClusterSet"); + + wxString db = wxT("Database"); + wxString ts = wxT("Tablespace"); + enableSQL2 = db.Cmp(factory->GetTypeName()) == 0 + || ts.Cmp(factory->GetTypeName()) == 0; wxNotebookPage *page=nbNotebook->GetPage(0); wxASSERT(page != NULL); @@ -311,8 +321,41 @@ void dlgProperty::CreateAdditionalPages() { - sqlPane = new ctlSQLBox(nbNotebook, CTL_PROPSQL, wxDefaultPosition, wxDefaultSize, wxTE_MULTILINE | wxSUNKEN_BORDER| wxTE_READONLY | wxTE_RICH2); + // create a panel + sqlPane = new wxPanel(nbNotebook); + + // add panel to the notebook nbNotebook->AddPage(sqlPane, wxT("SQL")); + + // create a flex grid sizer + wxFlexGridSizer *fgsizer = new wxFlexGridSizer(1, 5, 5); + + // add checkbox to the panel + chkReadOnly = new wxCheckBox(sqlPane, CTRLID_CHKSQLTEXTFIELD, wxT("Read only")); + chkReadOnly->SetValue(true); + fgsizer->Add(chkReadOnly, 1, wxALL | wxALIGN_LEFT, 5); + + // add the first text entry box + sqlTextField1 = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxDefaultPosition, wxDefaultSize, + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + fgsizer->Add(sqlTextField1, 1, wxALL | wxEXPAND, 5); + + // add the second text entry box (if needed) + if (enableSQL2) + { + sqlTextField2 = new ctlSQLBox(sqlPane, CTL_PROPSQL, + wxDefaultPosition, wxDefaultSize, + wxTE_MULTILINE | wxSUNKEN_BORDER | wxTE_RICH2); + fgsizer->Add(sqlTextField2, 1, wxALL | wxEXPAND, 5); + } + + fgsizer->AddGrowableCol(0); + fgsizer->AddGrowableRow(1); + fgsizer->AddGrowableRow(2); + + sqlPane->SetAutoLayout(true); + sqlPane->SetSizer(fgsizer); } @@ -506,6 +549,85 @@ } +void dlgProperty::OnChangeReadOnly(wxCommandEvent &ev) +{ + size_t pos; + bool showmessage; + + showmessage = chkReadOnly->GetValue() + && ! (!enableSQL2 && GetSql().Length() == 0 && sqlTextField1->GetText().Cmp(_("-- nothing to change")) == 0) + && ! (!enableSQL2 && GetSql().Length() == 0 && sqlTextField1->GetText().Cmp(_("-- definition incomplete")) == 0) + && ! (enableSQL2 && GetSql().Length() == 0 && GetSql2().Length() == 0 && sqlTextField1->GetText().Cmp(_("-- nothingto change")) == 0 && sqlTextField2->GetText().Length() == 0) + && ! (enableSQL2 && GetSql().Length() == 0 && GetSql2().Length() == 0 && sqlTextField1->GetText().Cmp(_("-- definitionincomplete")) == 0 && sqlTextField2->GetText().Length() == 0) + && (sqlTextField1->GetText().Cmp(GetSql()) != 0 || (enableSQL2 && sqlTextField2->GetText().Cmp(GetSql2()) != 0)); + + if (showmessage) + { + if (wxMessageBox(_("Are you sure you wish to cancel your edit?"), _("SQL editor"), wxYES_NO|wxNO_DEFAULT) == wxNO) + { + chkReadOnly->SetValue(false); + return; + } + } + + sqlTextField1->SetReadOnly(chkReadOnly->GetValue()); + if (enableSQL2) + { + sqlTextField2->SetReadOnly(chkReadOnly->GetValue()); + } + for (pos = 0; pos < nbNotebook->GetPageCount() - 1; pos++) + { + nbNotebook->GetPage(pos)->Enable(chkReadOnly->GetValue()); + } + + if (chkReadOnly->GetValue()) + { + FillSQLTextfield(); + } +} + + +void dlgProperty::FillSQLTextfield() +{ + // create a function because this is a duplicated code + sqlTextField1->SetReadOnly(false); + if (enableSQL2) + { + sqlTextField2->SetReadOnly(false); + } + if (btnOK->IsEnabled()) + { + wxString tmp; + if (cbClusterSet && cbClusterSet->GetSelection() > 0) + { + replClientData *data=(replClientData*)cbClusterSet->GetClientData(cbClusterSet->GetSelection()); + tmp.Printf(_("-- Execute replicated using cluster \"%s\", set %ld\n"), data->cluster.c_str(), data->setId); + } + sqlTextField1->SetText(tmp + GetSql()); + if (enableSQL2) + { + sqlTextField2->SetText(GetSql2()); + } + } + else + { + if (GetObject()) + sqlTextField1->SetText(_("-- nothing to change")); + else + sqlTextField1->SetText(_("-- definition incomplete")); + if (enableSQL2) + { + sqlTextField2->SetText(wxT("")); + } + } + sqlTextField1->SetReadOnly(true); + if (enableSQL2) + { + sqlTextField2->SetReadOnly(true); + } +} + + bool dlgProperty::tryUpdate(wxTreeItemId collectionItem) { ctlTree *browser=mainForm->GetBrowser(); @@ -608,7 +730,7 @@ mainForm->GetSqlPane()->SetReadOnly(true); } } - else if (item) + else if (item && chkReadOnly->GetValue()) { wxTreeItemId collectionItem=item; @@ -753,8 +875,25 @@ return; } - wxString sql=GetSql(); - wxString sql2=GetSql2(); + wxString sql; + wxString sql2; + if (chkReadOnly->GetValue()) + { + sql = GetSql(); + sql2 = GetSql2(); + } + else + { + sql = sqlTextField1->GetText(); + if (enableSQL2) + { + sql2 = sqlTextField2->GetText(); + } + else + { + sql2 = wxT(""); + } + } if (!apply(sql, sql2)) { @@ -768,27 +907,10 @@ void dlgProperty::OnPageSelect(wxNotebookEvent& event) { - if (sqlPane && event.GetSelection() == (int)nbNotebook->GetPageCount()-1) + if (sqlTextField1 && chkReadOnly->GetValue() && + event.GetSelection() == (int)nbNotebook->GetPageCount()-1) { - sqlPane->SetReadOnly(false); - if (btnOK->IsEnabled()) - { - wxString tmp; - if (cbClusterSet && cbClusterSet->GetSelection() > 0) - { - replClientData *data=(replClientData*)cbClusterSet->GetClientData(cbClusterSet->GetSelection()); - tmp.Printf(_("-- Execute replicated using cluster \"%s\", set %ld\n"), data->cluster.c_str(), data->setId); - } - sqlPane->SetText(tmp + GetSql() + GetSql2()); - } - else - { - if (GetObject()) - sqlPane->SetText(_("-- nothing to change")); - else - sqlPane->SetText(_("-- definition incomplete")); - } - sqlPane->SetReadOnly(true); + FillSQLTextfield(); } }
Guillaume Lelarge a écrit : > [...] > The patch attached fixed some issues: > * The "Are you sure..." message is displayed only when "Read Only" is > checked. > * No button is the default on the "Are you sure" message (which has a > weird behaviour... hitting Esc will validate the dialog and the user > will lose his changes... I'm not sure we should keep this). > * the first issue you had. > * Sizing problems fixed on Linux with a wxFlexGridSizer. > > Remaining issues: > * Objects' size on Windows (and Max OS X ?). > I worked on the remaining issue tonight and it seems I can fix it this way: I add a wxFlexGridSizer and a few sizers on the .xrc file. It works well on the Linux and on the Win32 plateforms (I only check with dlgDatabase.xrc file). If I'm right, this means I need to add these widgets on each properties' dialog that doesn't already have them (all but functions and trigger' ones). If I'm still right, I see two ways to deal with this : 1. I add the wxFlexGridSizer widget on each dialog that needs it, I then create a patch with my dlgProperty's changes and the .xrc files' changes. I apply this (when allowed), and I can work later on each xrc file to add full support of the wxFlexGridSizer. 2. I first fix each dialog with the wxFlexGridSizer to add full support, and then I can resubmit this patch. I tend to prefer the first one. Comments? -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
(apologies for not reviewing the latest patch yet - we moved offices on Monday so I've been tied up with that for the last few days) On Wed, Jul 9, 2008 at 12:07 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > I worked on the remaining issue tonight and it seems I can fix it this way: > I add a wxFlexGridSizer and a few sizers on the .xrc file. It works well on > the Linux and on the Win32 plateforms (I only check with dlgDatabase.xrc > file). If I'm right, this means I need to add these widgets on each > properties' dialog that doesn't already have them (all but functions and > trigger' ones). I don't understand what you mean. The SQL tab is programmatically added to each dialogue, so what do you propose to add the sizer to in the XRC files? Besides, anything that is added via an XRC file should be possible through C++ (the XRC files are translated to C++ in fact), so we shouldn't need to hack any XRC files and should able to continue with the current implementation in the base class. It seems to me that this is taking far too long to solve what should be a simple problem. Where are we going wrong? I need to try to find some time to look at this in more depth. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > (apologies for not reviewing the latest patch yet - we moved offices > on Monday so I've been tied up with that for the last few days) > No problem, don't worry :) > On Wed, Jul 9, 2008 at 12:07 AM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >> I worked on the remaining issue tonight and it seems I can fix it this way: >> I add a wxFlexGridSizer and a few sizers on the .xrc file. It works well on >> the Linux and on the Win32 plateforms (I only check with dlgDatabase.xrc >> file). If I'm right, this means I need to add these widgets on each >> properties' dialog that doesn't already have them (all but functions and >> trigger' ones). > > I don't understand what you mean. The SQL tab is programmatically > added to each dialogue, so what do you propose to add the sizer to in > the XRC files? > The last patch I sent works on GTK+. When I try it on Windows, it works for dlgFunction and dlgTrigger and doesn't with all other dlg*. I tried to understand what could be wrong with dlgDatabase. dlgFunction already has a wxFlexGridSizer on the .xrc file. The wxFlexGridSizer contains the notebook. So, what I did is adding another wxFlexGridSizer in dlgDatabase.xrc and a few other widgets to make it look like the dlgFunction.xrc file. See the patch attached. This is a quick and dirty patch... it adds Apply and Help buttons that are not handled, it also adds a style tag (for the wxDialog) which is not necessary. Anyways, with this new component on the xrc file, it works on Windows. > Besides, anything that is added via an XRC file should be possible > through C++ (the XRC files are translated to C++ in fact), so we > shouldn't need to hack any XRC files and should able to continue with > the current implementation in the base class. > > It seems to me that this is taking far too long to solve what should > be a simple problem. Where are we going wrong? I need to try to find > some time to look at this in more depth. > If you could, it would be great. But don't hurry, I can wait till you have more time :) Thanks. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com Index: pgadmin/ui/dlgDatabase.xrc =================================================================== --- pgadmin/ui/dlgDatabase.xrc (revision 7390) +++ pgadmin/ui/dlgDatabase.xrc (working copy) @@ -2,6 +2,10 @@ <resource> <object class="wxDialog" name="dlgDatabase"> <title></title> + <style>wxDEFAULT_DIALOG_STYLE|wxCAPTION|wxSYSTEM_MENU|wxRESIZE_BORDER|wxRESIZE_BOX|wxTHICK_FRAME</style> + <object class="wxFlexGridSizer"> + <cols>1</cols> + <object class="sizeritem"> <object class="wxNotebook" name="nbNotebook"> <object class="notebookpage"> <label>Properties</label> @@ -168,20 +172,70 @@ </object> </object> </object> - <object class="wxButton" name="wxID_HELP"> - <label>Help</label> - <pos>2,220d</pos> + <flag>wxALL|wxGROW|wxALIGN_CENTRE</flag> + <border>3</border> + </object> + <growablecols>0</growablecols> + <growablerows>0</growablerows> + <object class="spacer"> + <size>2,2d</size> + </object> + <object class="sizeritem"> + <object class="wxFlexGridSizer"> + <cols>9</cols> + <object class="spacer"> + <size>3,3d</size> + </object> + <object class="sizeritem"> + <object class="wxButton" name="wxID_HELP"> + <label>Help</label> + <pos>135,220d</pos> + </object> + </object> + <object class="spacer"> + <size>3,3d</size> + </object> + <object class="sizeritem"> + <object class="wxButton" name="wxID_APPLY"> + <label>Apply</label> + </object> + </object> + <object class="spacer"> + <size>3,3d</size> + </object> + <object class="sizeritem"> + <object class="wxButton" name="wxID_OK"> + <label>&OK</label> + <default>1</default> + <pos>135,220d</pos> + </object> + </object> + <object class="spacer"> + <size>3,3d</size> + </object> + <object class="sizeritem"> + <object class="wxButton" name="wxID_CANCEL"> + <label>&Cancel</label> + <pos>176,220d</pos> + </object> + </object> + <object class="spacer"> + <size>3,3d</size> + </object> + <growablecols>2</growablecols> + </object> + <flag>wxTOP|wxLEFT|wxRIGHT|wxGROW</flag> + </object> + <object class="spacer"> + <size>3,3d</size> + </object> + <object class="sizeritem"> + <object class="unknown" name="unkStatusBar"> + <size>-1,15d</size> + </object> + <flag>wxGROW|wxALIGN_CENTRE</flag> + <border>3</border> + </object> </object> - <object class="wxButton" name="wxID_OK"> - <label>&OK</label> - <default>1</default> - <pos>113,220d</pos> - </object> - <object class="wxButton" name="wxID_CANCEL"> - <label>&Cancel</label> - <pos>166,220d</pos> - </object> - <size>218,238d</size> - <style></style> </object> </resource>
On Wed, Jul 9, 2008 at 9:18 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > The last patch I sent works on GTK+. When I try it on Windows, it works for > dlgFunction and dlgTrigger and doesn't with all other dlg*. I tried to > understand what could be wrong with dlgDatabase. dlgFunction already has a > wxFlexGridSizer on the .xrc file. The wxFlexGridSizer contains the notebook. > So, what I did is adding another wxFlexGridSizer in dlgDatabase.xrc and a > few other widgets to make it look like the dlgFunction.xrc file. See the > patch attached. This is a quick and dirty patch... it adds Apply and Help > buttons that are not handled, it also adds a style tag (for the wxDialog) > which is not necessary. Anyways, with this new component on the xrc file, it > works on Windows. OK, with your v5 patch, plus the XRC patch it seems to work nicely on the Mac. Yay :-) So, I think the way forward is to start on your planned rewrite of the XRC files. One thought - should we add sizing to all dialogs for consistency? That will mean having to size the controls on every pane, but I think that would be sensible anyway (and probably not that difficult once the first couple are done. What do you think? -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit :
> On Wed, Jul 9, 2008 at 9:18 AM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>
>> The last patch I sent works on GTK+. When I try it on Windows, it works for
>> dlgFunction and dlgTrigger and doesn't with all other dlg*. I tried to
>> understand what could be wrong with dlgDatabase. dlgFunction already has a
>> wxFlexGridSizer on the .xrc file. The wxFlexGridSizer contains the notebook.
>> So, what I did is adding another wxFlexGridSizer in dlgDatabase.xrc and a
>> few other widgets to make it look like the dlgFunction.xrc file. See the
>> patch attached. This is a quick and dirty patch... it adds Apply and Help
>> buttons that are not handled, it also adds a style tag (for the wxDialog)
>> which is not necessary. Anyways, with this new component on the xrc file, it
>> works on Windows.
>
> OK, with your v5 patch, plus the XRC patch it seems to work nicely on the Mac.
>
Great news :)
I think you also check on Windows. No issue found?
> Yay :-)
>
> So, I think the way forward is to start on your planned rewrite of the
> XRC files. One thought - should we add sizing to all dialogs for
> consistency? That will mean having to size the controls on every pane,
> but I think that would be sensible anyway (and probably not that
> difficult once the first couple are done.
>
> What do you think?
>
I prefer to add sizing to all (properties') dialogs. This is a work I
can do, step by step. I think I should follow these steps in this order:
* one patch for each dialog to add the sizing.
* one patch to add sizing on common tabs (ie "Variables", "Privileges",
and "SQL").
* v5 patch for the read-write/read-only mode on sql textfields.
OK for these steps?
--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com
On Wed, Jul 9, 2008 at 12:31 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > I think you also check on Windows. No issue found? No, I don't have Windows to hand at the moment. I'm sure it will be fine here though if it works OK for you. Windows is pretty predictable in that way :-) > I prefer to add sizing to all (properties') dialogs. This is a work I can > do, step by step. I think I should follow these steps in this order: > > * one patch for each dialog to add the sizing. > * one patch to add sizing on common tabs (ie "Variables", "Privileges", > and "SQL"). > * v5 patch for the read-write/read-only mode on sql textfields. > > OK for these steps? Yup, sounds like a good plan to me. Thanks, Dave. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Wed, Jul 9, 2008 at 12:31 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > [...] >> I prefer to add sizing to all (properties') dialogs. This is a work I can >> do, step by step. I think I should follow these steps in this order: >> >> * one patch for each dialog to add the sizing. >> * one patch to add sizing on common tabs (ie "Variables", "Privileges", >> and "SQL"). >> * v5 patch for the read-write/read-only mode on sql textfields. >> >> OK for these steps? > > Yup, sounds like a good plan to me. > Back on this. The plan took a lot more time than I initially thought. Anyways, here is the v5 patch updated. This patch aims to allow changes on sql text fields. The checkbox need to be checked to allow typing in the text fields. When checked, other tabs are unavailable. When un-checked, a confirmation dialog aks the user if he wants to loose his changes. If yes, it replaces changes with the previous queries. If no, it keeps the checkbox checked. It seems to me it's ready for commit. Comments? -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Guillaume Lelarge a écrit : > [...] > Back on this. The plan took a lot more time than I initially thought. > Anyways, here is the v5 patch updated. > I'm tired for sure... here is the patch. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Вложения
On Tue, Aug 26, 2008 at 6:48 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Guillaume Lelarge a écrit : >> [...] >> Back on this. The plan took a lot more time than I initially thought. >> Anyways, here is the v5 patch updated. >> > > I'm tired for sure... here is the patch. :-( Well it looks good (you've *really* got the hang of XRC/wxSizers now :-) ) - but if I open the 'New Database' dialogue, enter a name, and then click on the SQL tab, I get an assert on OSX. Thread 0 Crashed: 0 libSystem.B.dylib 0x95dd4b9e __kill + 10 1 libSystem.B.dylib 0x95e4bec2 raise + 26 2 ...x_base_carbonud-2.8.0.dylib 0x014f7f84 wxTrap() + 18 3 libwx_macud_core-2.8.0.dylib 0x00f11912 wxGUIAppTraitsBase::ShowAssertDialog(wxString const&) + 254 4 ...x_base_carbonud-2.8.0.dylib 0x014f84f2 ShowAssertDialog(wchar_t const*, int, wchar_t const*, wchar_t const*, wchar_t const*, wxAppTraits*) + 412 5 ...x_base_carbonud-2.8.0.dylib 0x014f87aa wxAppConsole::OnAssertFailure(wchar_t const*, int, wchar_t const*, wchar_t const*, wchar_t const*) + 60 6 ...x_base_carbonud-2.8.0.dylib 0x014f862c wxOnAssert(wchar_t const*, int, char const*, wchar_t const*, wchar_t const*) + 232 7 libwx_macud_core-2.8.0.dylib 0x00e67149 wxChoice::GetString(unsigned int) const + 97 8 libwx_macud_core-2.8.0.dylib 0x00e69da4 wxComboBox::GetValue() const + 116 9 pgAdmin3-Debug 0x000973be dlgDatabase::GetSql() + 4312 (dlgDatabase.cpp:472) 10 pgAdmin3-Debug 0x000ca42d dlgProperty::FillSQLTextfield() + 383 (dlgProperty.cpp:616) 11 pgAdmin3-Debug 0x000cbc9e dlgProperty::OnPageSelect(wxNotebookEvent&) + 138 (dlgProperty.cpp:925) <snip> I also get a crash if I try to open the New Table dialogue. I suspect that is the possible incompatibility that you were worried about, though I guess it could be related to this patch. Thread 0 Crashed: 0 pgAdmin3-Debug 0x000197e3 wxStringBase::GetStringData() const + 9 (string.h:265) 1 ...x_base_carbonud-2.8.0.dylib 0x01564239 wxArrayString::Add(wxString const&, unsigned long) + 259 2 ...x_base_carbonud-2.8.0.dylib 0x0156435a wxArrayString::Copy(wxArrayString const&) + 88 3 ...x_base_carbonud-2.8.0.dylib 0x0156463c wxArrayString::operator=(wxArrayString const&) + 44 4 pgAdmin3-Debug 0x000486c2 ctlSecurityPanel::GetGrant(wxString const&, wxString const&, wxArrayString*) + 52 (ctlSecurityPanel.cpp:155) 5 pgAdmin3-Debug 0x000d3359 dlgSecurityProperty::GetGrant(wxString const&, wxString const&) + 71 (dlgProperty.cpp:1606) 6 pgAdmin3-Debug 0x000f612a dlgTable::GetSql() + 27284 (dlgTable.cpp:849) 7 pgAdmin3-Debug 0x000fb36c dlgTable::CheckChange() + 166 (dlgTable.cpp:985) 8 pgAdmin3-Debug 0x000ca26c dlgProperty::OnChange(wxCommandEvent&) + 26 (dlgProperty.cpp:550) 9 pgAdmin3-Debug 0x000f7013 dlgTable::OnChangeVacuum(wxCommandEvent&) + 2651 (dlgTable.cpp:958) 10 pgAdmin3-Debug 0x000fad5a dlgTable::Go(bool) + 15678 (dlgTable.cpp:465) 11 pgAdmin3-Debug 0x000cc6e1 dlgProperty::EditObjectDialog(frmMain*, ctlSQLBox*, pgObject*) + 707 (dlgProperty.cpp:1082) 12 pgAdmin3-Debug 0x000d5203 propertyFactory::StartDialog(frmMain*, pgObject*) + 41 (dlgProperty.cpp:1784) 13 pgAdmin3-Debug 0x0011b463 frmMain::OnAction(wxCommandEvent&) + 95 (events.cpp:147) -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Tue, Aug 26, 2008 at 6:48 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >> Guillaume Lelarge a écrit : >>> [...] >>> Back on this. The plan took a lot more time than I initially thought. >>> Anyways, here is the v5 patch updated. >>> >> I'm tired for sure... here is the patch. > > :-( > > Well it looks good (you've *really* got the hang of XRC/wxSizers now > :-) ) - but if I open the 'New Database' dialogue, enter a name, and > then click on the SQL tab, I get an assert on OSX. > This issue has nothing to do with this patch. I think this bug comes from my previous commit :-/ Without explicitely setting cbTemplate selection, cbTemplate->GetValue() gets out with a failed assert. AFAICS, my previous patch didn't modify this... but I find it weird that no one hits this bug before. Anyways, the good news is that it gave me an idea to make dlgLanguage growable. I just commited this dialog. I also commited the dlgDatabase fix. > [...] > I also get a crash if I try to open the New Table dialogue. I suspect > that is the possible incompatibility that you were worried about, > though I guess it could be related to this patch. > I'm sorry, I don't have this issue. Surprising because I don't see how the previous fix could have fixed it. No new patch, because the fix is already commited. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
[Forwarding to the list - and testing the new list config!] On Tue, Aug 26, 2008 at 10:50 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Dave Page a écrit : >> Well it looks good (you've *really* got the hang of XRC/wxSizers now >> :-) ) - but if I open the 'New Database' dialogue, enter a name, and >> then click on the SQL tab, I get an assert on OSX. >> > > This issue has nothing to do with this patch. I think this bug comes > from my previous commit :-/ > > Without explicitely setting cbTemplate selection, cbTemplate->GetValue() > gets out with a failed assert. AFAICS, my previous patch didn't modify > this... but I find it weird that no one hits this bug before. > > Anyways, the good news is that it gave me an idea to make dlgLanguage > growable. I just commited this dialog. I also commited the dlgDatabase fix. :-) >> [...] >> I also get a crash if I try to open the New Table dialogue. I suspect >> that is the possible incompatibility that you were worried about, >> though I guess it could be related to this patch. >> > > I'm sorry, I don't have this issue. Surprising because I don't see how > the previous fix could have fixed it. > > No new patch, because the fix is already commited. Well your recent patches seem to have fixed it anyway! On the subject of the table dialogue though, there's something not quite right on the Vacuum tab. Note the position of the Current value label in the attached screenshot. It's also a real shame that tabs cannot scroll on the mac - having the dialogue extra wide doesn't look so nice :-(. Perhaps that minimum size should be for Mac only, and other platforms could be the standard size (he says, blindly assuming you didn't do that already!)? -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Вложения
Dave Page a écrit : > On Tue, Aug 26, 2008 at 10:50 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >>> [...] >>> I also get a crash if I try to open the New Table dialogue. I suspect >>> that is the possible incompatibility that you were worried about, >>> though I guess it could be related to this patch. >>> >> I'm sorry, I don't have this issue. Surprising because I don't see how >> the previous fix could have fixed it. >> >> No new patch, because the fix is already commited. > > Well your recent patches seem to have fixed it anyway! > > On the subject of the table dialogue though, there's something not > quite right on the Vacuum tab. Note the position of the Current value > label in the attached screenshot. > It seems at the right place to me. There are three columns on this tab: first one for the labels, second one for the text box, and third one for static texts showing current values. > It's also a real shame that tabs cannot scroll on the mac - having the > dialogue extra wide doesn't look so nice :-(. It's even less nice when you open an existing table because there is one more tab... > Perhaps that minimum > size should be for Mac only, and other platforms could be the standard > size (he says, blindly assuming you didn't do that already!)? > No, I didn't. I tried to follow the size guidelines but it's pretty hard if we want the dialogs to display nicely. But it could be something to do. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Wed, Aug 27, 2008 at 9:10 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > It seems at the right place to me. There are three columns on this tab: > first one for the labels, second one for the text box, and third one for > static texts showing current values. Oh, yes - of course. I wonder if we should give the static text boxes a thin border so it's obviously they are there, even when empty? At the moment it just looks odd. /D -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Guillaume Lelarge a écrit : > Dave Page a écrit : > [...] >> It's also a real shame that tabs cannot scroll on the mac - having the >> dialogue extra wide doesn't look so nice :-(. > > It's even less nice when you open an existing table because there is one > more tab... > One other way to fix this would be to use wxAuiNotebook. That doesn't seem simple, but it certainly can be done. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Thu, Aug 28, 2008 at 12:15 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Guillaume Lelarge a écrit : >> Dave Page a écrit : >> [...] >>> It's also a real shame that tabs cannot scroll on the mac - having the >>> dialogue extra wide doesn't look so nice :-(. >> >> It's even less nice when you open an existing table because there is one >> more tab... >> > > One other way to fix this would be to use wxAuiNotebook. That doesn't > seem simple, but it certainly can be done. Well that may lead to some interestingly messy desktops (imagine - separated tabs as far as the eye can see :-p ), but it could be useful too. How does it solve this problem though? -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Thu, Aug 28, 2008 at 12:15 AM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >> Guillaume Lelarge a écrit : >>> Dave Page a écrit : >>> [...] >>>> It's also a real shame that tabs cannot scroll on the mac - having the >>>> dialogue extra wide doesn't look so nice :-(. >>> It's even less nice when you open an existing table because there is one >>> more tab... >>> >> One other way to fix this would be to use wxAuiNotebook. That doesn't >> seem simple, but it certainly can be done. > > Well that may lead to some interestingly messy desktops (imagine - > separated tabs as far as the eye can see :-p ), but it could be useful > too. How does it solve this problem though? > wxAuiNotebook tabs can scroll on the mac. And there's a nice button to get a list of all tabs. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Thu, Aug 28, 2008 at 8:23 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: >> Well that may lead to some interestingly messy desktops (imagine - >> separated tabs as far as the eye can see :-p ), but it could be useful >> too. How does it solve this problem though? >> > > wxAuiNotebook tabs can scroll on the mac. And there's a nice button to > get a list of all tabs. Oooh, that sounds nice. Any idea how much pain it would be to retro-fit into the dialogs? -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Thu, Aug 28, 2008 at 8:23 AM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >>> Well that may lead to some interestingly messy desktops (imagine - >>> separated tabs as far as the eye can see :-p ), but it could be useful >>> too. How does it solve this problem though? >>> >> wxAuiNotebook tabs can scroll on the mac. And there's a nice button to >> get a list of all tabs. > > Oooh, that sounds nice. Any idea how much pain it would be to > retro-fit into the dialogs? > It sure seems a lot of work. We can't just replace the wxNotebook with a wxAuiNotebook in the XRC files. I tried yesterday and it didn't work :-/ AFAICT, we need to write some code about a wxAuiManager and its panes. And there's probably more stuff to know before going into that. I'll first work on the server status window (remember the first part: Replace the wxNotebook widget with the wxAuiNotebook so users can close tabs and rearrange tabs order via drag-and-drop.) When the first part will be done, I'll have a better understanding of the AUI stuff. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Thu, Aug 28, 2008 at 8:56 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > It sure seems a lot of work. We can't just replace the wxNotebook with a > wxAuiNotebook in the XRC files. I tried yesterday and it didn't work :-/ > > AFAICT, we need to write some code about a wxAuiManager and its panes. > And there's probably more stuff to know before going into that. Yeah, that makes sense. > I'll first work on the server status window (remember the first part: > Replace the wxNotebook widget with the wxAuiNotebook so users can close > tabs and rearrange tabs order via drag-and-drop.) When the first part > will be done, I'll have a better understanding of the AUI stuff. OK, cool. It's not a major issue though - I'd like to redesign the table dialogue (time allowing), and the design I have in mind would be landscape rather than portrait anyway. Thanks. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Thu, Aug 28, 2008 at 8:56 AM, Guillaume Lelarge > [...] >> I'll first work on the server status window (remember the first part: >> Replace the wxNotebook widget with the wxAuiNotebook so users can close >> tabs and rearrange tabs order via drag-and-drop.) When the first part >> will be done, I'll have a better understanding of the AUI stuff. > > OK, cool. It's not a major issue though - I'd like to redesign the > table dialogue (time allowing), and the design I have in mind would be > landscape rather than portrait anyway. > Great, this is something I'm waiting for :) To get back to my patch, do you have any issue with it or can I commit? -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Dave Page a écrit : > On Wed, Aug 27, 2008 at 9:10 AM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >> It seems at the right place to me. There are three columns on this tab: >> first one for the labels, second one for the text box, and third one for >> static texts showing current values. > > Oh, yes - of course. I wonder if we should give the static text boxes > a thin border so it's obviously they are there, even when empty? At > the moment it just looks odd. > I tried many things but didn't manage to get a border, thin or fat. Perhaps wa can just put an N/A text on each static box? See attached patch. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com Index: pgadmin/dlg/dlgTable.cpp =================================================================== --- pgadmin/dlg/dlgTable.cpp (révision 7450) +++ pgadmin/dlg/dlgTable.cpp (copie de travail) @@ -970,6 +970,19 @@ stFactorAnCurr->SetLabel(NumToStr((tableAnlFactor == -1) ? settingAnlFactor : tableAnlFactor)); stVacDelayCurr->SetLabel(NumToStr((tableCostDelay == -1) ? settingCostDelay : tableCostDelay)); stVacLimitCurr->SetLabel(NumToStr((tableCostLimit == -1) ? settingCostLimit : tableCostLimit)); + + if (stBaseVacCurr->GetLabel().Length() == 0) + stBaseVacCurr->SetLabel(wxT("N/A")); + if (stBaseAnCurr->GetLabel().Length() == 0) + stBaseAnCurr->SetLabel(wxT("N/A")); + if (stFactorVacCurr->GetLabel().Length() == 0) + stFactorVacCurr->SetLabel(wxT("N/A")); + if (stFactorAnCurr->GetLabel().Length() == 0) + stFactorAnCurr->SetLabel(wxT("N/A")); + if (stVacDelayCurr->GetLabel().Length() == 0) + stVacDelayCurr->SetLabel(wxT("N/A")); + if (stVacLimitCurr->GetLabel().Length() == 0) + stVacLimitCurr->SetLabel(wxT("N/A")); if (connection->BackendMinimumVersion(8, 2)) { Index: pgadmin/ui/dlgTable.xrc =================================================================== --- pgadmin/ui/dlgTable.xrc (révision 7450) +++ pgadmin/ui/dlgTable.xrc (copie de travail) @@ -349,7 +349,7 @@ </object> <object class="sizeritem"> <object class="wxStaticText" name="stBaseVacCurr"> - <label></label> + <label>N/A</label> </object> <flag>wxALIGN_CENTER_VERTICAL|wxTOP|wxLEFT|wxRIGHT</flag> <border>4</border> @@ -369,7 +369,7 @@ </object> <object class="sizeritem"> <object class="wxStaticText" name="stBaseAnCurr"> - <label></label> + <label>N/A</label> </object> <flag>wxALIGN_CENTER_VERTICAL|wxTOP|wxLEFT|wxRIGHT</flag> <border>4</border> @@ -389,7 +389,7 @@ </object> <object class="sizeritem"> <object class="wxStaticText" name="stFactorVacCurr"> - <label></label> + <label>N/A</label> </object> <flag>wxALIGN_CENTER_VERTICAL|wxTOP|wxLEFT|wxRIGHT</flag> <border>4</border> @@ -409,7 +409,7 @@ </object> <object class="sizeritem"> <object class="wxStaticText" name="stFactorAnCurr"> - <label></label> + <label>N/A</label> </object> <flag>wxALIGN_CENTER_VERTICAL|wxTOP|wxLEFT|wxRIGHT</flag> <border>4</border> @@ -429,7 +429,7 @@ </object> <object class="sizeritem"> <object class="wxStaticText" name="stVacDelayCurr"> - <label></label> + <label>N/A</label> </object> <flag>wxALIGN_CENTER_VERTICAL|wxTOP|wxLEFT|wxRIGHT</flag> <border>4</border> @@ -449,7 +449,7 @@ </object> <object class="sizeritem"> <object class="wxStaticText" name="stVacLimitCurr"> - <label></label> + <label>N/A</label> </object> <flag>wxALIGN_CENTER_VERTICAL|wxTOP|wxLEFT|wxRIGHT</flag> <border>4</border> @@ -469,7 +469,7 @@ </object> <object class="sizeritem"> <object class="wxStaticText" name="stFreezeMinAgeCurr"> - <label></label> + <label>N/A</label> </object> <flag>wxALIGN_CENTER_VERTICAL|wxTOP|wxLEFT|wxRIGHT</flag> <border>4</border> @@ -489,7 +489,7 @@ </object> <object class="sizeritem"> <object class="wxStaticText" name="stFreezeMaxAgeCurr"> - <label></label> + <label>N/A</label> </object> <flag>wxALIGN_CENTRE_VERTICAL|wxALL</flag> <border>4</border>
On Sat, Aug 30, 2008 at 5:28 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Dave Page a écrit : >> On Thu, Aug 28, 2008 at 8:56 AM, Guillaume Lelarge >> [...] >>> I'll first work on the server status window (remember the first part: >>> Replace the wxNotebook widget with the wxAuiNotebook so users can close >>> tabs and rearrange tabs order via drag-and-drop.) When the first part >>> will be done, I'll have a better understanding of the AUI stuff. >> >> OK, cool. It's not a major issue though - I'd like to redesign the >> table dialogue (time allowing), and the design I have in mind would be >> landscape rather than portrait anyway. >> > > Great, this is something I'm waiting for :) > > To get back to my patch, do you have any issue with it or can I commit? No, please commit. BTW, I just noticed the minimum height for dlgServer is a little too small. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Sat, Aug 30, 2008 at 5:52 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: >> Oh, yes - of course. I wonder if we should give the static text boxes >> a thin border so it's obviously they are there, even when empty? At >> the moment it just looks odd. >> > > I tried many things but didn't manage to get a border, thin or fat. > Perhaps wa can just put an N/A text on each static box? See attached patch. Maybe use a disabled text box instead? -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Sat, Aug 30, 2008 at 5:52 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >>> Oh, yes - of course. I wonder if we should give the static text boxes >>> a thin border so it's obviously they are there, even when empty? At >>> the moment it just looks odd. >>> >> I tried many things but didn't manage to get a border, thin or fat. >> Perhaps wa can just put an N/A text on each static box? See attached patch. > > Maybe use a disabled text box instead? > I thought about it too but wondered how many users will try to find a way to enable them. If you still thinks it's a better idea, I'll work on it asap. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Sat, Aug 30, 2008 at 9:28 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Dave Page a écrit : >> On Sat, Aug 30, 2008 at 5:52 PM, Guillaume Lelarge >> <guillaume@lelarge.info> wrote: >> >>>> Oh, yes - of course. I wonder if we should give the static text boxes >>>> a thin border so it's obviously they are there, even when empty? At >>>> the moment it just looks odd. >>>> >>> I tried many things but didn't manage to get a border, thin or fat. >>> Perhaps wa can just put an N/A text on each static box? See attached patch. >> >> Maybe use a disabled text box instead? >> > > I thought about it too but wondered how many users will try to find a > way to enable them. > > If you still thinks it's a better idea, I'll work on it asap. Is there any reason why we don't just stick the current values in the main textboxes? -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Sat, Aug 30, 2008 at 9:28 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >> Dave Page a écrit : >>> On Sat, Aug 30, 2008 at 5:52 PM, Guillaume Lelarge >>> <guillaume@lelarge.info> wrote: >>> >>>>> Oh, yes - of course. I wonder if we should give the static text boxes >>>>> a thin border so it's obviously they are there, even when empty? At >>>>> the moment it just looks odd. >>>>> >>>> I tried many things but didn't manage to get a border, thin or fat. >>>> Perhaps wa can just put an N/A text on each static box? See attached patch. >>> Maybe use a disabled text box instead? >>> >> I thought about it too but wondered how many users will try to find a >> way to enable them. >> >> If you still thinks it's a better idea, I'll work on it asap. > > Is there any reason why we don't just stick the current values in the > main textboxes? > The current values could be the settings ones or the table ones. If we put the current values in the main textboxes, we will have to first check that at least one textbox value is different from the current value. If we find a difference, we could save the table settings. This is a bit hard, but not impossible. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Sat, Aug 30, 2008 at 9:44 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > The current values could be the settings ones or the table ones. If we > put the current values in the main textboxes, we will have to first > check that at least one textbox value is different from the current > value. If we find a difference, we could save the table settings. > > This is a bit hard, but not impossible. Oh, OK. I guess we'll just leave it as-is for now. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Sat, Aug 30, 2008 at 9:44 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >> The current values could be the settings ones or the table ones. If we >> put the current values in the main textboxes, we will have to first >> check that at least one textbox value is different from the current >> value. If we find a difference, we could save the table settings. >> >> This is a bit hard, but not impossible. > > Oh, OK. I guess we'll just leave it as-is for now. > > OK. I'll just commit the "Enabling SQL field" patch. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com