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

Fix/database settings spin box bug #9101

Merged

Conversation

jNullj
Copy link
Contributor

@jNullj jNullj commented Feb 11, 2023

Found a bug while adding another feature at the Database Settings widget...
The database settings for Max items history and Max history size are SpinBox enabled when loaded from metadata as disabled.
Also for Max history size, the size is ridiculously high, it says MB but the value loaded from default is in Bytes.
This pull request fixes both minor UI issues.

Screenshots

Before the bug when opening a database where the checkbox is unchecked:

  • ❎ Default size in MB
  • ❎ Enable state of SpinBox
    image
    After the fix:
  • ✅ Default size in MB
  • ✅ Enable state of SpinBox
    image

Testing strategy

Tested on linux manually against latest release (archlinux).
Also run coverage with 100% pass

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)
  • ❎ New feature (change that adds functionality)
  • ❎ Breaking change (causes existing functionality to change)
  • ❎ Refactor (significant modification to existing code)
  • ❎ Documentation (non-code change)

The default value in the Metadata is saved in Bytes while the UI shows the size in MB.
Old code used the default in metadata as is, if database max size is unset the gui presents the default in bytes insted of MB which results in a huge number.
This commit fixes this by adding the calculation to transform the value to MB.
SpinBoxes for items with disabled checkbox should not be enabled.
If the user saves those settings as dissabled then reload the database the UI defaults to enable the SpinBox.
This commit fixes this by setting enable to false if the setting checkbox is not checked.
@jNullj
Copy link
Contributor Author

jNullj commented Feb 11, 2023

CI seems to fail at build job for ubuntu in one of the tests. but the test failed seems unrelated to the changes and all other builds and the test coverage seems to success...
Can you re-trigger the CI pipe?

src/gui/dbsettings/DatabaseSettingsWidgetGeneral.cpp Outdated Show resolved Hide resolved
@@ -50,6 +50,7 @@ void DatabaseSettingsWidgetGeneral::initialize()
m_ui->historyMaxItemsCheckBox->setChecked(true);
} else {
m_ui->historyMaxItemsSpinBox->setValue(Metadata::DefaultHistoryMaxItems);
m_ui->historyMaxItemsSpinBox->setEnabled(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it easy to test it in GUI tests?

Copy link
Member

Choose a reason for hiding this comment

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

doesn't seem useful, this is a very minor UI setting. I also could not replicate the original problem reported of the max history items spin box being enabled even though the checkbox was unchecked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't seem useful, this is a very minor UI setting. I also could not replicate the original problem reported of the max history items spin box being enabled even though the checkbox was unchecked.

To replicate you could make a new db, disable both settings then close keepassxc and relaunch into the settings. I can replicate 100% of the time on linux at current release.

I could still add the test altho i also think its a bit too much for such a minor bug. I could also see that this code is run in coverage already.
image

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh you have to restart the program, missing detail :-)

Values in DatabaseSettingsWidgetGeneral to convert from bytes to MB are written a more reabable way.
@droidmonkey droidmonkey merged commit 20e8e52 into keepassxreboot:develop Feb 12, 2023
pull bot pushed a commit to tigerwill90/keepassxc that referenced this pull request Feb 13, 2023
Co-authored-by: jNullj <jNullj@users.noreply.github.com>
droidmonkey pushed a commit that referenced this pull request Feb 18, 2023
Co-authored-by: jNullj <jNullj@users.noreply.github.com>
droidmonkey pushed a commit that referenced this pull request Feb 19, 2023
Co-authored-by: jNullj <jNullj@users.noreply.github.com>
droidmonkey pushed a commit that referenced this pull request Feb 19, 2023
Co-authored-by: jNullj <jNullj@users.noreply.github.com>
Perlover added a commit to Perlover/keepassxc that referenced this pull request May 18, 2023
Release 2.7.5

- Add menu option to allow screenshots [keepassxreboot#8841]
- Add support for Botan 3 [keepassxreboot#9388]
- Increase max TOTP step to 24 hours [keepassxreboot#9149]
- Improve HTML export layout [keepassxreboot#8987]
- Turn search reset off by default [keepassxreboot#9153]
- Use QClipboard::clear() instead of setting blank text [keepassxreboot#9148]
- Hide group column header choice when not in search [keepassxreboot#9171]
- Improve look of KeePassXC logo and icons [keepassxreboot#9355]
- Add keyboard shortcuts for app and database settings [keepassxreboot#9007]
- Hide rename button from attachments preview panel [keepassxreboot#8842]
- Linux: Set SingleMainWindow in .desktop file [keepassxreboot#7430]

- Fix crash when search clears while creating new entry [keepassxreboot#9230]
- Fix crash when using Windows Hello in a Remote Desktop session [keepassxreboot#9006]
- Fix crash in Group Edit after enabling Browser Integration [keepassxreboot#8778]
- Fix canceling quick unlock when it is unavailable [keepassxreboot#9034]
- Set password input field font correctly [keepassxreboot#8732]
- Greatly improve performance when rendering entry view [keepassxreboot#9398]
- Fix various accessibility issues [keepassxreboot#9138]
- Fix arrows size when expand/collapse a group [keepassxreboot#9096]
- Select the clone instead of the original after cloning an entry [keepassxreboot#9070]
- Fix bugs with preview widget [keepassxreboot#9170]
- Fix status bar update when switching to other DB [keepassxreboot#9073]
- Fix database settings spin box bug [keepassxreboot#9101]
- Fix Ctrl+Tab shortcut to cycle databases in unlock dialog [keepassxreboot#8839]
- Fix TOTP QR code maintaining square ratio [keepassxreboot#9027]
- Fix Auto-Type configuration page on custom sequence selection [keepassxreboot#8752]
- Fix unexpected behavior of `--lock` when KeePassXC is not running [keepassxreboot#8889]
- Make open folder icon exempt from "Apply group icon to entry" [keepassxreboot#9205]
- Allow setting default file open directory with env var [keepassxreboot#9192]
- SSH Agent: Fix support for AES-256/GCM openssh keys [keepassxreboot#8968]
- Browser: Fix Native Messaging script path with BSD OS's [keepassxreboot#8835]
- MacOS: Fix text selection for Auto-Type clear field [keepassxreboot#9066]
- MacOS: Don't rely on AppleInterfaceStyle for theme switching [keepassxreboot#8615]
- Windows: Remove registry detection of desktop shortcut [keepassxreboot#9380]
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