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

Improve path handling when picking files with file dialogs #3473

Merged
merged 1 commit into from Aug 31, 2019

Conversation

@brainplot
Copy link
Contributor

brainplot commented Aug 24, 2019

This PR aims at the portions of code that handle picking files from the file systems.
It also improves the overall code style of those portions.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Refactor (significant modification to existing code)

Description and Context

  • Convert UNIX paths to native paths
    This commit fixes #3454. On Windows, QFileDialogs return paths with a UNIX-style path separator whereas when a .kdbx file is double-clicked in Explorer, it is passed to the KeepassXC process with Windows-style path separators. This caused the same file to be registered as two different files by KeepassXC depending on whether it was opened from the context menu or the file explorer. QDir::toNativeSeparators will adjust the path separator depending on the platform.
  • Let the window focus fix compile only on MacOS
    I saw the comment saying that MacOS needs additional code because the parent window looses focus after the file picker shows up so I just wrapped that code in an #ifdef statement so that it doesn't end up in the binary on platforms that don't need it.
  • Improve const correctness
    It just fixes a few variables that weren't marked as const (sorry, I'm kind of obsessed with const correctness! 😂)
  • Avoid imposing file extension on Linux
    I understand that this may be a bit controversial but that's why I made it as a separate commit so that I can easily discard it in case it's not a welcome change. Extensions on a Linux file system are essentially meaningless and extensions carry no information about a particular file. The OS treats files without extensions just as well as files that have one. Actually, files with no extension are very common on Linux. I saw that, on Linux, picking files for a save operation adds much more code than for the other two platforms: a QFileDialog instance was created from scratch instead of using the QFileDialog::getSaveFileName() convenience method because, according to the comment, the file picker doesn't automatically append the extension. I just felt like it was a bit "hacky" and, more importantly, it tried to forcibly change a behavior that's intended by the OS. Also, I tried to reproduce this thing and, on KDE, I get a checkbox in the file picker saying "Automatically append file extension (.kdbx)" which prevents me from saving files without the intended extension. If I leave it unchecked, I can of course save files without an extension. I'm sure other file pickers from other desktop environment also have this option. If I run the file command on a database saved without extension, it has no problem recognizing it; nor does KeepassXC have any kind of issue opening it and working with it. Anyways, my point is that it must be the user's choice whether a file must have an extension or not, instead of being imposed by the program.
$ file new_copy
new_copy: Keepass password database 2.x KDBX
  • Improve code style
    This commit improves (arguably small) things about the code style, such as const correctness, initialization of enum types with 0 which were flagged by clang-tidy as a pointer initialization with 0 for some reason but it's still good practice to use {} instead of 0 (which is technically an implicit conversion from int) and the dir variable which was passed by non-const value just to be reused inside the function as a local. It was kind of an ugly usage, in my opinion, so I made dir become a const-reference to some string from the outside and used a ternary operator + a reference to achieve the exact same thing as before but without unnecessary copies (it's easier in code than in words 😊).

P.S. Some function calls are now formatted differently than before because Clang Format decided to change the formatting, with fewer parameters. I set it to run on Save.

Testing strategy

I tested my changes manually and I ran all the tests (both on Linux and Windows). They all pass on Linux while only one fails on Windows, but so it did before I touched the code so it's unrelated to my changes.

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]
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Aug 24, 2019

Wow this looks really good to me. That is a lot of removed code that really did nothing, totally agree with your edits.

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Aug 24, 2019

Yeah, exactly! Thank you!

* QFileDialog returns UNIX paths, even on Windows. This patch converts what QFileDialog returns to the native path format.

* Improve const correctness

* Avoid imposing file extension on Linux

* This patch improves things like unneeded passes by values, missing const qualifiers, ugly copies because of variable reuse and consistency in variable names.
@droidmonkey droidmonkey force-pushed the brainplot:fix-filedialog branch from 8fb6741 to 1c13396 Aug 31, 2019
@droidmonkey droidmonkey merged commit fccbb98 into keepassxreboot:develop Aug 31, 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
@brainplot brainplot deleted the brainplot:fix-filedialog branch Aug 31, 2019
@droidmonkey droidmonkey added this to the v2.5.0 milestone Oct 16, 2019
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
2 participants
You can’t perform that action at this time.