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
  1. pablomartin4btc commented at 4:00 am on April 12, 2024: contributor

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

    Peek 2024-04-12 00-50

    Peek 2024-04-12 00-37

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

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

  4. DrahtBot added the label CI failed on Apr 12, 2024
  5. 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
  6. 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!

  7. pablomartin4btc force-pushed on Apr 12, 2024
  8. 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.
  9. DrahtBot removed the label CI failed on Apr 12, 2024
  10. 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!
  11. alfonsoromanz commented at 8:23 pm on April 17, 2024: contributor
    Tested ACK b26d2e81ec6ac4b5bd97bc7f7ec0c44e502b6a18. Only one dialog is opened for a single tx.
  12. shafi4117 approved
  13. shafi4117 approved
  14. BrandonOdiwuor approved
  15. 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

  16. luke-jr commented at 6:02 pm on May 7, 2024: member
    Why?
  17. shafi4117 approved
  18. pablomartin4btc force-pushed on May 10, 2024
  19. 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.

  20. pablomartin4btc commented at 2:13 am on May 11, 2024: contributor

    Updates:

  21. alfonsoromanz approved
  22. 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

  23. DrahtBot requested review from BrandonOdiwuor on May 24, 2024
  24. 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.

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

  26. 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!

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

  28. 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:

    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.

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

  30. pablomartin4btc marked this as a draft on Jul 30, 2024
  31. pablomartin4btc force-pushed on Aug 2, 2024
  32. hebasto referenced this in commit 1873e4116f on Aug 12, 2024
  33. pablomartin4btc marked this as ready for review on Aug 15, 2024
  34. 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
  35. pablomartin4btc force-pushed on Aug 15, 2024
  36. 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).
  37. DrahtBot added the label CI failed on Oct 14, 2024
  38. DrahtBot removed the label CI failed on Oct 14, 2024

github-metadata-mirror

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

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me