thunderbiscuit / padawan-wallet

The bitcoin wallet trainer on Android.
https://padawanwallet.com/
Apache License 2.0
116 stars 51 forks source link

Refactor build/broadcast fragments #111

Closed thunderbiscuit closed 3 years ago

thunderbiscuit commented 3 years ago

This issue will track the discussion around the refactoring of the Build and Broadcast fragments.

@sunidhi64 you're in charge! Would you mind describing a bit more what you have in mind for the refactoring?

sunidhi64 commented 3 years ago

I have looked up at the material alert dialog box, we can remove the the broadcast fragment and add a dialogue fragment instead. The dialogue fragment will ask the user to check if the address, amount and the fees inputted is correct or not. If it's correct, user can proceed to broadcast the transaction.

After the above feature is implemented, we can think of adding variable fee rate?

thunderbiscuit commented 3 years ago

Yes agreed! I think the variable fee rate as a separate PR makes sense.

The only thing I would say is that our problem with the Build/Broadcast fragments was that they were, indeed, two fragments. So one had to build the transaction (to check for the validity of the inputs), and then either toss it away and rebuild it in the next fragment (sort of what we do currently), or try to pass this pre-built transaction across the fragments. Passing it requires we serialize or parcelize it I think, and so that's another layer of complexity.

I'd suggest we move to an AlertDialog that is within the same fragment as the Build fragment we currently have, so as not to have to parcelize and pass that transaction object across fragments. You can take a look at how we did it in Sobi Wallet (Material Alert Dialog that stays within the current fragment). Does that sound good? Or did you have another reason to try and build a new fragment maybe that I didn't think about?

sunidhi64 commented 3 years ago

I searched about Material Alert Dialog and found that it works good within the same fragment until the phone`s configuration is not changed, like if we rotate our device then the dialog will not work. Otherwise we can have the AlertDialog in the the Build Fragment. I looked at how we did it in Sobi Wallet.

Another thing what design should we implement for the dialog box?

thunderbiscuit commented 3 years ago

Yes that's true, a change in config will destroy the data unless we specifically add it to the bundle so it can be retrieved in the case of a lifecycle change. But in this case I assume it's not too likely, particularly because I have disabled the landscape mode (so users can't trigger a configuration change by switching from portrait landscape). They could of course change apps while in the dialog, and the OS might decide to kill the app, which would also loose the data, but I don't think it's too big of a problem at the moment. Also, if the user changes apps or kills the app or something, we would like them to have to go through the process of verifying again (so the build and verification happen very closely together, to limit errors).

For design... what do you propose? I have a basic dialog I've been using for the very first fragment (where it says "Be Careful! you have to understand blablabla) and also for the "Hey we notice it's your first time opening this wallet would you like testnet coins blablabla). How do you feel about those? Good enough? I'm happy to see better designs too if you have other ideas.

sunidhi64 commented 3 years ago

Yes, then we can proceed to add the Alert Dialog within Build Fragment.

Yes the design is good but like if I want to make the sub-headings darker, can we do that? like send to address becomes bold.

thunderbiscuit commented 3 years ago

Yes I'm not sure about how to build a more custom one. Take a look at some of the docs from Google or maybe youtube videos and let me know what you find! You can start a PR with a very simple layout if you prefer, and clean it up later. Up to you!

sunidhi64 commented 3 years ago

Sure! I will look up at the docs and start with the basic first

sunidhi64 commented 3 years ago

I have created a draft pull request, please check the layout and the contents of the dialog box. I have used spannableString to create some changes to the string. Dialog Box

thunderbiscuit commented 3 years ago

This issue is done! Thanks to @sunidhi64.

Next we'll be looking at making the AlertDialog into a Dialog for more flexibility in the UI.