Skip to content

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

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

dsalt
Copy link

@dsalt dsalt commented May 26, 2025

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
Copy link
Author

dsalt commented May 26, 2025

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;

…n the current monitor.

Previously, they would be opened wherever the main window is. This could
easily be on a different virtual desktop (or workspace): if so, the dialogue
box would not be visible on any attached monitor until that workspace was
selected.

(This breaks some tests involving the database-open dialogue box.)
@dsalt dsalt force-pushed the fix/dbox-opening branch from 3e4206c to 9bd8b40 Compare May 28, 2025 18:24
@varjolintu
Copy link
Member

varjolintu commented May 31, 2025

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.

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.

@dsalt
Copy link
Author

dsalt commented Jun 1, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DB unlock dialog is not displayed on main screen
2 participants