thunderbiscuit / padawan-wallet

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

Bug: App crashes if the user clicks any button before the sync is completed. #187

Closed masterchief164 closed 1 year ago

masterchief164 commented 2 years ago

On the home screen if the user clicks either the send or the receive buttons the app crashes. No logs could be found as the app crashes before the logger can connect with Android studio. padawan bug

The issue can be seen in the GIF.

darkvoid32 commented 2 years ago

Seems like the same issue as #139. I thought that using the rootView would have fixed the issue, but seems that I am wrong about that.

darkvoid32 commented 2 years ago

Now that I look at the logs again : java.lang.IllegalArgumentException: No suitable parent found from the given view. Please provide a valid view.

Do you think that changing adding if (requireView().parent != null) before calling the snackbar (which is causing the crash) would solve this? Something like :

if (requireView().parent != null)
            fireSnackbar(requireView(), SnackbarLevel.INFO, "Wallet successfully synced!")

In which case we can just remove rootView variable since it does not solve the problem.

thunderbiscuit commented 2 years ago

Yeah I think something like that could work, or just a try/catch statement. It's not yet exactly the behaviour I totally want so we'll keep iterating on it, and because the wallet does sync no matter what (here it's just an issue of UI and snackbars), it's not a big deal if the snackbar doesn't show.

Plus snackbars in Compose are a bit differently handled, so I'm not sure yet how that will come in. For now I think a simple check for the parent view would solve our issue. @masterchief164 how does that sound?

masterchief164 commented 2 years ago

I think it would be better to add a progress bar and disable the all other buttons while the wallet is syncing and remove the progress bar and enable the buttons it once it's done.

thunderbiscuit commented 2 years ago

I like the idea of some visual to let the user know that the wallet is syncing! I wonder how we might integrate that in the future. A few thoughts on this however:

Disabling everything else while sync is happening is not quite the workflow we want, mostly because the sync call really isn't blocking at all (it's fired in a coroutine in a background thread); the bug comes from navigating to a different view and trying to draw a snackbar on a view that doesn't exist anymore. You can also imagine a sync call to a home bitcoin node over Tor that would be super slow to respond: in this case freezing navigation and buttons isn't as nice, also because whether the wallet is synced or not is not a blocker for doing other things.

Adding the progress bar might be interesting but I do think it would be a fully new feature (we could open a new issue for discussion on that maybe?) whereas for starters it would be great to fix our bug. The other thing about that is that if you want to work on the progress bar you'll have to wait for the Compose stuff to be merged, whereas this bug can probably be fixed immediately with no blockers, so at least we get small wins early on and build on that later..

thunderbiscuit commented 2 years ago

Masterchief are you interested in writing the PR to fix the bug?

masterchief164 commented 2 years ago

I'll take it up once I complete the previous task of adding the slider for the fees.

thunderbiscuit commented 2 years ago

Sounds good! But do checkout my comment on the #125 issue though. It's not ready for tackling just yet.

thunderbiscuit commented 1 year ago

This issue is no longer a problem!