Enhanced UI for creating and editing foreign keys #921

Merged
merged 1 commit into from Jan 13, 2017
@innermous
Member
innermous commented Jan 5, 2017 edited

Requires testing and thoughts.
@justinclift
@MKleusberg
@chrisjlocke

How this looks:
screenshot_20170105_041958

@chrisjlocke
Contributor

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
Member
justinclift commented Jan 5, 2017 edited

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
Member

@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. 😉

@innermous
Member

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

@justinclift
Member

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. 😄

@innermous
Member
innermous commented Jan 5, 2017 edited

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
Member

Cool, sounds good. 😄

@innermous
Member

@justinclift
@chrisjlocke
Looks this way now
screenshot_20170105_231534

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

@justinclift
Member

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.

src/ForeignKeyDelegate.cpp
+ : QWidget(parent)
+ , tablesComboBox(new QComboBox(this))
+ , idsComboBox(new QComboBox(this))
+ , m_btnReset(new QPushButton("&Reset", this))
@MKleusberg
MKleusberg Jan 5, 2017 Member

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

src/ForeignKeyDelegate.cpp
+ , m_btnReset(new QPushButton("&Reset", this))
+ , m_clauseEdit(new QLineEdit(this))
+ {
+ m_clauseEdit->setPlaceholderText("foreign key clauses(ON UPDATE, ON DELETE etc.)");
@MKleusberg
MKleusberg Jan 5, 2017 Member

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? 😉

@justinclift
justinclift Jan 5, 2017 Member

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

Just added another ToDo item for tomorrow. 😉

src/ForeignKeyDelegate.cpp
+ if (tablesComboBox->currentText().isEmpty() || idsComboBox->currentText().isEmpty())
+ return QString();
+ else
+ return QString("`%1`(`%2`) %3")
@MKleusberg
MKleusberg Jan 5, 2017 Member

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.

src/ForeignKeyDelegate.cpp
+ // 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*)\\`\\)(.*)");
@MKleusberg
MKleusberg Jan 5, 2017 Member

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

@innermous
innermous Jan 6, 2017 edited Member

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.

@innermous
innermous Jan 6, 2017 Member

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

@innermous
Member

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

@innermous
Member

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

@justinclift
Member

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

screen shot 2017-01-06 at 04 33 55
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:

screen shot 2017-01-06 at 04 37 25

Haven't tested any further than that though. 😄

+ editor->setGeometry(option.rect);
+}
+
+#include "ForeignKeyEditorDelegate.moc"
@revolter
revolter Jan 6, 2017 Member

What is this for?

@innermous
innermous Jan 6, 2017 edited Member

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.

src/ForeignKeyEditorDelegate.cpp
+ for (auto obj : objects) {
+ if ("table" == obj.gettype()) {
+ QString tableName = obj.table.name();
+ if (tableName == m_table.name())
@MKleusberg
MKleusberg Jan 6, 2017 edited Member

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.

+ 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());
@MKleusberg
MKleusberg Jan 6, 2017 Member

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.

@innermous
innermous Jan 6, 2017 Member

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

@MKleusberg
MKleusberg Jan 8, 2017 Member

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.

@innermous
innermous Jan 8, 2017 Member

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

@MKleusberg
MKleusberg Jan 8, 2017 Member

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:
screenshot_20170109_001146

But when you double click it, you will get this (or at least I do):
screenshot_20170109_001312
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 😃

@innermous
innermous Jan 9, 2017 Member

Kk. We'll see what I can do.

src/ForeignKeyEditorDelegate.cpp
+ 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());
@MKleusberg
MKleusberg Jan 6, 2017 Member

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:

  1. Add an empty row to the column combo box.
  2. Allow entering extra clauses even when no column was selected.
  3. Generate correct SQL when no column was selected (omit the '(...)' part in 'table(id) blah').
+ m_table.setConstraint({field}, sqlb::ConstraintPtr(fk));
+ }
+
+ model->setData(index, sql);
@MKleusberg
MKleusberg Jan 6, 2017 Member

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.

@innermous
innermous Jan 7, 2017 edited Member

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

@MKleusberg
MKleusberg Jan 8, 2017 Member

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

@innermous innermous closed this Jan 6, 2017
@justinclift
Member

New PR coming up? 😄

@innermous
Member
@MKleusberg MKleusberg added website and removed website labels Jan 8, 2017
@innermous innermous reopened this Jan 9, 2017
@innermous
Member

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
Member

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

@justinclift
Member

Just tried this out on OSX. The two drop down fields still have the sizing problem. ☹️
screen shot 2017-01-10 at 22 17 06
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. 😄

@innermous
Member

@justinclift I didn't do anything to it.

@justinclift
Member

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

@innermous
Member

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
Member

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

  1. I think the #inlcude <QRegularExpressionMatch> can be deleted now.
  2. 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) 😃
@innermous
Member

@MKleusberg

  1. Forgot to.
  2. All right.
@MKleusberg
Member

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 justinclift merged commit 1ae7537 into sqlitebrowser:master Jan 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@justinclift
Member

Finally merged, yay!

Thanks heaps for sticking with this @innermous. 😁

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

@innermous
Member
innermous commented Jan 13, 2017 edited

Why ...
It was not done yet.

@justinclift
Member

Sorry, I thought it was. 😦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment