Thread: [FEATURE] Add schema option to all relevant objects

Hi all,

I noticed that objects which can be moved to different schemas can't
be moved in PgAdmin, so I looked to see if there was any request to
have this implemented, and found this ticket:
http://code.pgadmin.org/trac/ticket/5

So I have now implemented this.  A schema drop-down box will appear in
the properties dialogue for each relevant object beneath the owner
drop-down.  I noticed that the Extensions properties dialogue already
had one in it (and I've changed how it works), but left it where it
was.  In order to implement these changes, I had to also fix quite a
few bugs, and while I was at it, implemented a few additional changes.
 They are as follows:

- Prevent functions having a complete rewrite when changing owner
- Add the ability to specify an owner for operators at creation time
- Fix invalid syntax on text search configuration, parser and template
when modifying the name
- Fix unescaped name when modifying text search configuration, parser
and template name
- Disabled the owner field on text search dictionaries as it cannot be modified
- Allow renaming types for versions 8.4 and higher

I also refactored some code, including re-basing the extensions class
on the pgSchema class rather than pgDatabase, replace explicit SET
OWNER clauses with common function and tore out a few things from the
text search classes which were just causing problems rather than
helping.

Anyway, patch attached.  I've tested every object's creation and
modification in various combinations on PostgreSQL 8.4 and 9.0, but if
this patch is a viable candidate for committing then will need testing
on earlier versions too.  Plus some code review since I'm a n00b.
Also look at how I'm selecting the list of schemas in case there
should be any additional ones which should be filtered out.

Cheers!

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Thom,

On Tue, 2011-07-05 at 20:23 +0100, Thom Brown wrote:
> [...]
> I noticed that objects which can be moved to different schemas can't
> be moved in PgAdmin, so I looked to see if there was any request to
> have this implemented, and found this ticket:
> http://code.pgadmin.org/trac/ticket/5
>

Exactly. I did a complete patch once, but my main issue, that I couldn't
fix, was the refresh of the schemas in the browser.

> So I have now implemented this.  A schema drop-down box will appear in
> the properties dialogue for each relevant object beneath the owner
> drop-down.

Did you fix the issue with the refresh of the browser? (I can't check
yet, I'm doing a last time compile of Jasmin's patch :) )

>   I noticed that the Extensions properties dialogue already
> had one in it (and I've changed how it works),

Oops, that was probably a bad move. Extensions don't have schemas by
themselves. Their objects have one, but not the extension in itself.
(Once again, I may be wrong as I didn't read your patch yet)

>  but left it where it
> was.  In order to implement these changes, I had to also fix quite a
> few bugs, and while I was at it, implemented a few additional changes.
>  They are as follows:
>
> - Prevent functions having a complete rewrite when changing owner
> - Add the ability to specify an owner for operators at creation time
> - Fix invalid syntax on text search configuration, parser and template
> when modifying the name
> - Fix unescaped name when modifying text search configuration, parser
> and template name
> - Disabled the owner field on text search dictionaries as it cannot be modified
> - Allow renaming types for versions 8.4 and higher
>

You're gonna hate me, but can you extract the fixes, one by one, from
the actual new feature? even if we won't apply all of them to 1.14, I
don't want to mix them with the real new feature (at least for
pgAdmin :) ).

> I also refactored some code, including re-basing the extensions class
> on the pgSchema class rather than pgDatabase

Bad move once again. An extension doesn't have a schema.

> , replace explicit SET
> OWNER clauses with common function

Can you explain that one?

>  and tore out a few things from the
> text search classes which were just causing problems rather than
> helping.
>

More explanations please?

> Anyway, patch attached.  I've tested every object's creation and
> modification in various combinations on PostgreSQL 8.4 and 9.0, but if
> this patch is a viable candidate for committing then will need testing
> on earlier versions too.  Plus some code review since I'm a n00b.
> Also look at how I'm selecting the list of schemas in case there
> should be any additional ones which should be filtered out.
>

Will look into it.

> Cheers!
>

Thanks a lot for your work. This patch was something I was waiting for.

BTW, are you going to char(11)?


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On Tue, Jul 5, 2011 at 8:47 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
>
> Thanks a lot for your work. This patch was something I was waiting for.

A few weeks back I had to talk Thom into posting a 2 line fix. I'm
beginning to think I may have unleashed some sort of rampant patch
beast...

> BTW, are you going to char(11)?

Are you? Did we have that conversation already? :-)

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, 2011-07-05 at 20:51 +0100, Dave Page wrote:
> On Tue, Jul 5, 2011 at 8:47 PM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
> >
> > Thanks a lot for your work. This patch was something I was waiting for.
>
> A few weeks back I had to talk Thom into posting a 2 line fix. I'm
> beginning to think I may have unleashed some sort of rampant patch
> beast...
>

You *may* have? you absolutely did. Which feels great honestly ;-) he
should keep the patches coming (even if it prevents me from working on
the documentation at the same time).

> > BTW, are you going to char(11)?
>
> Are you? Did we have that conversation already? :-)
>

I am. And we had a conversation about it. But I don't know for Thom. Or
did you tell me? if you did, I can only blame my memory...


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On Tue, Jul 5, 2011 at 8:57 PM, Guillaume Lelarge
<guillaume@lelarge.info> wrote:
> On Tue, 2011-07-05 at 20:51 +0100, Dave Page wrote:
>> On Tue, Jul 5, 2011 at 8:47 PM, Guillaume Lelarge
>> <guillaume@lelarge.info> wrote:
>> >
>> > Thanks a lot for your work. This patch was something I was waiting for.
>>
>> A few weeks back I had to talk Thom into posting a 2 line fix. I'm
>> beginning to think I may have unleashed some sort of rampant patch
>> beast...
>>
>
> You *may* have? you absolutely did. Which feels great honestly ;-) he
> should keep the patches coming (even if it prevents me from working on
> the documentation at the same time).

Oh yes - definitely +1. I'm certainly not complaining.

>> > BTW, are you going to char(11)?
>>
>> Are you? Did we have that conversation already? :-)
>>
>
> I am. And we had a conversation about it. But I don't know for Thom. Or
> did you tell me? if you did, I can only blame my memory...

:-). I'm just as bad. I think I'm the only EDBer going.


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 5 July 2011 20:47, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> Hi Thom,
>
> On Tue, 2011-07-05 at 20:23 +0100, Thom Brown wrote:
>> [...]
>> I noticed that objects which can be moved to different schemas can't
>> be moved in PgAdmin, so I looked to see if there was any request to
>> have this implemented, and found this ticket:
>> http://code.pgadmin.org/trac/ticket/5
>>
>
> Exactly. I did a complete patch once, but my main issue, that I couldn't
> fix, was the refresh of the schemas in the browser.
>
>> So I have now implemented this.  A schema drop-down box will appear in
>> the properties dialogue for each relevant object beneath the owner
>> drop-down.
>
> Did you fix the issue with the refresh of the browser? (I can't check
> yet, I'm doing a last time compile of Jasmin's patch :) )

I got it refreshing the node in the original schema, but not the
destination one.

>>   I noticed that the Extensions properties dialogue already
>> had one in it (and I've changed how it works),
>
> Oops, that was probably a bad move. Extensions don't have schemas by
> themselves. Their objects have one, but not the extension in itself.
> (Once again, I may be wrong as I didn't read your patch yet)

Well from reading the docs, it suggested the extension itself resided
within a schema, but maybe you're right.

>>  but left it where it
>> was.  In order to implement these changes, I had to also fix quite a
>> few bugs, and while I was at it, implemented a few additional changes.
>>  They are as follows:
>>
>> - Prevent functions having a complete rewrite when changing owner
>> - Add the ability to specify an owner for operators at creation time
>> - Fix invalid syntax on text search configuration, parser and template
>> when modifying the name
>> - Fix unescaped name when modifying text search configuration, parser
>> and template name
>> - Disabled the owner field on text search dictionaries as it cannot be modified
>> - Allow renaming types for versions 8.4 and higher
>>
>
> You're gonna hate me, but can you extract the fixes, one by one, from
> the actual new feature? even if we won't apply all of them to 1.14, I
> don't want to mix them with the real new feature (at least for
> pgAdmin :) ).

Hmmm... okay, I'll look at doing that.

>> I also refactored some code, including re-basing the extensions class
>> on the pgSchema class rather than pgDatabase
>
> Bad move once again. An extension doesn't have a schema.
>
>> , replace explicit SET
>> OWNER clauses with common function
>
> Can you explain that one?

Rather than building up the ALTER <object> SET OWNER in each class, I
made them all use the AppendOwnerChange function

>>  and tore out a few things from the
>> text search classes which were just causing problems rather than
>> helping.
>>
>
> More explanations please?

The GetQuoteIdentifier functions these class headers implemented (e.g.
include/schema/pgTextSearchConfiguration.h) didn't quote the name from
what I recall, so ended up deleting them, and they just inherit the
same function from its base class.

>> Cheers!
>>
>
> Thanks a lot for your work. This patch was something I was waiting for.
>
> BTW, are you going to char(11)?

Unfortunately no. :(

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 5 July 2011 20:51, Dave Page <dpage@pgadmin.org> wrote:
> On Tue, Jul 5, 2011 at 8:47 PM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>>
>> Thanks a lot for your work. This patch was something I was waiting for.
>
> A few weeks back I had to talk Thom into posting a 2 line fix. I'm
> beginning to think I may have unleashed some sort of rampant patch
> beast...

:/

>> BTW, are you going to char(11)?
>
> Are you? Did we have that conversation already? :-)

No, and no. :(

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, 2011-07-05 at 21:01 +0100, Thom Brown wrote:
> On 5 July 2011 20:47, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > Hi Thom,
> >
> > On Tue, 2011-07-05 at 20:23 +0100, Thom Brown wrote:
> >> [...]
> >> I noticed that objects which can be moved to different schemas can't
> >> be moved in PgAdmin, so I looked to see if there was any request to
> >> have this implemented, and found this ticket:
> >> http://code.pgadmin.org/trac/ticket/5
> >>
> >
> > Exactly. I did a complete patch once, but my main issue, that I couldn't
> > fix, was the refresh of the schemas in the browser.
> >
> >> So I have now implemented this.  A schema drop-down box will appear in
> >> the properties dialogue for each relevant object beneath the owner
> >> drop-down.
> >
> > Did you fix the issue with the refresh of the browser? (I can't check
> > yet, I'm doing a last time compile of Jasmin's patch :) )
>
> I got it refreshing the node in the original schema, but not the
> destination one.
>

Which is an issue. People may understand that pgadmin doesn't know about
new or altered objects if the object is created or changed outside of
the UI. But inside the UI, it should refresh its browser.

> >>   I noticed that the Extensions properties dialogue already
> >> had one in it (and I've changed how it works),
> >
> > Oops, that was probably a bad move. Extensions don't have schemas by
> > themselves. Their objects have one, but not the extension in itself.
> > (Once again, I may be wrong as I didn't read your patch yet)
>
> Well from reading the docs, it suggested the extension itself resided
> within a schema, but maybe you're right.
>

Not the extension, but the objects it owns.

> >>  but left it where it
> >> was.  In order to implement these changes, I had to also fix quite a
> >> few bugs, and while I was at it, implemented a few additional changes.
> >>  They are as follows:
> >>
> >> - Prevent functions having a complete rewrite when changing owner
> >> - Add the ability to specify an owner for operators at creation time
> >> - Fix invalid syntax on text search configuration, parser and template
> >> when modifying the name
> >> - Fix unescaped name when modifying text search configuration, parser
> >> and template name
> >> - Disabled the owner field on text search dictionaries as it cannot be modified
> >> - Allow renaming types for versions 8.4 and higher
> >>
> >
> > You're gonna hate me, but can you extract the fixes, one by one, from
> > the actual new feature? even if we won't apply all of them to 1.14, I
> > don't want to mix them with the real new feature (at least for
> > pgAdmin :) ).
>
> Hmmm... okay, I'll look at doing that.
>

Thanks.

> >> I also refactored some code, including re-basing the extensions class
> >> on the pgSchema class rather than pgDatabase
> >
> > Bad move once again. An extension doesn't have a schema.
> >
> >> , replace explicit SET
> >> OWNER clauses with common function
> >
> > Can you explain that one?
>
> Rather than building up the ALTER <object> SET OWNER in each class, I
> made them all use the AppendOwnerChange function
>

Oh, great. Thanks.

> >>  and tore out a few things from the
> >> text search classes which were just causing problems rather than
> >> helping.
> >>
> >
> > More explanations please?
>
> The GetQuoteIdentifier functions these class headers implemented (e.g.
> include/schema/pgTextSearchConfiguration.h) didn't quote the name from
> what I recall, so ended up deleting them, and they just inherit the
> same function from its base class.
>

OK, good thing too.

> >> Cheers!
> >>
> >
> > Thanks a lot for your work. This patch was something I was waiting for.
> >
> > BTW, are you going to char(11)?
>
> Unfortunately no. :(
>

Too bad. I owe you a beer next time.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On 5 July 2011 21:14, Jasmin Dizdarevic <jasmin.dizdarevic@gmail.com> wrote:
>> > > Did you fix the issue with the refresh of the browser? (I can't check
>> > > yet, I'm doing a last time compile of Jasmin's patch :) )
>> >
>> > I got it refreshing the node in the original schema, but not the
>> > destination one.
>> >
>>
>> Which is an issue. People may understand that pgadmin doesn't know about
>> new or altered objects if the object is created or changed outside of
>> the UI. But inside the UI, it should refresh its browser.
>
> He Tom,
> can't you use frmMain::GetNodePath and frmMain::SetCurrentNode to do this?
> I don't exactly know what you're doing, but it should be possible to replace
> old schema name with the new one.
> 1. Extract path of old object
> 2. replace old schema name with new one
> 3. select the object in the new schema
> ?

I'm not at all familiar with PgAdmin's codebase, I'm just feeling in
the dark so I don't know how easy it is to do that.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 5 July 2011 21:08, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Tue, 2011-07-05 at 21:01 +0100, Thom Brown wrote:
>> On 5 July 2011 20:47, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> > Hi Thom,
>> >
>> > On Tue, 2011-07-05 at 20:23 +0100, Thom Brown wrote:
>> >> [...]
>> >> I noticed that objects which can be moved to different schemas can't
>> >> be moved in PgAdmin, so I looked to see if there was any request to
>> >> have this implemented, and found this ticket:
>> >> http://code.pgadmin.org/trac/ticket/5
>> >>
>> >
>> > Exactly. I did a complete patch once, but my main issue, that I couldn't
>> > fix, was the refresh of the schemas in the browser.
>> >
>> >> So I have now implemented this.  A schema drop-down box will appear in
>> >> the properties dialogue for each relevant object beneath the owner
>> >> drop-down.
>> >
>> > Did you fix the issue with the refresh of the browser? (I can't check
>> > yet, I'm doing a last time compile of Jasmin's patch :) )
>>
>> I got it refreshing the node in the original schema, but not the
>> destination one.
>>
>
> Which is an issue. People may understand that pgadmin doesn't know about
> new or altered objects if the object is created or changed outside of
> the UI. But inside the UI, it should refresh its browser.
>
>> >>   I noticed that the Extensions properties dialogue already
>> >> had one in it (and I've changed how it works),
>> >
>> > Oops, that was probably a bad move. Extensions don't have schemas by
>> > themselves. Their objects have one, but not the extension in itself.
>> > (Once again, I may be wrong as I didn't read your patch yet)
>>
>> Well from reading the docs, it suggested the extension itself resided
>> within a schema, but maybe you're right.
>>
>
> Not the extension, but the objects it owns.

I don't understand the issue.  The current beta runs exactly the same
SQL.  And the reason why I rebased it was because otherwise the
Extensions dialogue couldn't benefit from the dlgProperty methods
relating to schemas.  I noticed there was mention of other schemas in
the code, so I may have not factored those into filtering the list of
schemas.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 5 July 2011 20:47, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Tue, 2011-07-05 at 20:23 +0100, Thom Brown wrote:
>>  but left it where it
>> was.  In order to implement these changes, I had to also fix quite a
>> few bugs, and while I was at it, implemented a few additional changes.
>>  They are as follows:
>>
>> - Prevent functions having a complete rewrite when changing owner
>> - Add the ability to specify an owner for operators at creation time
>> - Fix invalid syntax on text search configuration, parser and template
>> when modifying the name
>> - Fix unescaped name when modifying text search configuration, parser
>> and template name
>> - Disabled the owner field on text search dictionaries as it cannot be modified
>> - Allow renaming types for versions 8.4 and higher
>>
>
> You're gonna hate me, but can you extract the fixes, one by one, from
> the actual new feature? even if we won't apply all of them to 1.14, I
> don't want to mix them with the real new feature (at least for
> pgAdmin :) ).

Okay, I've picked out the patches and removed them from the main
patch.  All reattached.

textsearch*_syntax_fix.patch - These patches fix renaming the table so
it doesn't refer to the object as FTS.

remove_ts_*_quote_override.patch - These remove the overloading of the
GetQuotedIdentifier function which fail to quote the object name.

git apply textsearchdictionary_disable_owner.patch - disables the
owner dropdown when creating a text search template object as it
cannot be specified upon creation.  We should add an alter statement
to allow this, but for now, just disallow it rather than not work.

add_schemas_to_all_items_filtered.patch - same patch as before, but
with the above patch contents removed or adjusted to assume the fixes
have been applied.

I can't figure out how to refresh the destination node yet though.
This is a pain as various attempts often result in segfaults, and not
sure why.  At this point I hate nodes and will kill the next one I
see.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
> On 5 July 2011 20:47, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > On Tue, 2011-07-05 at 20:23 +0100, Thom Brown wrote:
> >>  but left it where it
> >> was.  In order to implement these changes, I had to also fix quite a
> >> few bugs, and while I was at it, implemented a few additional changes.
> >>  They are as follows:
> >>
> >> - Prevent functions having a complete rewrite when changing owner
> >> - Add the ability to specify an owner for operators at creation time
> >> - Fix invalid syntax on text search configuration, parser and template
> >> when modifying the name
> >> - Fix unescaped name when modifying text search configuration, parser
> >> and template name
> >> - Disabled the owner field on text search dictionaries as it cannot be modified
> >> - Allow renaming types for versions 8.4 and higher
> >>
> >
> > You're gonna hate me, but can you extract the fixes, one by one, from
> > the actual new feature? even if we won't apply all of them to 1.14, I
> > don't want to mix them with the real new feature (at least for
> > pgAdmin :) ).
>
> Okay, I've picked out the patches and removed them from the main
> patch.  All reattached.
>
> textsearch*_syntax_fix.patch - These patches fix renaming the table so
> it doesn't refer to the object as FTS.
>

Applied.

> remove_ts_*_quote_override.patch - These remove the overloading of the
> GetQuotedIdentifier function which fail to quote the object name.
>

Applied.

> git apply textsearchdictionary_disable_owner.patch - disables the
> owner dropdown when creating a text search template object as it
> cannot be specified upon creation.  We should add an alter statement
> to allow this, but for now, just disallow it rather than not work.
>

I didn't apply your patch. I added this feature instead.

> add_schemas_to_all_items_filtered.patch - same patch as before, but
> with the above patch contents removed or adjusted to assume the fixes
> have been applied.
>

Not yet done. I'll try to take care of this tomorrow.

> I can't figure out how to refresh the destination node yet though.
> This is a pain as various attempts often result in segfaults, and not
> sure why.  At this point I hate nodes and will kill the next one I
> see.
>

Thanks a lot for your patches.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
>> add_schemas_to_all_items_filtered.patch - same patch as before, but
>> with the above patch contents removed or adjusted to assume the fixes
>> have been applied.
>>
>
> Not yet done. I'll try to take care of this tomorrow.

Awesomes.  Thanks Guillaume.  Please ensure I haven't sneaked anything
in I shouldn't have, and that I don't break anything for previous
versions.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
> On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
> >> add_schemas_to_all_items_filtered.patch - same patch as before, but
> >> with the above patch contents removed or adjusted to assume the fixes
> >> have been applied.
> >>
> >
> > Not yet done. I'll try to take care of this tomorrow.
>
> Awesomes.  Thanks Guillaume.  Please ensure I haven't sneaked anything
> in I shouldn't have, and that I don't break anything for previous
> versions.
>

OK, just a few things that I fixed on your patch:
 * pgadmin/schema/pgIndex.cpp contained reversed change of one of your
   previous patches, I don't keep that.
 * all extension changes are wrong according to me because they aren't
   schema objects, but database objects. I don't keep them.
 * you added possibility to change a type, and a sequence, but forgot to
   add the code to detect the changes in CheckChange. I fixed that
 * in dlgForeignTable.cpp, you declare a direction variable, but don't
   use it. Fixed that too.
 * still in dlgForeignTable.cpp, you enable the schema widget only when
   the user is connected in 9.1 but this guy can have this window only
   when he's on 9.1. Fixed that.

And that should be all. The fixed patch is attached. There is one
remaining issue: how to refresh the object's parent node in the new
schema?


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On 8 July 2011 19:46, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
>> On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
>> >> add_schemas_to_all_items_filtered.patch - same patch as before, but
>> >> with the above patch contents removed or adjusted to assume the fixes
>> >> have been applied.
>> >>
>> >
>> > Not yet done. I'll try to take care of this tomorrow.
>>
>> Awesomes.  Thanks Guillaume.  Please ensure I haven't sneaked anything
>> in I shouldn't have, and that I don't break anything for previous
>> versions.
>>
>
> OK, just a few things that I fixed on your patch:
>  * pgadmin/schema/pgIndex.cpp contained reversed change of one of your
>   previous patches, I don't keep that.

Good spot.

>  * all extension changes are wrong according to me because they aren't
>   schema objects, but database objects. I don't keep them.

The reason why I based it on schema is because I wanted it to inherit
the schema combobox object and its source for a list of schemas so
lots of redundant code could be removed.  There should be no
functional difference, but I'm probably missing the point here. :)

>  * you added possibility to change a type, and a sequence, but forgot to
>   add the code to detect the changes in CheckChange. I fixed that

Good spot for sequence, but looking at the patch, the check is in
there for dlgType.

>  * in dlgForeignTable.cpp, you declare a direction variable, but don't
>   use it. Fixed that too.

Hehe, that wasn't mine.  It was in there already. :)

>  * still in dlgForeignTable.cpp, you enable the schema widget only when
>   the user is connected in 9.1 but this guy can have this window only
>   when he's on 9.1. Fixed that.

Yes, that check didn't need to be in there.

> And that should be all. The fixed patch is attached. There is one
> remaining issue: how to refresh the object's parent node in the new
> schema?

I played around with rebuilding the node path with the new schema name
to refresh it, but I kept getting endless data type issues, and have
no idea how this node stuff works, so I'm not sure what to do about
that.  Have any guidance?  If not, I might have another attempt at
working out how to manipulate nodes.

Thanks for spending time on this :)

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

>>  * all extension changes are wrong according to me because they aren't
>>   schema objects, but database objects. I don't keep them.
>
> The reason why I based it on schema is because I wanted it to inherit
> the schema combobox object and its source for a list of schemas so
> lots of redundant code could be removed.  There should be no
> functional difference, but I'm probably missing the point here. :)

It's a misuse of the class, because it's intended to represent the
schema the object is in, not one it's related to in some other way.
We've made the mistake of trying to use these classes in ways that
weren't intended in the past, and it's bitten us badly. I'm not keen
to repeat that, for the sake of a few lines of code to store a schema
name,

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 8 July 2011 20:15, Dave Page <dpage@pgadmin.org> wrote:
>>>  * all extension changes are wrong according to me because they aren't
>>>   schema objects, but database objects. I don't keep them.
>>
>> The reason why I based it on schema is because I wanted it to inherit
>> the schema combobox object and its source for a list of schemas so
>> lots of redundant code could be removed.  There should be no
>> functional difference, but I'm probably missing the point here. :)
>
> It's a misuse of the class, because it's intended to represent the
> schema the object is in, not one it's related to in some other way.
> We've made the mistake of trying to use these classes in ways that
> weren't intended in the past, and it's bitten us badly. I'm not keen
> to repeat that, for the sake of a few lines of code to store a schema
> name,

Whilst I don't see the actual impact, I'll concede the point since
I've only just started looking at code really so wouldn't have seen
the issue first-hand.  Extensions are a weird case for me since you
can assign them to a schema, but they are immune to being hidden by
the search path.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Friday, July 8, 2011, Thom Brown <thom@linux.com> wrote:
> On 8 July 2011 20:15, Dave Page <dpage@pgadmin.org> wrote:
>>>>  * all extension changes are wrong according to me because they aren't
>>>>   schema objects, but database objects. I don't keep them.
>>>
>>> The reason why I based it on schema is because I wanted it to inherit
>>> the schema combobox object and its source for a list of schemas so
>>> lots of redundant code could be removed.  There should be no
>>> functional difference, but I'm probably missing the point here. :)
>>
>> It's a misuse of the class, because it's intended to represent the
>> schema the object is in, not one it's related to in some other way.
>> We've made the mistake of trying to use these classes in ways that
>> weren't intended in the past, and it's bitten us badly. I'm not keen
>> to repeat that, for the sake of a few lines of code to store a schema
>> name,
>
> Whilst I don't see the actual impact, I'll concede the point since
> I've only just started looking at code really so wouldn't have seen
> the issue first-hand.  Extensions are a weird case for me since you
> can assign them to a schema, but they are immune to being hidden by
> the search path.

Think of an extension as 2 things - the packaging, and the contents.
The packaging is what we show under the Database node - it doesn't
have a schema. The contents do have a schema, and the packaging
mechanism gives you a simple way to change it.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, 2011-07-08 at 20:46 +0200, Guillaume Lelarge wrote:
> On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
> > On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
> > >> add_schemas_to_all_items_filtered.patch - same patch as before, but
> > >> with the above patch contents removed or adjusted to assume the fixes
> > >> have been applied.
> > >>
> > >
> > > Not yet done. I'll try to take care of this tomorrow.
> >
> > Awesomes.  Thanks Guillaume.  Please ensure I haven't sneaked anything
> > in I shouldn't have, and that I don't break anything for previous
> > versions.
> >
>
> OK, just a few things that I fixed on your patch:
>  * pgadmin/schema/pgIndex.cpp contained reversed change of one of your
>    previous patches, I don't keep that.
>  * all extension changes are wrong according to me because they aren't
>    schema objects, but database objects. I don't keep them.
>  * you added possibility to change a type, and a sequence, but forgot to
>    add the code to detect the changes in CheckChange. I fixed that
>  * in dlgForeignTable.cpp, you declare a direction variable, but don't
>    use it. Fixed that too.
>  * still in dlgForeignTable.cpp, you enable the schema widget only when
>    the user is connected in 9.1 but this guy can have this window only
>    when he's on 9.1. Fixed that.
>

I forgot one thing:

 * extract the SQL changes to commit them now in 1.14 and master.

> And that should be all. The fixed patch is attached. There is one
> remaining issue: how to refresh the object's parent node in the new
> schema?
>
>


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On 8 July 2011 20:52, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Fri, 2011-07-08 at 20:46 +0200, Guillaume Lelarge wrote:
>> On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
>> > On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> > > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
>> > >> add_schemas_to_all_items_filtered.patch - same patch as before, but
>> > >> with the above patch contents removed or adjusted to assume the fixes
>> > >> have been applied.
>> > >>
>> > >
>> > > Not yet done. I'll try to take care of this tomorrow.
>> >
>> > Awesomes.  Thanks Guillaume.  Please ensure I haven't sneaked anything
>> > in I shouldn't have, and that I don't break anything for previous
>> > versions.
>> >
>>
>> OK, just a few things that I fixed on your patch:
>>  * pgadmin/schema/pgIndex.cpp contained reversed change of one of your
>>    previous patches, I don't keep that.
>>  * all extension changes are wrong according to me because they aren't
>>    schema objects, but database objects. I don't keep them.
>>  * you added possibility to change a type, and a sequence, but forgot to
>>    add the code to detect the changes in CheckChange. I fixed that
>>  * in dlgForeignTable.cpp, you declare a direction variable, but don't
>>    use it. Fixed that too.
>>  * still in dlgForeignTable.cpp, you enable the schema widget only when
>>    the user is connected in 9.1 but this guy can have this window only
>>    when he's on 9.1. Fixed that.
>>
>
> I forgot one thing:
>
>  * extract the SQL changes to commit them now in 1.14 and master.
>
>> And that should be all. The fixed patch is attached. There is one
>> remaining issue: how to refresh the object's parent node in the new
>> schema?

Yes, I think I need to be more disciplined in what I include in
patches.  I got a bit carried away with this one in particular.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, 2011-07-08 at 20:06 +0100, Thom Brown wrote:
> On 8 July 2011 19:46, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
> >> On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
> >> >> add_schemas_to_all_items_filtered.patch - same patch as before, but
> >> >> with the above patch contents removed or adjusted to assume the fixes
> >> >> have been applied.
> >> >>
> >> >
> >> > Not yet done. I'll try to take care of this tomorrow.
> >>
> >> Awesomes.  Thanks Guillaume.  Please ensure I haven't sneaked anything
> >> in I shouldn't have, and that I don't break anything for previous
> >> versions.
> >>
> >
> > OK, just a few things that I fixed on your patch:
> >  * pgadmin/schema/pgIndex.cpp contained reversed change of one of your
> >   previous patches, I don't keep that.
>
> Good spot.
>
> >  * all extension changes are wrong according to me because they aren't
> >   schema objects, but database objects. I don't keep them.
>
> The reason why I based it on schema is because I wanted it to inherit
> the schema combobox object and its source for a list of schemas so
> lots of redundant code could be removed.  There should be no
> functional difference, but I'm probably missing the point here. :)
>
> >  * you added possibility to change a type, and a sequence, but forgot to
> >   add the code to detect the changes in CheckChange. I fixed that
>
> Good spot for sequence, but looking at the patch, the check is in
> there for dlgType.
>

I should have forgotten to "git diff" once more.

> >  * in dlgForeignTable.cpp, you declare a direction variable, but don't
> >   use it. Fixed that too.
>
> Hehe, that wasn't mine.  It was in there already. :)
>

Oops, sorry :)

> >  * still in dlgForeignTable.cpp, you enable the schema widget only when
> >   the user is connected in 9.1 but this guy can have this window only
> >   when he's on 9.1. Fixed that.
>
> Yes, that check didn't need to be in there.
>
> > And that should be all. The fixed patch is attached. There is one
> > remaining issue: how to refresh the object's parent node in the new
> > schema?
>
> I played around with rebuilding the node path with the new schema name
> to refresh it, but I kept getting endless data type issues, and have
> no idea how this node stuff works, so I'm not sure what to do about
> that.  Have any guidance?  If not, I might have another attempt at
> working out how to manipulate nodes.
>

I'll try to look into this, but I don't think I'll be lucky.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On Fri, 2011-07-08 at 20:53 +0100, Thom Brown wrote:
> On 8 July 2011 20:52, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > On Fri, 2011-07-08 at 20:46 +0200, Guillaume Lelarge wrote:
> >> On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
> >> > On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> > > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
> >> > >> add_schemas_to_all_items_filtered.patch - same patch as before, but
> >> > >> with the above patch contents removed or adjusted to assume the fixes
> >> > >> have been applied.
> >> > >>
> >> > >
> >> > > Not yet done. I'll try to take care of this tomorrow.
> >> >
> >> > Awesomes.  Thanks Guillaume.  Please ensure I haven't sneaked anything
> >> > in I shouldn't have, and that I don't break anything for previous
> >> > versions.
> >> >
> >>
> >> OK, just a few things that I fixed on your patch:
> >>  * pgadmin/schema/pgIndex.cpp contained reversed change of one of your
> >>    previous patches, I don't keep that.
> >>  * all extension changes are wrong according to me because they aren't
> >>    schema objects, but database objects. I don't keep them.
> >>  * you added possibility to change a type, and a sequence, but forgot to
> >>    add the code to detect the changes in CheckChange. I fixed that
> >>  * in dlgForeignTable.cpp, you declare a direction variable, but don't
> >>    use it. Fixed that too.
> >>  * still in dlgForeignTable.cpp, you enable the schema widget only when
> >>    the user is connected in 9.1 but this guy can have this window only
> >>    when he's on 9.1. Fixed that.
> >>
> >
> > I forgot one thing:
> >
> >  * extract the SQL changes to commit them now in 1.14 and master.
> >
> >> And that should be all. The fixed patch is attached. There is one
> >> remaining issue: how to refresh the object's parent node in the new
> >> schema?
>
> Yes, I think I need to be more disciplined in what I include in
> patches.  I got a bit carried away with this one in particular.
>

It's not easy to do, I understand you got carried away. Happened
sometimes to me too. Still happens actually.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On Fri, 2011-07-08 at 21:55 +0200, Guillaume Lelarge wrote:
> On Fri, 2011-07-08 at 20:06 +0100, Thom Brown wrote:
> > On 8 July 2011 19:46, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > > On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
> > >> On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > >> > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
> > [...]
> > > And that should be all. The fixed patch is attached. There is one
> > > remaining issue: how to refresh the object's parent node in the new
> > > schema?
> >
> > I played around with rebuilding the node path with the new schema name
> > to refresh it, but I kept getting endless data type issues, and have
> > no idea how this node stuff works, so I'm not sure what to do about
> > that.  Have any guidance?  If not, I might have another attempt at
> > working out how to manipulate nodes.
> >
>
> I'll try to look into this, but I don't think I'll be lucky.
>

I was wrong. I have it working. Code is ugly right now, so it'll need
some cleanup/refactoring/etc. Should be able to commit it tomorrow :)


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On 8 July 2011 20:57, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> It's not easy to do, I understand you got carried away. Happened
> sometimes to me too. Still happens actually.

Found out this patch doesn't contain my removal of a duplicate section
of code in dlgTextSearchConfiguration.cpp, so now if the name of a
text search configuration is changed, it appears in the SQL output
twice.  My original patch moved a whole section of code, but now the
removal of the original copy is missing.  Everything else seems to
work fine though.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, 2011-07-09 at 00:12 +0100, Thom Brown wrote:
> On 8 July 2011 20:57, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > It's not easy to do, I understand you got carried away. Happened
> > sometimes to me too. Still happens actually.
>
> Found out this patch doesn't contain my removal of a duplicate section
> of code in dlgTextSearchConfiguration.cpp, so now if the name of a
> text search configuration is changed, it appears in the SQL output
> twice.  My original patch moved a whole section of code, but now the
> removal of the original copy is missing.  Everything else seems to
> work fine though.
>

Will look into that tomorrow. Right now, I need some sleep :)


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On 9 July 2011 00:12, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Fri, 2011-07-08 at 21:55 +0200, Guillaume Lelarge wrote:
>> On Fri, 2011-07-08 at 20:06 +0100, Thom Brown wrote:
>> > On 8 July 2011 19:46, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> > > On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
>> > >> On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> > >> > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
>> > [...]
>> > > And that should be all. The fixed patch is attached. There is one
>> > > remaining issue: how to refresh the object's parent node in the new
>> > > schema?
>> >
>> > I played around with rebuilding the node path with the new schema name
>> > to refresh it, but I kept getting endless data type issues, and have
>> > no idea how this node stuff works, so I'm not sure what to do about
>> > that.  Have any guidance?  If not, I might have another attempt at
>> > working out how to manipulate nodes.
>> >
>>
>> I'll try to look into this, but I don't think I'll be lucky.
>>
>
> I was wrong. I have it working. Code is ugly right now, so it'll need
> some cleanup/refactoring/etc. Should be able to commit it tomorrow :)

Excellent.  I'll be interested to see what you needed to do to achieve
it.  Merci beaucoup.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, 2011-07-09 at 01:15 +0200, Guillaume Lelarge wrote:
> On Sat, 2011-07-09 at 00:12 +0100, Thom Brown wrote:
> > On 8 July 2011 20:57, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > > It's not easy to do, I understand you got carried away. Happened
> > > sometimes to me too. Still happens actually.
> >
> > Found out this patch doesn't contain my removal of a duplicate section
> > of code in dlgTextSearchConfiguration.cpp, so now if the name of a
> > text search configuration is changed, it appears in the SQL output
> > twice.  My original patch moved a whole section of code, but now the
> > removal of the original copy is missing.  Everything else seems to
> > work fine though.
> >
>
> Will look into that tomorrow. Right now, I need some sleep :)
>

You're right. I saw it on the diff as rejected, and couldn't understand
why you deleted that part. And I thought it was probably because of an
already applied patch (just like the changes on
pgadmin/schema/pgIndex.cpp).

Anyway, I re-added your code.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On Sat, 2011-07-09 at 00:23 +0100, Thom Brown wrote:
> On 9 July 2011 00:12, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > On Fri, 2011-07-08 at 21:55 +0200, Guillaume Lelarge wrote:
> >> On Fri, 2011-07-08 at 20:06 +0100, Thom Brown wrote:
> >> > On 8 July 2011 19:46, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> > > On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
> >> > >> On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> > >> > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
> >> > [...]
> >> > > And that should be all. The fixed patch is attached. There is one
> >> > > remaining issue: how to refresh the object's parent node in the new
> >> > > schema?
> >> >
> >> > I played around with rebuilding the node path with the new schema name
> >> > to refresh it, but I kept getting endless data type issues, and have
> >> > no idea how this node stuff works, so I'm not sure what to do about
> >> > that.  Have any guidance?  If not, I might have another attempt at
> >> > working out how to manipulate nodes.
> >> >
> >>
> >> I'll try to look into this, but I don't think I'll be lucky.
> >>
> >
> > I was wrong. I have it working. Code is ugly right now, so it'll need
> > some cleanup/refactoring/etc. Should be able to commit it tomorrow :)
>
> Excellent.  I'll be interested to see what you needed to do to achieve
> it.  Merci beaucoup.
>

Not much actually. See refresh.patch. ShowObject() in dlgProperty is
responsible of displaying objects in the treeview. I just added the code
that goes back to the Schemas node, and made it resfresh it.

add_schemas_v3.patch is the final patch. I'm intending to apply it
rather soon.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On 9 July 2011 09:28, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Sat, 2011-07-09 at 00:23 +0100, Thom Brown wrote:
>> On 9 July 2011 00:12, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> > On Fri, 2011-07-08 at 21:55 +0200, Guillaume Lelarge wrote:
>> >> On Fri, 2011-07-08 at 20:06 +0100, Thom Brown wrote:
>> >> > On 8 July 2011 19:46, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> >> > > On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
>> >> > >> On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> >> > >> > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
>> >> > [...]
>> >> > > And that should be all. The fixed patch is attached. There is one
>> >> > > remaining issue: how to refresh the object's parent node in the new
>> >> > > schema?
>> >> >
>> >> > I played around with rebuilding the node path with the new schema name
>> >> > to refresh it, but I kept getting endless data type issues, and have
>> >> > no idea how this node stuff works, so I'm not sure what to do about
>> >> > that.  Have any guidance?  If not, I might have another attempt at
>> >> > working out how to manipulate nodes.
>> >> >
>> >>
>> >> I'll try to look into this, but I don't think I'll be lucky.
>> >>
>> >
>> > I was wrong. I have it working. Code is ugly right now, so it'll need
>> > some cleanup/refactoring/etc. Should be able to commit it tomorrow :)
>>
>> Excellent.  I'll be interested to see what you needed to do to achieve
>> it.  Merci beaucoup.
>>
>
> Not much actually. See refresh.patch. ShowObject() in dlgProperty is
> responsible of displaying objects in the treeview. I just added the code
> that goes back to the Schemas node, and made it resfresh it.
>
> add_schemas_v3.patch is the final patch. I'm intending to apply it
> rather soon.

Oh I see.  I thought the objective was to refresh the old direct
parent node of the old object's location to make it disappear, then
refresh the new parent node of the object's new location to make it
reappear, then to set the focus on the new object.  Your way
simplifies it and works for me. :)

I'll try building it again and testing it with the new changes,
although because it's only on my little netbook and I want to do a
clean build, that could take some time, so you may have already
committed this by the time I get to test it.  But I'll let you know if
there's any problem.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, 2011-07-09 at 10:33 +0100, Thom Brown wrote:
> On 9 July 2011 09:28, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > On Sat, 2011-07-09 at 00:23 +0100, Thom Brown wrote:
> >> On 9 July 2011 00:12, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> > On Fri, 2011-07-08 at 21:55 +0200, Guillaume Lelarge wrote:
> >> >> On Fri, 2011-07-08 at 20:06 +0100, Thom Brown wrote:
> >> >> > On 8 July 2011 19:46, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> >> > > On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
> >> >> > >> On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> >> > >> > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
> >> >> > [...]
> >> >> > > And that should be all. The fixed patch is attached. There is one
> >> >> > > remaining issue: how to refresh the object's parent node in the new
> >> >> > > schema?
> >> >> >
> >> >> > I played around with rebuilding the node path with the new schema name
> >> >> > to refresh it, but I kept getting endless data type issues, and have
> >> >> > no idea how this node stuff works, so I'm not sure what to do about
> >> >> > that.  Have any guidance?  If not, I might have another attempt at
> >> >> > working out how to manipulate nodes.
> >> >> >
> >> >>
> >> >> I'll try to look into this, but I don't think I'll be lucky.
> >> >>
> >> >
> >> > I was wrong. I have it working. Code is ugly right now, so it'll need
> >> > some cleanup/refactoring/etc. Should be able to commit it tomorrow :)
> >>
> >> Excellent.  I'll be interested to see what you needed to do to achieve
> >> it.  Merci beaucoup.
> >>
> >
> > Not much actually. See refresh.patch. ShowObject() in dlgProperty is
> > responsible of displaying objects in the treeview. I just added the code
> > that goes back to the Schemas node, and made it resfresh it.
> >
> > add_schemas_v3.patch is the final patch. I'm intending to apply it
> > rather soon.
>
> Oh I see.  I thought the objective was to refresh the old direct
> parent node of the old object's location to make it disappear, then
> refresh the new parent node of the object's new location to make it
> reappear, then to set the focus on the new object.  Your way
> simplifies it and works for me. :)
>

I would prefer to only refresh the old parent node, and the new parent
node. But we have no way to know the new parent node. So I refresh all
schemas. Which will bring other performance issues for users with big
schemas.

> I'll try building it again and testing it with the new changes,
> although because it's only on my little netbook and I want to do a
> clean build, that could take some time, so you may have already
> committed this by the time I get to test it.  But I'll let you know if
> there's any problem.
>

I don't expect to commit this before tomorrow. So I sincerely hope your
build will be finished by then :)


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On Saturday, July 9, 2011, Guillaume Lelarge <guillaume@lelarge.info> wrote:

> I would prefer to only refresh the old parent node, and the new parent
> node. But we have no way to know the new parent node. So I refresh all
> schemas. Which will bring other performance issues for users with big
> schemas.

I wonder if we need to think about a way of passing more info down to
the lower guts of the execution mechanism. We kinda did that already
when we added the 2 part SQL execution, maybe now we should think
about a more extensible technique (maybe as simple as passing a simple
struct of stuff to deal with).

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 9 July 2011 12:56, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Sat, 2011-07-09 at 10:33 +0100, Thom Brown wrote:
>> On 9 July 2011 09:28, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> > On Sat, 2011-07-09 at 00:23 +0100, Thom Brown wrote:
>> >> On 9 July 2011 00:12, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> >> > On Fri, 2011-07-08 at 21:55 +0200, Guillaume Lelarge wrote:
>> >> >> On Fri, 2011-07-08 at 20:06 +0100, Thom Brown wrote:
>> >> >> > On 8 July 2011 19:46, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> >> >> > > On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
>> >> >> > >> On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>> >> >> > >> > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
>> >> >> > [...]
>> >> >> > > And that should be all. The fixed patch is attached. There is one
>> >> >> > > remaining issue: how to refresh the object's parent node in the new
>> >> >> > > schema?
>> >> >> >
>> >> >> > I played around with rebuilding the node path with the new schema name
>> >> >> > to refresh it, but I kept getting endless data type issues, and have
>> >> >> > no idea how this node stuff works, so I'm not sure what to do about
>> >> >> > that.  Have any guidance?  If not, I might have another attempt at
>> >> >> > working out how to manipulate nodes.
>> >> >> >
>> >> >>
>> >> >> I'll try to look into this, but I don't think I'll be lucky.
>> >> >>
>> >> >
>> >> > I was wrong. I have it working. Code is ugly right now, so it'll need
>> >> > some cleanup/refactoring/etc. Should be able to commit it tomorrow :)
>> >>
>> >> Excellent.  I'll be interested to see what you needed to do to achieve
>> >> it.  Merci beaucoup.
>> >>
>> >
>> > Not much actually. See refresh.patch. ShowObject() in dlgProperty is
>> > responsible of displaying objects in the treeview. I just added the code
>> > that goes back to the Schemas node, and made it resfresh it.
>> >
>> > add_schemas_v3.patch is the final patch. I'm intending to apply it
>> > rather soon.
>>
>> Oh I see.  I thought the objective was to refresh the old direct
>> parent node of the old object's location to make it disappear, then
>> refresh the new parent node of the object's new location to make it
>> reappear, then to set the focus on the new object.  Your way
>> simplifies it and works for me. :)
>>
>
> I would prefer to only refresh the old parent node, and the new parent
> node. But we have no way to know the new parent node. So I refresh all
> schemas. Which will bring other performance issues for users with big
> schemas.
>
>> I'll try building it again and testing it with the new changes,
>> although because it's only on my little netbook and I want to do a
>> clean build, that could take some time, so you may have already
>> committed this by the time I get to test it.  But I'll let you know if
>> there's any problem.
>>
>
> I don't expect to commit this before tomorrow. So I sincerely hope your
> build will be finished by then :)

Okay, tested it, and yes, it refreshes fine.  It would be nice if, at
a future point, we could use a targetted refresh and refocus on the
new item, but I'd say that's a nice-to-have.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, 2011-07-09 at 13:39 +0100, Dave Page wrote:
> On Saturday, July 9, 2011, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>
> > I would prefer to only refresh the old parent node, and the new parent
> > node. But we have no way to know the new parent node. So I refresh all
> > schemas. Which will bring other performance issues for users with big
> > schemas.
>
> I wonder if we need to think about a way of passing more info down to
> the lower guts of the execution mechanism. We kinda did that already
> when we added the 2 part SQL execution, maybe now we should think
> about a more extensible technique (maybe as simple as passing a simple
> struct of stuff to deal with).
>

Do you have an example? I'm not sure I really understand what you mean.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On Sat, 2011-07-09 at 15:14 +0100, Thom Brown wrote:
> On 9 July 2011 12:56, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> > On Sat, 2011-07-09 at 10:33 +0100, Thom Brown wrote:
> >> On 9 July 2011 09:28, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> > On Sat, 2011-07-09 at 00:23 +0100, Thom Brown wrote:
> >> >> On 9 July 2011 00:12, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> >> > On Fri, 2011-07-08 at 21:55 +0200, Guillaume Lelarge wrote:
> >> >> >> On Fri, 2011-07-08 at 20:06 +0100, Thom Brown wrote:
> >> >> >> > On 8 July 2011 19:46, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> >> >> > > On Thu, 2011-07-07 at 23:24 +0100, Thom Brown wrote:
> >> >> >> > >> On 7 July 2011 23:20, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> >> >> >> > >> > On Wed, 2011-07-06 at 11:59 +0100, Thom Brown wrote:
> >> >> >> > [...]
> >> >> >> > > And that should be all. The fixed patch is attached. There is one
> >> >> >> > > remaining issue: how to refresh the object's parent node in the new
> >> >> >> > > schema?
> >> >> >> >
> >> >> >> > I played around with rebuilding the node path with the new schema name
> >> >> >> > to refresh it, but I kept getting endless data type issues, and have
> >> >> >> > no idea how this node stuff works, so I'm not sure what to do about
> >> >> >> > that.  Have any guidance?  If not, I might have another attempt at
> >> >> >> > working out how to manipulate nodes.
> >> >> >> >
> >> >> >>
> >> >> >> I'll try to look into this, but I don't think I'll be lucky.
> >> >> >>
> >> >> >
> >> >> > I was wrong. I have it working. Code is ugly right now, so it'll need
> >> >> > some cleanup/refactoring/etc. Should be able to commit it tomorrow :)
> >> >>
> >> >> Excellent.  I'll be interested to see what you needed to do to achieve
> >> >> it.  Merci beaucoup.
> >> >>
> >> >
> >> > Not much actually. See refresh.patch. ShowObject() in dlgProperty is
> >> > responsible of displaying objects in the treeview. I just added the code
> >> > that goes back to the Schemas node, and made it resfresh it.
> >> >
> >> > add_schemas_v3.patch is the final patch. I'm intending to apply it
> >> > rather soon.
> >>
> >> Oh I see.  I thought the objective was to refresh the old direct
> >> parent node of the old object's location to make it disappear, then
> >> refresh the new parent node of the object's new location to make it
> >> reappear, then to set the focus on the new object.  Your way
> >> simplifies it and works for me. :)
> >>
> >
> > I would prefer to only refresh the old parent node, and the new parent
> > node. But we have no way to know the new parent node. So I refresh all
> > schemas. Which will bring other performance issues for users with big
> > schemas.
> >
> >> I'll try building it again and testing it with the new changes,
> >> although because it's only on my little netbook and I want to do a
> >> clean build, that could take some time, so you may have already
> >> committed this by the time I get to test it.  But I'll let you know if
> >> there's any problem.
> >>
> >
> > I don't expect to commit this before tomorrow. So I sincerely hope your
> > build will be finished by then :)
>
> Okay, tested it, and yes, it refreshes fine.  It would be nice if, at
> a future point, we could use a targetted refresh and refocus on the
> new item, but I'd say that's a nice-to-have.
>

Sure, maybe with Dave's idea.

BTW, just commited and pushed your patch. Thanks again.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com

On 9 July 2011 20:55, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Sat, 2011-07-09 at 15:14 +0100, Thom Brown wrote:
> BTW, just commited and pushed your patch. Thanks again.

\o/ Thanks again Guillaume.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Saturday, July 9, 2011, Guillaume Lelarge <guillaume@lelarge.info> wrote:
> On Sat, 2011-07-09 at 13:39 +0100, Dave Page wrote:
>> On Saturday, July 9, 2011, Guillaume Lelarge <guillaume@lelarge.info> wrote:
>>
>> > I would prefer to only refresh the old parent node, and the new parent
>> > node. But we have no way to know the new parent node. So I refresh all
>> > schemas. Which will bring other performance issues for users with big
>> > schemas.
>>
>> I wonder if we need to think about a way of passing more info down to
>> the lower guts of the execution mechanism. We kinda did that already
>> when we added the 2 part SQL execution, maybe now we should think
>> about a more extensible technique (maybe as simple as passing a simple
>> struct of stuff to deal with).
>>
>
> Do you have an example? I'm not sure I really understand what you mean.

(I'm not looking at any code right now, so please bear that in mind...)

Currently the code that executes the SQL that's generated by a dialog
when you click the ok button, just does a relatively simple check to
see if there's something to execute in either of the SQL boxes iirc.
I'm suggesting that we might have a more complex data structure that
we populate with info about "stuff to do" when the OK button is
clicked - for example, a schema name to refresh, an array of SQL
statements to run in separate transactions etc.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company