Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
MKleusberg merged 4 commits into from
Jun 8, 2018
Merged

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

MKleusberg merged 4 commits into from
Jun 8, 2018

Conversation

@revolter
Copy link
Member

@revolter revolter commented Jun 2, 2018

No description provided.

@justinclift justinclift added the enhancement Feature requests. label Jun 2, 2018
@justinclift
Copy link
Member

justinclift commented Jun 2, 2018

Thanks @revolter, good thinking. 😄

@revolter revolter force-pushed the hotfix/page-size-validation branch from 989c1cc to 0afb750 Compare June 3, 2018 10:17
MKleusberg
MKleusberg reviewed Jun 7, 2018
View reviewed changes
src/PageSizeSpinBox.h Outdated
{
Q_OBJECT
public:
PageSizeSpinBox(QWidget* parent = Q_NULLPTR);
Copy link
Member

@MKleusberg MKleusberg Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@revolter revolter Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@MKleusberg MKleusberg Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

revolter commented Jun 7, 2018

@MKleusberg, Any idea how to fix the build?

MKleusberg
MKleusberg requested changes Jun 7, 2018
View reviewed changes
Copy link
Member

@MKleusberg MKleusberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

MKleusberg commented Jun 7, 2018

Haha, you were faster than me here 😆

@revolter
Copy link
Member Author

revolter commented Jun 7, 2018

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

@MKleusberg
Copy link
Member

MKleusberg commented Jun 7, 2018 •
edited by revolter
Loading

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
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 hotfix/page-size-validation branch from cb8752c to 44727ca Compare June 7, 2018 19:56
@revolter
Copy link
Member Author

revolter commented Jun 7, 2018 •
edited
Loading

@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*)'

😢

@revolter
Replace QSpinBox with QComboBox
9fb162e 
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
Copy link
Member Author

revolter commented Jun 7, 2018

screen shot 2018-06-08 at 00 57 53

@justinclift
Copy link
Member

justinclift commented Jun 7, 2018

That does look a lot better. 😄

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

justinclift commented Jun 7, 2018

Yep, looks good. 😄

@revolter
Copy link
Member Author

revolter commented Jun 7, 2018

And it also built without errors ❤️

@MKleusberg
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
@revolter
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 hotfix/page-size-validation branch June 8, 2018 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MKleusberg MKleusberg MKleusberg approved these changes

@justinclift justinclift Awaiting requested review from justinclift

Assignees

No one assigned

Labels

enhancement Feature requests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@revolter @justinclift @MKleusberg