search

commons-app / apps-android-commons

The Wikimedia Commons Android app allows users to upload pictures from their Android phone/tablet to Wikimedia Commons
https://commons-app.github.io/
Apache License 2.0
1.03k stars 1.23k forks source link

[Bug]: BottomSheet Multiple Instance are created on multiple taps #5580

Open neeldoshii opened 8 months ago

neeldoshii commented 8 months ago

Summary

When the more from Bottom Navigation is tapped the bottom sheet opens. But if we tap multiple times the instance of bottom sheet is created multiple times this could lead to

  1. Performance issues as more memory is being consumed when multiple instance are created.
  2. Users Needs to click on back multiple times
  3. Memory Leak.

Device Info

API level: 34 Android version: 14 Device manufacturer: Google Device model: sdk_gphone64_arm64 Device: emu64a Network type: wifi App version name: 4.2.1-debug-main~17855ca37

Steps to reproduce

  1. Click multiple time on More from Bottom Navigation.
  2. Multiple Bottom-sheet is created.

Expected behaviour

Only one instance of bottom sheet to be created even on multiple taps. This can be handled by creating flags when the bottomsheet is opened and if the flag is true no new bottomsheet will be created as already one instance is created.

Actual behaviour

Multiple instance are created.

Device name

No response

Android version

14

Commons app version

4.2.1

Device logs

No response

Screen-shots

https://github.com/commons-app/apps-android-commons/assets/60827173/89a0a93f-f679-4d80-b9e9-69e21dc84ff1

Would you like to work on the issue?

Yes

shashankiitbhu commented 8 months ago

@neeldoshii Isn't this rare to occur practically, I cannot reproduce this on one of my Physical Devices. (You have to click really fast on a very responsive device I suppose) , also the solution you suggest isn't common (or mostly required for creating bottom sheets) - this would add another layer of complexity to it. what do you think? also, I would suggest trying to test this behavior (bottom sheet) on other standard Android apps (Like Google Tasks for example) because this behavior can also be observed there (and many other apps as according to standard documentation you don't need to handle the case that you mentioned)

rohit9625 commented 8 months ago

We can't reproduce this issue as I suspect it happens only on your device because of a recent change you made on a bottom sheet by this PR #5578 Please resolve this behavior in that PR :) There is no need to create another issue for that.

ShashwatKedia commented 8 months ago

We can't reproduce this issue as I suspect it happens only on your device because of a recent change you made on a bottom sheet by this PR #5578 Please resolve this behavior in that PR :) There is no need to create another issue for that.

On, the contrary I was able to reproduce this behaviour on the main branch, though it was very difficult to reproduce and occurred once in 5 tries :(

neeldoshii commented 8 months ago

Isn't this rare to occur practically, I cannot reproduce this on one of my Physical Devices. (You have to click really fast on a very responsive device I suppose)

@shashankiitbhu No, this is common to occur on older device where the memory of the device is very low. When you click on the button and it didn't loaded the bottom-sheet faster than usual the user tends to perform multiple tap on it which leads to creating multiple instance. Considering your point if you consider newer device its rare to occur but thinking from x% of users who are still using >Android 10 version device with less than 2-3GB ram device. Its worth considering wide range of users.

This would add another layer of complexity to it. what do you think?

This won't be complex task and it won't be large refactoring. It would just require to pass bundle to the bottomsheet. if the bottomsheet claas bundle is null --> create instance else --> its instance is already created and won't create multiple sheet. The refactor would be less than 30 lines of code.

I would still like to have feedback from other reviewers. Thanks

shashankiitbhu commented 8 months ago

This won't be complex task and it won't be large refactoring. It would just require to pass bundle to the bottomsheet.

I understand that it won't be a difficult task but my point is it's rather unnecessary and adding flags like this before opening the bottom sheet is not really a recommended method.

As I mentioned in a previous comment that you should check out some standard Android apps (built by Google itself following the Best Practices) , there too you can reproduce this issue mentioned. (Google tasks for example)

My suggestion wasn't related to the complexity of tasks but rather maintaining the best practices and code quality.

If we were to act on every possibility like this (let's take slow response time on older devices for example) that would require is to handle 100s of unnecessary cases , you can reproduce this kind of discrepancies on older devices for almost every app available on Play Store.

neeldoshii commented 8 months ago

We can't reproduce this issue as I suspect it happens only on your device because of a recent change you made on a bottom sheet by this PR https://github.com/commons-app/apps-android-commons/pull/5578 Please resolve this behavior in that PR :) There is no need to create another issue for that.

@rohit9625 Thanks for your reply. The issue was tested on the main branch. Coming toward PR #5578, this PR only contains refactors (adding one of the textview, add onClickFunction to open it in the browser) thats it no creation of bottom sheet is touched here. Also this PR is not yet merged into main branch. This issue solely relies towards the bottomsheet instance creation. Correct me if I am wrong.

I have tested the issue on my 2-3 older testing device and did stress testing towards it. In order to reproduce, you need to perform multiple taps (double, thriple tap on the more button). The bottomsheet is created when the button is clicked so if the button is tapped twice thrice so its creates instance on the number of click done.

I hope I cleared your point. Thanks

On, the contrary I was able to reproduce this behaviour on the main branch, though it was very difficult to reproduce and occurred once in 5 tries :(

Yes, This is highly debatable topic I would say lets work on this issue based on the conclusion based on the discussion, complexity and time. This issue won't have high impact though but will work towards the increasing small performance.

rohit9625 commented 8 months ago

@neeldoshii Apologies for the confusion! I misunderstood the problem; I initially thought it might be related to activity creation, possibly caused by certain conditional statements.

However, I haven't pulled from the main branch in a while, so I can't reproduce the issue on my device. Thus, I suspected it might be related to a recent pull request.