Re: Copy/Paste table(s) functions - git context patch - Mailing list 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