No app crash when building QR code#89
Conversation
|
Hi @magshee, thanks for your contribution! I always prefer system APIs over 3rd party libraries, and your PR is very clean and reasonable. I'll test the changes myself next week and go ahead merging it thereafter. |
There was a problem hiding this comment.
Pull request overview
Replaces QR code generation in the project-sharing view to avoid crashes previously triggered by the CarBode library, and simplifies the QR payload construction.
Changes:
- Remove
CarBodeusage and generate QR codes viaCoreImage(CIFilter.qrCodeGenerator). - Replace
@State-based QR payload string with a computed property.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import AVFoundation | ||
| import CarBode | ||
| import SwiftUI | ||
| import CoreImage.CIFilterBuiltins | ||
|
|
||
| struct ShareProjectQRCode: View { | ||
| let project: Project | ||
|
|
||
| @State var dataString = "" | ||
| private static let context = CIContext() | ||
|
|
||
| private var dataString: String { | ||
| let server = project.url.relativeString | ||
| .replacingOccurrences(of: "https://", with: "") | ||
| return "cospend://\(server)/\(project.token)/\(project.password)" | ||
| } | ||
|
|
||
| private var qrCodeImage: UIImage { | ||
| generateQRCode(from: dataString) | ||
| } |
There was a problem hiding this comment.
UIImage is used in this view (qrCodeImage / generateQRCode), but the file doesn't import UIKit. This will fail to compile on iOS unless UIKit is imported (or the implementation is rewritten to use Image(decorative:)/CGImage without UIImage).
| private var qrCodeImage: UIImage { | ||
| generateQRCode(from: dataString) | ||
| } | ||
|
|
||
| var body: some View { | ||
| VStack { | ||
| Text(dataString).font(.caption) | ||
| CarBode.CBBarcodeView(data: $dataString, barcodeType: .constant(.qrCode), orientation: .constant(.up), onGenerated: nil) | ||
| .aspectRatio(contentMode: .fit) | ||
|
|
||
| Image(uiImage: qrCodeImage) | ||
| .interpolation(.none) | ||
| .resizable() | ||
| .scaledToFit() | ||
| } |
There was a problem hiding this comment.
qrCodeImage is a computed property that regenerates a UIImage every time body is recomputed. Since project is immutable here, consider caching the generated image (e.g., compute once in init, or store it in @State and set it in .task/.onAppear) to avoid unnecessary CoreImage work during view updates.
There was a problem hiding this comment.
I currently don't think caching the computed image is necessary, as it will not be generated that often propably. However, I refactored the entire creation logic in a ViewModel and computed the QR Code in the initializer.
I hope this makes it easier to optimize performance, whether that’s your choice now or in the future.
Sounds great! I saw the review comments from copilot and will address the issues in the next days. |
magshee
force-pushed
the
fix/app-crash-when-showing-qr-code
branch
from
18a693f to
1bcfaaf
Compare
|
I reviewed and tested your changes, ready to approve. Though two things I have to mention:
|
InteractionEngineer
merged commit 67ce82e
into
InteractionEngineer:main
I don't want to interfere with the release planning, but just as an information:
Sure, I'll take a look at it. Feel free to assign the issue to me! :) |
|
Thanks for the advice! Annoyingly, I can't assign the issue to you so feel free to grab it. It'll for sure boost my motivation to work with someone else on this project. Looking forward to good progress! |
Closes #88
The app crashes due to an exception from the CarBode library. This was fixed once with PR70, unfortunately after the merge of PR71, these changes have been lost.
Changes from this PR:
CoreImagefunctions instead ofCarBodelibrary to construct the QR codedataStringto reduce view renderings