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

Change settings checkbox texts to positive phrasing #4429

Closed

Conversation

ameyer0
Copy link
Contributor

@ameyer0 ameyer0 commented Mar 11, 2020

Type of change

  • ✅ New feature (non-breaking change which adds functionality)

Description and Context

Currently, some checkbox texts in the settings use negative phrasing so it is confusing what the checkbox actually does. This pull request changes the checkboxes to have positive phrasing, and reverses their effects to match.

Current text:

  • Don't mark database as modified for non-data changes
  • Don't require password repeat when it's visible
  • Don't hide passwords when editing them
  • Don't use placeholder for empty password fields

New text:

  • Mark database as modified for non-data changes
  • Require password repeat when it's visible
  • Hide passwords when editing them
  • Use placeholder for empty password fields

Variables related to these settings have been renamed to reflect the new behavior.

This is a non-breaking change because existing installations' configurations will be upgraded while preserving the behavior the user originally set.

Fixes #3601

Screenshots

Screenshot_20200311_150017
Screenshot_20200311_150207

Testing strategy

  • Manual testing in KDE for the GUI changes.
  • Added a new unit test testUpgrade to cover updating settings from an old config file.

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]
  • ✅ My change requires a change to the documentation, and I have updated it accordingly.
  • ✅ I have added tests to cover my changes.

ameyer0 added 3 commits March 11, 2020 13:49
* Change settings checkboxes with negative phrasing (i.e. "don't do
  something") to positive phrasing, and reverse their effects
  accordingly.
* Change internal names of affected settings to stay consistent with
  their effect.
* Add a test config file with all deprecated config settings, set to
  non-default values.
* Load the config file and check the updated versions of the deprecated
  config settings.
* Fail if the updated settings have different values than their
  deprecated equivalents.
Update old config files to use new versions of settings which had their
checkboxes in the settins page reversed, while retaining the user's
preferred behavior.
@droidmonkey droidmonkey added pr: refactoring Pull request that refactors code ux labels Mar 11, 2020
@droidmonkey
Copy link
Member

Great work on this!

@wolframroesler
Copy link
Contributor

Are you still working on this @ameyer0? If you don't have time, I could try and take care of these merge conflicts.

@ameyer0
Copy link
Contributor Author

ameyer0 commented Apr 8, 2020

@wolframroesler I've been meaning to get back to it, but it'll probably be at least a week until I have time. You're welcome to try and work on the conflicts in the meantime, thanks!

@wolframroesler
Copy link
Contributor

@ameyer0 Not sure what's the easiest way for me to contribute to your branch. Do I have to fork your fork of the repo and submit a PR to your branch? Maybe you have to grant me access (https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork)? Note that I'm not a maintainer of the keepassxc repo.

@droidmonkey
Copy link
Member

droidmonkey commented Apr 9, 2020

@wolframroesler it would be easiest to checkout his branch, make your changes, then push to your own fork. Then make a new PR. We can close this one and he still gets credit for the initial commit.

@wolframroesler
Copy link
Contributor

Merge conflicts solved, please have a look at #4574.

@ameyer0 ameyer0 deleted the feature/positive-checkbox-texts branch June 2, 2020 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: refactoring Pull request that refactors code ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Positive checkbox texts in application settings dialog
3 participants