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

Provide callback instead of doing UI tasks of calling app #49 #50

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

sunkup
Copy link
Member

@sunkup sunkup commented Feb 19, 2025

Purpose

cert4android shouldn't show the TrustCertificateActivity and a notification by itself. Instead it should execute a callback which is provided by the calling application.

Short description

  • stop passing the foreground state (appInForeground) to cert4android
  • start passing callback function getUserDecision and the coroutine scope in which it should run
  • adapt tests

Note

I would like to avoid the delay(1000) in testCheck_MultipleDecisionsForSameCert_Negative, but I could not make it work using a countdown latch and using a mutex quickly became complicated too. I also thought of launching a 6th seperate coroutine which would watch registry.pendingDecisions.size with a while loop and release the semaphore as soon as 5 pending decisions are reached, but could not make that work either. In the end the delay(1000) is quite simple and seems to work well, but let me know if you find a cleaner way.

@sunkup sunkup added the refactoring Quality improvement of existing functions label Feb 19, 2025
@sunkup sunkup self-assigned this Feb 19, 2025
@sunkup sunkup linked an issue Feb 19, 2025 that may be closed by this pull request
2 tasks
@sunkup sunkup changed the title [WIP] Provide callback instead of doing UI tasks of calling app #49 Feb 19, 2025
@sunkup sunkup force-pushed the 49-provide-callback-instead-of-doing-ui-tasks-of-calling-app branch 3 times, most recently from 3d64c67 to fe9ba2d Compare February 19, 2025 14:42
@sunkup sunkup force-pushed the 49-provide-callback-instead-of-doing-ui-tasks-of-calling-app branch from fe9ba2d to f3d1cc0 Compare February 19, 2025 14:49
@sunkup sunkup force-pushed the 49-provide-callback-instead-of-doing-ui-tasks-of-calling-app branch 7 times, most recently from d88df8e to 2776ce7 Compare February 26, 2025 14:02
@sunkup sunkup force-pushed the 49-provide-callback-instead-of-doing-ui-tasks-of-calling-app branch from 2776ce7 to 7ec1ded Compare February 26, 2025 14:05
@sunkup sunkup marked this pull request as ready for review February 26, 2025 14:15
@sunkup sunkup requested a review from ArnyminerZ February 26, 2025 14:16
@rfc2822
Copy link
Member

rfc2822 commented Feb 27, 2025

I see that the activity was completely removed together with all its strings (+ translations). While that the UI action should be done in the calling application, it could still be helpful to have a helping template/building block in cert4android. It's also not really the task of a calling app to know too much about certificates etc.

So I suggest to keep M3 Composables (most importantly the screen that shows the certificate contents) that could be used by the calling apps. If this would be a "big library", it should probably go into cert4android-ui-m3 or something like that, but in our case I think we can just keep it in cert4android.

@sunkup sunkup removed the request for review from ArnyminerZ March 4, 2025 09:23
@sunkup sunkup requested a review from ArnyminerZ March 4, 2025 12:47
Copy link
Member

@ArnyminerZ ArnyminerZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works correctly

@sunkup
Copy link
Member Author

sunkup commented Mar 10, 2025

@ArnyminerZ I have removed the theme and some of the dependencies

@sunkup sunkup requested a review from ArnyminerZ March 10, 2025 10:09
Copy link
Member

@ArnyminerZ ArnyminerZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Eventually we should move all the UI things to their own module, as Ricki said. But again, this isn't that big of a library, so I don't even think it's worth it. I'd just merge like is.

@sunkup sunkup requested a review from rfc2822 March 10, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Quality improvement of existing functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide callback instead of doing UI tasks of calling app
3 participants