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 - pass ifce, process mgmt #234

Merged
merged 3 commits into from Nov 23, 2016

Conversation

Projects
4 participants
@tezeb
Contributor

tezeb commented Nov 23, 2016

This is a first refactoring step to extract functionalities from MainWindow. It compiles and works(mostly?). I'm sharing to propose direction and gather feedback. It consist of 3 commits.
First one(e15c93f) is an interface "design" for "backend"(pass or (git/gpg...)). This was my main driver for refactoring.
Second commit(1e06efd) moves code from MainWindow to appropriate classes. I've tried keeping changes to minimum to ease reviewing at this stage. Unfortunately, as it showed up, I had to move (probably all) process management code into my interface as well. I think it shall be further separated.
Last commit contains localization updates.

As I've already mentioned next step is extraction of process mgmt into separate class. I also think that there is a huge potential for using signal/slots mechanism for MainWindow<->Pass Interface(process mgmt) communication.

And special thx goes to @YoshiMan for extracting QtPassSettings, which was a huge help!

@annejan

This comment has been minimized.

Member

annejan commented Nov 23, 2016

This sounds like exactly the kind of refactoring I was thinking about but never got the focus to do :)
Thank you very much, will look into the changes and provide feedback tomorrow.

@jounathaen

This comment has been minimized.

Member

jounathaen commented Nov 23, 2016

Great, that somebody starts this :-)
👍 👍 👍

@annejan annejan added the refactoring label Nov 23, 2016

@annejan annejan merged commit 2ff206a into IJHack:master Nov 23, 2016

2 of 4 checks passed

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

This comment has been minimized.

Member

annejan commented Nov 23, 2016

Moved the #ifdef __APPLE__

In the not so wise words of me 😈
Works on my machine .. merged!

@annejan

This comment has been minimized.

Member

annejan commented Nov 23, 2016

According to static analysis this change even removes a identical_branches "issue" in a moc_
https://scan.coverity.com/projects/ijhack-qtpass and reduces complexity a lot . . Thanks for the effort @tezeb !!

@YoshiMan

This comment has been minimized.

YoshiMan commented Nov 23, 2016

@tezeb awesome work. I love it. thumbs up

@tezeb

This comment has been minimized.

Contributor

tezeb commented Nov 24, 2016

wow, that was fast merge :) thx
Glad that you liked the changes.

@annejan regarding "housekeeping" commit, do you have some tool/config for fixing style?

@annejan

This comment has been minimized.

Member

annejan commented Nov 24, 2016

I have set up clang default standards in vim and QtCreator . .
To fix in commandline I use clang-format -i *.cpp *.h

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