keepassxc icon indicating copy to clipboard operation
keepassxc copied to clipboard

Make browser-triggered dialogue boxes appear on the current workspace (and related clean-up)

Open dsalt opened this issue 7 months ago • 3 comments

This ensures that various dialogue boxes which are opened in response to in-browser actions appear on the current workspace (and, most likely, centred on the monitor where the pointer currently is) instead of the workspace containing the main window.

Fixes #10328.

There is a related minor clean-up: I noticed that the db unlock dialogue box being placed above other windows was only happening on Linux, but other dboxes had this unconditionally. I have therefore brought it into line with the others.

Testing strategy

  • Ensure in advance that there is at least one password and passkey for the test site with no stored access control info and that there is a saved browser plugin key with the chosen test name.
  • Start keepassxc or, if running, lock the test database.
  • Move its main window to another workspace.
  • Ensure that the browser plugin has no connection to the test db.
  • Trigger each dbox in turn via the browser plugin:
    • database unlock;
    • key association request (use the chosen test name);
    • key storage confirmation;
    • browser access control;
    • passkey confirmation.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

dsalt avatar May 26 '25 02:05 dsalt

Not entirely sure if the patch below is the right way to fix the testguifdosecrets test failure – it's probably not – but it's definitely to do with the fact that the db-open dbox in question is no longer parented so it won't be found by the findChild templated method.

diff --git a/src/gui/DatabaseTabWidget.h b/src/gui/DatabaseTabWidget.h
index a5074a84..638bccb2 100644
--- a/src/gui/DatabaseTabWidget.h
+++ b/src/gui/DatabaseTabWidget.h
@@ -127,6 +127,10 @@ private:
     QPointer<ImportWizard> m_importWizard;
     QTimer m_lockDelayTimer;
     bool m_databaseOpenInProgress;
+
+public:
+    // For the unit tests. Required as db open dbox isn't parented.
+    auto& getDatabaseOpenDialog() const { return *m_databaseOpenDialog; }
 };
 
 #endif // KEEPASSX_DATABASETABWIDGET_H
diff --git a/tests/gui/TestGuiFdoSecrets.cpp b/tests/gui/TestGuiFdoSecrets.cpp
index fc7e218e..7c725983 100644
--- a/tests/gui/TestGuiFdoSecrets.cpp
+++ b/tests/gui/TestGuiFdoSecrets.cpp
@@ -1894,7 +1894,7 @@ bool TestGuiFdoSecrets::driveNewDatabaseWizard()
 bool TestGuiFdoSecrets::driveUnlockDialog(DatabaseWidget* target)
 {
     processEvents();
-    auto dbOpenDlg = m_tabWidget->findChild<DatabaseOpenDialog*>();
+    auto* dbOpenDlg = &(m_tabWidget->getDatabaseOpenDialog()); // m_tabWidget->findChild<DatabaseOpenDialog*>();
     VERIFY(dbOpenDlg);
     if (!dbOpenDlg->isVisible()) {
         return false;

dsalt avatar May 26 '25 16:05 dsalt

This change breaks for example this: https://github.com/keepassxreboot/keepassxc/pull/10608. Without passing the parent to the popup dialogs, those always appear at the center of the screen, and are not related to the application window position.

Maybe it would be preferred that the parent is used to position the popup when the application is on the same workspace. If not, then the popup position will not matter. Not sure if that is possible though.

varjolintu avatar May 31 '25 05:05 varjolintu

This change breaks for example this: #10608. Without passing the parent to the popup dialogs, those always appear at the center of the screen, and are not related to the application window position.

So this would be restoring the previous handling of that particular dialogue box; and I think that it's fair to say that that change was itself a breaking change regarding the interaction with workspaces.

Maybe it would be preferred that the parent is used to position the popup when the application is on the same workspace. If not, then the popup position will not matter. Not sure if that is possible though.

I'm not sure that it's possible without bypassing Qt, but I don't think that that's the right choice anyway. If some criterion is to be used to determine whether to position the dbox relative to the main window then it seems to me that the right one is whether the dbox is being opened because of an action in the main window. If so, fine, pass the parent window handle/object; otherwise let it be opened at the centre of the screen with the pointer.

dsalt avatar Jun 01 '25 16:06 dsalt