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

[pablomartin4btc]

Limit to one the transaction details dialogs that a user can open.

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).