New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from Aug 27, 2018

Conversation

Projects
None yet
3 participants
@mgrojo
Copy link
Contributor

mgrojo commented Jul 15, 2018

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.
Issue #530: constraints on table prevents new record being added
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

This comment has been minimized.

Copy link
Member

justinclift commented Jul 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Jul 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Jul 15, 2018

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

This comment has been minimized.

Copy link
Member

justinclift commented Jul 15, 2018

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

This comment has been minimized.

Copy link
Member

MKleusberg commented Jul 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Jul 16, 2018

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

This comment has been minimized.

Copy link
Member

justinclift commented Jul 16, 2018

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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Jul 16, 2018

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 😄

mgrojo added some commits Jul 18, 2018

Dialog as fallback for failure after empty row insertion and read onl…
…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.
Visual improvements for the Add Record dialog
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

This comment has been minimized.

Copy link
Member

MKleusberg commented Aug 5, 2018

@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
Copy link
Member

MKleusberg left a comment

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:

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

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

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

This comment has been minimized.

@MKleusberg

MKleusberg Aug 16, 2018

Member

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

This comment has been minimized.

@mgrojo

mgrojo Aug 22, 2018

Author Contributor

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?

This comment has been minimized.

@MKleusberg

MKleusberg Aug 23, 2018

Member

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);
class EditDelegate: public QStyledItemDelegate {
public:
EditDelegate(QObject* parent=nullptr): QStyledItemDelegate(parent) {}
virtual QWidget* createEditor(QWidget *parent, const QStyleOptionViewItem &option, const QModelIndex &index) const {

This comment has been minimized.

@MKleusberg

MKleusberg Aug 16, 2018

Member

Two more unused parameter warnings here.

ui->sqlTextEdit->setText(stmt);
}

void AddRecordDialog::itemChanged(QTreeWidgetItem *item, int column)

This comment has been minimized.

@MKleusberg

MKleusberg Aug 16, 2018

Member

And one unused parameter warning for column here.

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

This comment has been minimized.

@MKleusberg

MKleusberg Aug 16, 2018

Member

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

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

This comment has been minimized.

@MKleusberg

MKleusberg Aug 16, 2018

Member

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

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

This comment has been minimized.

@MKleusberg

MKleusberg Aug 16, 2018

Member

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

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?"),

This comment has been minimized.

@MKleusberg

MKleusberg Aug 16, 2018

Member

"their defaults". I think at least 😄

This comment has been minimized.

@mgrojo

mgrojo Aug 22, 2018

Author Contributor

Yes, definitely.

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

This comment has been minimized.

@MKleusberg

MKleusberg Aug 16, 2018

Member

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

This comment has been minimized.

@mgrojo

mgrojo Aug 22, 2018

Author Contributor

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

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

This comment has been minimized.

@MKleusberg

MKleusberg Aug 16, 2018

Member

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 😉

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

This comment has been minimized.

@MKleusberg

MKleusberg Aug 16, 2018

Member

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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Aug 22, 2018

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:

  1. 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?

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

This comment has been minimized.

Copy link
Member

MKleusberg commented Aug 23, 2018

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

  1. 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.
Improvements in the "Add Record" dialog
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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Aug 23, 2018

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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Aug 24, 2018

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

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Aug 24, 2018

It seems to work as expected if you change the QPushButton into a QToolButton:
screenshot_20180824_151059
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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Aug 24, 2018

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.

mgrojo added some commits Aug 24, 2018

User access to the Add Record dialog
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

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Aug 24, 2018

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

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Aug 27, 2018

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

@MKleusberg MKleusberg merged commit ce032d9 into master Aug 27, 2018

1 check passed

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

@MKleusberg MKleusberg deleted the add_record_dialog branch Aug 27, 2018

@mgrojo

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Aug 27, 2018

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

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