Initial Template for Request Payment page #435

johnny9 · 2024-11-22T18:12:08Z

These commits introduce the Request Payment flow. This view is available under the "Receive" tab on the Desktop Wallets main navigation view.

The Request Payment form itself currently only includes fields currently supported by the Qt wallet (label, message, and amount). Follow up PR will add support for local notes.

The Amount field in the Request form takes only valid Satoshi/Bitcoin amounts and supports flipping the units with the associated button toggle.

When the Continue button is pressed the Request Confirmation page gets pushed to the stack will eventually reflect the fields inputted in the previous form backed by the output of the WalletModel result. In its current implemention it just has statically defined properties to check the visual layout. The page also contains an address field with the copy icon that can be used to move the address into your clipboard.

Ubuntu screenshots

johnny9 · 2024-11-22T18:16:25Z

The piece I'm most concerned about this form is the Amount input field and how to properly sanitize and convert the field. The current object backing it doesnt quite feel right but I am limited by the inability to extend the actual TextInput class.

MarnixCroes · 2024-11-22T19:51:04Z

src/qml/bitcoinamount.cpp

+{
+    QString result = text;
+
+    // Remove any invalid characters


Is this the right thing to do?
QT gui simply refuses to paste when there are invalid characters.

Perhaps the above has been missed?

maybe a question for designers: but I agree that trying to input invalid characters resulting in nothing happening is not helpful to users, but as a scaffold, the current approach seems good enough for now

MarnixCroes · 2024-11-22T19:55:53Z

The piece I'm most concerned about this form is the Amount input field and how to properly sanitize and convert the field. The current object backing it doesnt quite feel right but I am limited by the inability to extend the actual TextInput class.

yeah there are a couple weird things:

decimals when having sats as unit
unnecessary leading zeroes when switching between sats en BTC. i.e. it can display things like 000100 sats

johnny9 · 2024-11-25T18:41:34Z

From from 1b869eb to adff8f

Rebased with main and used PageStack in RequestPayment.qml
Fixed lint issue
Fixed mac build issue with missing bitcoinamount.h in Makefile

GBKS · 2024-11-28T10:20:23Z

Looking good. I know we talked about this before, that the basic concept of the receive page has changed a bit. Away from the two separate pages, and towards creating the QR code directly on the page. Just realized I already implemented this in the web prototype here (note the differences in responsive layouts).

The amount input logic, which you bring up, is not properly implemented in the web prototype. There definitely has to be some sanitation. Something to look out for is that when I type 5000 sats and switch to ₿, it should then show 0.00 005 000 ₿ and not 5000 ₿. There should be a conversion happening. If this is too messy right now, I'd just come back to it in a separate PR.

In a web prototype update I am working on, I am tying the unit toggle to the application setting. So when you switch the unit in the amount input, the wallet balance display in the top left also changes. Wonder if that is intuitive or not. What do you think? In the send page, that unit setting should at least live on a user flow level. So when you toggle, the unit in the amount input, but also the fee selector changes, as well as the unit in the review screen.

Small thing about the input and amount fields is that I can enter unlimited characters (and emoji). Should probs be restricted in some way.

johnny9 · 2024-12-01T12:48:41Z

Looking good. I know we talked about this before, that the basic concept of the receive page has changed a bit. Away from the two separate pages, and towards creating the QR code directly on the page. Just realized I already implemented this in the web prototype here (note the differences in responsive layouts).

I will remove the confirmation page and get the request page in alignment with the latest. I won't be able to get QR codes functioning in this PR as that will have to be a follow up once the requests are working end-to-end.

Small thing about the input and amount fields is that I can enter unlimited characters (and emoji). Should probs be restricted in some way.

I'll double check this. I've made some improvements with the amount but I havent tried emojis

johnny9 · 2024-12-09T17:21:22Z

Updated the form to be a single page. Does not include QR code generation, separation of address text into individual items with their own background, or the copy tooltip. I'd prefer to add these details afterwards.

GBKS · 2024-12-11T09:35:58Z

Looks great. I know you want to exclude various interaction details and handle those in follow-ups, so I won't comment on those. Just one question about the layout. Does this already include the mobile one where the QR code is inline and the buttons are static at the bottom?

GBKS · 2025-01-29T09:44:52Z

Does this need more work, or could it be merged after a rebase?

johnny9 · 2025-02-02T18:04:09Z

update from 622a114 to efe0986

Rebased with main

GBKS · 2025-02-03T08:22:13Z

@johnny9 thanks of rebasing. What state is this PR in? From our conversation last week, I think you said that you'd like to get this basic page merged even if it is not 100% yet, and then have smaller PRs to iron out the kinks, right?

pablomartin4btc

ACK efe0986

Tested the PR manually and also checked it with @GBKS's prototype.

@johnny9 please, for future reference, try to keep the description of the PR concise if possible and updated (eg confirmation page removal) as a reviewer that, could be different approaches, starts from the top it could be misleading or confusing.

Look & feel

@GBKS, I find the layout a bit unbalanced in terms of alignment, but mainly I'm not sure about all the requested fields as in other wallets the process is more straight forward and simplified with just a QR and as you have in the prototype on the top-right margin menu ("...") the user could add the amount (and labels) later so I'd keep it simpler in that sense. This (having the amount (and labels) first, as in your prototype or this PR) or having the QR first could be configurable as I think most times a user won't require an amount but perhaps as in a store the user will put the amount (and labels) every time(?) and we could add that flexibility.

Ubuntu's screenshot.

When we go back and forth from B to Sats, do we want to complete with 0's all the time? (eg 2.3 B to Sats = 230000000 then back to B shows 2.30000000).

Do we need the payment order or id (#)? Is this going to be shown in history somehow? In current QT I think we could remove these records.

QT's screenshot.
Code wise
- I think there's an unattended feedback above.
- For the BitcoinUnits conversions we'll need to have different converters and they'll have the responsibility of conversion and not the amount itself, this could be added later on a follow-up.
- In terms of commits, I think there are too many, the added icons ones could be all into 1 instead of 3 different and similarly the ones adding, merging and removing the same page (eg confirmation) could be also consolidated. I understand the chronological order but a more streamlined commit history would make it easier to review and follow the code.

pablomartin4btc · 2025-02-03T17:57:07Z

src/qml/bitcoinamount.cpp

+{
+    QString result = text;
+
+    // Remove any invalid characters


Perhaps the above has been missed?

GBKS · 2025-02-04T13:40:27Z

Look & feel

@pablomartin4btc please see the rationale in the design docs here. Great to hear your feedback, and I'd like to start tracking feedback like this, but wait with acting on it after the MVP release and we have had more chance to actually use it and do user testing.

johnny9 · 2025-02-05T14:32:56Z

Update from efe0986 to 9ad34f0

Rebased with main

MarnixCroes

9ad34f0

it's a bit hard to review/test as it is not clear to me which things are finale and which are for follow ups.
for example

Continue button should be Create payment request when amount/label/message inputs are given? (instead of always Create bitcoin address)
validating the amount, i.e. not more than max BTC supply etc
Clear button only "clears" address entry field

and several other nits

This control is primarily used in wallet forms where the user is expected to input values for sending or receiving transactions. It contains a Label describing the input as well as a TextInput where the actual string is entered. A Icon is also available on the right side of the component for extra actions.

The copy icon will be used to initiate a copy to clipboard when it is clicked.

The flip-veritcal icon is primarily used when flipping the units displayed when clicked

The pending icon will be used to show the status of transactions

BitcoinAmount is a helper object can be used to manage inputs in sats or btc.

This page contains the form for the user to fill out to create a payment request.

johnny9 · 2025-03-10T15:22:04Z

Update from cb6d96a to 13535ca

Rebased with main and squashed RequestPayment form commits

johnny9 · 2025-03-11T12:46:05Z

Tested on Ubuntu:
Click Receive tab and make sure QML loads
@jarolrod

davidgumberg · 2025-03-12T17:20:48Z

src/qml/pages/wallet/RequestPayment.qml

+
+                spacing: 5
+
+                Item {


nit: Maybe amount input deserves it's own control?

Yes, it will be. I haven't committed to doing that just yet as I don't have a clear idea of exactly how the BitcoinAmounts and the Bitcoin Address inputs will be handled. I think the PR to do the input formatting of the amounts should take care of having this component extracted out with clear signals/properties.

davidgumberg · 2025-03-12T17:21:39Z

src/qml/pages/wallet/RequestPayment.qml

+                        anchors.left: parent.left
+                        anchors.verticalCenter: parent.verticalCenter
+                        horizontalAlignment: Text.AlignLeft
+                        color: Theme.color.neutral9


nit: should be neutral7 like the other labels

I'll double check this. There has been some flip flopping on the design side but it should be decided now

davidgumberg · 2025-03-12T17:30:35Z

src/qml/pages/wallet/RequestPayment.qml

+                            requestCounter = requestCounter + 1
+                            clearRequest.visible = true
+                            title.text = qsTr("Payment request #" + requestCounter)
+                            address.text = "bc1q f5xe y2tf 89k9 zy6k gnru wszy 5fsa truy 9te1 bu"


Not likely to happen, but it seems to me like a bad idea to put a placeholder address here when someone curious might build this branch and try to use it with mainnet funds, maybe insert an invalid character or something else to indicate this is a placeholder?

Ill remove the address from the code asap

hebasto

ACK 13535ca, I have reviewed the code and it looks OK.

Unaddressed nits can be resolved in follow-ups after receiving input from designers.

johnny9 changed the title ~~Request Payment page~~ Initial Template for Request Payment page Nov 22, 2024

MarnixCroes reviewed Nov 22, 2024

View reviewed changes

johnny9 force-pushed the request-payment-form branch 2 times, most recently from e642461 to adff8f2 Compare November 25, 2024 18:39

johnny9 mentioned this pull request Dec 9, 2024

Create payment request form for Desktop johnny9/BitcoinCoreAppDevelopment#26

Open

jarolrod added Needs rebase Wallet labels Dec 17, 2024

johnny9 force-pushed the request-payment-form branch from 622a114 to efe0986 Compare February 2, 2025 18:03

pablomartin4btc reviewed Feb 3, 2025

View reviewed changes

src/qml/bitcoinamount.cpp

{

QString result = text;

// Remove any invalid characters

Copy link

Contributor

pablomartin4btc Feb 3, 2025

Choose a reason for hiding this comment

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

Perhaps the above has been missed?

johnny9 force-pushed the request-payment-form branch from efe0986 to 9ad34f0 Compare February 5, 2025 01:39

MarnixCroes reviewed Feb 13, 2025

View reviewed changes

johnny9 added 5 commits March 10, 2025 11:18

qml: Add copy icon

e8dd09d

The copy icon will be used to initiate a copy to clipboard when it is clicked.

qml: Add flip-vertical icon

f7cc57d

The flip-veritcal icon is primarily used when flipping the units displayed when clicked

qml: Add pending icon

b2d8c7b

The pending icon will be used to show the status of transactions

qml: Introduce BitcoinAmount

9f4411c

BitcoinAmount is a helper object can be used to manage inputs in sats or btc.

johnny9 force-pushed the request-payment-form branch from 9ad34f0 to cb6d96a Compare March 10, 2025 15:19

qml: Introduce RequestPayment page

13535ca

This page contains the form for the user to fill out to create a payment request.

johnny9 force-pushed the request-payment-form branch from cb6d96a to 13535ca Compare March 10, 2025 15:21

hebasto removed the Needs rebase label Mar 11, 2025

davidgumberg reviewed Mar 12, 2025

View reviewed changes

hebasto approved these changes Mar 17, 2025

View reviewed changes

hebasto merged commit 100eedf into bitcoin-core:main Mar 17, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Template for Request Payment page #435

Initial Template for Request Payment page #435

johnny9 commented Nov 22, 2024 •

edited

Loading

johnny9 commented Nov 22, 2024 •

edited

Loading

MarnixCroes Nov 22, 2024

pablomartin4btc Feb 3, 2025

davidgumberg Mar 12, 2025

hebasto Mar 17, 2025

MarnixCroes commented Nov 22, 2024

johnny9 commented Nov 25, 2024

GBKS commented Nov 28, 2024

johnny9 commented Dec 1, 2024

johnny9 commented Dec 9, 2024

GBKS commented Dec 11, 2024

GBKS commented Jan 29, 2025

johnny9 commented Feb 2, 2025

GBKS commented Feb 3, 2025

pablomartin4btc left a comment

pablomartin4btc Feb 3, 2025

GBKS commented Feb 4, 2025

johnny9 commented Feb 5, 2025

MarnixCroes left a comment

johnny9 commented Mar 10, 2025 •

edited

Loading

johnny9 commented Mar 11, 2025

davidgumberg Mar 12, 2025

johnny9 Mar 17, 2025

davidgumberg Mar 12, 2025

johnny9 Mar 17, 2025

davidgumberg Mar 12, 2025 •

edited

Loading

hebasto Mar 17, 2025

johnny9 Mar 17, 2025

hebasto left a comment

Initial Template for Request Payment page #435

Initial Template for Request Payment page #435

Conversation

johnny9 commented Nov 22, 2024 • edited Loading

johnny9 commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarnixCroes commented Nov 22, 2024

johnny9 commented Nov 25, 2024

GBKS commented Nov 28, 2024

johnny9 commented Dec 1, 2024

johnny9 commented Dec 9, 2024

GBKS commented Dec 11, 2024

GBKS commented Jan 29, 2025

johnny9 commented Feb 2, 2025

GBKS commented Feb 3, 2025

pablomartin4btc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GBKS commented Feb 4, 2025

johnny9 commented Feb 5, 2025

MarnixCroes left a comment

Choose a reason for hiding this comment

johnny9 commented Mar 10, 2025 • edited Loading

johnny9 commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgumberg Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

johnny9 commented Nov 22, 2024 •

edited

Loading

johnny9 commented Nov 22, 2024 •

edited

Loading

johnny9 commented Mar 10, 2025 •

edited

Loading

davidgumberg Mar 12, 2025 •

edited

Loading