Bugfix - don't allow multiple dialogs for same tx in TransactionView

[pablomartin4btc]

Limit to one the transaction details dialogs that a user can open. If a transaction details dialog is already open, bring it to the foreground when a user clicks on its row again - as suggested by flack.

Currently a user can open unlimited tx details dialogs for the same tx id.

This PR fixes the issue.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	hebasto
Stale ACK	BrandonOdiwuor, alfonsoromanz

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin-core/gui/runs/23737775174}

[flack]

would be nice if a click on a row where the dialog is already open would bring that dialog to the foreground

[pablomartin4btc]

would be nice if a click on a row where the dialog is already open would bring that dialog to the foreground

Great suggestion! I'll try that since I have to fix a lint. Thanks!

[pablomartin4btc]

Updates:

• Fixed lint failure.
• Bring an already open dialog to the foreground as @flack suggested.

[alfonsoromanz]

Tested ACK b26d2e8. Only one dialog is opened for a single tx.

[BrandonOdiwuor]

Tested ACK b26d2e8

Tested successfully on Ubuntu 22.04.4 LTS. Only one dialogue is created per transaction and the clicked transaction is brought to the foreground

Screencast.from.24-04-2024.03.35.10.ALASIRI.webm

[luke-jr]

Why?

[pablomartin4btc]

Why?

Discussed a bit offline... Please feel free to add more context here and any concerns you have.

[pablomartin4btc]

Updates:

• added the call to raise() as @alfonsoromanz suggested finding that the fix wasn't working properly on Mac without it, this is not causing any other effects on Linux or Windows which I both tested.
• added includes of QString in both src/qt/transactiondescdialog.h and src/qt/transactionview.cpp, without them bitcoin-qt was crashing on Windows on the tx id hash string comparison (note: bitcoin-qt was cross-compiled for Windows with WSL Ubuntu 24.04 beta; it would be good to double check on native Windows compilation with VS 2022)

[alfonsoromanz]

Re ACK ba13f10. I tested in Mac and now the dialog is brought to the foreground

pr.mov

[hebasto]

Currently a user can open unlimited tx details dialogs for the same tx id.

Concept ACK. Such a behavior can confuse the user.

[hebasto]

Tested successfully on Ubuntu 22.04.4 LTS. Only one dialogue is created per transaction and the clicked transaction is brought to the foreground

Tested on Ubuntu 24.04. It works with QT_QPA_PLATFORM=xcb, but fails with QT_QPA_PLATFORM=wayland.

[pablomartin4btc]

Tested on Ubuntu 24.04. It works with QT_QPA_PLATFORM=xcb, but fails with QT_QPA_PLATFORM=wayland.

I'll take a look, thanks!

[pablomartin4btc]

@hebasto it seems there are some issues with Wayland on how these actions are being handled (this one specific to KDE but found some old ones in gnome).

As a workaround (tried other things as well), might be not elegant, but this works fine on both X11/Xorg and Wayland:

--- a/src/qt/transactionview.cpp
+++ b/src/qt/transactionview.cpp
@@ -671,8 +671,11 @@ bool TransactionView::detailsAlreadyShown(const QModelIndex &idx)
 {
     for (TransactionDescDialog* dlg : m_opened_dialogs) {
         if (dlg->getTransactionId() == idx.data(TransactionTableModel::TxHashRole).toString()) {
-            dlg->activateWindow();
-            dlg->raise();
+            auto eFlags = dlg->windowFlags();
+            dlg->setWindowFlags(eFlags|Qt::WindowStaysOnTopHint);
+            dlg->show();
+            dlg->setWindowFlags(eFlags);
+            dlg->show();
             return true;
         }
     }

I can update the code with that snippet above or we can do it on a follow-up.

[hebasto]

@hebasto it seems there are some issues with Wayland on how these actions are being handled (this one specific to KDE but found some old ones in gnome).

As a workaround (tried other things as well), might be not elegant, but this works fine on both X11/Xorg and Wayland:

The approach you suggested works for me.

Here are other references to this workaround:

• https://stackoverflow.com/questions/6087887/bring-window-to-front-raise-show-activatewindow-don-t-work/10808934
• https://forum.qt.io/topic/6032/bring-window-to-front-raise-show-activatewindow-don-t-work-on-windows

I can update the code with that snippet above or we can do it on a follow-up.

I think we should first fix our GUIUtil::bringToFront() first in a separate PR, which will also address similar issues in other cases.

Then we can apply the fixed GUIUtil::bringToFront() in this PR.

[pablomartin4btc]

I think we should first fix our GUIUtil::bringToFront() first in a separate PR, which will also address similar issues in other cases.

Then we can apply the fixed GUIUtil::bringToFront() in this PR.

Ok, I'll open a new PR for GUIUtil::bringToFront() and will put this one on draft in the meantime.

[pablomartin4btc]

Updates:

• No functional changes:
  • Using GUIUtil::bringToFront() that was recently fixed (and merged) for Wayland;
  • Removed unnecessary extra line in (src/qt/transactiondescdialog.h).

[pablomartin4btc]

Updates:

• Rebased (fixed previous CI failure - previous releases, depends DEBUG... ).

[w0xlt]

would be nice if a click on a row where the dialog is already open would bring that dialog to the foreground

I think this is a better approach.
Simply limiting dialog windows can be confusing, as the user may not understand why more windows are not opening,

[pablomartin4btc]

would be nice if a click on a row where the dialog is already open would bring that dialog to the foreground

I think this is a better approach. Simply limiting dialog windows can be confusing, as the user may not understand why more windows are not opening,

Yup, that's done. I'll update the PR's description to make it clearer.

[DrahtBot]

There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

[pablomartin4btc]

Temporarily drafting it since I've found some issues on Qt 6 while reviewing #904. WIP.

[diegoviola]

Temporarily drafting it since I've found some issues on Qt 6 while reviewing #904. WIP.

Can you say what those issues are?

[pablomartin4btc]

Can you say what those issues are?

This fix just doesn't work on Qt6 anymore as it was on Qt5, so I need to investigate how the same behaviour can be replicated.

[diegoviola]

This fix just doesn't work on Qt6 anymore as it was on Qt5, so I need to investigate how the same behaviour can be replicated.

I suggest looking into requestActivate() for getting this done.

If my understanding is correct, in the context of Wayland, there is no "top" and "front", and focus isn't something that can be stolen from an app either. So I don't think GUIUtil::bringToFront which calls Qt::WindowStaysOnTopHint has any chance of working.

requestActivate() would, in theory, work everywhere. And in the Wayland context, I think it would use something like the xdg-activation protocol to tell the compositor "a window wants focus". At most, I expect to receive a notification informing the user of that fact, but not actually stealing keyboard focus from the current application.

[diegoviola]

from the current application

Application window/surface.

[diegoviola]

At most, I expect to receive a notification informing the user of that fact, but not actually stealing keyboard focus from the current application.

Not quite right:

Consolatis │ dviola: I'd expect most compositors to raise and give keyboard focus to a surface requesting activation with a valid token (e.g. the surface the token originated from had keyboard or pointer focus at the time of issuing the token and not too much time has passed). I think some compositors treat "invalid" tokens as some kind of urgency hint but AFAIR that is not stated anywhere in the protocol