Re: First test of the Database Designer - Mailing list pgadmin-hackers

On Sat, 2011-06-11 at 12:58 -0430, Luis Ochoa wrote:
> On Sat, Jun 11, 2011 at 3:06 AM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
>         Hi,
>
>         I tried Luis's repo this morning, and here are the things I've
>         found
>         that still need some work before reaching the first goal:
>
>         * I have an assertion each time I launch pgAdmin2 from your
>         repo:
>          Debug: ./src/common/cmdline.cpp(446): assert "i != (-1)"
>         failed in
>          Found(): unknown switch
>
> Well this error happens when this source code is executed:
>     wxCmdLineParser cmdParser(cmdLineDesc, argc, argv);
>     if (cmdParser.Parse() != 0)
>         return false;
>
>     if ((cmdParser.Found(wxT("q")) && cmdParser.Found(wxT("qc"))) ||
>       (cmdParser.Found(wxT("S")) && cmdParser.Found(wxT("Sc"))) ||
>       (cmdParser.Found(wxT("d")) && cmdParser.Found(wxT("dc"))))
> ....
> here an assertion is raised, I always got this assert and I just
> ignored it, because I believe isn't related to my source code and this
> is probably a side effect from that xiul/gsoc2010  merge with
> gleu/ticket and the merge with pgadmin3 repo at github, and in fact I
> don't merge mi repo with original pgadmin3 from start of my coding at
> gsoc, because that process make me loss a lot of time with conflicts
> in parts of pgAdmin source code that I didn't modify in any way, in
> fact my source code is very isolate right now.
>

You can't have it both ways. Either your code is really isolated, and
you shouldn't have conflicts that makes you lose so much time, or you
code isn't.

The assertion is no big deal. But the fact your code is not correctly
merged is big one. This really is a big issue because it means we can't
simply integrate your patch in pgAdmin.

>         * I should be able to right click on an empty schema to create
>         a table
>
> Good idea, I don't thought on this because I'm not focused on
> usability right now, just I want everything works as good as possible
> with so little time to do many things.
>

Once again, I'm waiting for a complete patch. And without usability, it
can't be commited in the pgadmin repository.

I know you're working on that issue, and that's good. But you need to
keep in mind we've setup a few milestones where we specifically said
that they will be reached once we got a commitable patch. So usability
is important for each milestone.

>         * What is the "short name" of a table?
>
> That is part of the  automatic fk naming subsystem, I explain here:
>
> Table A [source of fk] ---------< Table B [destination of fk]
>
>
> - At beginning of fk, all pk or uk column from source table are added
> to destination table.
> - Each of this columns needs a name, an automatic one at the
> beginning.
> - To create this name there are two options:
> source table name  + _ + original column name
> or
> source short table name (if exists) + _ + original column name
>
> - But if user choose to use their own name he can do it, and then auto
> naming subsystem for this column is off, later if he user likes it, he
> is able to turn it on by right click on that column.
>
> - Constraint name from fk when autonaming is used, is taken from short
> name too.
>
> In little words this is just a way of helping user to have and good
> automatic naming convention if he likes to use, but if he doesn't like
> he just edit column name and constraint name, and auto naming
> subsystem turn off.
>

OK, fine with me.

>         * UI of new table dialog:
>          * the "Generate Short Name" button should not be there
>          * focus should be on the first textbox
>          * OK should be the default button (iow, hitting enter should
>         trigger
>           it)
>          * Cancel should behave like any cancel button (iow, hitting
>         esc should
>            trigger it)
>
> Well again, I'm not focused on usability right now I just wanted
> everything works fine, but in my testing:
>
> - Focus is on first texbox at least on my development environment.

Not in mine. I can't type the table name right away. I need to click in
the textbox.

> - Enter at first textbox failed, right, I'll fixed it.

Good.

> - Right now pressing esc trigger cancel at least on my development
> environment.

Not in mine.

> - What should I do with generate short name? change position or delete
> it?
>

The former (change position).

>         * UI of relationships
>          * it should be possible to create relationships between
>         existing
>            columns
>
> This is a new feature that requires changing the subsystem of
> relationship creation, and I didn't do at first goal as I told you at
> beginning only fixing of old source code and not adding new features,
> but if you want it I'll be glad of doing it but this probably will
> affect my proposal schedule and I need your confirmation. Should I do
> this feature?
>

When are you supposed to do it? if it's already in your planning, that's
fine with me.

>         * adding a char(n) type doesn't ask for its length
>
> I'll fix it.
>

Thanks.

>         * moreover, the list of datatype seems weird to me... you
>         should list
>          all available types and when the user choose a type with a
>         length
>          and/or a precision, it should ask them (it's already like
>         that for
>          varchar, but not for char, numeric, etc)
>
> This list is just as we agree at last year, some important datatypes
> and list with all when user need more.

Yes, but that's not what happened. I have a list of important data
types, and I can have a dialog with some types. That dialog should
contain all the datatypes available on this server.

> What should I modify the list? or How do you believe it should be
> done?
>

Grabing all the datatypes in the pg_type catalog.

> I'll fix precision of types.
>

Thanks.

>         * I can't use the del key when I select an object (table and
>          relationship)
>
> Keyboard shorcuts is a new feature (need modification of framework,
> and figures is not as easy as adding a new event to some class), I
> didn't do this as I explain you above. Again, I'll be glad of doing it
> but this probably will affect my proposal schedule and I need your
> confirmation. Should I do this feature?
>

When are you supposed to do it? if it's already in your planning, that's
fine with me.

>         * if I create a relationship between t1 and t2, and then
>         between t2 and
>          t1, once you start moving one of these tables, the two
>         relationships
>          appear as just one
>
> As I explain you right now angle lines don't allow me to have this
> kind of realtionships and a new kind of lines (orthogonal lines)
> should be implemented for this reason. Again, I'll be glad of doing it
> but this probably will affect my proposal schedule and I need your
> confirmation. Should I do this feature?
>

When are you supposed to do it? if it's already in your planning, that's
fine with me.

>         * if the user asks for a new design, the sql textbox should be
>         empty too
>
> I'll fix that.
>

OK.

>         * UI of relationship contextual menu
>          * you should only keep one item of "1:1" and "1:M", it's one
>         or the
>            other, so you must not offer both with a checkbox
> I'll fix that.
>

OK.

>          * rather than menu items "Foreign Key from Primary Key", and
>         "Foreign
>            Key from Unique Key", you should have a sub menu entitled
>         "Foreign
>            Key From", and, for the items in the submenu, you should
>         have
>            "Primary Key", and "Unique Constraint "+name of each unique
>            constraint...
> I'll implement this.
>

Thanks.

>
>         this way, the user  knows quickly which constraint is
>            used, and could change it quickly... and you have one less
>         dialog
>
>         * in a table box, there is a "Indexes" item never used. It
>         should
>          disappeared.
>
> You're right, index part right now are out of scope but in a future
> them should be coded.
>

Yeah, but they aren't right now. Many things should be implemented, it
doesn't mean they should appear now. That's a confusing UI.

>         * I can move a table outside of the drawing area, and I can
>         never get it
>          back
>
> That's right but only for bottom and right sides, because I would
> implement a function that auto resize canva if user go in any of this
> direction, but right now that function was harder to code that I
> believes and is no implemented, because this I'll disallow moving
> figures to this sides.
>

OK.

>
>         Even if it's a long list of complaints, I didn't really hit a
>         bug like I
>         did in the previous GSoC, so that's quite good. I could even
>         draw my
>         test schema almost completely (almost because Luis's repo
>         doesn't
>         support yet self referencing table).
>
> Self foreign key is a new feature that needs orthogonal lines features
> to be implemented.
>

Yes.

>         That test schema always ended with
>         a crash of pgAdmin. That doesn't happen any more, and it's
>         good. But
>         it's not good enough to be commitable
>
> I agree with you at this point is not commitable becuase last patch
> will be the really commitable one, but as you explain me, I'll make
> this patch commitable.
>

We can't wait for last patch. If we wait for last patch, it'll be a huge
one, and a lot of work will be needed. I really prefer to move one step
at a time.


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