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

Make sure only powers of two are entered for the page size #1405

Merged
merged 4 commits into from Jun 8, 2018

Conversation

Projects
None yet
3 participants
@revolter
Copy link
Member

revolter commented Jun 2, 2018

No description provided.

@revolter revolter requested review from MKleusberg and justinclift Jun 2, 2018

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jun 2, 2018

Thanks @revolter, good thinking. 😄

@revolter revolter force-pushed the revolter:hotfix/page-size-validation branch from 989c1cc to 0afb750 Jun 3, 2018

{
Q_OBJECT
public:
PageSizeSpinBox(QWidget* parent = Q_NULLPTR);

This comment has been minimized.

@MKleusberg

MKleusberg Jun 7, 2018

Member

I'd prefer a nullptr here instead of the Qt specific macro.

This comment has been minimized.

@revolter

revolter Jun 7, 2018

Author Member

I think it added it like this by default, but I'm not sure.

This comment has been minimized.

@MKleusberg

MKleusberg Jun 7, 2018

Member

Yeah, it does that. I think the default code snippets that are generated are made to work on all platforms and settings. But since our code only build with C++11 anyway we can use nullptr here.

@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 7, 2018

@MKleusberg, Any idea how to fix the build?

@MKleusberg
Copy link
Member

MKleusberg left a comment

Looks good to me 😄 There are two problems to be fixed however.

  1. The spinbox still allows you to enter invalid values manually. Maybe add a slot in PageSizeSpinBox here and connect it to the valueChanged() signal, then check the new value there and fix it if it's invalid. Not sure if that's the best approach but it's the first one I could come up with 😉

  2. The build still fails because Travis is using cmake. So you'll need to add the two new files to the CMakeLists.txt file, too 😄

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Jun 7, 2018

Haha, you were faster than me here 😆

@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 7, 2018

Could we somehow automate this required add of new files to the CMakeLists.txt?

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Jun 7, 2018

Hmmm, I don't think so. Qt Creator updates the src.pro file automatically if that's the project file you have opened. It can also open cmake projects somehow (never tried it though to be honest) and will update the CMakeLists.txt file automatically then but not the src.pro file anymore. So one of them always needs to be adjusted manually I guess.

I always did that manually so far because it doesn't take long at all. You'll just need to add src/PageSizeSpinBox.h to the list starting with set(SQLB_MOC_HDR and src/PageSizeSpinBox.cpp to the list starting with set(SQLB_SRC.

@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 7, 2018

The spinbox still allows you to enter invalid values manually.

Actually, it's not. If you deselect the input field, it jumps to the last valid value, and if you directly click OK it passes the same last valid value. But I can see how this could be confusing.

@revolter revolter force-pushed the revolter:hotfix/page-size-validation branch from cb8752c to 44727ca Jun 7, 2018

@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 7, 2018

@MKleusberg,

CMakeFiles/test-sqlobjects.dir/__/CipherDialog.cpp.o: In function `Ui_CipherDialog::setupUi(QDialog*)':
CipherDialog.cpp:(.text._ZN15Ui_CipherDialog7setupUiEP7QDialog[_ZN15Ui_CipherDialog7setupUiEP7QDialog]+0x41b): undefined reference to `PageSizeSpinBox::PageSizeSpinBox(QWidget*)'

😢

Replace QSpinBox with QComboBox
Having a QSpinBox didn't make too much sense when we only have 8 valid
values. Forcing the user to type a valid value would have required a
warning message too, along with translations.

Having a QComboBox makes it clear (obviously) what values we are
expecting, without any risk of invalid values or confusion.
@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 7, 2018

screen shot 2018-06-08 at 00 57 53

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jun 7, 2018

That does look a lot better. 😄

@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 7, 2018

And with thousands separator:

screen shot 2018-06-08 at 01 01 08

It might show with . instead of , for others.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jun 7, 2018

Yep, looks good. 😄

@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 7, 2018

And it also built without errors ❤️

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Jun 8, 2018

Funny that we didn't think of this before - makes actually a lot more sense this way 😄 I'll go ahead and merge this then. Thanks, Iulian!

@MKleusberg MKleusberg merged commit 9c2cec6 into sqlitebrowser:master Jun 8, 2018

1 check passed

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

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 8, 2018

Yeah, I know. I was thinking of better and better ways to validate the input when I found https://stackoverflow.com/questions/12616120/validation-in-qspinbox#comment41015189_20702720.

@revolter revolter deleted the revolter:hotfix/page-size-validation branch Jun 8, 2018

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