-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Generate embeddings and summaries of a group in context menu #11832
Conversation
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.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun
, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
@@ -386,7 +396,6 @@ public void editGroup(GroupNodeViewModel oldGroup) { | |||
} | |||
|
|||
public void chatWithGroup(GroupNodeViewModel group) { | |||
// This should probably be done some other way. Please don't blame, it's just a thing to make it quick and fast. | |||
if (currentDatabase.isEmpty()) { | |||
dialogService.showErrorDialogAndWait(Localization.lang("Unable to chat with group"), Localization.lang("No library is selected.")); |
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.
Is this branch ever it in th execution? Maybe, it can be replaced by a LOGGER.warn
statement?
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.
Well, the code really can't proceed without a database.
I don't like that if there is no database, there is no reasonably visible response from the JabRef. Just a LOGGER.warn. Not sure what should I do
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.
What I mean, we could do assert !currentDatabase.isEmpty()
. -- Thus: My bet is: This branch is never hit during real use in JabRef.
This creates an additional translation string - therefore I was thinking and worrying.
if (currentDatabase.isEmpty()) { | ||
dialogService.notify(Localization.lang("Unable to generate embeddings. No library is selected.")); | ||
return; | ||
} |
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.
Is this branch ever it in th execution? Maybe, it can be replaced by a LOGGER.warn
statement?
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.
See #11832 (comment)
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.
porposal for now. Re-use "Unable to chat"
and add a LOGGER.warn with the full message. If user enounters this situation, we will see in the log.
if (currentDatabase.isEmpty()) { | ||
dialogService.notify(Localization.lang("Unable to generate summaries. No library is selected.")); | ||
return; | ||
} | ||
|
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.
Is this branch ever it in th execution? Maybe, it can be replaced by a LOGGER.warn
statement?
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.
See #11832 (comment)
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.
porposal for now. Re-use "Unable to chat"
and add a LOGGER.warn with the full message. If user enounters this situation, we will see in the log.
src/main/java/org/jabref/logic/ai/summarization/SummariesService.java
Outdated
Show resolved
Hide resolved
Unable\ to\ generate\ embeddings.\ No\ library\ is\ selected.=Unable to generate embeddings. No library is selected. | ||
Unable\ to\ generate\ summaries.\ No\ library\ is\ selected.=Unable to generate summaries. No library is selected. |
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, these can be removed (see above) --> logger.warn outputs are not localized.
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.
See #11832 (comment)
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 strings should then vanish.
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.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun
, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
Updated, but can you tell me what should I do with that branch without database? Leave as is or use LOGGER instead of a dialogService? |
I put an assert there) |
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.
micro comment.
@@ -59,6 +59,12 @@ public SummariesService(AiPreferences aiPreferences, | |||
this.taskExecutor = taskExecutor; | |||
} | |||
|
|||
/** | |||
* Start generating summary of a {@link BibEntry}, if it was already generated. |
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, there is something missing before "if it was"
* Start generating summary of a {@link BibEntry}, if it was already generated. | |
* Start generating summary of a {@link BibEntry}, independent of whether if it was already generated. |
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.
Oops, it should mean: if it wasn't
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.
If it wasn't, it will start summarization task and put a ProcessingInfo
in the map.
If it is generating, it will return current ProcessingInfo
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.
Oops, it should mean: if it wasn't
Just reword and commit 😅
# Conflicts: # src/main/java/org/jabref/gui/groups/GroupTreeView.java # src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java # src/main/java/org/jabref/gui/sidepane/SidePaneContentFactory.java # src/main/java/org/jabref/logic/ai/summarization/SummariesService.java # src/main/resources/l10n/JabRef_en.properties # src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java
Partially solves https://github.com/JabRef/jabref-issue-melting-pot/issues/546
Mandatory checks
- [ ] Change inCHANGELOG.md
described in a way that is understandable for the average user (if applicable)- [ ] Tests created for changes (if applicable)