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

translation service api key config #379

Merged
merged 41 commits into from
Feb 6, 2024

Conversation

phlpsong
Copy link
Collaborator

Jerry23011
Jerry23011 previously approved these changes Feb 1, 2024
@tisfeng
Copy link
Owner

tisfeng commented Feb 1, 2024

I tried it, it looks great.

I just adjust the settings page window size, make it looks better.

image

@phlpsong
Copy link
Collaborator Author

phlpsong commented Feb 5, 2024

I didn't modify this Picker value by code before, but after I chose a value, everything seems ok, as well as no strange log.

Yes, the value in UserDefaults is an empty string, which will cause this warning.

@CanglongCl
Copy link
Collaborator

I didn't modify this Picker value by code before, but after I chose a value, everything seems ok, as well as no strange log.

Yes, the value in UserDefaults is an empty string, which will cause this warning.

We should replace key type of deepLTranslation to DeepLAPIUsagePriority instead of String

55956

@CanglongCl
Copy link
Collaborator

CanglongCl commented Feb 5, 2024

@CanglongCl Could you kindly share the root cause of this warning? Did update Publish from background cause this warning?

I'm not quite familiar with concurrency as well. But usually I fix those warning with replace all codes that influence UI with DispatchQueue.main.async and it always works. But this time it seems it is caused by old .alert() which is deprecated.

@phlpsong
Copy link
Collaborator Author

phlpsong commented Feb 5, 2024

I didn't modify this Picker value by code before, but after I chose a value, everything seems ok, as well as no strange log.

Yes, the value in UserDefaults is an empty string, which will cause this warning.

We should replace key type of deepLTranslation to DeepLAPIUsagePriority instead of String

55956

I have trid to replace it with DeepLAPIUsagePriority and conforms Defaults.Serializable protocol, but the warning still exists.

@CanglongCl
Copy link
Collaborator

CanglongCl commented Feb 5, 2024

I didn't modify this Picker value by code before, but after I chose a value, everything seems ok, as well as no strange log.

Yes, the value in UserDefaults is an empty string, which will cause this warning.

We should replace key type of deepLTranslation to DeepLAPIUsagePriority instead of String
55956

I have trid to replace it with DeepLAPIUsagePriority and conforms Defaults.Serializable protocol, but the warning still exists.

49343

We should not use rawValue here. Just use type it self.

@tisfeng tisfeng self-requested a review February 5, 2024 16:16
Copy link
Owner

@tisfeng tisfeng 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.

@tisfeng
Copy link
Owner

tisfeng commented Feb 5, 2024

Please continue to review.

@tisfeng tisfeng merged commit 25ec93d into tisfeng:dev Feb 6, 2024
5 checks passed
@tisfeng
Copy link
Owner

tisfeng commented Feb 6, 2024

Good job!

@phlpsong
Copy link
Collaborator Author

phlpsong commented Feb 6, 2024

Thank you guys. 🙌


extension OpenAIModels: EnumLocalizedStringConvertible {
var title: String {
rawValue
Copy link
Owner

Choose a reason for hiding this comment

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

This code looks a bit redundant, I don't see where it's being used.

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.

4 participants