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

Add compression options to database settings #1419

Merged
merged 2 commits into from
Jan 24, 2018
Merged

Conversation

phoerious
Copy link
Member

Description

This patch adds a radio button group for choosing between GZip and no compression to the database settings.

Motivation and context

There is not really a reason not to enable compression, but KeePass2 has a setting for this and KeePassXC used to just save with whatever compression setting the database was originally opened with. That way you probably wouldn't notice if compression is (accidentally?) turned off and even if you do, you couldn't change it without downloading KeePass2.

How has this been tested?

By saving and opening with and without compression in both KeePassXC and KeePass2.

Screenshots (if appropriate):

capture

Types of changes

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

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]

@phoerious phoerious added this to the v2.3.0 milestone Jan 23, 2018
@phoerious phoerious requested a review from a team January 23, 2018 20:10
@droidmonkey
Copy link
Member

I dont understand why you would NOT want to compress your database. Its silly to have a choice, but I understand that keepass2 has it.

@phoerious
Copy link
Member Author

phoerious commented Jan 23, 2018

If KeePass2 didn't have it, I wouldn't have bothered adding an option and probably just changed the saving procedure to always use compression. But right now we just keep whatever the DB was originally saved with, which is the worst of all options, because if your DB is uncompressed, you have no way to change it.

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.

Button title change requested

<item>
<widget class="QRadioButton" name="compressionGZip">
<property name="text">
<string>GZip (recommended)</string>
Copy link
Member

Choose a reason for hiding this comment

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

Recommend rename this to "Compressed (GZip)". Its recommended by virtue of being defaulted to compressed.

Copy link
Member Author

@phoerious phoerious Jan 23, 2018

Choose a reason for hiding this comment

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

I don't think that goes well with the context. "compressed" is not a "compression" (which is what the group box is called). Also the other option would have to be "uncompressed" then, which is not a compression method either and looks too similar to "compressed" IMHO.

Copy link
Member

@droidmonkey droidmonkey Jan 23, 2018

Choose a reason for hiding this comment

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

I only say that because most users (especially on windows) have no idea what gzip is. After teaching my mother to use this software I realized we have to be careful about being too complex.

Perhaps we can just reduce this to a simple checkbox that says "Enable Compression (recommended)". Gzip is implied in the backend, if keepass2 enables support for another algorithm (lza) then we can add an algorithm combo box.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but on the other hand it's the wording KeePass uses. I don't really think it matters here how we call it, non-technical users will probably not understand it in any case and simply leave it as is if it is "recommended".

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually thinking about adding another algorithm, but the only thing we have in code is zlib. Everything else would have required an external lib.

Copy link
Contributor

@adolfogc adolfogc Jan 24, 2018

Choose a reason for hiding this comment

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

@phoerious Other algorithms may be supported by using KF5Archive.

Edit: reference: https://api.kde.org/frameworks/karchive/html/classKCompressionDevice.html

@@ -168,6 +171,8 @@ void DatabaseSettingsWidget::save()
}
}

Copy link
Member

Choose a reason for hiding this comment

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

I know this is a simple implementation of a radio button set, but it's not scalable and we really should be wrapping the buttons in a QButtonGroup and calling checkedButton()

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no QButtonGroup widget in the designer. I could add a generic QWidget and promote it, but I don't really see the benefit. Button groups work with any parent widget and all the QButtonGroup is giving us is the index of the selected button, but not a way to set the active button, so it's not really making things easier. The documentation for QButtonGroup also suggests using a QGroupBox as a container with a visual representation.

Copy link
Member

Choose a reason for hiding this comment

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

I was surprised to see the QGroupBox did not inherit from QButtonGroup...

Copy link
Member Author

Choose a reason for hiding this comment

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

As I said, exclusive buttons are not specific to QButtonGroup.

@phoerious phoerious force-pushed the feature/compression branch from 26a0b05 to 75d2895 Compare January 23, 2018 23:21
@phoerious
Copy link
Member Author

phoerious commented Jan 23, 2018

I changed it to a single checkbox. I like that option, although the group box is a little redundant now (but the checkbox doesn't align well with the other options without it).

capture

@phoerious
Copy link
Member Author

Comparison screenshot with the old 2.2.4 settings dialog. Just for the lulz. 😁

capture

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.

Love it!

@phoerious phoerious merged commit 6e25003 into develop Jan 24, 2018
@phoerious phoerious deleted the feature/compression branch January 24, 2018 09:18
phoerious added a commit that referenced this pull request Feb 27, 2018
- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494]
- Add SSH Agent feature [#1098, #1450, #1463]
- Add preview panel with details of the selected entry [#879, #1338]
- Add more and configurable columns to entry table and allow copying of values by double click [#1305]
- Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608]
- Deprecate KeePassHTTP [#1392]
- Add support for Steam one-time passwords [#1206]
- Add support for multiple Auto-Type sequences for a single entry [#1390]
- Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060]
- Replace qHttp with cURL for website icon downloads [#1460]
- Remove lock file [#1231]
- Add option to create backup file before saving [#1385]
- Ask to save a generated password before closing the entry password generator [#1499]
- Resolve placeholders recursively [#1078]
- Add Auto-Type button to the toolbar [#1056]
- Improve window focus handling for Auto-Type dialogs [#1204, #1490]
- Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412]
- Add optional dark tray icon [#1154]
- Add new "Unsafe saving" option to work around saving problems with file sync services [#1385]
- Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537]
- Add diceware password generator to CLI [#1406]
- Add --key-file option to CLI [#816, #824]
- Add DBus interface for opening and closing KeePassXC databases [#283]
- Add KDBX compression options to database settings [#1419]
- Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327]
- Correct reference resolution in entry fields [#1486]
- Fix window state and recent databases not being remembered on exit [#1453]
- Correct history item generation when configuring TOTP for an entry [#1446]
- Correct multiple TOTP bugs [#1414]
- Automatic saving after every change is now a default [#279]
- Allow creation of new entries during search [#1398]
- Correct menu issues on macOS [#1335]
- Allow compilation on OpenBSD [#1328]
- Improve entry attachments view [#1139, #1298]
- Fix auto lock for Gnome and Xfce [#910, #1249]
- Don't remember key files in file dialogs when the setting is disabled [#1188]
- Improve database merging and conflict resolution [#807, #1165]
- Fix macOS pasteboard issues [#1202]
- Improve startup times on some platforms [#1205]
- Hide the notes field by default [#1124]
- Toggle main window by clicking tray icon with the middle mouse button [#992]
- Fix custom icons not copied over when databases are merged [#1008]
- Allow use of DEL key to delete entries [#914]
- Correct intermittent crash due to stale history items [#1527]
- Sanitize newline characters in title, username and URL fields [#1502]
- Reopen previously opened databases in correct order [#774]
- Use system's zxcvbn library if available [#701]
- Implement various i18n improvements [#690, #875, #1436]
@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
file format pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants