Skip to content

feat: detect bitcoin/monero uri's #1097

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 24, 2025

Conversation

detherminal
Copy link
Contributor

@detherminal detherminal commented Mar 17, 2025

Closes #1069

Detects URI Schemes for Bitcoin and Monero and uses them.

fix: use uri instead of manual parsing

reformat: remove braces and use efficient variables

refactor: add forgotten vars
@detherminal
Copy link
Contributor Author

Just squashed changes.

Copy link
Collaborator

@julian-CStack julian-CStack left a comment

Choose a reason for hiding this comment

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

Would be better to handle all URIs for all coins. That is somewhat set up already here:

static PaymentUriData? parsePaymentUri(

That is already used in several places but mostly just for handling QR code reads such as here:
Future<void> _scanQr() async {

That _scanQr() function can probably be refactored into two functions. One for the scanning and a second for handing the parsing of the content. That parsing function can then be used on paste as well.

Rather than attempting a full check and parse every time a character is typed it would be better to hook into the on paste event. See https://api.flutter.dev/flutter/material/TextField/contextMenuBuilder.html and https://api.flutter.dev/flutter/material/AdaptiveTextSelectionToolbar-class.html

Finally, this PR only applies to the standard mobile send screen when it should be consistent across all send forms where it would make sense to have this such as on desktop (and possibly some other places as well. Would need to look around the code to find out).

@detherminal
Copy link
Contributor Author

Would be better to handle all URIs for all coins. That is somewhat set up already here:

static PaymentUriData? parsePaymentUri(

That is already used in several places but mostly just for handling QR code reads such as here:

Future<void> _scanQr() async {

That _scanQr() function can probably be refactored into two functions. One for the scanning and a second for handing the parsing of the content. That parsing function can then be used on paste as well.

Rather than attempting a full check and parse every time a character is typed it would be better to hook into the on paste event. See https://api.flutter.dev/flutter/material/TextField/contextMenuBuilder.html and https://api.flutter.dev/flutter/material/AdaptiveTextSelectionToolbar-class.html

Finally, this PR only applies to the standard mobile send screen when it should be consistent across all send forms where it would make sense to have this such as on desktop (and possibly some other places as well. Would need to look around the code to find out).

I think this contextMenuBuilder does not detect keyboard paste events like Ctrl+P which is almost what everybody use. Do you think it should be that way? This is the way that covers almost all use cases.

@julian-CStack
Copy link
Collaborator

Yup, I forgot about the contextMenuBuilder limitation/didn't register when looking it up.

I guess onChanged is the only real option without getting crazy and detecting key press combinations, etc.

It would still be good to do some kind of relatively inexpensive check in onChanged before fully attempting a parse of a uri if possible. Trying to avoid any kind of ui jank while typing (even though paste is almost always going to happen here) is a must. Any ui lag while typing isn't nice.

Everything else in my original comment (besides the contextMenuBuilder stuff) is still valid though I believe

@detherminal
Copy link
Contributor Author

Seperated the _scanQr function into two pieces as well as the ones in desktop_send.

@julian-CStack julian-CStack merged commit f1014a6 into cypherstack:staging Mar 24, 2025
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants