Thread: Fixing issues between objects and properties dialogs
Hi, There is at least one bug that I know since I started to use pgAdmin, and I've seen no work on it. When someone connects to a database, the treeview is built from informations gathered in the database. Not only do we fill the treeview, but we also create a lot of pgObject objects (pgTable, pgSequence, you name it). These objects are used when someone clicks on a treeview node to display informations in the properties tab of the browser, and in the SQL pane. They are also used when the user opens an object properties' dialog. The object is used all the time by the properties' dialog while it's only usable till the object's node is refreshed in the treeview. Try this: * open an object properties' dialog * refresh the object's node in the treeview * change something in the properties' dialog * click OK Or try this * open an object properties' dialog * drop the object in the treeview * change something in the properties' dialog * click OK Sure crash in both case, whatever the object you're working with. Question is: how can we fix that? I see two potential fixes: * prevent refresh/drop while a properties dialog is open * close a properties dialog if the object associated is about to be refreshed/dropped Not sure which one is better. I prefer the latter, but it'll be more difficult to code. Also, it would be a good time to prevent a misbehaviour. Nothing prevents a user to get two properties' dialog for the same object. Comments? -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
The former fix seems more appropriate. Operations in frmMain shouldn't cause dialogues to close. Simple fix seems to be to store a pointer to the dialog in pgObject; if not null, disallow drop, or if trying to view properties, use the pointer to raise the existing dialogue. Clear the pointer when the dialog is closed. On Sunday, July 24, 2011, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Hi, > > There is at least one bug that I know since I started to use pgAdmin, > and I've seen no work on it. > > When someone connects to a database, the treeview is built from > informations gathered in the database. Not only do we fill the treeview, > but we also create a lot of pgObject objects (pgTable, pgSequence, you > name it). These objects are used when someone clicks on a treeview node > to display informations in the properties tab of the browser, and in the > SQL pane. They are also used when the user opens an object properties' > dialog. The object is used all the time by the properties' dialog while > it's only usable till the object's node is refreshed in the treeview. > > Try this: > > * open an object properties' dialog > * refresh the object's node in the treeview > * change something in the properties' dialog > * click OK > > Or try this > > * open an object properties' dialog > * drop the object in the treeview > * change something in the properties' dialog > * click OK > > Sure crash in both case, whatever the object you're working with. > > Question is: how can we fix that? > > I see two potential fixes: > > * prevent refresh/drop while a properties dialog is open > > * close a properties dialog if the object associated is about to be > refreshed/dropped > > Not sure which one is better. I prefer the latter, but it'll be more > difficult to code. > > Also, it would be a good time to prevent a misbehaviour. Nothing > prevents a user to get two properties' dialog for the same object. > > Comments? > > > -- > Guillaume > http://blog.guillaume.lelarge.info > http://www.dalibo.com > > > -- > Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgadmin-hackers > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sun, 2011-07-24 at 12:58 +0100, Dave Page wrote: > The former fix seems more appropriate. Operations in frmMain shouldn't > cause dialogues to close. > Fine with me. That should be simpler to do. > Simple fix seems to be to store a pointer to the dialog in pgObject; > if not null, disallow drop, or if trying to view properties, use the > pointer to raise the existing dialogue. Clear the pointer when the > dialog is closed. > I thought of something similar, but this is OK with me. I expect it to be quite a big patch. Should I work on 1.14 or master? this is surely a bug, so 1.14 should be OK. But I'm afraid this is too much changes to code it on a beta release. Any strong opinion on this matter? -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On Sun, 2011-07-24 at 14:11 +0200, Guillaume Lelarge wrote: > On Sun, 2011-07-24 at 12:58 +0100, Dave Page wrote: > > The former fix seems more appropriate. Operations in frmMain shouldn't > > cause dialogues to close. > > > > Fine with me. That should be simpler to do. > > > Simple fix seems to be to store a pointer to the dialog in pgObject; > > if not null, disallow drop, or if trying to view properties, use the > > pointer to raise the existing dialogue. Clear the pointer when the > > dialog is closed. > > > > I thought of something similar, but this is OK with me. > > I expect it to be quite a big patch. Should I work on 1.14 or master? > this is surely a bug, so 1.14 should be OK. But I'm afraid this is too > much changes to code it on a beta release. Any strong opinion on this > matter? > And another question. Let's say I open a table properties' dialog, and I try to refresh the whole database. Should it refresh everything except this table, or nothing and complains about properties' dialogs being open, preventing the object to be refreshed? -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On Sun, Jul 24, 2011 at 1:11 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > On Sun, 2011-07-24 at 12:58 +0100, Dave Page wrote: >> The former fix seems more appropriate. Operations in frmMain shouldn't >> cause dialogues to close. >> > > Fine with me. That should be simpler to do. > >> Simple fix seems to be to store a pointer to the dialog in pgObject; >> if not null, disallow drop, or if trying to view properties, use the >> pointer to raise the existing dialogue. Clear the pointer when the >> dialog is closed. >> > > I thought of something similar, but this is OK with me. > > I expect it to be quite a big patch. Should I work on 1.14 or master? > this is surely a bug, so 1.14 should be OK. But I'm afraid this is too > much changes to code it on a beta release. Any strong opinion on this > matter? Seems like something for 1.15 to me. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sun, Jul 24, 2011 at 2:23 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > On Sun, 2011-07-24 at 14:11 +0200, Guillaume Lelarge wrote: >> On Sun, 2011-07-24 at 12:58 +0100, Dave Page wrote: >> > The former fix seems more appropriate. Operations in frmMain shouldn't >> > cause dialogues to close. >> > >> >> Fine with me. That should be simpler to do. >> >> > Simple fix seems to be to store a pointer to the dialog in pgObject; >> > if not null, disallow drop, or if trying to view properties, use the >> > pointer to raise the existing dialogue. Clear the pointer when the >> > dialog is closed. >> > >> >> I thought of something similar, but this is OK with me. >> >> I expect it to be quite a big patch. Should I work on 1.14 or master? >> this is surely a bug, so 1.14 should be OK. But I'm afraid this is too >> much changes to code it on a beta release. Any strong opinion on this >> matter? >> > > And another question. Let's say I open a table properties' dialog, and I > try to refresh the whole database. Should it refresh everything except > this table, or nothing and complains about properties' dialogs being > open, preventing the object to be refreshed? Nothing. "There are properties dialogues open for one or more objects that would be refreshed. Please close the properties dialogues and try again." -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sun, 2011-07-24 at 15:09 +0100, Dave Page wrote: > On Sun, Jul 24, 2011 at 2:23 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > > On Sun, 2011-07-24 at 14:11 +0200, Guillaume Lelarge wrote: > >> On Sun, 2011-07-24 at 12:58 +0100, Dave Page wrote: > >> > The former fix seems more appropriate. Operations in frmMain shouldn't > >> > cause dialogues to close. > >> > > >> > >> Fine with me. That should be simpler to do. > >> > >> > Simple fix seems to be to store a pointer to the dialog in pgObject; > >> > if not null, disallow drop, or if trying to view properties, use the > >> > pointer to raise the existing dialogue. Clear the pointer when the > >> > dialog is closed. > >> > > >> > >> I thought of something similar, but this is OK with me. > >> > >> I expect it to be quite a big patch. Should I work on 1.14 or master? > >> this is surely a bug, so 1.14 should be OK. But I'm afraid this is too > >> much changes to code it on a beta release. Any strong opinion on this > >> matter? > >> > > > > And another question. Let's say I open a table properties' dialog, and I > > try to refresh the whole database. Should it refresh everything except > > this table, or nothing and complains about properties' dialogs being > > open, preventing the object to be refreshed? > > Nothing. "There are properties dialogues open for one or more objects > that would be refreshed. Please close the properties dialogues and try > again." > Just a quick update. Still working on it, and I have good results so far. I'm now able to prevent refresh and drop (refresh for all objects, drop for schemas only) if an object's properties dialog is open. Patch attached. Obviously WIP. Not so obvious, it's dirty code (some wxLogError, not really efficient code, etc). But comments very welcome. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
Attachment
On Tue, 2011-08-09 at 00:30 +0200, Guillaume Lelarge wrote: > On Sun, 2011-07-24 at 15:09 +0100, Dave Page wrote: > > On Sun, Jul 24, 2011 at 2:23 PM, Guillaume Lelarge > > <guillaume@lelarge.info> wrote: > > > On Sun, 2011-07-24 at 14:11 +0200, Guillaume Lelarge wrote: > > >> On Sun, 2011-07-24 at 12:58 +0100, Dave Page wrote: > > >> > The former fix seems more appropriate. Operations in frmMain shouldn't > > >> > cause dialogues to close. > > >> > > > >> > > >> Fine with me. That should be simpler to do. > > >> > > >> > Simple fix seems to be to store a pointer to the dialog in pgObject; > > >> > if not null, disallow drop, or if trying to view properties, use the > > >> > pointer to raise the existing dialogue. Clear the pointer when the > > >> > dialog is closed. > > >> > > > >> > > >> I thought of something similar, but this is OK with me. > > >> > > >> I expect it to be quite a big patch. Should I work on 1.14 or master? > > >> this is surely a bug, so 1.14 should be OK. But I'm afraid this is too > > >> much changes to code it on a beta release. Any strong opinion on this > > >> matter? > > >> > > > > > > And another question. Let's say I open a table properties' dialog, and I > > > try to refresh the whole database. Should it refresh everything except > > > this table, or nothing and complains about properties' dialogs being > > > open, preventing the object to be refreshed? > > > > Nothing. "There are properties dialogues open for one or more objects > > that would be refreshed. Please close the properties dialogues and try > > again." > > > > Just a quick update. Still working on it, and I have good results so > far. I'm now able to prevent refresh and drop (refresh for all objects, > drop for schemas only) if an object's properties dialog is open. > > Patch attached. Obviously WIP. Not so obvious, it's dirty code (some > wxLogError, not really efficient code, etc). But comments very welcome. > Seems it hits final release. See attached patch. Protects against server or database disconnection, and browser refresh. If a user tries to open many times the same object properties, it will raise the previous window rather than opening a new one. I think it's pretty neat :) It's been a huge pgAdmin's issue for a long time. Any comments before I commit it? -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On Wed, Aug 10, 2011 at 11:06 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > On Tue, 2011-08-09 at 00:30 +0200, Guillaume Lelarge wrote: >> On Sun, 2011-07-24 at 15:09 +0100, Dave Page wrote: >> > On Sun, Jul 24, 2011 at 2:23 PM, Guillaume Lelarge >> > <guillaume@lelarge.info> wrote: >> > > On Sun, 2011-07-24 at 14:11 +0200, Guillaume Lelarge wrote: >> > >> On Sun, 2011-07-24 at 12:58 +0100, Dave Page wrote: >> > >> > The former fix seems more appropriate. Operations in frmMain shouldn't >> > >> > cause dialogues to close. >> > >> > >> > >> >> > >> Fine with me. That should be simpler to do. >> > >> >> > >> > Simple fix seems to be to store a pointer to the dialog in pgObject; >> > >> > if not null, disallow drop, or if trying to view properties, use the >> > >> > pointer to raise the existing dialogue. Clear the pointer when the >> > >> > dialog is closed. >> > >> > >> > >> >> > >> I thought of something similar, but this is OK with me. >> > >> >> > >> I expect it to be quite a big patch. Should I work on 1.14 or master? >> > >> this is surely a bug, so 1.14 should be OK. But I'm afraid this is too >> > >> much changes to code it on a beta release. Any strong opinion on this >> > >> matter? >> > >> >> > > >> > > And another question. Let's say I open a table properties' dialog, and I >> > > try to refresh the whole database. Should it refresh everything except >> > > this table, or nothing and complains about properties' dialogs being >> > > open, preventing the object to be refreshed? >> > >> > Nothing. "There are properties dialogues open for one or more objects >> > that would be refreshed. Please close the properties dialogues and try >> > again." >> > >> >> Just a quick update. Still working on it, and I have good results so >> far. I'm now able to prevent refresh and drop (refresh for all objects, >> drop for schemas only) if an object's properties dialog is open. >> >> Patch attached. Obviously WIP. Not so obvious, it's dirty code (some >> wxLogError, not really efficient code, etc). But comments very welcome. >> > > Seems it hits final release. See attached patch. Protects against server > or database disconnection, and browser refresh. If a user tries to open > many times the same object properties, it will raise the previous window > rather than opening a new one. > > I think it's pretty neat :) It's been a huge pgAdmin's issue for a long > time. > > Any comments before I commit it? It's too late for 1.14 - that's why I said upstream it should be for 1.15 (RC1 may well be next week). Otherwise, nice work :-) -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, 2011-08-11 at 08:52 +0100, Dave Page wrote: > On Wed, Aug 10, 2011 at 11:06 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > > On Tue, 2011-08-09 at 00:30 +0200, Guillaume Lelarge wrote: > >> On Sun, 2011-07-24 at 15:09 +0100, Dave Page wrote: > >> > On Sun, Jul 24, 2011 at 2:23 PM, Guillaume Lelarge > >> > <guillaume@lelarge.info> wrote: > >> > > On Sun, 2011-07-24 at 14:11 +0200, Guillaume Lelarge wrote: > >> > >> On Sun, 2011-07-24 at 12:58 +0100, Dave Page wrote: > >> > >> > The former fix seems more appropriate. Operations in frmMain shouldn't > >> > >> > cause dialogues to close. > >> > >> > > >> > >> > >> > >> Fine with me. That should be simpler to do. > >> > >> > >> > >> > Simple fix seems to be to store a pointer to the dialog in pgObject; > >> > >> > if not null, disallow drop, or if trying to view properties, use the > >> > >> > pointer to raise the existing dialogue. Clear the pointer when the > >> > >> > dialog is closed. > >> > >> > > >> > >> > >> > >> I thought of something similar, but this is OK with me. > >> > >> > >> > >> I expect it to be quite a big patch. Should I work on 1.14 or master? > >> > >> this is surely a bug, so 1.14 should be OK. But I'm afraid this is too > >> > >> much changes to code it on a beta release. Any strong opinion on this > >> > >> matter? > >> > >> > >> > > > >> > > And another question. Let's say I open a table properties' dialog, and I > >> > > try to refresh the whole database. Should it refresh everything except > >> > > this table, or nothing and complains about properties' dialogs being > >> > > open, preventing the object to be refreshed? > >> > > >> > Nothing. "There are properties dialogues open for one or more objects > >> > that would be refreshed. Please close the properties dialogues and try > >> > again." > >> > > >> > >> Just a quick update. Still working on it, and I have good results so > >> far. I'm now able to prevent refresh and drop (refresh for all objects, > >> drop for schemas only) if an object's properties dialog is open. > >> > >> Patch attached. Obviously WIP. Not so obvious, it's dirty code (some > >> wxLogError, not really efficient code, etc). But comments very welcome. > >> > > > > Seems it hits final release. See attached patch. Protects against server > > or database disconnection, and browser refresh. If a user tries to open > > many times the same object properties, it will raise the previous window > > rather than opening a new one. > > > > I think it's pretty neat :) It's been a huge pgAdmin's issue for a long > > time. > > > > Any comments before I commit it? > > It's too late for 1.14 - that's why I said upstream it should be for > 1.15 (RC1 may well be next week). > Sure, I wasn't gonna commit it on 1.14. If RC1 may be next week, I really have to send a reminder to our translators. > Otherwise, nice work :-) > Thanks. Forgot to tell that it also prevents drop when the properties dialog is open. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com
On Thu, 2011-08-11 at 21:16 +0200, Guillaume Lelarge wrote: > On Thu, 2011-08-11 at 08:52 +0100, Dave Page wrote: > > On Wed, Aug 10, 2011 at 11:06 PM, Guillaume Lelarge > > <guillaume@lelarge.info> wrote: > > > On Tue, 2011-08-09 at 00:30 +0200, Guillaume Lelarge wrote: > > >> On Sun, 2011-07-24 at 15:09 +0100, Dave Page wrote: > > >> > On Sun, Jul 24, 2011 at 2:23 PM, Guillaume Lelarge > > >> > <guillaume@lelarge.info> wrote: > > >> > > On Sun, 2011-07-24 at 14:11 +0200, Guillaume Lelarge wrote: > > >> > >> On Sun, 2011-07-24 at 12:58 +0100, Dave Page wrote: > > >> > >> > The former fix seems more appropriate. Operations in frmMain shouldn't > > >> > >> > cause dialogues to close. > > >> > >> > > > >> > >> > > >> > >> Fine with me. That should be simpler to do. > > >> > >> > > >> > >> > Simple fix seems to be to store a pointer to the dialog in pgObject; > > >> > >> > if not null, disallow drop, or if trying to view properties, use the > > >> > >> > pointer to raise the existing dialogue. Clear the pointer when the > > >> > >> > dialog is closed. > > >> > >> > > > >> > >> > > >> > >> I thought of something similar, but this is OK with me. > > >> > >> > > >> > >> I expect it to be quite a big patch. Should I work on 1.14 or master? > > >> > >> this is surely a bug, so 1.14 should be OK. But I'm afraid this is too > > >> > >> much changes to code it on a beta release. Any strong opinion on this > > >> > >> matter? > > >> > >> > > >> > > > > >> > > And another question. Let's say I open a table properties' dialog, and I > > >> > > try to refresh the whole database. Should it refresh everything except > > >> > > this table, or nothing and complains about properties' dialogs being > > >> > > open, preventing the object to be refreshed? > > >> > > > >> > Nothing. "There are properties dialogues open for one or more objects > > >> > that would be refreshed. Please close the properties dialogues and try > > >> > again." > > >> > > > >> > > >> Just a quick update. Still working on it, and I have good results so > > >> far. I'm now able to prevent refresh and drop (refresh for all objects, > > >> drop for schemas only) if an object's properties dialog is open. > > >> > > >> Patch attached. Obviously WIP. Not so obvious, it's dirty code (some > > >> wxLogError, not really efficient code, etc). But comments very welcome. > > >> > > > > > > Seems it hits final release. See attached patch. Protects against server > > > or database disconnection, and browser refresh. If a user tries to open > > > many times the same object properties, it will raise the previous window > > > rather than opening a new one. > > > > > > I think it's pretty neat :) It's been a huge pgAdmin's issue for a long > > > time. > > > > > > Any comments before I commit it? > > > > It's too late for 1.14 - that's why I said upstream it should be for > > 1.15 (RC1 may well be next week). > > > > Sure, I wasn't gonna commit it on 1.14. > > If RC1 may be next week, I really have to send a reminder to our > translators. > > > Otherwise, nice work :-) > > > > Thanks. Forgot to tell that it also prevents drop when the properties > dialog is open. > Commited. -- Guillaume http://blog.guillaume.lelarge.info http://www.dalibo.com