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

Disable merge button when db is locked. #1881

Merged
merged 2 commits into from
Jul 8, 2018

Conversation

louib
Copy link
Member

@louib louib commented Apr 28, 2018

Related to #1875

Description

This patch disables the merge button when the current database is locked.

Motivation and context

Attempting to merge the database when the db is locked can cause database corruption, as stated in #1875

How has this been tested?

Locally. Had a hard time writing unit tests for the GUI.

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@louib louib added the bug label Apr 28, 2018
@louib louib requested review from droidmonkey and a team and removed request for droidmonkey April 28, 2018 01:33
Copy link
Member

@droidmonkey droidmonkey 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, one comment

bool inDatabaseTabWidget = (currentIndex == DatabaseTabScreen);
bool inWelcomeWidget = (currentIndex == WelcomeScreen);
bool inDatabaseTabWidgetOrWelcomeWidget = inDatabaseTabWidget || inWelcomeWidget;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this condition always true?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we have 4 option for the currentIndex

enum StackedWidgetIndex

@louib
Copy link
Member Author

louib commented Apr 30, 2018

@droidmonkey I managed to add a unit test after all!

Copy link
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

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

Nice work!

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Jul 3, 2018

@louib can you rebase this?

@louib
Copy link
Member Author

louib commented Jul 4, 2018

@TheZ3ro will do!

@louib
Copy link
Member Author

louib commented Jul 4, 2018

@TheZ3ro looks like it's already up-to-date with develop. Did you want this on another branch?

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Jul 5, 2018

@louib github tells me that This branch is out-of-date with the base branch

@droidmonkey droidmonkey merged commit d06819e into keepassxreboot:develop Jul 8, 2018
@phoerious phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: bugfix Pull request that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants