Enhanced UI for creating and editing foreign keys by vtronko · Pull Request #921 · sqlitebrowser/sqlitebrowser

vtronko · 2017-01-05T02:19:00Z

Requires testing and thoughts.
@justinclift
@MKleusberg
@chrisjlocke

How this looks:

chrisjlocke · 2017-01-05T07:50:17Z

My only thought is that the dropdown of selectable fields contains all known fields. It would be preferable (or desirable, or likeable) if the selection was limited to those that had an index. eg, its unlikely you'd select 'movie('release_date') and if you did want it, you would have created an index on it. Some tables can be quite large, so having the list limited could be useful...

Just a thought. Apart from that, looks very useful. 👍

justinclift · 2017-01-05T10:33:28Z

Interesting thought @chrisjlocke. It might be better to show all fields, but (if possible) have the ones with an index be shown differently to the ones without. Maybe bold/italic or something?

Saying this from personal experience with creating tables in PostgreSQL recently and forgetting to put an index on a column I was using... 😉

justinclift · 2017-01-05T10:36:06Z

@innermous The screenshot for this looks really good to me too. I haven't tried compiling the code and using it on anything yet, but probably will in a few hours time. 😉

vtronko · 2017-01-05T15:54:01Z

@chrisjlocke
@justinclift
Unlikely but possible.
I still don't understand what I need to change, I didn't understand suggestions with bold indices :(

justinclift · 2017-01-05T17:09:40Z

Ahhhh. I'm not sure if it can be done, I was just thinking that using italics (or something) to show which columns in the foreign table had indexes. It's just a minor thought, and definitely not required. 😄

vtronko · 2017-01-05T17:12:18Z

Anyway, I'll make it work my way. Two combo-boxes is better and less cluttered than one. I'll update PR and description soon.

justinclift · 2017-01-05T20:21:10Z

Cool, sounds good. 😄

vtronko · 2017-01-05T21:19:18Z

@justinclift
@chrisjlocke
Looks this way now

I won't change anything, if anyone wants to enhance this - suggest your changes.

justinclift · 2017-01-05T21:39:18Z

Looks good to me. 😄

MKleusberg

Thanks, this looks promising 😃 I've added some comments and will do some more testing tomorrow, but I think other than those comments this should be fine.

MKleusberg · 2017-01-05T22:06:13Z

+        : QWidget(parent)
+        , tablesComboBox(new QComboBox(this))
+        , idsComboBox(new QComboBox(this))
+        , m_btnReset(new QPushButton("&Reset", this))


We'll need to add a tr(...) here.

MKleusberg · 2017-01-05T22:07:11Z

+        , m_btnReset(new QPushButton("&Reset", this))
+        , m_clauseEdit(new QLineEdit(this))
+    {
+        m_clauseEdit->setPlaceholderText("foreign key clauses(ON UPDATE, ON DELETE etc.)");


Another tr(...) here and maybe start with a capital letter ("Foreign key...") but I'm not sure on the latter. What's more 'correct' in English, @justinclift? 😉

Hmmm, I'll have to see it in context to decide.

Just added another ToDo item for tomorrow. 😉

MKleusberg · 2017-01-05T22:09:52Z

+        if (tablesComboBox->currentText().isEmpty() || idsComboBox->currentText().isEmpty())
+            return QString();
+        else
+            return QString("`%1`(`%2`) %3")


Better to remove the ` quotes here and call sqlb::escapeIdentifier() instead. This handles quoting of the quote char correctly and makes it easier to change the quote char in the future if need arises.

MKleusberg · 2017-01-05T22:11:24Z

+    // is that we can grab table(), columns() and constraint() only
+    // from foreign keys that had actually passed thru lexer. For a
+    // brand new foreign key those methods return empty fields.
+    QRegularExpression re("\\`(\\w*)\\`\\(\\`(\\w*)\\`\\)(.*)");


Probably needs to handle quoting of ` quotes correctly, too 😦 Might become ugly.

Read comment.
I believe this is temporary solution until we can avoid regex step and just touch good ol' POD fields.
Explanation: if we can make EditTableDialog transform constraint (e.g.

table(id) ON DELETE CASCADE

)
to the fk->setTable(), fk->setColumn() and fk->setConstraint() on the fly we'd avoid regex at all.
We just need a way to do it. We don't have many options tho.

Okay, I found workaround without parsing at all. You might not like it.

vtronko · 2017-01-06T02:35:13Z

You might not like the hack (ownership passed vice versa) but it works like clockwork and eliminates any need in parsing.

vtronko · 2017-01-06T03:39:11Z

UPD: renamed ForeignKeyDelegate to ForeignKeyEditorDelegate. Reserved ForeignKeyDelegate for another class related to #614

justinclift · 2017-01-06T04:38:34Z

The code compiles fine here, but the display isn't correct for the "Foreign Key" widget pieces:

Resizing the table or choosing different values makes no difference.

After clicking to another line, so the values chosen in the foreign key part are accepted, the chosen values then display ok:

Haven't tested any further than that though. 😄

revolter · 2017-01-06T06:19:58Z

+    editor->setGeometry(option.rect);
+}
+
+#include "ForeignKeyEditorDelegate.moc"


What is this for?

MOC requires that if QObject derived class is declared withing cpp file.

MKleusberg

Just did some more testing 😃 Here are some tricky corner cases I've found.

MKleusberg · 2017-01-06T16:33:59Z

+    QSharedPointer<sqlb::ForeignKeyClause> fk = m_table.constraint({field}, sqlb::Constraint::ForeignKeyConstraintType).dynamicCast<sqlb::ForeignKeyClause>();
+    if (!fk.isNull()) {
+        fkEditor->tablesComboBox->setCurrentText(fk->table());
+        fkEditor->idsComboBox->setCurrentText(fk->columns().first());


This crashes when trying to edit this foreign key:

CREATE TABLE test( a int, FOREIGN KEY(a) REFERENCES table2 -- No columns set here! );

In this situation the reference is to the primary key of the other table. I think we need to support this case as well because referencing to the primary key of a table is a different thing then referencing a column which just happens to be the primary key. As far as I can tell this means:

Add an empty row to the column combo box.

Allow entering extra clauses even when no column was selected.

Generate correct SQL when no column was selected (omit the '(...)' part in 'table(id) blah').

MKleusberg · 2017-01-06T16:46:05Z

+    for (auto obj : objects) {
+        if ("table" == obj.gettype()) {
+            QString tableName = obj.table.name();
+            if (tableName == m_table.name())


This is wrong. You can actually set a reference to the same table, e.g. for tree structures etc. Unfortunately this makes things difficult for new tables and maybe for those cases in which a table is renamed, because even in these cases I'd like to select the current table from the list. Not sure how to handle this best to be honest.

MKleusberg · 2017-01-06T16:57:47Z

+    sqlb::FieldPtr field = m_table.fields().at(column);
+    QSharedPointer<sqlb::ForeignKeyClause> fk = m_table.constraint({field}, sqlb::Constraint::ForeignKeyConstraintType).dynamicCast<sqlb::ForeignKeyClause>();
+    if (!fk.isNull()) {
+        fkEditor->tablesComboBox->setCurrentText(fk->table());


There is a tricky side case for this: when the referenced table is the first table in the list, the currentIndexChanged() signal isn't emitted because the current index doesn't actually change, it's the same before as after. But this means that the slot in line 101 won't be called and the columns combo box won't get filled.

Why so? There's no selected index by default.

Not sure about that. I just noticed that the column combo box doesn't get filled when the foreign key references the first table. A quick fix that I tried was to first call setCurrentIndex(-1) and then setCurrentText(...):

fkEditor->tablesComboBox->setCurrentIndex(-1); fkEditor->tablesComboBox->setCurrentText(fk->table());

With this extra line it works just fine, but it's sort of a hack.

I still don't get what's wrong. Did you try to build it yet?

Hmm sorry, I'm bad at explaining these things... But let me try with an example:
I'm using this database (you'll need to open it in DB4S, obviously). There is a table aaa and a table bbb. Table bbb has a foreign key to table aaa. Now, when you edit table bbb, you'll see the foreign key constraint like this:

But when you double click it, you will get this (or at least I do):

You can see that the columns combo box is empty and that it's grey (=disabled). I need to change the table to some other table (sqlite_sequence) and then back to aaa - only then the columns list is filled.

For me that happens whenever the referenced table is the first in the list. Hope this makes some more sense 😃

Kk. We'll see what I can do.

MKleusberg · 2017-01-06T17:03:17Z

+        m_table.setConstraint({field}, sqlb::ConstraintPtr(fk));
+    }
+
+    model->setData(index, sql);


Maybe need to check for the return value here: if the newly set foreign key won't work, you'll see an error message but afterwards the new foreign key will still be set. This makes it look like it actually did work even though it hasn't. Not sure however what to best do here if false is returned.

@MKleusberg return value where? Can you be more specific, please.

Ah sorry, never mind this. This was just me being stupid and confusing two things 😉 All good here!

justinclift · 2017-01-06T19:20:14Z

New PR coming up? 😄

vtronko · 2017-01-07T14:32:47Z

@justinclift nah.

vtronko · 2017-01-09T22:05:40Z

Updated it. Handles fk's without explicit keys now and allows to set fk constraint to same table. Renaming is not supported yet(it never was though with line edit widget)

justinclift · 2017-01-10T19:17:40Z

Excellent @innermous. I'll try this new code out on OSX in a bit. 😄

justinclift · 2017-01-10T22:18:27Z

Just tried this out on OSX. The two drop down fields still have the sizing problem. ☹️

I'll take a better look at this tomorrow, and see if I can figure out a solution for that. It's probably something simple. 😄

vtronko · 2017-01-10T22:38:27Z

@justinclift I didn't do anything to it.

justinclift · 2017-01-10T22:47:05Z

Yeah, I thought that was the case. No worries. I should be able to figure it out.

vtronko · 2017-01-12T01:00:09Z

Soooooo it's quiet again...
@MKleusberg can we merge it? From my POV there're no edge cases and everything except renaming works smooth.

MKleusberg · 2017-01-12T19:51:53Z

Yep, ok from my point of view, too. If you have the time, I have just two minor suggestions left:

I think the #inlcude <QRegularExpressionMatch> can be deleted now.
Can you add a TODO comment somewhere to add support for filling the table and id combo boxes with the current table scheme when creating a new table? I agree it's not super important right away, but maybe we should add that in the future. And with the TODO it won't be forgotten (hopefully) 😃

vtronko · 2017-01-12T20:01:50Z

@MKleusberg

Forgot to.
All right.

MKleusberg · 2017-01-12T20:05:30Z

No worries. Thanks for sticking to this PR! I really think it's a big improvement in comparison to the old 'editor'! Just feel free to merge this anytime 😃

justinclift · 2017-01-13T12:51:26Z

Finally merged, yay!

Thanks heaps for sticking with this @innermous. 😁

(Those drop downs still need fixing on OSX, but that can be done separately.)

vtronko · 2017-01-13T15:58:12Z

Why ...
It was not done yet.

justinclift · 2017-01-13T16:06:23Z

Sorry, I thought it was. 😦

justinclift added this to the 3.10.0 - A Rubber Cactus Has Many Uses milestone Jan 5, 2017

justinclift added constraints & foreign keys enhancement Feature requests. labels Jan 5, 2017

justinclift assigned vtronko Jan 5, 2017

vtronko force-pushed the master branch from 85d5eea to 6a2d14b Compare January 5, 2017 21:21

MKleusberg reviewed Jan 5, 2017

View reviewed changes

vtronko force-pushed the master branch from 6a2d14b to 948e40e Compare January 6, 2017 02:33

vtronko force-pushed the master branch from 948e40e to 331f884 Compare January 6, 2017 03:37

revolter reviewed Jan 6, 2017

View reviewed changes

MKleusberg reviewed Jan 6, 2017

View reviewed changes

vtronko closed this Jan 6, 2017

MKleusberg added website and removed website labels Jan 8, 2017

vtronko reopened this Jan 9, 2017

vtronko force-pushed the master branch from 331f884 to 453e27b Compare January 9, 2017 22:04

Enhanced UI for creating and editing foreign keys

4f7473f

vtronko force-pushed the master branch from 453e27b to 4f7473f Compare January 13, 2017 11:00

justinclift merged commit 1ae7537 into sqlitebrowser:master Jan 13, 2017

justinclift mentioned this pull request Jan 28, 2017

Foreign key editing widget is too small #951

Closed

3 of 13 tasks

filisko mentioned this pull request Oct 4, 2017

ON DELETE and ON UPDATE Actions #1162

Closed

Conversation

vtronko commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisjlocke commented Jan 5, 2017

Uh oh!

justinclift commented Jan 5, 2017 • edited by chrisjlocke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinclift commented Jan 5, 2017

Uh oh!

vtronko commented Jan 5, 2017

Uh oh!

justinclift commented Jan 5, 2017

Uh oh!

vtronko commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinclift commented Jan 5, 2017

Uh oh!

vtronko commented Jan 5, 2017

Uh oh!

justinclift commented Jan 5, 2017

Uh oh!

MKleusberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vtronko Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vtronko commented Jan 6, 2017

Uh oh!

vtronko commented Jan 6, 2017

Uh oh!

justinclift commented Jan 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vtronko Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MKleusberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MKleusberg Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vtronko commented Jan 5, 2017 •

edited

Loading

justinclift commented Jan 5, 2017 •

edited by chrisjlocke

Loading

vtronko commented Jan 5, 2017 •

edited

Loading

vtronko Jan 6, 2017 •

edited

Loading

vtronko Jan 6, 2017 •

edited

Loading

MKleusberg Jan 6, 2017 •

edited

Loading

vtronko Jan 7, 2017 •

edited

Loading

vtronko commented Jan 13, 2017 •

edited

Loading