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

5 participants