Issue #530: constraints on table prevents new record being added #1477

mgrojo · 2018-07-15T16:02:29Z

This adds a new dialog for adding records to a table. The current approach
is broken for several cases where foreign keys and check constraints are
impeding the insertion of an empty record. With the dialog, the user can
inspect the constraints (tooltip) and type of fields and add values
consistent with the requirements. The data is only inserted when the user
presses the Save button.

The dialog is modelled after the Edit Table or Edit Index dialog. An upper
frame allows entering the data using widgets. The lower frame previews the
SQL statement that will be used.

The old approach for adding records is still accessible pressing Tab on
the last cell of the table.

Pending design decisions

Should be the old approach removed? Apparently it is not causing problems, because it seems disabled for tables with constraints (I haven't look at how it's done)
Should the SQL frame be editable (like the 'Create Index', although it's not working very well in that case) or read-only (like the 'Create Table' dialog)? I don't see a reason for disallowing the user to edit the SQL statement, even when he can enter whatever SQL code he wants (possibly not even related to an INSERT).
Is the current naive approach for numbers in SQL correct? Values recognized as numbers are left unquoted, any other value is quoted as a string. The field types is no taken into account.

This adds a new dialog for adding records to a table. The current approach is broken for several cases where foreign keys and check constraints are impeding the insertion of an empty record. With the dialog, the user can inspect the constraints (tooltip) and type of fields and add values consistent with the requirements. The data is only inserted when the user presses the Save button. The dialog is modelled after the Edit Table or Edit Index dialog. An upper frame allows entering the data using widgets. The lower frame previews the SQL statement that will be used. The old approach for adding records is still accessible pressing Tab on the last cell of the table.

justinclift · 2018-07-15T16:06:05Z

Should be the old approach removed?

If you're asking "should we remove the ability to create a new row by pressing TAB on the last cell of the last line" then No Way. 😉

That's probably one of the things I use most when typing out rows of values. eg type stuff, tab, type stuff, tab, type stuff, tab. Can be for 50+ rows at a time.

mgrojo · 2018-07-15T16:16:38Z

If you're asking "should we remove the ability to create a new row by pressing TAB on the last cell of the last line" then No Way. 😉

I meant that 😄 That's because there were several problems with it when there were constraints. Like rows inserted in the display that failed at the SQLite level and were left in an inconsistent state. But I think we have somehow fix that, and that kind of tables do not insert a row when pressing Tab. It can probably left as is.

This PR should solve or improve the user experience for several of the issues in the foreign keys & constraints label.

For example: #530 #601 #463 #1350...

mgrojo · 2018-07-15T16:22:05Z

The commit also fixes the filter used when pressing Ctrl+Shift+Click for navigating to the referenced table. It is now using the '=' filter instead of the 'Containing' one, which makes more sense.

justinclift · 2018-07-15T17:20:43Z

Cool. This sounds good though I haven't tried out the new code yet. I should do in the next few days.

And yeah, definitely lets not kill the TAB thing. I'm not sure of a good way to handle tables with constraints either. Those that would cause insert failure if no row data is manually entered anyway.

I tend to avoid them when typing out lots of data... which is probably just learned behaviour from having previously hit problems and forgetting why. 😄

MKleusberg · 2018-07-15T21:40:30Z

I'm not sure about this to be honest 😉 Of course you're right that adding records needs to be fixed and that it can't be done without changing the UI in some way. But I feel like an extra dialog is too much for adding a record - not from the code perspective but from the user perspective because it's such a common and apparently minor thing that a dialog popping up feels irritating and maybe even distracting because you'll need to reposition it so you can see other records etc. On the other hand it's definitely good for some people like #127 shows 😄

In my opinion it feels better to always show an additional line in the table view which you can fill out and which gets inserted when you leave it or press tab after the last column. It could be populated with default values initially and because it's not inserted into the database until it's deemed ready by the user we don't have issues with constraints. I actually started implementing this a while ago and it did seem to be not too hard to program but I somehow got distracted and stopped working on it and by now the multithreading patch has changed the SqliteTableModel class anyway...

That all said, I haven't yet tested this PR in any in-depth way 😄 Also can you maybe put the '=' in an extra commit (which you can just push)? Makes more sense for the history and it's faster. Also regarding this:

Should the SQL frame be editable (like the 'Create Index', although it's not working very well in that case) [...]

That's definitely not intended 😉 I'll fix that in a second. So no reason to think about copying that behaviour 😄

mgrojo · 2018-07-16T20:41:36Z

I considered also fixing the current approach, but sincerely I didn't know how to add a record in the table view that it is not actually inserted in the data base. But this is because I get lost in the view/model interactions 😉

I reached this solution thinking a possible future record view, similar to how other database front-ends have. Ideally record insertion should work in both cases, but in table view, the still not inserted row has to look different in some way. And the moment to actually insert in the database has to be designed and it has to be under user control, etc.

I'm not against fixing the row insertion in the current table view, but if it isn't a near reality I'd prefer to include this solution. Otherwise we have a non-working insertion for tables with constraints. I wouldn't mind if it's later replaced by a working row insertion. From the ashes of this dialog, maybe I can implement a record view 😄

I propose a middle approach for the short term. I've looked into how the Tab is not working for tables with constraints. We actually try to insert, but now the error returned by SQLite is checked before adding a row to the model. Previously it was more broken (maybe I fixed that in 7ed1b1d or it was with the new threading changes, I don't know), because the row was left created in the model and view but not in the database. What can be done is: when pressing "Add Record" or Tab key on last cell, we try first the current simple row-insertion approach, if it fails we open the Add Record dialog. This is simple to implement in this branch.

justinclift · 2018-07-16T21:03:07Z

What can be done is: when pressing "Add Record" or Tab key on last cell, we try first the current simple row-insertion approach, if it fails we open the Add Record dialog. This is simple to implement in this branch.

The theory of that sounds ok to me. Still haven't tried the code in this PR yet though. 😇

mgrojo · 2018-07-16T22:01:41Z

Also can you maybe put the '=' in an extra commit (which you can just push)? Makes more sense for the history and it's faster.

@MKleusberg Do you mean to make a commit only with this in the master branch and push it? Can this merely manually done or is a git trick appropriate for this? I'm still a novice at that 😄

…y text The insertion of an empty row is always tried. When it fails due to constraints and foreign keys, the Add Record Dialog is open so the user can enter values for the new record considering the constraints. When the table has not constraints, or the row insertion provides valid values, the user is still able to insert rows using the simple approach. SQL preview in dialog is now read-only.

QLineEdit as item delegate for the value, so it is more visible that we are supposed to edit the value. Remove last end-of-line in tool-tip.

MKleusberg · 2018-08-05T14:06:06Z

@MKleusberg Do you mean to make a commit only with this in the master branch and push it? Can this merely manually done or is a git trick appropriate for this? I'm still a novice at that 😄

Looks like this is already done, right? 😄 I don't think there is a good git trick for this because it's all in one file. But now that the change is in the master branch I guess rebasing this branch should get rid of the change here.

I'm not against fixing the row insertion in the current table view, but if it isn't a near reality I'd prefer to include this solution. Otherwise we have a non-working insertion for tables with constraints. I wouldn't mind if it's later replaced by a working row insertion.

You're right of course 😄 This is definitely already a big improvement over the current problems. I'll have a closer look at the code over the next couple of days, but in general I agree that we should merge this first and then look into other UI approaches without any need to hurry.

MKleusberg

I've made several comments throughout the code. As always, don't be discouraged - it's just me writing down every thought I have 😉 And it's 99% very minor stuff 😄

I have two more general suggestions:

I like the new dialog a lot and I can see how I might want to use it from time to time even though there's no constraint error, especially for large tables where you would have to scroll a lot to see the entire row. Maybe we can add a button or menu item for "voluntarily" invoking the new dialog. Not sure where to put it though.
I'd like to have a bit more control over the values I type in. One thing is that I can't insert a number as a string which is especially a problem for values like "012" or "1.00" which currently will be inserted as "12" and "1". The other thing is that I can't set a field to NULL or distinguish between empty string and NULL in any way. Again, not sure how to best add this.

MKleusberg · 2018-08-16T10:02:24Z

src/AddRecordDialog.cpp

+class NoEditDelegate: public QStyledItemDelegate {
+public:
+    NoEditDelegate(QObject* parent=nullptr): QStyledItemDelegate(parent) {}
+    virtual QWidget* createEditor(QWidget *parent, const QStyleOptionViewItem &option, const QModelIndex &index) const {


I'm getting unused parameter warning for all parameters here. They should probably be silenced 😄

I wonder why I don't get these warnings. Is it because of the CMake flags or the GCC version (I'm using 6.3.1)?

They should probably be silenced

Do you mean through general compiler flags or by other ways that apply only to this particular code?

Oh you're probably right. I was using qmake. A quick search didn't yield any results for Wall or similar in the CMakeLists.txt file. So maybe we don't show as many warning there for some reason.

As for the silencing I meant here in the code, either by removing the name of the parameters (like I do most of the time)

virtual QWidget* createEditor(QWidget* /*parent*/, const QStyleOptionViewItem& /*option*/, const QModelIndex& /*index*/) const {

or by using the Q_UNSUED macro

virtual QWidget* createEditor(QWidget *parent, const QStyleOptionViewItem &option, const QModelIndex &index) const { Q_UNUSED(parent); Q_UNUSED(option); Q_UNUSED(index);

MKleusberg · 2018-08-16T10:02:46Z

src/AddRecordDialog.cpp

+class EditDelegate: public QStyledItemDelegate {
+public:
+    EditDelegate(QObject* parent=nullptr): QStyledItemDelegate(parent) {}
+    virtual QWidget* createEditor(QWidget *parent, const QStyleOptionViewItem &option, const QModelIndex &index) const {


Two more unused parameter warnings here.

MKleusberg · 2018-08-16T10:03:29Z

src/AddRecordDialog.cpp

+    ui->sqlTextEdit->setText(stmt);
+}
+
+void AddRecordDialog::itemChanged(QTreeWidgetItem *item, int column)


And one unused parameter warning for column here.

MKleusberg · 2018-08-16T10:13:51Z

src/MainWindow.cpp

@@ -696,7 +697,11 @@ void MainWindow::addRecord()
    {
        selectTableLine(row);
    } else {
-        QMessageBox::warning(this, QApplication::applicationName(), tr("Error adding record:\n") + db.lastError());


That's pretty clever replacing the error message by the dialog but trying the old approach first 😄

MKleusberg · 2018-08-16T10:15:35Z

src/AddRecordDialog.cpp

@@ -0,0 +1,247 @@
+#include "AddRecordDialog.h"
+#include "ui_AddRecordDialog.h"
+#include "sqlitetablemodel.h"


The "sqlitetablemodel.h" include isn't needed I guess.

MKleusberg · 2018-08-16T10:47:14Z

src/AddRecordDialog.cpp

+            if (isNumeric)
+                vals << value.toString();
+            else
+                vals << QString("'%1'").arg(value.toString());


This needs escaping for apostrophes in the value. So you'll need to add something like .replace("'", "''").

MKleusberg · 2018-08-16T10:51:12Z

src/AddRecordDialog.cpp

+    else if (button == ui->buttonBox->button(QDialogButtonBox::RestoreDefaults)) {
+        if (QMessageBox::warning(this,
+                                 QApplication::applicationName(),
+                                 tr("Are you sure you want to restore all the entered values to its defaults?"),


"their defaults". I think at least 😄

Yes, definitely.

MKleusberg · 2018-08-16T10:54:46Z

src/AddRecordDialog.ui

+              <string>Value</string>
+             </property>
+             <property name="toolTip">
+              <string>Default value</string>


Not sure about this tooltip because the column is not supposed to be about default values, it's only pre-filled with them. So maybe something like "Values to insert. Pre-filled default values are inserted automatically unless they are changed."

Honestly, I don't remember why I added this, so I've simply used your suggested text.

MKleusberg · 2018-08-16T10:57:15Z

src/AddRecordDialog.ui

+   <item>
+    <widget class="QDialogButtonBox" name="buttonBox">
+     <property name="whatsThis">
+      <string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;&lt;span style=&quot; font-weight:600;&quot;&gt;Save&lt;/span&gt; will submit the shown SQL statement to the database for inserting the new record.&lt;/p&gt;&lt;p&gt;&lt;span style=&quot; font-weight:600;&quot;&gt;Restore Defaults&lt;/span&gt; will restore the initial values in the &lt;span style=&quot; font-weight:600;&quot;&gt;Value&lt;/span&gt; column.&lt;/p&gt;&lt;p&gt;&lt;span style=&quot; font-weight:600;&quot;&gt;Cancel&lt;/span&gt; will close this dialog without executing the query.&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>


The button is "Defaults" on my system. But it's probably "Restore Defaults" (which I would prefer but whatever) on other platforms because Qt generally tries to mimic the style of the platform with these buttons. So not sure if we should change the tooltip here or not bother. Just wanted to point this out 😉

MKleusberg · 2018-08-16T10:59:42Z

src/AddRecordDialog.cpp

+        if (!defaultValue.isEmpty()) {
+            QFont font;
+            font.setItalic(true);
+            tbitem->setData(kValue, Qt::DisplayRole, f->defaultValue());


Just noticed that for string default values we're including the quotes here which isn't correct and a bit annoying too when you don't want to replace the default value but just modify it a bit. But that's not your fault, it's in the grammar parser. I'll take a look at it some time if I don't forget...

mgrojo · 2018-08-22T17:24:55Z

Thanks for the review, @MKleusberg

I've made several comments throughout the code. As always, don't be discouraged - it's just me writing down every thought I have wink And it's 99% very minor stuff smile

Don't worry. I appreciate your comments.

I have two more general suggestions:

I like the new dialog a lot and I can see how I might want to use it from time to time even though there's no constraint error, especially for large tables where you would have to scroll a lot to see the entire row. Maybe we can add a button or menu item for "voluntarily" invoking the new dialog. Not sure where to put it though.

Something like the new "Open Database" button, maybe?

I'd like to have a bit more control over the values I type in. One thing is that I can't insert a number as a string which is especially a problem for values like "012" or "1.00" which currently will be inserted as "12" and "1". The other thing is that I can't set a field to NULL or distinguish between empty string and NULL in any way. Again, not sure how to best add this.

A simple approach is to treat as string the columns of type TEXT even when the value is a number. At least this is probably an improvement over the current implementation.

            if (isNumeric && item->text(kType) != "TEXT")
                vals << value.toString();
            else
                vals << QString("'%1'").arg(value.toString().replace("'", "''"));

MKleusberg · 2018-08-23T11:48:48Z

I like the new dialog a lot and I can see how I might want to use it from time to time even though there's no constraint error, especially for large tables where you would have to scroll a lot to see the entire row. Maybe we can add a button or menu item for "voluntarily" invoking the new dialog. Not sure where to put it though.

Something like the new "Open Database" button, maybe?

Yep, that sounds good 😄

I'd like to have a bit more control over the values I type in. One thing is that I can't insert a number as a string which is especially a problem for values like "012" or "1.00" which currently will be inserted as "12" and "1". The other thing is that I can't set a field to NULL or distinguish between empty string and NULL in any way. Again, not sure how to best add this.

A simple approach is to treat as string the columns of type TEXT even when the value is a number. At least this is probably an improvement over the current implementation.
            if (isNumeric && item->text(kType) != "TEXT")
                vals << value.toString();
            else
                vals << QString("'%1'").arg(value.toString().replace("'", "''"));

Good idea as well! 😄 This way we can avoid more buttons and stuff and still do it right in the vast majority of cases. Maybe even add a rule for NULL values:

If the value is empty and the column is not NOT NULL (stupid double negation here), insert NULL.
If the value is numeric and the column type is numeric (we have Field::isInteger() for that which is better than item->text(kType) != "TEXT" because the latter treats BLOBs, VARCHARs, etc. as integers as well), don't quote the value.
Otherwise quote the value.

Display of NULL values using DisplayRole (no focus) or place holder text (when focus). Set value to NULL through a context menu and shortcut in the value line edit. Take text type affinity into account for quoting or not entered numbers. New isType and affinity functions in sqlitetypes. Clarify wording of constraints in tooltip for value. Added the same tooltip for the type. Escape quotes inside string values. Removed unused parameters warnings. Other wording or code improvements based on the pull-request review: #1477.

mgrojo · 2018-08-23T15:35:32Z

I've made a new commit with most of the suggestions. I plan to include also the button with the popup menu for the dialog or the in-line insertion.

I've included an option for setting to NULL a value similar to what we already have for the table or the cell editor. With this I would not include a rule for null values.

I've taken into account the text affinity (that will include VARCHAR and other variations) for quoting or not the numbers.

mgrojo · 2018-08-24T12:27:49Z

I'm unable to preserve the triggering of the action in the New Record button with a click if I add a popup menu. In the toolbar the Open Database and the Save SQL File in the Execute SQL tab the action can be triggered with a single click release and the popup menu with a delayed click (without releasing the mouse button). I think the difference is that the working cases are QActions in a QToolbar and the New Record button is a QPushButton. Qtcreator does not allow to add a QAction to the layout where the New Record button is, nor morphing the layout to a QToolBar. Any ideas for not having to make two clicks for adding a new record?

This is the look after a single click in the New Record button.

MKleusberg · 2018-08-24T13:15:29Z

It seems to work as expected if you change the QPushButton into a QToolButton:

You'll need to re-setup its connections though. Also even though they look the same on my system, it might be better to also change the delete button to a QToolButton - just to make sure the two look the same on all platforms.

mgrojo · 2018-08-24T14:38:47Z

Yes, you're right. I tried that, but didn't noticed that it was erasing the connections. The buttons don't look the same in my system if they are not of the same class, so I change both, as you've suggested. I'll make a new commit soon.

The Add Record dialog is now accessible for the user. The New Record button is converted to a QToolButton and a new pop-up menu is added to it for invoking the in-line table insertion (New Record) or the Add Record dialog (Insert Values...). What's This information for the button updated.

mgrojo · 2018-08-24T15:19:46Z

Now it's ready for final review and eventual merge.

MKleusberg · 2018-08-27T19:35:02Z

Cool, thanks @mgrojo! Let's merge this then 😄 I have to say I really like the new dialog 👍

mgrojo · 2018-08-27T21:25:11Z

Thanks, @MKleusberg. I'm glad you like it 👍

Fix build problem introduced in previous commit on this branch

b6f6496

mgrojo added 2 commits July 18, 2018 21:59

Visual improvements for the Add Record dialog

2ec7b51

QLineEdit as item delegate for the value, so it is more visible that we are supposed to edit the value. Remove last end-of-line in tool-tip.

MKleusberg reviewed Aug 16, 2018

View reviewed changes

MKleusberg mentioned this pull request Aug 16, 2018

Allow Insert/Update/Delete for Views on Browse Data tab #141

Closed

mgrojo added 2 commits August 24, 2018 16:58

Merge branch 'master' into add_record_dialog

32213c7

MKleusberg merged commit ce032d9 into master Aug 27, 2018

MKleusberg deleted the add_record_dialog branch August 27, 2018 19:35

MKleusberg added this to the 3.11.0 - Where we take over the world! :D milestone Aug 27, 2018

justinclift assigned mgrojo Aug 28, 2018

mgrojo mentioned this pull request Sep 2, 2018

bug re autoincrementing New Record #514

Closed

mgrojo mentioned this pull request Dec 1, 2018

Getting a FOREIGN KEY constraint failed issue #1638

Closed

mgrojo mentioned this pull request Apr 26, 2020

Unexpected window popup instead of direct grid entry #2213

Closed

11 tasks

Uh oh!

Issue #530: constraints on table prevents new record being added #1477

Issue #530: constraints on table prevents new record being added #1477

Uh oh!

Conversation

mgrojo commented Jul 15, 2018

Pending design decisions

Uh oh!

justinclift commented Jul 15, 2018

Uh oh!

mgrojo commented Jul 15, 2018

Uh oh!

mgrojo commented Jul 15, 2018

Uh oh!

justinclift commented Jul 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MKleusberg commented Jul 15, 2018

Uh oh!

mgrojo commented Jul 16, 2018

Uh oh!

justinclift commented Jul 16, 2018

Uh oh!

mgrojo commented Jul 16, 2018

Uh oh!

MKleusberg commented Aug 5, 2018

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!

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!

mgrojo commented Aug 22, 2018

Uh oh!

MKleusberg commented Aug 23, 2018

Uh oh!

mgrojo commented Aug 23, 2018

Uh oh!

mgrojo commented Aug 24, 2018

Uh oh!

MKleusberg commented Aug 24, 2018

Uh oh!

mgrojo commented Aug 24, 2018

Uh oh!

mgrojo commented Aug 24, 2018

Uh oh!

MKleusberg commented Aug 27, 2018

Uh oh!

mgrojo commented Aug 27, 2018

Uh oh!

Uh oh!

justinclift commented Jul 15, 2018 •

edited

Loading