gui: Fix manual coin control with multiple wallets loaded #17457

pull promag wants to merge 4 commits into bitcoin:master from promag:2019-11-fix-15725 changing 5 files +260 −65
  1. promag commented at 10:07 PM on November 12, 2019: member

    This PR makes coin selection work when multiple wallets are loaded - each loaded wallet has it's own coin control dialog.

    Closes #15725.

  2. DrahtBot commented at 10:10 PM on November 12, 2019: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18656 (gui: Add a Make unsigned button next to Send by achow101)
    • #18027 ("PSBT Operations" dialog by gwillen)
    • #17611 (gui: Make send and receive widgets extend QWidget by promag)
    • #17509 (gui: save and load PSBT by Sjors)
    • #17463 (Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" by luke-jr)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. promag commented at 10:12 PM on November 12, 2019: member

    Note to reviewers: previously CoinControlDialog::updateLabels was called with 2 different "dialogs" (CoinControlDialog and SendCoinsDialog) and both have the same set of labels except for labelCoinControlInsuffFunds. So, in order to remove static members and usages of findChild, most of the code of updateLabels is now duplicate.

    Let me know if you prefer the 2nd commit split. IMO I'd prefer to follow up later.

  4. DrahtBot added the label GUI on Nov 12, 2019
  5. jonasschnelli commented at 12:08 AM on November 16, 2019: contributor

    Travis seems to fail:

    qt/coincontroldialog.cpp: In member function ‘void CoinControlDialog::updateView()’:
3030qt/coincontroldialog.cpp:607:84: error: invalid conversion from ‘CoinControlDialog*’ to ‘int’ [-fpermissive]
3031         CCoinControlWidgetItem *itemWalletAddress = new CCoinControlWidgetItem(this);
3032                                                                                    ^
3033In file included from qt/coincontroldialog.cpp:9:0:
3034./qt/coincontroldialog.h:35:14: note:   initializing argument 1 of ‘CCoinControlWidgetItem::CCoinControlWidgetItem(int)’
3035     explicit CCoinControlWidgetItem(int type = Type) : QTreeWidgetItem(type) {}
  6. promag force-pushed on Nov 16, 2019
  7. promag force-pushed on Nov 17, 2019
  8. promag force-pushed on Nov 17, 2019
  9. promag force-pushed on Nov 17, 2019
  10. promag force-pushed on Nov 17, 2019
  11. promag commented at 12:41 PM on November 17, 2019: member

    @jonasschnelli all fixed.

  12. Sjors changes_requested
  13. Sjors commented at 7:32 PM on November 19, 2019: member

    I can confirm this fixes #15725, but 7b5b91f37b1158d9d1280d7d43858a56936de372 introduces a new issue: if you select some coins, close the coin control dialog, click Clear All, then reopen the coin control dialog, the coins are still selected.

  14. in src/qt/sendcoinsdialog.cpp:698 in 7b5b91f37b outdated 
     651 | @@ -652,6 +652,16 @@ void SendCoinsDialog::updateFeeMinimizedLabel()
 652 |      }
 653 |  }
 654 |  
 655 | +CoinControlDialog* SendCoinsDialog::coinControlDialog()
 656 | +{
 657 | +    if (!m_coin_control_dialog) {
 658 | +        m_coin_control_dialog = new CoinControlDialog(platformStyle, this);
    hebasto commented at 7:03 AM on November 20, 2019:

    Assigning a parent to a QDialog widget changes its default location. Is it desirable?

    promag commented at 7:49 AM on November 20, 2019:

    I think so, at least it looked fine to me.

  15. in src/qt/sendcoinsdialog.cpp:699 in 7b5b91f37b outdated 
     651 | @@ -652,6 +652,16 @@ void SendCoinsDialog::updateFeeMinimizedLabel()
 652 |      }
 653 |  }
 654 |  
 655 | +CoinControlDialog* SendCoinsDialog::coinControlDialog()
 656 | +{
 657 | +    if (!m_coin_control_dialog) {
 658 | +        m_coin_control_dialog = new CoinControlDialog(platformStyle, this);
 659 | +        m_coin_control_dialog->setModel(model);
    hebasto commented at 7:06 AM on November 20, 2019:

    Why not pass model to the CoinControlDialog constructor?

    promag commented at 7:47 AM on November 20, 2019:

    I can make that change, but I think it's unrelated here.

    Sjors commented at 8:41 AM on November 20, 2019:

    We use the setModel() approach in various places where the model isn't known at construction time, but that's not the case here. But agree it's not really related.

  16. in src/qt/sendcoinsdialog.cpp:808 in 7b5b91f37b outdated 
     762 | @@ -753,10 +763,7 @@ void SendCoinsDialog::coinControlFeatureChanged(bool checked)
 763 |  // Coin Control: button inputs -> show actual coin control dialog
 764 |  void SendCoinsDialog::coinControlButtonClicked()
 765 |  {
 766 | -    CoinControlDialog dlg(platformStyle);
 767 | -    dlg.setModel(model);
 768 | -    dlg.exec();
 769 | -    coinControlUpdateLabels();
 770 | +    coinControlDialog()->open();
    hebasto commented at 7:10 AM on November 20, 2019:

    Notes for other reviewers from Qt docs:

    Avoid using this [exec()] function; instead, use open().

  17. hebasto commented at 7:11 AM on November 20, 2019: member

    Concept ACK.

  18. promag commented at 7:50 AM on November 20, 2019: member

    @Sjors good catch, will fix.

  19. in src/qt/coincontroldialog.cpp:542 in b89c5778ca outdated 
     544 | +    QLabel *l2 = ui->labelCoinControlAmount;
 545 | +    QLabel *l3 = ui->labelCoinControlFee;
 546 | +    QLabel *l4 = ui->labelCoinControlAfterFee;
 547 | +    QLabel *l5 = ui->labelCoinControlBytes;
 548 | +    QLabel *l7 = ui->labelCoinControlLowOutput;
 549 | +    QLabel *l8 = ui->labelCoinControlChange;
    hebasto commented at 8:27 AM on November 20, 2019:

    As updateLabels() is a member function now, we could get rid of these intermediate variables (lN). Maybe scriptdiff? This will improve code readability significantly.

    promag commented at 11:36 AM on November 20, 2019:

    I agree but I think that refactor, and others like #17457 (review), could come next.

    I'm also very tempted to fix other things, but I just want to ditch the static coin control to fix the issue.

    hebasto commented at 11:42 AM on November 20, 2019:

    @promag Agree with your point.

  20. hebasto commented at 9:34 AM on November 20, 2019: member

    If CoinControlDialog was open once, the output list is not updated when new coins arrive.

  21. DrahtBot added the label Needs rebase on Nov 22, 2019
  22. gui: Fix Cannot queue arguments of type size_t b4097bf91c
  23. gui: Fix itemWalletAddress leak when not tree mode 101714135e
  24. gui: Refactor CoinControlDialog usage
    This removes the nested event loop when.
    04401ed679
  25. gui: Remove static members from CoinControlDialog 1d544f7303
  26. promag force-pushed on Nov 25, 2019
  27. DrahtBot removed the label Needs rebase on Nov 25, 2019
  28. instagibbs commented at 7:08 PM on January 9, 2020: member

    @gwillen take a look at this?

  29. fanquake added this to the "In progress" column in a project
  30. gwillen commented at 8:48 PM on January 27, 2020: contributor

    Sorry for the delay, I am happy to take a look, but I'm getting the same build failure that Travis is seeing. Not sure why only some Travis builds get it and not others.

    (EDIT, I misread before:) Looks like a rebase refactored something and you need to update a callsite to match.

  31. promag commented at 9:17 PM on January 27, 2020: member

    @gwillen I'll fix the build, sorry.

  32. DrahtBot cross-referenced this on Feb 11, 2020 from issue "PSBT Operations" dialog by gwillen
  33. DrahtBot cross-referenced this on Feb 11, 2020 from issue gui: Make send and receive widgets extend QWidget by promag
  34. DrahtBot cross-referenced this on Feb 11, 2020 from issue gui: save and load PSBT by Sjors
  35. DrahtBot cross-referenced this on Feb 11, 2020 from issue Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" by luke-jr
  36. promag cross-referenced this on Apr 9, 2020 from issue gui: Fix leak in CoinControlDialog::updateView by promag
  37. DrahtBot cross-referenced this on Apr 13, 2020 from issue rpc: gui: Don't change behavior based on private keys disabled, instead add new buttons/rpcs/menu items by achow101
  38. DrahtBot cross-referenced this on Apr 15, 2020 from issue gui: Add a `Make unsigned` button next to `Send` by achow101
  39. DrahtBot added the label Needs rebase on Apr 23, 2020
  40. DrahtBot commented at 2:47 AM on April 23, 2020: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

    🐙 This pull request conflicts with the target branch and needs rebase.

  41. promag cross-referenced this on May 5, 2020 from issue gui: Fix manual coin control with multiple wallets loaded by promag
  42. promag commented at 11:31 PM on May 5, 2020: member

    Created #18894 with just the fix. I'll submit the remaining changes later.

  43. promag closed this on May 5, 2020
  44. promag deleted the branch on May 5, 2020
  45. jonasschnelli referenced this in commit 8d17f8dc17 on May 13, 2020
  46. jonasschnelli referenced this in commit 246e878e78 on May 13, 2020
  47. sidhujag referenced this in commit 4460ecfc57 on May 14, 2020
  48. sidhujag referenced this in commit 2dde03529c on May 14, 2020
  49. promag cross-referenced this on May 21, 2020 from issue http: Release work queue after event base finish by promag
  50. promag cross-referenced this on May 29, 2020 from issue gui, refactor: Register Qt meta types in application constructor by promag
  51. jnewbery removed this from the "In progress" column in a project
  52. UdjinM6 referenced this in commit 32122a5b9d on Oct 16, 2020
  53. UdjinM6 referenced this in commit 98cc6a3562 on Oct 16, 2020
  54. UdjinM6 referenced this in commit cde2defb62 on Oct 16, 2020
  55. UdjinM6 referenced this in commit dbabf35b43 on Oct 16, 2020
  56. UdjinM6 referenced this in commit 02efeb0c74 on Nov 9, 2020
  57. PastaPastaPasta referenced this in commit 66a9fa0c7c on Jun 27, 2021
  58. PastaPastaPasta referenced this in commit 472a487b3b on Jun 28, 2021
  59. PastaPastaPasta referenced this in commit 0863d79bb4 on Jun 29, 2021
  60. PastaPastaPasta referenced this in commit b03649bacf on Jul 1, 2021
  61. PastaPastaPasta referenced this in commit 7434733e47 on Jul 1, 2021
  62. PastaPastaPasta referenced this in commit 1421e714ed on Jul 14, 2021
  63. PastaPastaPasta referenced this in commit 8af8bbc20e on Jul 15, 2021
  64. bitcoin locked this on Feb 15, 2022
Contributors
promagDrahtBotjonasschnelliSjorshebastoinstagibbsgwillen
Labels
GUINeeds rebase
Linked (view graph)
#15725 Manual coin control dialog interacts badly with multiple wallets#18578 gui: Fix leak in CoinControlDialog::updateView#18894 gui: Fix manual coin control with multiple wallets loaded#19033 http: Release work queue after event base finish#19104 gui, refactor: Register Qt meta types in application constructor
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC