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

Fix Flutter Issues #271

Merged
merged 6 commits into from
Dec 16, 2024
Merged

Fix Flutter Issues #271

merged 6 commits into from
Dec 16, 2024

Conversation

tobitech
Copy link
Contributor

Story: https://app.shortcut.com/smileid/story/xxx

Summary

  • Expose the initialisers of Selfie capture screen and Enhanced Selfie capture screen publicly so that Flutter can have access to it.

Known Issues

N/A.

Test Instructions

N/A.

Screenshot

N/A

@tobitech tobitech self-assigned this Dec 16, 2024
@tobitech tobitech requested a review from a team as a code owner December 16, 2024 07:37
@prfectionist
Copy link

prfectionist bot commented Dec 16, 2024

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Screen Brightness
The screen brightness is being stored but there's no visible code that restores it. This could affect user experience if the brightness is modified during selfie capture.

@prfectionist
Copy link

prfectionist bot commented Dec 16, 2024

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Best practice
Restore system state by cleaning up modified screen brightness when view is dismissed

Consider restoring the original screen brightness when the view is dismissed by
implementing a cleanup mechanism in a .onDisappear() modifier.

Sources/SmileID/Classes/SelfieCapture/View/EnhancedSelfieCaptureScreen.swift [8]

 private(set) var originalBrightness = UIScreen.main.brightness
 
+var body: some View {
+    GeometryReader { proxy in
+        // ... existing content
+    }
+    .onDisappear {
+        UIScreen.main.brightness = originalBrightness
+    }
+}
+
Suggestion importance[1-10]: 9

Why: This is a critical suggestion addressing a potential resource leak where screen brightness changes aren't properly cleaned up, which could affect user experience across the app.

9
Enhancement
Improve type safety and compilation performance by marking concrete View implementations as final

Mark the struct as final since it's a concrete View implementation that's not meant
to be inherited from.

Sources/SmileID/Classes/SelfieCapture/View/EnhancedSelfieCaptureScreen.swift [3]

-public struct EnhancedSelfieCaptureScreen: View {
+public final struct EnhancedSelfieCaptureScreen: View {
Suggestion importance[1-10]: 4

Why: While adding 'final' to the struct can provide minor compilation optimizations and clearer intent, structs in Swift are already non-inheritable by default, making this suggestion's impact relatively low.

4

Copy link

github-actions bot commented Dec 16, 2024

Warnings
⚠️ The source files were changed, but the tests remain unmodified. Consider updating or adding to the tests to match the source changes.
⚠️

Sources/SmileID/Classes/SmileID.swift#L159 - Function should have complexity 10 or less: currently complexity equals 11 (cyclomatic_complexity)

⚠️

Sources/SmileID/Classes/SmileID.swift#L186 - TODOs should be resolved (- Fix when Michael changes thi...). (todo)

Generated by 🚫 Danger Swift against f2d1e21

Copy link

@robin-ankele robin-ankele left a comment

Choose a reason for hiding this comment

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

Looks good. Great work @tobitech

@tobitech tobitech merged commit db4d026 into main Dec 16, 2024
3 checks passed
@tobitech tobitech deleted the fix-flutter-issues branch December 16, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants