Allow selecting any open database in unlock dialog

[aswild]

When showing the database unlock dialog from browser or auto-type and
there's more than one open database, add tabs to the unlock dialog so
that the user can choose which database to unlock.

This implements part of #2322 and improves the UX when working with
multiple databases after version 2.6 switched to using the dialog for
browser unlock.

Make the DatabaseOpenDialog window Application-Modal so that it blocks
input to the main UI when the dialog is open. This reduces corner cases
by avoiding the possibility of databases getting closed or unlocked
behind the open dialog.

Screenshots

See #5427 (comment)

Testing strategy

All current tests pass; I'm not sure if or how to add a GUI test for this functionality. When building with asan, I can't find any new leaks relative to the tip of the develop branch.

Type of change

• ✅ New feature (change that adds functionality)

[droidmonkey]

Why pass file names and not database widget objects directly? No need for extra lookup function in the tab widget.

Also, don't involve database open widget in this, this should just be an open dialog change (and tab widget).

[aswild]

Why pass file names and not database widget objects directly? No need for extra lookup function in the tab widget.

I think I used a string list because it was easier to put into the QComboBox, but a list of DatabaseWidget objects should work too.

Also, don't involve database open widget in this, this should just be an open dialog change (and tab widget).

I was hoping for that, but got tripped up because of how the database open widget is the entire UI of the open dialog. In an attempt to make the UI look nicer (replacing the filename display with the drop-down) I ended up going back and forth between the unlock widget and the unlock dialog.

I can refactor to avoid touching DatabaseOpenWidget by adding another title label and drop-down box in the dialog above the "Unlock KeePassXC Database" heading.

(P.S. thanks for your patience & code reviews, I'm learning a lot as I work through this feature)

[droidmonkey]

Yes please just add this combo box above the implanted open widget in the dialog. That minimizes the interactions and keeps the conversation btwn the dialog and tab widget. The whole process is going to be refactored at some point, don't want to add more complexity.

[aswild]

Yeah, that makes sense, good plan to keep the logic complexity down.

Back to my old nemesis, trying to figure out Qt layouts... A first attempt at a mockup turned out pretty bad, there's some sort of outer margin/padding messing up the coloring and things go crazy when resizing. This is going to be really tough to make visually consistent with the size & margins of DatabaseOpenWidget's UI without actually changing DatabaseOpenWidget.

[droidmonkey]

I can take a crack at it, been deep diving in gui layouts lately

[aswild]

As an experiment, I went back to one of my original ideas, using tabs rather than a drop-down.

It's not quite as pretty as a properly laid-out combo box above the unlock widget, but it has the same data/logic flow between DatabaseOpenDialog and DatabaseTabWidget without needing to modify DatabaseOpenWidget.

I can use this to polish that logic and it'd be straightforward to swap out the QTabBar for a QComboBox if you're able to come up with a good-looking layout.

[phoerious]

I actually like this approach more. It may be a tiny bit weird semantically if you think of this dialog window just "remotely" unlocking one of the tabs from the main window, but after all, it's easier to understand and more clearly visible. I think it's also faster to navigate, since it requires fewer clicks and less precision.

[droidmonkey]

Thats actually super slick, although it begs the question why not just use the main window to begin with and ditch the dialog.

[aswild]

Thats actually super slick

I have a mostly-working prototype using this design (aswild@a9d4671), but still need to fix the Merge intent, which is unique in that the database being unlocked is different than the dbWidget that the complete signal is sent back to.

although it begs the question why not just use the main window to begin with and ditch the dialog.

FWIW, that's exactly what v2.5 and earlier did. Changing to the dialog in 2.6 was enough of an inconvenience that I've been running a custom build of 2.6.1 with those commits reverted. I'm here now because I'd rather help contribute an upstream fix rather than run my own fork indefinitely.

[droidmonkey]

The main convenience of the dialog is that it is encapsulated in a quirky workflow in the code. The dialog drives the process (that is what the Intent piece is about). That is what I want to refactor anyway.

I think it would be perfectly OK to show the main window, have the global message widget say something like "Select the database to unlock to perform [ACTION]", then when the db is unlocked the action is executed. Main window is minimized or trayed again depending on its previous state.

[aswild]

I'm back from vacation and finally updated this PR to the new implementation which uses tabs in the unlock dialog rather than modifying DatabaseOpenWidget.

[aswild]

Any update on this? I've been running a build with these changes for a few weeks an it's been great.

[droidmonkey]

We were working on 2.6.2, will return to this shortly

[aswild]

Ah yeah, no worries. I just rebased on the latest develop branch (including 2.6.2)

[ziegenberg]

Any updates on this PR? I'd love to see it released.

[aswild]

Any updates on this PR? I'd love to see it released.

I just rebased again, no conflicts and it seems to still be working without having to change anything. I've been running custom builds of this commit on top of the 2.6.X releases all year with good success.

I'd also love to see this released; I assume the developers have been busy with the more substantial 2.7 changes along with continuing to maintain the 2.6 branch.

[aswild]

rebased again, these CI failures appear to be spurious unrelated things that I can't reproduce locally.

[droidmonkey]

Excellent I will review this one next

[droidmonkey]

This works really well, however when the current selected database is locked and any other database is unlocked, the unlock dialog will not appear. That would be another improvement to this change. Additionally, I don't like adding the dependency to DatabaseTabWidget, instead we should pass which database was unlocked and let the tab widget switch to that database itself.

[aswild]

Thanks for the feedback!

when the current selected database is locked and any other database is unlocked, the unlock dialog will not appear

I'm not able to reproduce this from the browser integration. In the main UI I tried having one unlocked database, and a second locked database with its tab selected. When I click "reopen database" I get the unlock dialog for the locked+active db (and no tab on the unlock dialog for the already-unlocked database).

I don't like adding the dependency to DatabaseTabWidget, instead we should pass which database was unlocked and let the tab widget switch to that database itself.

I'm not quite sure what you mean here, are you referring to this code in DatabaseOpenDialog::complete?

if (m_intent != Intent::Merge) {
    // Update the current database in the main UI to match what we just unlocked
    auto* tabWidget = qobject_cast<DatabaseTabWidget*>(parentWidget());
    tabWidget->setCurrentIndex(tabWidget->indexOf(m_currentDbWidget));
}
emit dialogFinished(accepted, m_currentDbWidget);

I can see how it's weird for the DatabaseOpenDialog to be modifying its DatabaseTabWidget parent to change the active tab. Should it instead emit a signal to the tab widget telling it which DatabaseWidget was unlocked, and let the tab widget itself call setCurrentIndex?

[droidmonkey]

Interesting when I tested Auto-Type that wasn't the case, probably an Auto-Type specific behavior.

For the second part, yes exactly use signal slots to communicate to the main tab widget.

[aswild]

Gotcha. I'll test out auto type and make some changes

[aswild]

OK, I was able to reproduce what you're seeing with Auto-Type (using the global auto-type shortcut key). What is the behavior that we want here?

Currently, if any databases are unlocked, then the auto-type dialog is displayed for those databases, regardless of which tab is open. Only if all databases are closed is the unlock dialog displayed (with tabs, just like the browser unlock).

I can think of a couple ways,

1. Make it behave like the browser - if the active DB is locked, then display the unlock dialog for it (and any other locked DBs). After unlocking, the background-unlocked DB can also be included in autotype search results.
2. Modify the auto-type dialog to add a "unlock other database" button that opens the unlock dialog for all open locked DBs.

Option 1 should be relatively straightforward to implement, and provides consistency between the browser extension and auto-type UX. Option 2 could have UX advantages, but I'm not sure what the best design would be since I don't personally use auto-type very often.

[droidmonkey]

I prefer Option 1, this is kind of an edge case, but we should be consistent between Browser and Auto-Type... especially when the browser plugin can invoke the Auto-Type dialog in the future.

[aswild]

Updated; I think auto-type should work as expected now, but I appreciate any testing since I don't use auto-type very often.

One note on the latest update - I had to move the logic for database re-lock after auto-type from performGlobalAutoType() to a signal handler that's called after the unlock dialog completes. I think this is sane, but want to draw attention to it since it affects security-related code.

[droidmonkey]

I consider this to implement the full gambit, the case 3 is actually an Auto-Type request and is out of scope of the unlock dialog. You'll be eligible for the bounty when this is merged.

[droidmonkey]

I added a fix in a new commit to the browser unlock workflow. If the main window is in the background it will end up showing on top of other windows after everything is said and done. This fix ensures that it goes back to the background again.

[aswild]

Awesome, thanks for the merge! I'm really excited to have this feature land

[droidmonkey]

Go ahead and submit the bounty request over at https://www.bountysource.com/issues/63849610-allow-to-choose-database-from-unlock-dialog-for-auto-type-and-browser

[droidmonkey]

@aswild ypu need to claim the bounty now in bountysource. It is just marked in progress by you, I cannot award it yet until you claim.

[aswild]

@droidmonkey Sorry about that, I'm new at this. I think I hit the right button to claim it this time; thanks for the poke