General improvements #35
Conversation
Also removed dialog box which would appear if any error was enountered. Added an option to choose auto language which changes the prompt to use the coding language present in screenshot
There was a problem hiding this comment.
The PR is well thought through and includes much needed functionalities. Thanks for your contribution.
@Ornithopter-pilot Please go through it once as well
| errorMessage = `Error: ${error.message}`; | ||
| return { valid: false, error: 'OpenAI server error' }; | ||
| } else { | ||
| return { valid: false, error: error.message || 'Unknown error' }; |
There was a problem hiding this comment.
These are added to bypass the dialog box I believe? Could you explain the diff please?
I think we can make them more meaningful either way!
| @@ -334,14 +307,6 @@ export class ProcessingHelper { | |||
| if (existingExtraScreenshots.length === 0) { | |||
| console.log("Extra screenshot files don't exist on disk"); | |||
| mainWindow.webContents.send(this.deps.PROCESSING_EVENTS.NO_SCREENSHOTS); | |||
|
|
|||
| dialog.showMessageBox(mainWindow, { | |||
There was a problem hiding this comment.
The removal lgtm.
What we could do is to add console logging just to be on the safe side of receiving meaningful feedback
| role: "system" as const, | ||
| content: language === 'auto' ? | ||
| "You are a coding challenge interpreter. Analyze the screenshots of the coding problem, identify the programming language being used, and extract all relevant information. Return the information in JSON format with these fields: problem_statement (which must include the identified programming language), constraints, example_input, example_output. Just return the structured JSON without any other text." : | ||
| "You are a coding challenge interpreter. Analyze the screenshots of the coding problem and extract all relevant information. Return the information in JSON format with these fields: problem_statement, constraints, example_input, example_output. Just return the structured JSON without any other text." |
There was a problem hiding this comment.
just modifed some prompts, If auto is slected it instructs the LLM to autodetect the language and try to provied the answer in that lanugage. in case of multiple languges test etc
There was a problem hiding this comment.
Oh cool, I get it now. Sorry for the misunderstanding.
| @@ -661,7 +627,31 @@ export class ProcessingHelper { | |||
| } | |||
|
|
|||
| // Create prompt for solution generation | |||
| const promptText = ` | |||
| const promptText = language === 'auto' ? ` | |||
There was a problem hiding this comment.
@Ornithopter-pilot Need a deep review on this since it is the heart of the code
There was a problem hiding this comment.
@Ornithopter-pilot Need a deep review on this since it is the heart of the code
@bhaumikmaan, thanks for the heads-up! I don't have enough time to do a deep dive today, but I'll definitely check it out later. In the meantime, if you get a chance, feel free to give it a look yourself and share any insights. You know what they say ...two pairs of eyes are better than one! 😄
There was a problem hiding this comment.
@Ornithopter-pilot Need a deep review on this since it is the heart of the code
@bhaumikmaan, thanks for the heads-up! I don't have enough time to do a deep dive today, but I'll definitely check it out later. In the meantime, if you get a chance, feel free to give it a look yourself and share any insights. You know what they say ...two pairs of eyes are better than one! 😄
any update? if this pull would be merged or not codebase seems solid to me except the mouse Handeling thing we need to come up with the different approach to make sure mouse tracking doesn't happen when cursor leaves the browser which is happening currently
There was a problem hiding this comment.
Not tracking what you are referring to here. Is it a new issue, if so please make an issue for it. If it is for this PR, it still needs changes
|
hope to have this shipped soon, sounds so cool to me :) Though hope the change be tested thoroughly |
| @@ -400,7 +400,7 @@ function showMainWindow(): void { | |||
| ...state.windowSize | |||
| }); | |||
| } | |||
| state.mainWindow.setIgnoreMouseEvents(false); | |||
| state.mainWindow.setIgnoreMouseEvents(true, { forward: true }); | |||
There was a problem hiding this comment.
I think this will prevent the user from interacting with the initial config screen (for example, setting the API key). If you already had the API key saved locally before this change, it's easy to overlook
There was a problem hiding this comment.
you can interact it for the when you start... you will not be able interact it after you hide once and show it...
There was a problem hiding this comment.
I don't think that's a good user experience.
There was a problem hiding this comment.
on the other hand i think it is better..when you startm it is ineteractable...but after hiding and showing again it is not which will help you interact with other apps.
There was a problem hiding this comment.
I guess this is more complicated than I thought, but Is it possible to have a different behavior for each screen? Interact with the settings screen/button but ignore the mouse for the rest?
|
Any updates on getting this merged in? |
Changes have been requested, as that happens it will go forward |
Added the ability to clickthrough the window. The cursor will also not change.
Also removed dialog box which would appear if any error was encountered, for eg trying to solve before taking screenshot.
Added an option to choose auto language which changes the prompt to use the coding language present in screenshot