Re: Copy/Paste table(s) functions - git context patch

From: Guillaume Lelarge <guillaume(at)lelarge(dot)info>
To: Vladimir Kokovic <vladimir(dot)kokovic(at)gmail(dot)com>
Cc: Dave Page <dpage(at)pgadmin(dot)org>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: Copy/Paste table(s) functions - git context patch
Date: 2011-10-03 20:07:24
Message-ID: 1317672445.2115.27.camel@localhost.localdomain
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

On Sat, 2011-09-24 at 17:43 +0200, Guillaume Lelarge wrote:
> On Sun, 2011-09-18 at 15:48 +0200, Vladimir Kokovic wrote:
> > Hi,
> >
> > After a while, here's a new patch for Copy/Paste table(s) functions.
> >
>
> Sorry for not replying sooner. This quick mail to tell you that I'll
> work on it ASAP. Perhaps tomorrow. Anyway, I didn't forget your patch,
> I'm just fighting to find some time to review it (and actually, if
> someone else is interested in reviewing it, it would be fine with me).
>

OK, took some time tonight to review it.

* When I copy a table, and then paste it, I can't paste it once more.
Why is that?
* If I paste a table and there is an error (for example, the table
already exists), I can't paste it once more.
* If I paste a table, and click on the dialog asking me if I'm sure I
want to copy the table here, I can't paste it once more.
* If I paste a table to the same schema I copied it from, and if I click
cancel on the dialog asking me for a suffix, it keeps asking me for a
suffix (wether I click OK or cancel, same behaviour)
* If I paste a table to the another schema I copied it from, and that
the same table name exists in that schema, it asks me for a suffix. If
I click cancel, it still tries to paste the table (it shouldn't, I
clicked cancel).
* If I paste a table to the another schema I copied it from, and that
the same table name exists in that schema, it asks me for a suffix. If
I delete the suffix by default and click OK, it still tries to paste
the table (it shouldn't, it's obvious it would fail).
* If I paste a table and the table already exists, it asks me for a
suffix. If I put ".whatever", it fails because the table name isn't
quoted (I have a CREATE TABLE foo.bar.whatever, instead of CREATE
TABLE foo."bar.whatever")
* By the way, when it fails to create the table, it shouldn't try to
insert into it right after.
* With drag and drop, the cursor changes too late... and you should
change the cursor, it looks non standard).

* I don't like at all the "Copy tables from the list..." dialog. It
shouldn't display tables from every servers.
There are unexplained colors (green, blue, and red).
Checkboxes are not usable on servers and databases.
Unless you explain why, I think you should get rid of it, not fix it.

On the code itself:

* ctlCheckTreeView.cpp is a widget, that shouldn't have any behaviour
specific to one window. At least, not in the way this is done now.
* To check if a table exists, you look into pg_tables. You should better
browse the treeview.
* BTW, you already have the function to do it (findTable)
* Messages shouldn't be in uppercase (COPIED FROM...).

I also don't understand this code in pgColumn.cpp:
@@ -327,7 +328,8 @@ wxString pgColumn::GetDefinition()

if ((sql == wxT("integer") || sql == wxT("bigint") ||
sql == wxT("pg_catalog.integer") || sql ==
wxT("pg_catalog.bigint"))
- && (GetDefault() == seqDefault1 || GetDefault() ==
seqDefault2))
+ && ((sequence9 && !GetSerialSequence().IsEmpty()) ||
+ (!sequence9 && (GetDefault() == seqDefault1 || GetDefault() ==
seqDefault2))))
{
if (sql.Right(6) == wxT("bigint"))
sql = wxT("bigserial");

Another issue: I launch a copy and disconnect during copy. After I
reconnect, it crashes. But the table is there with its 6 million rows.
And sometimes I don't need to reconnect: it crashes during the copy.

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

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Guillaume Lelarge 2011-10-03 20:35:59 Thread manager
Previous Message Guillaume Lelarge 2011-10-03 18:56:56 Re: Updated Latvian translation