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, fixed merge conflicts #4574

Conversation

wolframroesler
Copy link
Contributor

This PR fixes the merge conflicts in #4429, which in turn implements #3601. Credits for the actual work go to @ameyer0, I only merged develop into it and fixed the merge conflicts. Hopefully I didn't break anything; please have a good look at my changes, @ameyer0 and @droidmonkey!

I'm getting the following error in the unit tests:

$ tests/testcli -silent
Testing TestCli
FAIL!  : TestCli::testClip() Compared values are not the same
   Actual   (clipboard->text())  : ""
   Expected (QString("Password")): "Password"
   Loc: [/home/wolfram/src/keepassxc/tests/TestCli.cpp(485)]
Totals: 60 passed, 1 failed, 1 skipped, 0 blacklisted, 10674ms

but I'm not sure if it's related to the changes on this branch, could be something's wrong on my computer. Didn't check if the same error occurs on the original branch or on develop.

@droidmonkey
Copy link
Member

The clipboard test fails occasionally

@wolframroesler
Copy link
Contributor Author

Is there anything left for me to do here?

ameyer0 and others added 4 commits April 22, 2020 14:25
* 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 force-pushed the feature/positive-checkbox-texts branch from 757ae0f to 616f779 Compare April 22, 2020 20:47
@wolframroesler
Copy link
Contributor Author

OMG can anyone help with the new merge conflicts? @phoerious maybe? Seems like there have been substantial changes in the way options are handled.

@phoerious
Copy link
Member

Yeah, basically everything. Use the new enum config interface instead of strings.

@droidmonkey
Copy link
Member

We still want this PR though, it makes a lot of sense

@wolframroesler
Copy link
Contributor Author

Seems like a re-write of the original changes is required, about which I don't know enough to be helpful here. Can you have a look, @ameyer0?

@ameyer0
Copy link
Contributor

ameyer0 commented May 7, 2020

Yeah, I'll give it a try, hopefully have something in the next day or two.

@phoerious
Copy link
Member

It should be rather straight-forward. All settings have an equivalent enum key now. If they don't you have to define a new one in core/Config.h and map it in core/Config.cpp.

The CLI test failure has been resolved on develop in the meantime.

@ameyer0
Copy link
Contributor

ameyer0 commented May 8, 2020

Merge conflicts are fixed in #4715.

@wolframroesler
Copy link
Contributor Author

Awesome, thanks.

@wolframroesler
Copy link
Contributor Author

PR closed, use #4715 instead.

@droidmonkey droidmonkey removed this from the v2.6.0 milestone Jul 6, 2020
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.

4 participants