[WIP] 🚧 Rework Platform Views #146
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
This PR was closed because it has been stalled for 7 days with no activity. |
* rough poc of platform views migration * remove unncessary delegate controller * resolve comments * upgrade pigeon, get codegen improvements * updated configurations * code formatting and linting * - remove title bar - add format/lint to build script * - added smartSelfieEnrollment to SmileID class returning SmileIDSdkResult - added deprecation notice for SmileIDSmartSelfieEnrollment widget * remove unnecessary format * remove unused didSubmitBiometricKycJob parameter * - added smile_id_smart_selfie_authentication method - added smile_id_smart_selfie_enrollment_enhanced method - added smile_id_smart_selfie_authentication_enhanced_method * refactor products api to separate implementation * remove xcode file creation comments * remove need for client to wrap application in a UINavigationController * fix linting issues flutter * resolve linting issues on android * - attend to issues with PR - lock orientation on android - set preferred colorScheme to light mode on iOS * remove portrait hosting controller attempt at locking iOS to portrait mode * fix force light mode on ios * add back button * - added document verification product - added document verification enhanced product - added biometric kyc product * - added selfie capture product - added document capture product * linting code --------- Co-authored-by: Juma Allan <allanjuma@gmail.com>
* fixed issue with back button not being aligned at the top * lock orientation using custom smile id controller. * apply perfectionist weak var fix
jumaallan
force-pushed
the
epic/rework-platform-views
branch
from
fd4d146 to
4f72e77
Compare
* Migration from platform views 🚧 (#136) * rough poc of platform views migration * remove unncessary delegate controller * resolve comments * upgrade pigeon, get codegen improvements * updated configurations * code formatting and linting * - remove title bar - add format/lint to build script * - added smartSelfieEnrollment to SmileID class returning SmileIDSdkResult - added deprecation notice for SmileIDSmartSelfieEnrollment widget * remove unnecessary format * remove unused didSubmitBiometricKycJob parameter * - added smile_id_smart_selfie_authentication method - added smile_id_smart_selfie_enrollment_enhanced method - added smile_id_smart_selfie_authentication_enhanced_method * refactor products api to separate implementation * remove xcode file creation comments * remove need for client to wrap application in a UINavigationController * fix linting issues flutter * resolve linting issues on android * - attend to issues with PR - lock orientation on android - set preferred colorScheme to light mode on iOS * remove portrait hosting controller attempt at locking iOS to portrait mode * fix force light mode on ios * add back button * - added document verification product - added document verification enhanced product - added biometric kyc product * - added selfie capture product - added document capture product * linting code --------- Co-authored-by: Juma Allan <allanjuma@gmail.com> * feature: typesafe results for SmileID widgets - SmileSelfieEnrollment * resolve linting issues on android * Added dart linter format to the build script to ensure dart code is formatted before merging * remove format temporarily from build script * update flutter version in analyse and lint build script * - refactor plugin to widget communication - added lint.sh for flutter and kotlin linting * - resolve merge conflicts * update ios flutter version to resolve swift package manager issue * resolve comments * fixed lint issues * retrieve packages --------- Co-authored-by: Juma Allan <allanjuma@gmail.com>
* - typesafe results client/dart apis update - typesafe result backend(ios) api update - typesafe result backend(android) api update * lint fixes * resolve perfectionist suggestions * improve code and apply lint * updated the mapper * undo my smart ass changes * re organized project directories * re organized project directories * updated products to use a more abstract setup * removed unnecessary dependency on binaryMessenger --------- Co-authored-by: Juma Allan <allanjuma@gmail.com>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![prfectionist[bot]](https://avatars.githubusercontent.com/in/1052367?s=60&v=4){kind=small}
Comment on lines
136
to
140
| return <Object?>[ | ||
| jobType?.index, | ||
| jobType, | ||
| jobId, | ||
| userId, | ||
| extras, |
There was a problem hiding this comment.
Suggestion: The code directly uses jobType?.index which will cause a runtime error when the enum value is passed directly in the updated Pigeon version. Update to use jobType directly as it's done in other parts of the updated code. [possible issue, importance: 9]
New proposed code:
Object encode() {
return <Object?>[
- jobType?.index,
+ jobType,
jobId,
userId,
extras,
];
}
Comment on lines
+146
to
+164
| body: SmileIDDocumentVerification( | ||
| countryCode: 'GH', | ||
| documentType: "DRIVERS_LICENSE", | ||
| onResult: (result) { | ||
| switch (result) { | ||
| case SmileIDSdkResultSuccess<DocumentCaptureResult>( | ||
| :final data | ||
| ): | ||
| final snackBar = SnackBar( | ||
| content: Text("Success: ${data.prettyPrint}")); | ||
| ScaffoldMessenger.of(context).showSnackBar(snackBar); | ||
| case SmileIDSdkResultError<DocumentCaptureResult>( | ||
| :final error | ||
| ): | ||
| final snackBar = SnackBar(content: Text("Error: $error")); | ||
| ScaffoldMessenger.of(context).showSnackBar(snackBar); | ||
| } | ||
| Navigator.of(context).pop(); | ||
| }, |
There was a problem hiding this comment.
Suggestion: The pattern matching in the switch statement is missing a default case. If the result is neither a success nor an error, the code will throw an exception. Add a default case to handle unexpected result types. [possible issue, importance: 7]
Suggested change
| body: SmileIDDocumentVerification( | |
| countryCode: 'GH', | |
| documentType: "DRIVERS_LICENSE", | |
| onResult: (result) { | |
| switch (result) { | |
| case SmileIDSdkResultSuccess<DocumentCaptureResult>( | |
| :final data | |
| ): | |
| final snackBar = SnackBar( | |
| content: Text("Success: ${data.prettyPrint}")); | |
| ScaffoldMessenger.of(context).showSnackBar(snackBar); | |
| case SmileIDSdkResultError<DocumentCaptureResult>( | |
| :final error | |
| ): | |
| final snackBar = SnackBar(content: Text("Error: $error")); | |
| ScaffoldMessenger.of(context).showSnackBar(snackBar); | |
| } | |
| Navigator.of(context).pop(); | |
| }, | |
| onResult: (result) { | |
| switch (result) { | |
| case SmileIDSdkResultSuccess<DocumentCaptureResult>( | |
| :final data | |
| ): | |
| final snackBar = SnackBar( | |
| content: Text("Success: ${data.prettyPrint}")); | |
| ScaffoldMessenger.of(context).showSnackBar(snackBar); | |
| case SmileIDSdkResultError<DocumentCaptureResult>( | |
| :final error | |
| ): | |
| final snackBar = SnackBar(content: Text("Error: $error")); | |
| ScaffoldMessenger.of(context).showSnackBar(snackBar); | |
| default: | |
| final snackBar = SnackBar(content: Text("Unexpected result type")); | |
| ScaffoldMessenger.of(context).showSnackBar(snackBar); | |
| } | |
| Navigator.of(context).pop(); | |
| }, |
Comment on lines
+118
to
+133
| SmileID.api.doEnhancedKycAsync( | ||
| FlutterEnhancedKycRequest( | ||
| country: "GH", | ||
| idType: "DRIVERS_LICENSE", | ||
| idNumber: "B0000000", | ||
| callbackUrl: "https://somedummyurl.com/demo", | ||
| partnerParams: FlutterPartnerParams( | ||
| jobType: FlutterJobType.enhancedKyc, | ||
| jobId: userId, | ||
| userId: userId, | ||
| extras: { | ||
| "name": "Dummy Name", | ||
| "work": "SmileID", | ||
| }), | ||
| timestamp: authResponse!.timestamp, | ||
| signature: authResponse.signature)) |
There was a problem hiding this comment.
Suggestion: The FlutterEnhancedKycRequest is missing the consentInformation parameter which was present in the previous code. This parameter is important for compliance with data protection regulations and should be included. [possible issue, importance: 8]
Suggested change
| SmileID.api.doEnhancedKycAsync( | |
| FlutterEnhancedKycRequest( | |
| country: "GH", | |
| idType: "DRIVERS_LICENSE", | |
| idNumber: "B0000000", | |
| callbackUrl: "https://somedummyurl.com/demo", | |
| partnerParams: FlutterPartnerParams( | |
| jobType: FlutterJobType.enhancedKyc, | |
| jobId: userId, | |
| userId: userId, | |
| extras: { | |
| "name": "Dummy Name", | |
| "work": "SmileID", | |
| }), | |
| timestamp: authResponse!.timestamp, | |
| signature: authResponse.signature)) | |
| SmileID.api.doEnhancedKycAsync( | |
| FlutterEnhancedKycRequest( | |
| country: "GH", | |
| idType: "DRIVERS_LICENSE", | |
| idNumber: "B0000000", | |
| callbackUrl: "https://somedummyurl.com/demo", | |
| partnerParams: FlutterPartnerParams( | |
| jobType: FlutterJobType.enhancedKyc, | |
| jobId: userId, | |
| userId: userId, | |
| extras: { | |
| "name": "Dummy Name", | |
| "work": "SmileID", | |
| }), | |
| timestamp: authResponse!.timestamp, | |
| signature: authResponse.signature, | |
| consentInformation: FlutterConsentInformation( | |
| consentGrantedDate: "", | |
| personalDetailsConsentGranted: true, | |
| contactInfoConsentGranted: true, | |
| documentInfoConsentGranted: true))) |
Comment on lines
+61
to
+71
| Future<SmileIDSdkResult<SmartSelfieCaptureResult>> smartSelfieEnrollment({ | ||
| required SmartSelfieCreationParams creationParams, | ||
| }) async { | ||
| try { | ||
| final result = | ||
| await productsInterface.smartSelfieEnrollment(creationParams); | ||
| return SmileIDSdkResultSuccess(result); | ||
| } on PlatformException catch (e) { | ||
| return SmileIDSdkResultError(e.message ?? _defaultSdkErrorMessage); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The smartSelfieEnrollment method and other similar methods in this class are instance methods, but they're not accessible because there's no instance of SmileID being created. These methods should be static to match the class's usage pattern. [possible issue, importance: 9]
Suggested change
| Future<SmileIDSdkResult<SmartSelfieCaptureResult>> smartSelfieEnrollment({ | |
| required SmartSelfieCreationParams creationParams, | |
| }) async { | |
| try { | |
| final result = | |
| await productsInterface.smartSelfieEnrollment(creationParams); | |
| return SmileIDSdkResultSuccess(result); | |
| } on PlatformException catch (e) { | |
| return SmileIDSdkResultError(e.message ?? _defaultSdkErrorMessage); | |
| } | |
| } | |
| static Future<SmileIDSdkResult<SmartSelfieCaptureResult>> smartSelfieEnrollment({ | |
| required SmartSelfieCreationParams creationParams, | |
| }) async { | |
| try { | |
| final result = | |
| await productsInterface.smartSelfieEnrollment(creationParams); | |
| return SmileIDSdkResultSuccess(result); | |
| } on PlatformException catch (e) { | |
| return SmileIDSdkResultError(e.message ?? _defaultSdkErrorMessage); | |
| } | |
| } |
Comment on lines
+425
to
+428
| final formattedResult = jsonEncode({ | ||
| 'documentFrontFile': data.documentFrontFile, | ||
| 'documentBackFile': data.documentBackFile | ||
| }); |
There was a problem hiding this comment.
Suggestion: The code is encoding only a subset of the available data fields from DocumentCaptureResult. If other fields like selfieFile or livenessFiles contain important information, they should be included in the formatted result. [general, importance: 5]
Suggested change
| final formattedResult = jsonEncode({ | |
| 'documentFrontFile': data.documentFrontFile, | |
| 'documentBackFile': data.documentBackFile | |
| }); | |
| final formattedResult = jsonEncode({ | |
| 'documentFrontFile': data.documentFrontFile, | |
| 'documentBackFile': data.documentBackFile, | |
| 'selfieFile': data.selfieFile, | |
| 'livenessFiles': data.livenessFiles, | |
| 'didSubmitDocumentVerificationJob': data.didSubmitDocumentVerificationJob | |
| }); |
Comment on lines
+92
to
103
| viewType: SmileIDSmartSelfieAuthenticationEnhanced.viewType, | ||
| layoutDirection: Directionality.of(context), | ||
| creationParams: creationParams, | ||
| creationParams: widget.creationParams, | ||
| creationParamsCodec: const StandardMessageCodec(), | ||
| onFocus: () { | ||
| params.onFocusChanged(true); | ||
| }, | ||
| ) | ||
| ..addOnPlatformViewCreatedListener(params.onPlatformViewCreated) | ||
| ..addOnPlatformViewCreatedListener(_onPlatformViewCreated) | ||
| ..create(); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Suggestion: The platform view initialization is missing the registration of the API with the platform view. Add a listener for platform view creation to register the API with the newly created view. [possible issue, importance: 8]
Suggested change
| viewType: SmileIDSmartSelfieAuthenticationEnhanced.viewType, | |
| layoutDirection: Directionality.of(context), | |
| creationParams: creationParams, | |
| creationParams: widget.creationParams, | |
| creationParamsCodec: const StandardMessageCodec(), | |
| onFocus: () { | |
| params.onFocusChanged(true); | |
| }, | |
| ) | |
| ..addOnPlatformViewCreatedListener(params.onPlatformViewCreated) | |
| ..addOnPlatformViewCreatedListener(_onPlatformViewCreated) | |
| ..create(); | |
| }, | |
| ); | |
| return PlatformViewsService.initExpensiveAndroidView( | |
| id: params.id, | |
| viewType: SmileIDSmartSelfieAuthenticationEnhanced.viewType, | |
| layoutDirection: Directionality.of(context), | |
| creationParams: widget.creationParams, | |
| creationParamsCodec: const StandardMessageCodec(), | |
| onFocus: () { | |
| params.onFocusChanged(true); | |
| }, | |
| ) | |
| ..addOnPlatformViewCreatedListener(params.onPlatformViewCreated) | |
| ..addOnPlatformViewCreatedListener((id) => api.registerViewId(id)) | |
| ..create(); |
Comment on lines
107
to
110
| return UiKitView( | ||
| viewType: viewType, | ||
| creationParams: creationParams, | ||
| viewType: SmileIDSmartSelfieAuthentication.viewType, | ||
| creationParams: widget.creationParams, | ||
| creationParamsCodec: const StandardMessageCodec(), |
There was a problem hiding this comment.
Suggestion: The iOS UiKitView is missing the onPlatformViewCreated callback to register the API with the platform view. This will prevent the API from receiving callbacks from the native side. [possible issue, importance: 8]
Suggested change
| return UiKitView( | |
| viewType: viewType, | |
| creationParams: creationParams, | |
| viewType: SmileIDSmartSelfieAuthentication.viewType, | |
| creationParams: widget.creationParams, | |
| creationParamsCodec: const StandardMessageCodec(), | |
| return UiKitView( | |
| viewType: SmileIDSmartSelfieAuthentication.viewType, | |
| creationParams: widget.creationParams, | |
| creationParamsCodec: const StandardMessageCodec(), | |
| onPlatformViewCreated: (id) => api.registerViewId(id), | |
| ); |
Comment on lines
123
to
126
| return UiKitView( | ||
| viewType: viewType, | ||
| creationParams: creationParams, | ||
| viewType: SmileIDDocumentVerification.viewType, | ||
| creationParams: widget.creationParams, | ||
| creationParamsCodec: const StandardMessageCodec(), |
There was a problem hiding this comment.
Suggestion: The iOS UiKitView implementation is missing the onPlatformViewCreated callback needed to register the API with the platform view. This will prevent communication between Flutter and native iOS code. [possible issue, importance: 8]
Suggested change
| return UiKitView( | |
| viewType: viewType, | |
| creationParams: creationParams, | |
| viewType: SmileIDDocumentVerification.viewType, | |
| creationParams: widget.creationParams, | |
| creationParamsCodec: const StandardMessageCodec(), | |
| return UiKitView( | |
| viewType: SmileIDDocumentVerification.viewType, | |
| creationParams: widget.creationParams, | |
| creationParamsCodec: const StandardMessageCodec(), | |
| onPlatformViewCreated: (id) => api.registerViewId(id), | |
| ); |
Comment on lines
+1
to
+6
| import 'result_clients_interfaces.dart'; | ||
| import 'smile_id_product_views_api.dart'; | ||
| import 'smile_id_sdk_result.dart'; | ||
| import 'smileid_messages.g.dart'; | ||
|
|
||
| class DocumentVerificationAdapter extends SmileIDProductViewsResultApi { |
There was a problem hiding this comment.
Suggestion: The adapter classes are missing a method to register the view ID with the platform channel. Without this, the platform views won't be able to communicate with the Flutter side. [possible issue, importance: 9]
Suggested change
| import 'result_clients_interfaces.dart'; | |
| import 'smile_id_product_views_api.dart'; | |
| import 'smile_id_sdk_result.dart'; | |
| import 'smileid_messages.g.dart'; | |
| class DocumentVerificationAdapter extends SmileIDProductViewsResultApi { | |
| class DocumentVerificationAdapter extends SmileIDProductViewsResultApi { | |
| final DocumentCaptureResultClient client; | |
| DocumentVerificationAdapter(this.client) { | |
| SmileIDProductsResultApi.setUp(this); | |
| } | |
| void registerViewId(int id) { | |
| setupMethodChannel(SmileIDDocumentVerification.viewType, id); | |
| } |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
Story: https://app.shortcut.com/smileid/story/xxx
Summary
A few sentences/bullet points about the changes
Known Issues
Any shortcomings in your work. This may include corner cases not correctly handled or issues related
to but not within the scope of your PR. Design compromises should be discussed here if they were not
already discussed above.
Test Instructions
Concise test instructions on how to verify that your feature works as intended. This should include
changes to the development environment (if applicable) and all commands needed to run your work.
Screenshot
If applicable (e.g. UI changes), add screenshots to help explain your work.
PR Type
Enhancement, Documentation
Description
Reworked platform views with type-safe results
Added SmileIDProductsApi for unified product handling
Migrated to Pigeon v22.7.1 codegen
Improved error handling and result delivery
Changes walkthrough 📝
7 files
Update Pigeon generated code with new message typesAdd sealed class for type-safe SDK resultsAdd unified API for SmileID product viewsAdd interfaces for handling product resultsAdd adapters for handling product resultsAdd Android implementation of SmileIDProductsApiAdd iOS implementation of SmileIDProductsApi1 files
Update example app to use new type-safe results76 files