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

Support iOS 16 Live Text #332

Merged
merged 3 commits into from
Sep 27, 2022
Merged

Support iOS 16 Live Text #332

merged 3 commits into from
Sep 27, 2022

Conversation

ba01ei
Copy link
Contributor

@ba01ei ba01ei commented Sep 21, 2022

This change allows Agrume to optionally enable iOS 16 Live Text.

Demo:

livetext2.mov

@ba01ei ba01ei force-pushed the master branch 3 times, most recently from 921e40d to 1f9d6fd Compare September 23, 2022 16:19
Copy link
Owner

@JanGorman JanGorman left a comment

Choose a reason for hiding this comment

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

Thanks for opening the PR, this looks great and it's a nice addition to the library! I added some suggestions in the PR

Comment on lines 73 to 81
if enableLiveText {
if #available(iOS 16, *) {
if let image = image {
Task {
await analyzeImage(image)
}
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can simplify this expression and get rid of some of the nesting:

Suggested change
if enableLiveText {
if #available(iOS 16, *) {
if let image = image {
Task {
await analyzeImage(image)
}
}
}
}
if #available(iOS 16, *), enableLiveText, let image = image {
Task {
await analyzeImage(image)
}
}

Comment on lines 500 to 520
private func analyzeImage(_ image: UIImage) async {
guard ImageAnalyzer.isSupported else {
return
}

let analyzer = ImageAnalyzer()
let interaction = await MainActor.run {
let interaction = ImageAnalysisInteraction()
imageView.addInteraction(interaction)
return interaction
}
let configuration = ImageAnalyzer.Configuration([.text, .machineReadableCode])
do {
let analysis = try await analyzer.analyze(image, configuration: configuration)
await MainActor.run {
interaction.analysis = analysis
interaction.preferredInteractionTypes = .automatic
}
} catch {
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

What if we changed the method to not be async and instead encapsulate that inside of the method itself. Then we don't need the await MainActor.run dance to construct the interaction. We could then also make use of Task { @MainActor in

Another alternative would be to keep the method async but annotate it to run on the MainActor:

@MainActor
private func analyzeImage(_ image: UIImage) async {
  
}
Suggested change
private func analyzeImage(_ image: UIImage) async {
guard ImageAnalyzer.isSupported else {
return
}
let analyzer = ImageAnalyzer()
let interaction = await MainActor.run {
let interaction = ImageAnalysisInteraction()
imageView.addInteraction(interaction)
return interaction
}
let configuration = ImageAnalyzer.Configuration([.text, .machineReadableCode])
do {
let analysis = try await analyzer.analyze(image, configuration: configuration)
await MainActor.run {
interaction.analysis = analysis
interaction.preferredInteractionTypes = .automatic
}
} catch {
}
}
private func analyzeImage(_ image: UIImage) {
guard ImageAnalyzer.isSupported else {
return
}
let interaction = ImageAnalysisInteraction()
imageView.addInteraction(interaction)
let analyzer = ImageAnalyzer()
let configuration = ImageAnalyzer.Configuration([.text, .machineReadableCode])
Task { @MainActor in
do {
let analysis = try await analyzer.analyze(image, configuration: configuration)
interaction.analysis = analysis
interaction.preferredInteractionTypes = .automatic
} catch {
print(error.localizedDescription)
}
}
}

@ba01ei
Copy link
Contributor Author

ba01ei commented Sep 26, 2022

Thanks a lot for the feedback @JanGorman I have updated the code based on your suggestions.

@ba01ei ba01ei requested a review from JanGorman September 26, 2022 17:12
@JanGorman
Copy link
Owner

Awesome, thanks @ba01ei!

@JanGorman JanGorman merged commit 72009dc into JanGorman:master Sep 27, 2022
Repository owner deleted a comment from CarlosR1993 Sep 27, 2022
@ba01ei
Copy link
Contributor Author

ba01ei commented Sep 27, 2022

Awesome, thanks @ba01ei!

Thanks a lot for merging @JanGorman. Do you plan to add a new release or tag some time soon?

@JanGorman
Copy link
Owner

@ba01ei Sorry, didn't get around to it yesterday. I just published the new version: https://github.com/JanGorman/Agrume/releases/tag/5.8.5

@hyouuu
Copy link
Contributor

hyouuu commented Oct 6, 2022

Screen Shot 2022-10-05 at 10 14 20 PM

@JanGorman @ba01ei did you test it? Seems the dependency was not added

@JanGorman
Copy link
Owner

Interesting, it was working fine when I tried it

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

Successfully merging this pull request may close these issues.

3 participants