Don't auto close database unlock dialog when underlying file is unavailable - #12200
Don't auto close database unlock dialog when underlying file is unavailable#12200droidmonkey with Copilot wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the database unlock dialog would auto-close when the underlying file is unavailable by keeping the dialog open so users can retry once the file becomes accessible.
- Continue showing the unlock dialog even when the file is missing
- Improve error handling in the database opening process
- Add a new GUI test to verify the unlock dialog remains open when the database file is missing
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/gui/TestGui.h | Added declaration for the new test case |
| tests/gui/TestGui.cpp | Added testOpenMissingDatabaseFile() to validate dialog behavior |
| src/gui/DatabaseTabWidget.cpp | Modified logic to avoid early exit when file is missing and fallback to the original file path for consistency |
| src/gui/DatabaseOpenWidget.cpp | Updated error handling for database open failures to display a warning message |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #12200 +/- ##
========================================
Coverage 64.37% 64.38%
========================================
Files 378 378
Lines 39840 39850 +10
========================================
+ Hits 25646 25654 +8
- Misses 14194 14196 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
da1d1ef to
23ad80f
Compare
phoerious
added
the
pr: ai-assisted
23ad80f to
3219e27
Compare
3219e27 to
d848671
Compare
There was a problem hiding this comment.
I don't really like this solution. If this is only about tab restoration at startup, I have a better solution: Return a bool in addDatabaseTab() whether adding the tab is successful. The value is passed through by MainWindow::openDatabase() and in MainWindow::restoreConfigState() all failed tabs will be retried once after a certain timeout.
| connect(&m_fileExistsTimer, &QTimer::timeout, this, [this] { | ||
| if (!QFile::exists(m_filename)) { | ||
| m_ui->messageWidget->showMessage(tr("The database file does not exist or is not accessible."), | ||
| MessageWidget::Warning, | ||
| fileExistsCheckInterval + 500); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Not sure whether I like this. It's a bit confusing if such a message suddenly pops up after an arbitrary time interval. Also, what happens in between when the user tries to unlock the non-existent file?
This also causes an unnecessary check after 5sec for every database. At least the timer should be stopped when the database is loaded correctly.
There was a problem hiding this comment.
A file can become unavailable at any time after being successfully detected/loaded. If network share gets severed, etc.
Perhaps that check should be moved to the open sequence and refuse to open if the file cannot be read. Technically that already happens but deep in the database code with direct file access.
There was a problem hiding this comment.
See my comment above. That'd be my preferred solution.
| // The file does not exist, revert back to the cleaned path for comparison | ||
| dbFilePath = QDir::toNativeSeparators(dbWidget->database()->filePath()); | ||
| } | ||
| if (dbFilePath.compare(canonicalFilePath, FILE_CASE_SENSITIVE) == 0) { |
There was a problem hiding this comment.
We're potentially comparing canonicalised to non-canonicalised paths there. This could open a database twice.
There was a problem hiding this comment.
Yah we do a poor job of file path handling and comparison throughout the code base. Qt doesn't make that any easier.
…available Co-authored-by: droidmonkey <2809491+droidmonkey@users.noreply.github.com>
d848671 to
66ff42c
Compare
|
Will reimpmement at a later date |
|
Hello, the implementation was removed from milestone v2.7.11 and sadly also not not reimplemented in v2.7.12 For which version a new reimplementation is foreseen? |
|
Not anytime in the near future, this is deprioritized right now |
It is really sad to read this statement, because here are many use cases impacted by this request. For example:
linked Issue Tickets with different use cases (marked as duplicates): |
This PR addresses the issue where KeePassXC would automatically close the database unlock dialog when the underlying database file is temporarily unavailable, preventing users from retrying once the file becomes accessible.
Problem
When KeePassXC starts automatically on user login and tries to open the most recently used database file, if that file is stored on cloud storage (like Google Drive) that hasn't finished mounting yet, the unlock dialog would never appear. This creates a race condition where users have to manually reselect their database file after the cloud storage becomes available.
Solution
The fix implements the suggestion from @droidmonkey to keep the unlock dialog open even when the file is missing, allowing users to retry when the file becomes available.
Key Changes
DatabaseTabWidget.cpp:
canonicalFilePath.isEmpty()(indicating file doesn't exist)DatabaseOpenWidget.cpp:
Database::open()fails during header readingTest Coverage
Added
testOpenMissingDatabaseFile()test to verify:User Experience
Before this fix:
After this fix:
This provides a much better user experience for cloud storage scenarios and other cases where files may be temporarily unavailable.
Fixes #9539.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
224.0.0.251esm.ubuntu.com/usr/lib/apt/methods/https(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.