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

perf: enable to customize OpenAI models #493

Merged
merged 15 commits into from
Apr 10, 2024
Merged

Conversation

tisfeng
Copy link
Owner

@tisfeng tisfeng commented Apr 6, 2024

close #489

image

@phlpsong
Copy link
Collaborator

phlpsong commented Apr 6, 2024

Screen.Recording.2024-04-06.at.17.17.20.mp4

I found a issue when testing this pr. When switching models, the menu will occasionally pop up at the wrong cell. When you click on the custom OpenAI service, the menu will pop up in the OpenAI service cell, and vice versa may also occur.

It looks like the service is wrong when popup the menu. You can check the below screenshots which I'm clicking the custom OpenAI service but the service type is OpenAI.
image

This also occurs in the dev branch. We need to take a look.

@tisfeng
Copy link
Owner Author

tisfeng commented Apr 6, 2024

Screen.Recording.2024-04-06.at.17.17.20.mp4
I found a issue when testing this pr. When switching models, the menu will occasionally pop up at the wrong cell. When you click on the custom OpenAI service, the menu will pop up in the OpenAI service cell, and vice versa may also occur.

It looks like the service is wrong when popup the menu. You can check the below screenshots which I'm clicking the custom OpenAI service but the service type is OpenAI. image

This also occurs in the dev branch. We need to take a look.

It looks fixed using setClickBlock instead of setMouseUpBlock.

-        [self.serviceModelButton setMouseUpBlock:^(EZButton *_Nonnull button) {
+        [self.serviceModelButton setClickBlock:^(EZButton *_Nonnull button) {
            mm_strongify(self);
            [self showModelSelectionMenu:button];
        }];

@phlpsong
Copy link
Collaborator

phlpsong commented Apr 7, 2024

I just noticed that you created customOpenAIVaildModels and openAIVaildModels , I would prefer to store them in one place and extract when needed since they are generally the same with the AvailableModels, it maybe redundant to store them.

Please let me know what you think.

@tisfeng
Copy link
Owner Author

tisfeng commented Apr 7, 2024

I just noticed that you created customOpenAIVaildModels and openAIVaildModels , I would prefer to store them in one place and extract when needed since they are generally the same with the AvailableModels, it maybe redundant to store them.

Please let me know what you think.

I know, this code is ugly, but I just wanted to simply finish this PR.

The main problem at the moment is that OpenAIService + ConfigurableService and CustomOpenAIService + ConfigurableService have too much similar and duplicated code.

This part of the code needs to be refactored and optimized, but I'm not very familiar with SwiftUI, so if you'd like, we can open a separate issue later to refactor it, which will make it easier for subsequent feature, like #479 and #492 .

phlpsong
phlpsong previously approved these changes Apr 8, 2024
Copy link
Collaborator

@phlpsong phlpsong left a comment

Choose a reason for hiding this comment

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

LGTM

@tisfeng
Copy link
Owner Author

tisfeng commented Apr 8, 2024

Please continue to review, code duplication will be resolved by this issue #497 .

@tisfeng tisfeng enabled auto-merge (squash) April 10, 2024 15:25
@tisfeng tisfeng merged commit 06929ff into dev Apr 10, 2024
5 checks passed
@tisfeng tisfeng deleted the customize-openai-models branch April 10, 2024 15:26
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.

None yet

2 participants