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

refactoring, new QtPassSettings class, all settings should be read and written here #224

Merged
merged 37 commits into from Nov 20, 2016

Conversation

Projects
2 participants
@YoshiMan

YoshiMan commented Nov 20, 2016

I refactored the mainwindow.
Now we have a QtPassSeetings class, where you can read the settings in a static way.
e.g. QtPassSettings::getVersion(), you will get the version as int or "0" as default fallback
or QtPassSettings::getVersion(4), you will get version as int or "4" as fallback

hope you like it.
can you please check for memory leaks or other mistakes i made.

Janosch Knack and others added some commits Nov 17, 2016

@annejan annejan added the refactoring label Nov 20, 2016

@annejan annejan merged commit 7e288a6 into IJHack:master Nov 20, 2016

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Snap CI The Snap CI build passed on Nov 20, 2016!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linthub Linthub has cleared the pull request, no code style suggestions found.
Details
@annejan

This comment has been minimized.

Member

annejan commented Nov 20, 2016

Thanks for this big change of the configuration model.
This makes a lot of things way more readable and probably stable too.

I'll boot up a Windows VM and fix the windows errors.
After that I'll do some thorough testing and checking.

@YoshiMan

This comment has been minimized.

YoshiMan commented Nov 20, 2016

yeah that was a big change. took me some time ^^
Additional checks should be made through version changes. especially the useClipboard config.
If you fresh install, everything should work. But i dont know what happens, when you have an older version.

@YoshiMan

This comment has been minimized.

YoshiMan commented Nov 20, 2016

@annejan. how was your testing?

@annejan

This comment has been minimized.

Member

annejan commented Nov 20, 2016

So far so good, both "scenario driven" testing and static analysis haven't found any regression.

I was also triggered to do some other minor fixes in other places that I had postponed out of lack of inspiration.

Great work @YoshiMan thanks for the effort!

The clipboard settings seem to work fine for existing users too . .

@YoshiMan

This comment has been minimized.

YoshiMan commented Nov 20, 2016

Your welcome. QtPass is a realy nice tool.
What is "senario driven" testing?
And can we do some thing like unit-testing?
I would appreciate some unit-tests. :)

@annejan

This comment has been minimized.

Member

annejan commented Nov 20, 2016

Scenario driven in this case is just a lot of different configs I can easily switch out and a big set of different password files (some broken on purpose) . .

I would love to add unit testing and have looked into it a bit a couple of months ago but could not find a good starting point.
I've been working test driven on a couple of projects recently and really like the way of working from idea to test to code and seeing the tests clear one by one . .

Qt has a framework for unit tests http://doc.qt.io/qt-5/qtest-overview.html although I have to admit I have no experience with it yet.
If I can find the time the coming days I'll try and set up some minimal tests and hook it up in the CI flow of the project.
If you have any ideas or even some working tests feel free to create a pull request . .

@YoshiMan

This comment has been minimized.

YoshiMan commented Nov 20, 2016

Alright, so you are doing some "manually unit-testing" :D
I'll have look at the testing framework, too.
I don't have any experience with cpp or Qt. So it is important that someone reviews my code and that there are some unit tests :)

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