Bugfix - don’t allow multiple dialogs for same tx in TransactionView #817

pull pablomartin4btc wants to merge 1 commits into bitcoin-core:master from pablomartin4btc:gui-fix-tx-view-only-one-dialog-per-tx-id changing 4 files +28 −8

pablomartin4btc commented at 4:00 am on April 12, 2024: contributor

Limit to one the transaction details dialogs that a user can open.
DrahtBot commented at 4:00 am on April 12, 2024: contributor

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 commented at 6:16 am on April 12, 2024: contributor

🚧 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
DrahtBot added the label CI failed on Apr 12, 2024
flack commented at 12:31 pm on April 12, 2024: contributor

would be nice if a click on a row where the dialog is already open would bring that dialog to the foreground
pablomartin4btc commented at 2:49 pm on April 12, 2024: contributor

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 force-pushed on Apr 12, 2024
pablomartin4btc commented at 3:20 pm on April 12, 2024: contributor
Updates:
- Fixed lint failure.
- Bring an already open dialog to the foreground as @flack suggested.
DrahtBot removed the label CI failed on Apr 12, 2024
in src/qt/transactionview.cpp:674 in b26d2e81ec outdated
664@@ -663,3 +665,14 @@ void TransactionView::closeOpenedDialogs() 665 } 666 m_opened_dialogs.clear(); 667 } 668+ 669+bool TransactionView::detailsAlreadyShown(const QModelIndex &idx) 670+{ 671+ for (TransactionDescDialog* dlg : m_opened_dialogs) { 672+ if (dlg->getTransactionId() == idx.data(TransactionTableModel::TxHashRole).toString()) { 673+ dlg->activateWindow();
alfonsoromanz commented at 8:20 pm on April 17, 2024:

The dialog is not brought to the foreground in my case (I am using Mac). In order to bring it to the foreground I had to also call raise(). Thoughts?

0 dlg->activateWindow(); 1 dlg->raise()
pablomartin4btc commented at 4:17 am on April 18, 2024:

Thanks for reviewing and spotting that. I tried on Ubuntu to call only dlg->raise() but doesn’t seem to work (also try other things like dlg->setFocus(), dlg->setFocusPolicy(Qt::StrongFocus), dlg->focusWidget() and some combinations), until I found this in the QT documentation about activateWindow() (which I should have checked first):

This function performs the same operation as clicking the mouse on the title bar of a top-level window. On X11, the result depends on the Window Manager. If you want to ensure that the window is stacked on top as well you should also call raise. Note that the window must be visible, otherwise activateWindow() has no effect.

On Windows, if you are calling this when the application is not currently the active one then it will not make it the active window. It will change the color of the taskbar entry to indicate that the window has changed in some way. This is because Microsoft does not allow an application to interrupt what the user is currently doing in another application.

So, since you tried it on Mac already, let me take a look on how it behaves on Windows.

alfonsoromanz commented at 11:57 am on April 18, 2024:

In Mac, it seems that activateWindow() only makes the window active, i.e: the title of the dialog gets highlighted and gets the keyboard input focus. The raise() function brings the dialog on top. So, at least in Mac, it makes sense to keep both IMO

pablomartin4btc commented at 2:52 pm on April 18, 2024:

Yes, it needs both activateWindow() & raise(), let me try it on Windows and I’ll incorporate your suggestion, thanks!
alfonsoromanz commented at 8:23 pm on April 17, 2024: contributor

Tested ACK b26d2e81ec6ac4b5bd97bc7f7ec0c44e502b6a18. Only one dialog is opened for a single tx.
shafi4117 approved
shafi4117 approved
BrandonOdiwuor approved
BrandonOdiwuor commented at 12:40 pm on April 24, 2024: contributor

Tested ACK b26d2e81ec6ac4b5bd97bc7f7ec0c44e502b6a18

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 commented at 6:02 pm on May 7, 2024: member

Why?
shafi4117 approved
pablomartin4btc force-pushed on May 10, 2024
pablomartin4btc commented at 10:46 pm on May 10, 2024: contributor

Why?

Discussed a bit offline… Please feel free to add more context here and any concerns you have.
pablomartin4btc commented at 2:13 am on May 11, 2024: contributor
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 approved
alfonsoromanz commented at 3:02 pm on May 24, 2024: contributor

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

https://github.com/bitcoin-core/gui/assets/19962151/dd21afaa-b48d-4082-be4c-3c3bdc965cfa
DrahtBot requested review from BrandonOdiwuor on May 24, 2024
hebasto commented at 2:10 pm on July 15, 2024: member

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

Concept ACK. Such a behavior can confuse the user.
hebasto commented at 2:36 pm on July 15, 2024: member

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 commented at 2:49 pm on July 15, 2024: contributor

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!

Type	Reviewers
Concept ACK	hebasto
Stale ACK	BrandonOdiwuor, alfonsoromanz

pablomartin4btc commented at 10:18 pm on July 17, 2024: contributor

@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:

 0--- a/src/qt/transactionview.cpp
 1+++ b/src/qt/transactionview.cpp
 2@@ -671,8 +671,11 @@ bool TransactionView::detailsAlreadyShown(const QModelIndex &idx)
 3 {
 4     for (TransactionDescDialog* dlg : m_opened_dialogs) {
 5         if (dlg->getTransactionId() == idx.data(TransactionTableModel::TxHashRole).toString()) {
 6-            dlg->activateWindow();
 7-            dlg->raise();
 8+            auto eFlags = dlg->windowFlags();
 9+            dlg->setWindowFlags(eFlags|Qt::WindowStaysOnTopHint);
10+            dlg->show();
11+            dlg->setWindowFlags(eFlags);
12+            dlg->show();
13             return true;
14         }
15     }

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

hebasto commented at 2:52 pm on July 29, 2024: member
@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 commented at 12:00 pm on July 30, 2024: contributor

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 marked this as a draft on Jul 30, 2024
pablomartin4btc force-pushed on Aug 2, 2024
hebasto referenced this in commit 1873e4116f on Aug 12, 2024
pablomartin4btc marked this as ready for review on Aug 15, 2024

gui: Fix multiple dialogs for same tx in TransactionView

Only one tx details dialog that a user can open per tx id is enough.

0f77a6e219

pablomartin4btc force-pushed on Aug 15, 2024
pablomartin4btc commented at 3:52 pm on August 15, 2024: contributor
Updates:
- No functional changes:
  
  Using GUIUtil::bringToFront() that was recently fixed (and merged) for Wayland;
  
  Removed unnecessary extra line in (src/qt/transactiondescdialog.h).
DrahtBot added the label CI failed on Oct 14, 2024
DrahtBot removed the label CI failed on Oct 14, 2024

Contributors
pablomartin4btc DrahtBot flack alfonsoromanz shafi4117 BrandonOdiwuor luke-jr hebasto

Review Requested
BrandonOdiwuor

Bugfix - don’t allow multiple dialogs for same tx in TransactionView #817

Reviews