Skip to content
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

Added option Immediate for passcode timeout. #4204

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

parneet-guraya
Copy link
Contributor

Fix: #4173

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed
  • 🔖 Capability is checked or not needed
  • 🔙 Backport requests are created or not needed: /backport to stable-xx.x
  • 📅 Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

Signed-off-by: parneet-guraya <[email protected]>
@parneet-guraya parneet-guraya added the 3. to review Waiting for reviews label Sep 13, 2024
@parneet-guraya parneet-guraya self-assigned this Sep 13, 2024
Copy link
Contributor

Codacy

Lint

TypemasterPR
Warnings8693
Errors131131

SpotBugs

CategoryBaseNew
Bad practice66
Correctness1111
Dodgy code7878
Internationalization33
Malicious code vulnerability33
Performance66
Security11
Total108108

Lint increased!

Copy link
Contributor

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/4204-talk.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud Talk app.

@sowjanyakch
Copy link
Contributor

@parneet-guraya - I tested this and lock screen is always active on my Samsung A52 (because inactivity time here is set 0, which practically means no inactive time?). It works when inactivity time is set to 1 sec.

@parneet-guraya
Copy link
Contributor Author

@parneet-guraya - I tested this and lock screen is always active on my Samsung A52 (because inactivity time here is set 0, which practically means no inactive time?). It works when inactivity time is set to 1 sec.

@sowjanyakch
Hmm... If by always active you mean that if we leave and comeback to the app immediately , lock screen shows? then it is what I aimed for (basically no time window (timeout) i.e 0, lock will be active immediately). Let me know if you meant something else :)

@sowjanyakch
Copy link
Contributor

@parneet-guraya When the device's screen lock is set to "Immediately", the screen should lock as soon as you stop interacting with the app. That's the behavior you aimed for and this PR does this job. When I tested with my Samsung A52, screen lock sets immediately after I stop interacting with app. Then I entered password to unlock screen. Screen is unlocked and then immediately locks again, prompting me to re-enter password. This is a repeated behavior and I had to uninstall app to be able to use it again. What is your testing device and what is the output you are getting?

@parneet-guraya
Copy link
Contributor Author

the screen should lock as soon as you stop interacting with the app.

@sowjanyakch , okay this got me, 'by not interacting' I interpreted that the app's still in foreground but user's not using it. But, this is not what you meant right? Just to be on the same page, this is how the functionality should work ->

  • App goes to background when either user press the recent screen button/swipe and hold gesture or completely goes back to home. On return the lock should show.

It's a bit strange behavior that you're seeing. I have tested on these devices and works fine-->

  • Oneplus (Android 14)
  • Realme (Android 12)
  • Google pixel 6a (Android 14)

Although there's another bug that executes the dialog multiple times , this is how you can reproduce ->

  • When prompted for password try loosing the focus by either clicking on the home button or the one that shows recent apps. Now this happens regardless of what timeout preference we have.

Is that what you're seeing?

copy.mp4

@sowjanyakch
Copy link
Contributor

  • App goes to background when either user press the recent screen button/swipe and hold gesture or completely goes back to home. On return the lock should show.

You are right.

Is that what you're seeing?

copy.mp4

Open app and set screen lock inactivity timeout to "Immediate" and press recent screen button.
Immediately open the app from the recents screen - I see this bug when I try to unlock screen. It happens only when timeout option is set to Immediate.

@parneet-guraya
Copy link
Contributor Author

Open app and set screen lock inactivity timeout to "Immediate" and press recent screen button. Immediately open the app from the recents screen - I see this bug when I try to unlock screen. It happens only when timeout option is set to Immediate.

Strange..., okay when you set immediate option it doesn't show the fingerprint dialog just after setting it? Because for me this bug only occurs when dialog is showing and then I try to loose focus by going back. Also, if it's possible could you post the video of the behavior too?

@sowjanyakch
Copy link
Contributor

sowjanyakch commented Sep 18, 2024

Open app and set screen lock inactivity timeout to "Immediate" and press recent screen button. Immediately open the app from the recents screen - I see this bug when I try to unlock screen. It happens only when timeout option is set to Immediate.

Please change the Screen lock type to PIN or Pattern and try to reproduce the issue.

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@mahibi
Copy link
Collaborator

mahibi commented Oct 9, 2024

What's the state for this PR?
@sowjanyakch @parneet-guraya IIRC setting the value to
<item>1</item>
and just call it
<item>@string/nc_screen_lock_timeout_immediate</item>
solved the problem that lock screen blocks the app and everything was just working as expected?
Is thee any reason against this?

Edit:
i added a comment on #3815 (comment) which could have a bigger impact on this topic.
So the "immediate" option for the current implementation would be an interim solution at best.

@parneet-guraya
Copy link
Contributor Author

@mahibi , It worked fine on the devices I was testing on except @sowjanyakch 's samsung device. So, my idea was to fix the Lock screen first. Since, you proposed the idea (#3815) to integrate this functionality in the library. Until that finishes, it is blocked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we have passcode timeout Immediate?
3 participants