getodk / collect

ODK Collect is an Android app for filling out forms. It's been used to collect billions of data points in challenging environments around the world. Contribute and make the world a better place! ✨📋✨
https://docs.getodk.org/collect-intro
Other
719 stars 1.37k forks source link

Opening two forms by double-clicking on Nexus 5X(Android 8.1) #5316

Open dbemke opened 2 years ago

dbemke commented 2 years ago

ODK Collect version

v2021.3.4, the store version (v2022.3.6), master 8edc6ea06e23dfa7c453fde86efb81e16d591085

Android version

8.1

Device used

Nexus 5X

Problem description

Just after downloading the ODK Collect app on Nexus 5x with Andorid 8.1 the user is able to double-click on the form (open two forms). I wasn’t able to reproduce it unless it was the first attempt after downloading the app. I checked other Android versions but I didn’t manage to reproduce it.

Steps to reproduce the problem

  1. Download ODK Collect app.
  2. Open the demo project.
  3. Go to "Get Blank Form" and download any form.
  4. Go to "Fill Blank Form".
  5. Double-click on the form.
  6. Tap device back button and ignore the changes.

After that the second form opens.

Expected behavior

The user shouldn’t be able to open two forms by double-clicking. Only one form should open.

seadowg commented 1 year ago

@dbemke does this only happen on the Nexus 5X?

dbemke commented 1 year ago

We weren't able to reproduce it on other devices, so I think it's just Nexus 5X. On the Nexus 5X it happens every time after installing the app.

seadowg commented 1 year ago

@grzesiek2010 it looks to me like we should be protected from this in the code (with the MultiClickGuard). Do you agree with that?

grzesiek2010 commented 1 year ago

Yes we protect it using MultiClickGuard so maybe the device is so slow that the 1s click debounce is not enough. @dbemke do you think it takes more than 1s between clicking on the form for the first time and moving to the form view?

dbemke commented 1 year ago

I had a look again and opening a form for the first time after installing the app is slower (compared to opening second time etc). It takes some time because I'm able to to tap the form more than twice before it opens, so probably more than 1 second.

grzesiek2010 commented 1 year ago

so probably more than 1 second

So it is what it is. I think we shouldn't change that 1s click debounce because it would have bad impact on other views/elements.

seadowg commented 1 year ago

I had a look again and opening a form for the first time after installing the app is slower (compared to opening second time etc). It takes some time because I'm able to to tap the form more than twice before it opens, so probably more than 1 second.

Could you try with multiple forms rather than reinstalling the app? It would be good to know if this happens on the first time any forms loads or just every time a form is loaded for the first time.

dbemke commented 1 year ago

I tried multiple times before filling the issue and this is what I managed to check/ reproduce or eliminate:

seadowg commented 1 year ago

@grzesiek2010 the only thing I can think of is that there is some JIT compilation happening when a form is clicked for the first time that creates a delay. We could consider hardening MultiClickGuard with a lock as well as a timer: don't let another click execute until the code for the last has finished executing. This wouldn't work with an if and allowClick but would work with the newer MultiClickSafeOnClickListener. I'll make a little prototype of that now and share with @dbemke to play with.

grzesiek2010 commented 1 year ago

We can take a deeper look at it but it's not important so I would rather move it to v2023.1

seadowg commented 1 year ago

Yup. We tried with a lock to see if it fixed it, but it didn't. I'd like to say we don't have to worry about this as it's on one device, but I can't work out what's happening here and that makes me suspicious. Will schedule this for the next release as @grzesiek2010 suggests.

grzesiek2010 commented 1 year ago

Will schedule this for the next release as @grzesiek2010 suggests.

You meant v2023.1 not v2022.4 right?

seadowg commented 1 year ago

@grzesiek2010 whoops! Thanks for catching that.

grzesiek2010 commented 1 year ago

I've tried to reproduce the issue but to no avail. I've created a really slow configuration on an emulator and even tried huge forms but everything was fine. I think the list of forms is something that we might want to protect more than other elements of the app because opening multiple forms might cause strange problems/crashes. Maybe we should block those buttons for a little bit longer than 1s? Maybe 3s? Something like this: https://github.com/getodk/collect/commit/5e69f8b8da7f7d2f0c88f0592348adbe3240a383 @seadowg what do you think?

seadowg commented 1 year ago

Maybe we should just prevent further clicks by throwing up a "loading" dialog? I'm still confused as to what is taking time here. Maybe we need to investigate reducing the amount of synchronous work happening in FormEntryActivity#onCreate?

On a related note, I think we can probably simplify our MultiClickGuard code: we currently only block clicks that have a matching "class name", but it looks like this is applied really inconsistently, and I'd imagine isn't really helping (it might even by causing problems).

grzesiek2010 commented 1 year ago

Maybe we should just prevent further clicks by throwing up a "loading" dialog?

Theoretically, there is a loading dialog when a form is being loaded.

On a related note, I think we can probably simplify our MultiClickGuard code: we currently only block clicks that have a matching "class name", but it looks like this is applied really inconsistently, and I'd imagine isn't really helping (it might even by causing problems).

As I remember the rationale was not to block fast users :smile: I mean without that users might experience slow performance when they click really fast, for example FIll Blank Form and then a form they want to open.

seadowg commented 1 year ago

Theoretically, there is a loading dialog when a form is being loaded.

It's part of the FormEntryActivity though, so won't be shown until we've left the form list screen. I'm suggesting showing a dialog immediately after a form list item is clicked. It might be hard to know when to hide that though...

Would a good first step here be to investigate where a delay could be happening? In theory, the Activity transition should be too fast for us to need to up the debounce value.

As I remember the rationale was not to block fast users 😄 I mean without that users might experience slow performance when they click really fast, for example FIll Blank Form and then a form they want to open.

Ah right! Hmmm, well we should review how it's being used as part of any work we do here then to check we're passing the correct values through. I'd be tempted to experiment with dropping the debounce down to something lower and get rid of the className thing if I'm being honest, but I do see how that might introduce multi click problems.