Skip to content

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented May 30, 2022

The SendCoins_UnauthenticatedPaymentRequest and SendCoins_AuthenticatedPaymentRequest sub-QFrame's of the SendCoinsEntry widget have been unused since bitcoin/bitcoin#17165.

Removed all dead code. The resulted SendCoinsEntry widget has been simplified.

@hebasto hebasto added Refactoring UI All about "look and feel" labels May 30, 2022
@DrahtBot
Copy link
Contributor

DrahtBot commented May 31, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #560 (Make error message layout consistent by w0xlt)
  • #534 (Add translatable address error message by w0xlt)
  • #533 (Add more detailed address error message by w0xlt)

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.

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code reviewed ACK a57cd02

No functional changes.

@laanwj
Copy link
Member

laanwj commented Jun 8, 2022

Concept ACK, nice cleanup

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a57cd02

  • This PR does an excellent clean-up to sendcoinentry.ui file by:

    • Removing no longer used code.
    • Simplifying the widget type for the updated UI elements.
    • Fixing indentations.
    • Fixing the parent class for the BitcoinAmountField widget.
  • I was able to successfully compile and run this PR on Ubuntu 22.04 with Qt version 5.15.3

  • I observed no functional or behavioral changes in the GUI.

As @laanwj rightly pointed out. The comment mentioning “Stacked widget” shall also be removed.

@hebasto hebasto force-pushed the 220530-sendcoins branch from a57cd02 to 7ab72b9 Compare June 12, 2022 11:41
@hebasto
Copy link
Member Author

hebasto commented Jun 12, 2022

Updated a57cd02 -> 7ab72b9 (pr612.01 -> pr612.02, diff):

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK 7ab72b9

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 7ab72b9

Changes since my last review:

  • @laanwj's comment is rightly addressed.

I searched for comments mentioning "Stacked ..." in src/qt/sendcoinsentry.* files, and I found none. This shows that all these comments are appropriately removed.

@hebasto hebasto merged commit 18d9189 into bitcoin-core:master Jun 21, 2022
@hebasto hebasto deleted the 220530-sendcoins branch June 21, 2022 10:19
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 21, 2022
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Refactoring UI All about "look and feel"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants