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

Table->Modify. No changes,direct cancel raises error #1547

Closed
pamtbaau opened this Issue Sep 28, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@pamtbaau
Copy link

pamtbaau commented Sep 28, 2018

Details for the issue

What did you do?

I open a well working db, right click on table and tap modify, or open modify via menu. Immediately tap Cancel. Error pops up.

What did you expect to see?

No error...

What did you see instead?

image

Useful extra information

The info below often helps, please fill it out if you're able to. :)

What operating system are you using?

  • Windows: ( version: 10 )
  • Linux: ( distro: ___ )
  • Mac OS: ( version: ___ )
  • Other: ___

What is your DB4S version?

  • 3.10.1
  • 3.10.0
  • 3.9.1
  • Other: Nightly
    Version 3.10.99 (Sep 27 2018)
    Qt Version 5.8.0
    SQLite Version 3.25.1

Did you also

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Sep 29, 2018

I cannot reproduce it with my database files. What are the different check options in the Tools menu saying about this database?

@pamtbaau

This comment has been minimized.

Copy link
Author

pamtbaau commented Sep 29, 2018

Ah... PRAGMA foreign_key_check; returned a tuple in one of my tables. Foreign Key on that tuple is indeed invalid.

Never used these checks in the Tools menu before...

Maybe showing the result of the foreign key check on 'Cancel' might clarify a lot to the user...

I guess the issue may be closed.

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Sep 29, 2018

I think it's better to keep it as an enhancement. You are right, it should say the user what happens. And maybe it should say it before trying to perform any change in the database, otherwise it will fail on Ok or Cancel, without being our fault.

mgrojo added a commit that referenced this issue Sep 29, 2018

Better messages for a table edition when foreign-key check detect pro…
…blems

The message when there are problems and the user has made changes and
accepted them must be different to the case when the user has cancelled
and any possible changes have been already reverted by the dialog.

In the first case the fault may be ours and we actually revert changes.
In the second case, we know for sure the problems were there before
editing the table. We warn user about it, since we already know.

The last error from the DB is inappropriate for both cases because it
hasn't been updated by the foreign-key check.

Also fixed the typo in a function identifier.

Room for improvement: maybe the check should be done before opening the
dialog, so we don't let the user edit the table until the issues have
been solved.

See issue #1547
@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Sep 29, 2018

I made a change. It can actually be considered a bug, because the message from the database engine that we were displaying wasn't actually from this check, but from other previous operation.

I've also put two different messages for the case of accepting changes or cancelling them. In the second case we know for sure that our changes are not the reason for the failing check, so we warn the user to fix the problem himself.

This table did not pass a foreign-key check.
You should run 'Tools | Foreign-Key Check' and fix the reported issues.

Have you a copy of the database for confirming tomorrow that it's working for you as well? Otherwise, if you're interested, you can disable foreign keys in the pragmas tab, make some insertion incoherent to the foreign keys, and then reenable the foreign key. Then you can reproduce the same situation.

There is also room for improvement: maybe the check should be done before opening the
dialog, so we don't let the user edit the table until the issues have
been solved. But I don't want to add a penalty for big databases by performing two checks, one before and other after; and I don't dare to change that now, since we plan to release soon and that may break anything if I overlook something.

@pamtbaau

This comment has been minimized.

Copy link
Author

pamtbaau commented Sep 30, 2018

I can confirm the new clarifying message on 'Cancel'. Good to the point message and call to action.
Thanks!

Without knowing, my database has been "corrupt" (albeit benign) for quite some time without knowing. The check tools weren't around last time I used SB, so these are quite good new features.

Would it be an idea to add some option for a 'health' check on database save. E.g. a popup asking whether a check should be run before saving? In preferences it could be set as 'never', 'always', 'ask'

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Sep 30, 2018

Would it be an idea to add some option for a 'health' check on database save. E.g. a popup asking whether a check should be run before saving? In preferences it could be set as 'never', 'always', 'ask'

Yes, it might be helpful. But we won't see it in the near time, since we want to close all the relevant pending issues and freeze the interface texts for translation. Would you mind opening an enhancement request with the idea so it is not forgotten?

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