Skip to content
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

Sorry for last error :) #370

Merged
merged 5 commits into from Apr 26, 2018
Merged

Sorry for last error :) #370

merged 5 commits into from Apr 26, 2018

Conversation

FiloSpaTeam
Copy link
Contributor

I changed some things.

  • Now there is a real ToolBar more adaptable to GNU/Linux DE and Mac OSx.
  • Update IT translations
  • Some refactor about QtPassSettings Singleton (i missed some instances 😁 )
  • Now Git Pull and Git Push Button are always visible - as suggested from GNOME 3 Guidelines, users prefers to have buttons visible and disable than hide/show.

@annejan
Copy link
Member

annejan commented Apr 26, 2018

No worries, that's why we have automated testing 😁

@annejan annejan self-requested a review April 26, 2018 09:53
@FiloSpaTeam
Copy link
Contributor Author

Oh, if you prefer to have TextBesideIcon change the code. Actually i set to FollowStyle (ToolBar icons)

@coveralls
Copy link

coveralls commented Apr 26, 2018

Coverage Status

Coverage increased (+0.06%) to 6.217% when pulling ea60d28 on UnitooTeam:master into fef6236 on IJHack:master.

@annejan
Copy link
Member

annejan commented Apr 26, 2018

I think I prefer FollowStyle since it's more coherent.

@FiloSpaTeam
Copy link
Contributor Author

Ok, perfect. Another thing. Why Travis CI build failed? I can't understand 😆

@annejan
Copy link
Member

annejan commented Apr 26, 2018

Restarted the job, Travis has glitches sometimes . .
Looks like it timed-out installing a prerequisites.

@codecov
Copy link

codecov bot commented Apr 26, 2018

Codecov Report

Merging #370 into master will increase coverage by 0.05%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #370      +/-   ##
=========================================
+ Coverage     6.2%   6.25%   +0.05%     
=========================================
  Files          39      39              
  Lines        2706    2684      -22     
  Branches      348     347       -1     
=========================================
  Hits          168     168              
+ Misses       2532    2510      -22     
  Partials        6       6
Impacted Files Coverage Δ
src/mainwindow.h 0% <ø> (ø) ⬆️
src/qtpasssettings.h 0% <ø> (ø) ⬆️
src/mainwindow.cpp 0% <0%> (ø) ⬆️
src/passworddialog.cpp 59.18% <0%> (ø) ⬆️
src/qtpasssettings.cpp 0.92% <0%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fef6236...ea60d28. Read the comment docs.

@FiloSpaTeam
Copy link
Contributor Author

Ok understood! Thanks.
If you want some changes tell me 👍

Copy link
Member

@annejan annejan left a comment

Choose a reason for hiding this comment

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

Just those commented out lines, the rest looks perfect to me.

//}
// void QtPassSettings::setSplitterRight(const int &splitterRight) {
// getInstance()->setValue(SettingsConstants::splitterRight, splitterRight);
//}
Copy link
Member

Choose a reason for hiding this comment

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

If you could remove these (instead of commenting out) that would be wonderful.

static int getSplitterRight(const int &defaultValue = QVariant().toInt());
static void setSplitterRight(const int &splitterRight);
// static int getSplitterRight(const int &defaultValue = QVariant().toInt());
// static void setSplitterRight(const int &splitterRight);
Copy link
Member

Choose a reason for hiding this comment

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

Same with these commented out lines.

@annejan annejan merged commit a9422ba into IJHack:master Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants