Limit to one the transaction details dialogs that a user can open.
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
-
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.
-
DrahtBot added the label CI failed on Apr 12, 2024
-
flack commented at 12:31 pm on April 12, 2024: contributorwould 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
-
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 likedlg->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 thatactivateWindow()
only makes the window active, i.e: the title of the dialog gets highlighted and gets the keyboard input focus. Theraise()
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 bothactivateWindow()
&raise()
, let me try it on Windows and I’ll incorporate your suggestion, thanks!alfonsoromanz commented at 8:23 pm on April 17, 2024: contributorTested ACK b26d2e81ec6ac4b5bd97bc7f7ec0c44e502b6a18. Only one dialog is opened for a single tx.shafi4117 approvedshafi4117 approvedBrandonOdiwuor approvedBrandonOdiwuor commented at 12:40 pm on April 24, 2024: contributorTested 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
luke-jr commented at 6:02 pm on May 7, 2024: memberWhy?shafi4117 approvedpablomartin4btc force-pushed on May 10, 2024pablomartin4btc commented at 10:46 pm on May 10, 2024: contributorWhy?
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: contributorUpdates:
- 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 bothsrc/qt/transactiondescdialog.h
andsrc/qt/transactionview.cpp
, without thembitcoin-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 approvedalfonsoromanz commented at 3:02 pm on May 24, 2024: contributorRe 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, 2024hebasto commented at 2:10 pm on July 15, 2024: memberCurrently 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: memberTested 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 withQT_QPA_PLATFORM=wayland
.pablomartin4btc commented at 2:49 pm on July 15, 2024: contributorTested on Ubuntu 24.04. It works with
QT_QPA_PLATFORM=xcb
, but fails withQT_QPA_PLATFORM=wayland
.I’ll take a look, thanks!
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: contributorI 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, 2024pablomartin4btc force-pushed on Aug 2, 2024hebasto referenced this in commit 1873e4116f on Aug 12, 2024pablomartin4btc marked this as ready for review on Aug 15, 2024gui: Fix multiple dialogs for same tx in TransactionView
Only one tx details dialog that a user can open per tx id is enough.
pablomartin4btc force-pushed on Aug 15, 2024pablomartin4btc commented at 3:52 pm on August 15, 2024: contributorUpdates:
- No functional changes:
- Using GUIUtil::bringToFront() that was recently fixed (and merged) for
Wayland
; - Removed unnecessary extra line in (
src/qt/transactiondescdialog.h
).
- Using GUIUtil::bringToFront() that was recently fixed (and merged) for
DrahtBot added the label CI failed on Oct 14, 2024DrahtBot removed the label CI failed on Oct 14, 2024
This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 23:20 UTC
More mirrored repositories can be found on mirror.b10c.me