Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
AI - MVP - v1 #1
AI - MVP - v1 #1
Changes from 14 commits
d9d58f6
7c358c9
0a05213
812820a
7f92e0d
cee2278
37d0534
d9b6c3e
c0b56f3
19d8aa3
2dcaa3a
f389bad
971fdce
ddacd47
1da98b5
9ffeaa4
3bf8ac1
14b7c72
784434f
7fac426
8a12515
221f961
185de4e
267efc9
62ea22e
4ddf173
f9bd0d0
43845ae
c9cf1a0
6cec13a
066747c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Style: Either always empty lines between variable declarations or never. Sometimes, I catch myself to mix too. However, then mostly there are JavaDoc comments the separted variables.
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 applies for every-every field?
What if I have these fields in a class:
I can't group them with new lines? And what will be the right order for this fields?
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.
checkstyle will tell about the order. Grouping is OK, add comments then. However, this could make the code more unreadable. Just keep one empty line between each "thing".
If reasonable, you could create a nested
record
class for more clearer grouping. However, in your case, this doesn't seem to be necessary.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.
Something is wrong here.
java.lang.IllegalArgumentException: openAiApiKey cannot be null or empty. API keys can be generated here: https://platform.openai.com/account/api-keys
at [email protected]/dev.ai4j.openai4j.OpenAiClient$Builder.openAiApiKey(OpenAiClient.java:119)
at [email protected]/dev.langchain4j.model.openai.OpenAiChatModel.(OpenAiChatModel.java:80)
at [email protected]/dev.langchain4j.model.openai.OpenAiChatModel$OpenAiChatModelBuilder.build(OpenAiChatModel.java:50)
at [email protected]/org.jabref.logic.ai.AiConnection.(AiConnection.java:24)
at [email protected]/org.jabref.gui.entryeditor.AiChatTab.lambda$setUpAiConnection$0(AiChatTab.java:75)
at [email protected]/com.sun.javafx.binding.ExpressionHelper$Generic.fireValueChangedEvent(ExpressionHelper.java:372)
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.
Oh, very bad. I'll check that out.
The verification in AI preferences works like this:
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.
Huh, I cannot reproduce the issue.
What is the right way to reset the JabRef settings? I've seen a button "Reset preferences" in Preferences window. Is that's 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.
In case a user can "accidentally" disable the chat, we need to think. However, I think, it is an "informed" decision. Thus, we can just drop the chat. --> Just remove the comment line.
If we don't want to drop it, we should try to keep it: I don't know about the relation between
aiConnection
andaiChat
.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.
Next iteration: Do it similar to the "recommendation" tab. The tab is shown as default, but asks for an OK
The checkmark "modern AI technologies" is too general. Think of the GDPR which asks software vendors to be very specific what happens with their data.
Thus, move from the following tab the setting to the "Entry editor" tab, next to the "recommendations". "Show AI Tab", and ask for the OpenAI token.
In case there will be more preferences, we first add them there. If they are too much, we might move the detail AI prrefernces to the separate tab "AI".
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.
Good idea, but autoformat will kill the indentation.
I am "working" to get palantir ready for this. That will reformat the whole source code. See JabRef#672 for details.
Just restructure in methods. Maybe, this is a point, where you need fxml? 😅
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 really need advice on this topic.
Main challenges:
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.
Move that to the top of the class (so that all constants are grouped together).
For a PR to JabRef, that needs to be stylable via
base.css
, so that it can be themed.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.
Where can I see an example of how other UI classes refer to
base.css
file, so I can learn from that and implement it here?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.
Hmm, haven't found it myself. But I've seen something in
base.css
. I'll show it in the next commit