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

Allow setting group icons to children groups/entries #3273

Merged
merged 4 commits into from Jun 19, 2019

Conversation

@mddrex
Copy link
Contributor

mddrex commented Jun 14, 2019

Set an icon for a group and automatically use it for subgroups and
subentries.

Type of change

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

Description and Context

Applying icons recursively can be helpful if the database owner follows a policy
in which entries always have the same icon as their parent group (which also is
the standard setting for new entries).
After reorganization of the database, the icons keep their old icon which was
the icon of their old parent.
To avoid setting the icon of each entry individually, a new checkbox is
introduced in the "Icon" category of the EditGroupWidget.
Since the EditIconWidget is also used by the EditEntryWidget, the checkbox
also appears there but checking the box has no effect in this case.

The ApplyGroupIconTo(Entry*) is adjusted to be more generally applicable. Until now, the method was intended to be only run on entry creation (config checks).

Fixes #3228
Fixes #1314
Fixes #2361

Screenshots

After reorganization of the entries, the icons of the entries are mingled
wildly.

Screenshot from 2019-06-13 22-49-19

To match the icons of the entries to their new parent group, open the edit mode
of the parent and select the icon.

Screenshot from 2019-06-13 22-50-37

To apply the icon recursively, check the box.
A message box requests confirmation to ensure that the option is checked
intentionally, to avoid accidental overwriting of icons.

Screenshot from 2019-06-13 22-50-54
Screenshot from 2019-06-13 22-51-05
Screenshot from 2019-06-13 22-51-13

Testing strategy

A new unit test is introduced in TestGroup which tests the new option with
a database which contains two nested subgroups in the root group.
Setting the icon by number as well as UUID is tested.

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. [x]

Possible alternative

Another possible method would be to introduce an option in the context menu of
selected entries.

Annotations

ASAN reports a memory leak if a custom icon is added or deleted (libglib).
This leak already existed in previous commits (and is not addressed by the
proposed changes).

Two new strings are introduced (checkbox and message box) and need to be
translated.

src/core/Group.cpp Outdated Show resolved Hide resolved
src/gui/EditWidgetIcons.cpp Outdated Show resolved Hide resolved
src/core/Group.cpp Outdated Show resolved Hide resolved
src/gui/EditWidgetIcons.cpp Outdated Show resolved Hide resolved
src/gui/EditWidgetIcons.cpp Outdated Show resolved Hide resolved
@mddrex

This comment has been minimized.

Copy link
Contributor Author

mddrex commented Jun 15, 2019

Thanks for your annotations. I tried to correct all of them.

I tried to prevent to blue folder icon being set to an entry on entry creation as requested. But if the user chooses the folder icon for an entry and decides to "Apply the icon to child entries" (or all children too), the blue folder icon will be set (as the user has requested it actively). I hope that this is ok.

Screenshot from 2019-06-15 16-07-18

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 15, 2019

Yes if the user explicitly selects the blue folder and asks to apply to everything then that should be allowed. In fact that fixes and issue we have!

Love the combo button!

src/gui/EditWidgetIcons.cpp Outdated Show resolved Hide resolved
src/gui/group/EditGroupWidget.cpp Show resolved Hide resolved
MuehlburgPhoenix and others added 4 commits Jun 10, 2019
- replace the checkbox by a menu button
- allow more options to apply icons (child groups, child entries)
- extend tests in TestGroup (applying icons for groups/entries only)
- do not use this-> in code touched by previous commit
- add missing EOF newlines
- prevent blue folder icon being set for entries (on entry creation only)
- do not use "recursively"
- make clear that also the icon of the edited group/entry will be set
- remove user confirmation prompt
* Do not show apply to button for entries
* Increase padding of button text
@droidmonkey droidmonkey added this to the v2.5.0 milestone Jun 19, 2019
@droidmonkey droidmonkey added the ux label Jun 19, 2019
@droidmonkey droidmonkey changed the title Set icons recursively Allow setting group icons to children groups/entries Jun 19, 2019
@droidmonkey droidmonkey merged commit bb8377a into keepassxreboot:develop Jun 19, 2019
4 checks passed
4 checks passed
CodeFactor No issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details
@affinityv

This comment has been minimized.

Copy link

affinityv commented Jun 22, 2019

I don't know if this is related, but I had some weird icon changes to the blue folder for ordinary entries; I've manually changed the icons for those random entries. I've used various versions over time, first noticed this issue with 2.4.3 though, but it may have been there already (if it was I don't know when it occurred exactly).

phoerious added a commit that referenced this pull request Oct 26, 2019
Added

- Add 'Paper Backup' aka 'Export to HTML file' to the 'Database' menu [[#3277](#3277)]
- Add statistics panel with information about the database (number of entries, number of unique passwords, etc.) to the Database Settings dialog [[#2034](#2034)]
- Add offline user manual accessible via the 'Help' menu [[#3274](#3274)]
- Add support for importing 1Password OpVault files [[#2292](#2292)]
- Implement Freedesktop.org secret storage DBus protocol so that KeePassXC can be used as a vault service by libsecret [[#2726](#2726)]
- Add support for OnlyKey as an alternative to YubiKeys (requires yubikey-personalization >= 1.20.0) [[#3352](#3352)]
- Add group sorting feature [[#3282](#3282)]
- Add feature to download favicons for all entries at once [[#3169](#3169)]
- Add word case option to passphrase generator [[#3172](#3172)]
- Add support for RFC6238-compliant TOTP hashes [[#2972](#2972)]
- Add UNIX man page for main program [[#3665](#3665)]
- Add 'Monospaced font' option to the notes field [[#3321](#3321)]
- Add support for key files in auto open [[#3504](#3504)]
- Add search field for filtering entries in Auto-Type dialog [[#2955](#2955)]
- Complete usernames based on known usernames from other entries [[#3300](#3300)]
- Parse hyperlinks in the notes field of the entry preview pane [[#3596](#3596)]
- Allow abbreviation of field names in entry search [[#3440](#3440)]
- Allow setting group icons recursively [[#3273](#3273)]
- Add copy context menu for username and password in Auto-Type dialog [[#3038](#3038)]
- Drop to background after copying a password to the clipboard [[#3253](#3253)]
- Add 'Lock databases' entry to tray icon menu [[#2896](#2896)]
- Add option to minimize window after unlocking [[#3439](#3439)]
- Add option to minimize window after opening a URL [[#3302](#3302)]
- Request accessibility permissions for Auto-Type on macOS [[#3624](#3624)]
- Browser: Add initial support for multiple URLs [[#3558](#3558)]
- Browser: Add entry-specific browser integration settings [[#3444](#3444)]
- CLI: Add offline HIBP checker (requires a downloaded HIBP dump) [[#2707](#2707)]
- CLI: Add 'flatten' option to the 'ls' command [[#3276](#3276)]
- CLI: Add password generation options to `Add` and `Edit` commands [[#3275](#3275)]
- CLI: Add XML import [[#3572](#3572)]
- CLI: Add CSV export to the 'export' command [[#3278](#3278)]
- CLI: Add `-y --yubikey` option for YubiKey [[#3416](#3416)]
- CLI: Add `--dry-run` option for merging databases [[#3254](#3254)]
- CLI: Add group commands (mv, mkdir and rmdir) [[#3313](#3313)].
- CLI: Add interactive shell mode command `open` [[#3224](#3224)]

Changed

- Redesign database unlock dialog [ [#3287](#3287)]
- Rework the entry preview panel [ [#3306](#3306)]
- Move notes to General tab on Group Preview Panel [[#3336](#3336)]
- Enable entry actions when editing an entry and cleanup entry context menu  [[#3641](#3641)]
- Improve detection of external database changes  [[#2389](#2389)]
- Warn if user is trying to use a KDBX file as a key file [[#3625](#3625)]
- Add option to disable KeePassHTTP settings migrations prompt [[#3349](#3349), [#3344](#3344)]
- Re-enabled Wayland support (no Auto-Type yet) [[#3520](#3520), [#3341](#3341)]
- Add icon to 'Toggle Window' action in tray icon menu [[3244](#3244)]
- Merge custom data between databases only when necessary [[#3475](#3475)]
- Improve various file-handling related issues when picking files using the system's file dialog [[#3473](#3473)]
- Add 'New Entry' context menu when no entries are selected [[#3671](#3671)]
- Reduce default Argon2 settings from 128 MiB and one thread per CPU core to 64 MiB and two threads to account for lower-spec mobile hardware [ [#3672](#3672)]
- Browser: Remove unused 'Remember' checkbox for HTTP Basic Auth [[#3371](#3371)]
- Browser: Show database name when pairing with a new browser [[#3638](#3638)]
- Browser: Show URL in allow access dialog [[#3639](#3639)]
- CLI: The password length option `-l` for the CLI commands `Add` and `Edit` is now `-L` [[#3275](#3275)]
- CLI: The `-u` shorthand for the `--upper` password generation option has been renamed to `-U` [[#3275](#3275)]
- CLI: Rename command `extract` to `export`. [[#3277](#3277)]

Fixed

- Improve accessibility for assistive technologies [[#3409](#3409)]
- Correctly unlock all databases if `--pw-stdin` is provided [[#2916](#2916)]
- Fix password generator issues with special characters [[#3303](#3303)]
- Fix KeePassXC interrupting shutdown procedure [[#3666](#3666)]
- Fix password visibility toggle button state on unlock dialog [[#3312](#3312)]
- Fix potential data loss if database is reloaded while user is editing an entry [[#3656](#3656)]
- Fix hard-coded background color in search help popup [[#3001](#3001)]
- Fix font choice for password preview [[#3425](#3425)]
- Fix handling of read-only files when autosave is enabled [[#3408](#3408)]
- Handle symlinks correctly when atomic saves are disabled [[#3463](#3463)]
- Enable HighDPI icon scaling on Linux [[#3332](#3332)]
- Make Auto-Type on macOS more robust and remove old Carbon API calls [[#3634](#3634), [[#3347](#3347))]
- Hide Share tab if KeePassXC is compiled without KeeShare support and other minor KeeShare improvements [[#3654](#3654), [[#3291](#3291), [#3029](#3029), [#3031](#3031), [#3236](#3236)]
- Correctly bring window to the front when clicking tray icon on macOS [[#3576](#3576)]
- Correct application shortcut created by MSI Installer on Windows [[#3296](#3296)]
- Fix crash when removing custom data [[#3508](#3508)]
- Fix placeholder resolution in URLs [[#3281](#3281)]
- Fix various inconsistencies and platform-dependent compilation bugs [[#3664](#3664), [#3662](#3662), [#3660](#3660), [#3655](#3655), [#3649](#3649), [#3417](#3417), [#3357](#3357), [#3319](#3319), [#3318](#3318), [#3304](#3304)]
- Browser: Fix potential leaking of entries through the browser integration API if multiple databases are opened [[#3480](#3480)]
- Browser: Fix password entropy calculation [[#3107](#3107)]
- Browser: Fix Windows registry settings for portable installation [[#3603](#3603)]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.