-
Notifications
You must be signed in to change notification settings - Fork 520
General improvements #35
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
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the app used to open a defauly error dialog box, like in windows, whenever it encountered error....i removed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That must be because of some error code - I don't think the removal you did is necessary
electron/ProcessingHelper.ts
Outdated
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal lgtm.
What we could do is to add console logging just to be on the safe side of receiving meaningful feedback
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ornithopter-pilot Need a deep review on this since it is the heart of the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is for settings where llms are instructed to detect which language the solution has to be given based on the screenshot. For eg. one question maybe in Js other SQL. if you set to auto. it will instruct the llm to detect the language and provide soluition in that prgramming language
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense however you shouldn't default to said auto
The reason is because what about the case where a language can't be detected - there should be some reasonable fallback which was python that you changed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's a good user experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
the conflict was just an extra line :) |
the project has changed somewhat when i last committed i think...can you specifyt which comment you were referring to? |
Here - See the comments are reply please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: electron/ProcessingHelper.ts(615,11): error TS2304: Cannot find name 'problemInfo'.
Error: Process completed with exit code 2.
Build Failed as well. Please refer and fix
regarding the build fail, I will have to look inrto...havent interacted with this project since the last commit... Regarding
I thinki explained well enough....You will be able to interact with the program whenever you launch first, you cant ineteract after you hide once, you will have to restart the program to interact with it again...maybe a shortcut would be good, but it defeats the purpose if keyloggers are used... |
From what I understand @anuchin has implemented it by doing:
Does this mean that the panel for screen-shots and solution etc can only be seen once and once hidden it can't be brought back? |
no, you can see them every time, but you cant interact with them via mouse...shortcuts will owrk to hide and ungide. Let me be more clear. When you launch the app everytime, you can interact it with via mouse and keyboard, so you can click on settings etc..then when press the hide button...you can only interact via shortcuts. The app will no longer detect mouse inputs like clicks, cursor will not change when you pass it on the app's screens shot, solutions etc. You close the app, and lauinch it again, you can interact with mouse and edit the configs etc..The mouse interactive is disabled when you first hide the app after each launch. |
Great, looks good to me. 2 questions because this somehow hampers the functionalities:
|
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