Enabling SQL text field in the SQL tab of object dialog

[Guillaume Lelarge-3]

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 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.

Ideas ? Comments ?

(this is obviously not for 1.8)

Regards.


--
Guillaume.
<!-- http://abs.traduc.org/
     http://lfs.traduc.org/
     http://docs.postgresqlfr.org/ -->

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

[Dave Page-3]

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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

[Guillaume Lelarge-3]

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);
     }
 }
 


--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Dave Page-3]

On Sat, Jun 14, 2008 at 3:01 PM, Guillaume Lelarge
<guillaume@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Guillaume Lelarge-3]

Dave Page a écrit :

> On Sat, Jun 14, 2008 at 3:01 PM, Guillaume Lelarge
> <guillaume@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Guillaume Lelarge-3]

Guillaume Lelarge a écrit :

> Dave Page a écrit :
>> On Sat, Jun 14, 2008 at 3:01 PM, Guillaume Lelarge
>> <guillaume@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Dave Page-3]

On Mon, Jun 16, 2008 at 8:58 PM, Guillaume Lelarge
<guillaume@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Dave Page-3]

On Mon, Jun 16, 2008 at 9:11 PM, Dave Page <dpage@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Guillaume Lelarge-3]

Dave Page a écrit :
> On Mon, Jun 16, 2008 at 8:58 PM, Guillaume Lelarge
> <guillaume@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Dave Page-3]

On Mon, Jun 16, 2008 at 9:34 PM, Guillaume Lelarge
<guillaume@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Guillaume Lelarge-3]

Dave Page a écrit :

> On Mon, Jun 16, 2008 at 9:34 PM, Guillaume Lelarge
> <guillaume@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Dave Page-3]

On Mon, Jun 16, 2008 at 11:07 PM, Guillaume Lelarge
<guillaume@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Guillaume Lelarge-3]

Dave Page a écrit :
> On Mon, Jun 16, 2008 at 11:07 PM, Guillaume Lelarge
> <guillaume@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Dave Page-3]

On Tue, Jun 17, 2008 at 8:59 AM, Guillaume Lelarge
<guillaume@...> 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

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

[Guillaume Lelarge-3]

Dave Page a écrit :

> On Tue, Jun 17, 2008 at 8:59 AM, Guillaume Lelarge
> <guillaume@...> 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, wxDefaultSiz