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.
<details> <summary>Currently a user can open unlimited tx details dialogs for the same tx id.</summary>
</details> <details> <summary>This PR fixes the issue.</summary>
</details>
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
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 copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 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.
<sub>Debug: https://github.com/bitcoin-core/gui/runs/23737775174</sub>
would be nice if a click on a row where the dialog is already open would bring that dialog to the foreground
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!
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();
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?
dlg->activateWindow();
dlg->raise()
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.
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
Yes, it needs both activateWindow() & raise(), let me try it on Windows and I'll incorporate your suggestion, thanks!
Tested ACK b26d2e81ec6ac4b5bd97bc7f7ec0c44e502b6a18. Only one dialog is opened for a single tx.
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
Why?
Why?
Discussed a bit offline... Please feel free to add more context here and any concerns you have.
Updates:
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.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)
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
Currently a user can open unlimited tx details dialogs for the same tx id.
Concept ACK. Such a behavior can confuse the user.
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.
Tested on Ubuntu 24.04. It works with
QT_QPA_PLATFORM=xcb, but fails withQT_QPA_PLATFORM=wayland.
I'll take a look, thanks!
@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 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:
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.
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.
Updates:
Wayland;src/qt/transactiondescdialog.h).
Only one tx details dialog that a user can open per tx id is enough.
Updates:
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,
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.
<!--8ac04cdde196e94527acabf64b896448-->
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.
Temporarily drafting it since I've found some issues on Qt 6 while reviewing #904. WIP.
Temporarily drafting it since I've found some issues on Qt 6 while reviewing #904. WIP.
Can you say what those issues are?
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.
@pablomartin4btc I've just tried your code from this branch and it works fine on top of the current master, take a look:
https://github.com/user-attachments/assets/a6dd0b43-15c0-42de-b379-598a581ab44a
