Skip to content

fix: remove redundant permission dialogs and fix language code handling #70

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.

Sign up for GitHub

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

Merged
hubo1989 merged 4 commits into from
Mar 25, 2026
Merged

fix: remove redundant permission dialogs and fix language code handling #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c1a81b9
fix: remove redundant permission dialogs and fix language code handling
Mar 24, 2026
90f521a
fix: address CodeRabbit review feedback
Mar 25, 2026
cf984b0
fix: show system settings guidance after permission denial
Mar 25, 2026
d4f9aab
fix: refresh permission status after system prompt to handle grant
Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions ScreenTranslate/App/Coordinators/TextTranslationCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,17 @@ final class TextTranslationCoordinator {
permissionManager.refreshPermissionStatus()

if !permissionManager.hasAccessibilityPermission {
// Show permission request dialog - direct call since we're already @MainActor
let granted = permissionManager.requestAccessibilityPermission()
// Directly trigger system permission prompt
permissionManager.requestAccessibilityPermission()
permissionManager.refreshPermissionStatus()

if !granted {
// User declined or permission not granted - show error
permissionManager.showPermissionDeniedError(for: .accessibility)
return false
if permissionManager.hasAccessibilityPermission {
return true
}

// Still not granted (user denied) — guide to System Settings
permissionManager.showPermissionDeniedError(for: .accessibility)
return false
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
return true
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -160,7 +163,7 @@ final class TextTranslationCoordinator {
} catch let error as TextTranslationError {
await hideLoadingIndicator()
logger.error("Translation failed: \(error.localizedDescription)")
appDelegate?.showCaptureError(.captureFailure(underlying: error))
showTranslationError(error)

} catch {
await hideLoadingIndicator()
Expand Down Expand Up @@ -218,7 +221,7 @@ final class TextTranslationCoordinator {
}
} catch let error as TextTranslationError {
logger.error("Translation failed: \(error.localizedDescription)")
appDelegate?.showCaptureError(.captureFailure(underlying: error))
showTranslationError(error)
return
} catch {
logger.error("Unexpected error during translation: \(error.localizedDescription)")
Expand All @@ -232,13 +235,28 @@ final class TextTranslationCoordinator {
logger.info("Successfully inserted translated text")
} catch let error as TextInsertService.InsertError {
logger.error("Text insertion failed: \(error.localizedDescription)")
appDelegate?.showCaptureError(.captureFailure(underlying: error))
let alert = NSAlert()
alert.alertStyle = .warning
alert.messageText = String(localized: "textTranslation.error.insertFailed")
alert.informativeText = error.localizedDescription
alert.addButton(withTitle: String(localized: "common.ok"))
alert.runModal()
} catch {
logger.error("Unexpected error during translate and insert: \(error.localizedDescription)")
appDelegate?.showCaptureError(.captureFailure(underlying: error))
}
}

/// Shows an error alert for translation failures
private func showTranslationError(_ error: TextTranslationError) {
let alert = NSAlert()
alert.alertStyle = .warning
alert.messageText = error.errorDescription ?? String(localized: "error.translation.failed")
alert.informativeText = error.recoverySuggestion ?? ""
alert.addButton(withTitle: String(localized: "common.ok"))
alert.runModal()
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// MARK: - UI Helpers

/// Shows a brief loading indicator for text translation
Expand Down
30 changes: 29 additions & 1 deletion ScreenTranslate/Features/Settings/MultiEngineSettingsSection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ struct MultiEngineSettingsSection: View {
} else {
viewModel.settings.parallelEngines[0] = engine
}
viewModel.settings.saveParallelEngines()
}
)) {
// Standard engines
Expand Down Expand Up @@ -187,6 +188,7 @@ struct MultiEngineSettingsSection: View {
} else {
viewModel.settings.parallelEngines.append(engine)
}
viewModel.settings.saveParallelEngines()
}
)) {
// Standard engines
Expand Down Expand Up @@ -239,6 +241,7 @@ struct MultiEngineSettingsSection: View {
ForEach(enabledEngines.filter { $0.id != .custom }, id: \.id) { config in
Button(config.id.localizedName) {
viewModel.settings.parallelEngines[index] = config.id
viewModel.settings.saveParallelEngines()
}
}
if !compatibleConfigs.isEmpty {
Expand All @@ -261,6 +264,7 @@ struct MultiEngineSettingsSection: View {
if viewModel.settings.parallelEngines.count > 1 {
Button {
viewModel.settings.parallelEngines.remove(at: index)
viewModel.settings.saveParallelEngines()
} label: {
Image(systemName: "xmark.circle.fill")
.font(.caption)
Expand All @@ -283,6 +287,7 @@ struct MultiEngineSettingsSection: View {
if !viewModel.settings.parallelEngines.contains(config.id) {
Button {
viewModel.settings.parallelEngines.append(config.id)
viewModel.settings.saveParallelEngines()
} label: {
HStack {
Image(systemName: engineIcon(config.id))
Expand Down Expand Up @@ -361,7 +366,15 @@ struct MultiEngineSettingsSection: View {
engine: engine,
config: Binding(
get: { viewModel.settings.engineConfigs[engine] ?? .default(for: engine) },
set: { viewModel.settings.engineConfigs[engine] = $0 }
set: { newValue in
// Use full property assignment instead of subscript
// to guarantee @Observable persistence
Logger.settings.info("Engine config updated: \(engine.rawValue), isEnabled=\(newValue.isEnabled)")
var configs = viewModel.settings.engineConfigs
configs[engine] = newValue
viewModel.settings.engineConfigs = configs
viewModel.settings.saveEngineConfigs()
}
)
)
}
Expand All @@ -376,6 +389,7 @@ struct MultiEngineSettingsSection: View {
} else {
viewModel.settings.compatibleProviderConfigs[state.index] = savedConfig
}
viewModel.settings.saveCompatibleConfigs()
}
)
}
Expand Down Expand Up @@ -503,6 +517,7 @@ struct MultiEngineSettingsSection: View {

await MainActor.run {
viewModel.settings.compatibleProviderConfigs.remove(at: index)
viewModel.settings.saveCompatibleConfigs()
}
} catch {
// Log error but don't remove config if credential migration fails
Expand Down Expand Up @@ -538,7 +553,9 @@ struct MultiEngineSettingsSection: View {
customConfig.customName = jsonString
customConfig.isEnabled = true
viewModel.settings.engineConfigs[.custom] = customConfig
viewModel.settings.saveEngineConfigs()
viewModel.settings.parallelEngines[index] = .custom
viewModel.settings.saveParallelEngines()
}
}
} catch {
Expand Down Expand Up @@ -567,7 +584,9 @@ struct MultiEngineSettingsSection: View {
customConfig.customName = jsonString
customConfig.isEnabled = true
viewModel.settings.engineConfigs[.custom] = customConfig
viewModel.settings.saveEngineConfigs()
viewModel.settings.parallelEngines.append(.custom)
viewModel.settings.saveParallelEngines()
}
}
} catch {
Expand Down Expand Up @@ -599,13 +618,15 @@ struct MultiEngineSettingsSection: View {
customConfig.customName = jsonString
customConfig.isEnabled = true
viewModel.settings.engineConfigs[.custom] = customConfig
viewModel.settings.saveEngineConfigs()

// Set as primary engine
if viewModel.settings.parallelEngines.isEmpty {
viewModel.settings.parallelEngines = [.custom]
} else {
viewModel.settings.parallelEngines[0] = .custom
}
viewModel.settings.saveParallelEngines()
}
}
} catch {
Expand All @@ -617,6 +638,7 @@ struct MultiEngineSettingsSection: View {
@ViewBuilder
private func engineCard(_ engine: TranslationEngineType) -> some View {
let config = viewModel.settings.engineConfigs[engine] ?? .default(for: engine)
let _ = Logger.settings.info("engineCard \(engine.rawValue): isEnabled=\(config.isEnabled), fromDefault=\(viewModel.settings.engineConfigs[engine] == nil)")
// Built-in engines (apple, mtranServer) and Ollama don't need API keys
// For others, we check if they require API key (simplified check - in real use would check keychain)
let isConfigured = !engine.requiresAPIKey || config.isEnabled
Expand Down Expand Up @@ -697,6 +719,7 @@ struct MultiEngineSettingsSection: View {
} else {
viewModel.settings.parallelEngines.append(config.id)
}
viewModel.settings.saveParallelEngines()
}
}

Expand Down Expand Up @@ -738,6 +761,7 @@ struct MultiEngineSettingsSection: View {
private func toggleCompatibleEngineInParallel(config: CompatibleTranslationProvider.CompatibleConfig) {
if isCompatibleEngineSelected(config) {
viewModel.settings.parallelEngines.removeAll { $0 == .custom }
viewModel.settings.saveParallelEngines()
} else {
// Set this compatible config as custom and add to parallel engines
var customConfig = viewModel.settings.engineConfigs[.custom] ?? .default(for: .custom)
Expand All @@ -746,8 +770,10 @@ struct MultiEngineSettingsSection: View {
customConfig.customName = jsonString
customConfig.isEnabled = true
viewModel.settings.engineConfigs[.custom] = customConfig
viewModel.settings.saveEngineConfigs()
if !viewModel.settings.parallelEngines.contains(.custom) {
viewModel.settings.parallelEngines.append(.custom)
viewModel.settings.saveParallelEngines()
}
}
}
Expand Down Expand Up @@ -794,13 +820,15 @@ struct MultiEngineSettingsSection: View {
customConfig.customName = jsonString
customConfig.isEnabled = true
viewModel.settings.engineConfigs[.custom] = customConfig
viewModel.settings.saveEngineConfigs()
binding.primaryEngine = .custom
}
}
} else {
binding.primaryEngine = TranslationEngineType(rawValue: newValue) ?? .apple
}
viewModel.settings.sceneBindings[scene] = binding
viewModel.settings.saveSceneBindings()
}
)) {
ForEach(enabledEngines.filter { $0.id != .custom }, id: \.id) { config in
Expand Down
Loading