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

Add options to auto generate embeddings and summaries #11833

Merged
merged 7 commits into from
Oct 6, 2024

Conversation

InAnYan
Copy link
Collaborator

@InAnYan InAnYan commented Sep 26, 2024

Partially closes the issue https://github.com/JabRef/jabref-issue-melting-pot/issues/546.

NOTE: Please, merge the #11835, before this, to reduce merge conflicts.

New options:
image

I think this space after "Enable AI" is because of the help button.

I'll address the help buttons in other PR.

Mandatory checks

~- [ ] Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
- [ ] Tests created for changes (if applicable)

@@ -28,7 +35,8 @@
* Use this class in the logic and UI.
*/
public class IngestionService {
private final Map<LinkedFile, ProcessingInfo<LinkedFile, Void>> ingestionStatusMap = new HashMap<>();
// We use a {@link TreeMap} here for the same reasons we use it in {@link ChatHistoryService}.
private final TreeMap<LinkedFile, ProcessingInfo<LinkedFile, Void>> ingestionStatusMap = new TreeMap<>((o1, o2) -> o1 == o2 ? 0 : o1.getLink().compareTo(o2.getLink()));
Copy link
Member

Choose a reason for hiding this comment

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

You can use the more easier way with Compartor.comparing( ... )
https://www.baeldung.com/java-8-comparator-comparing

Copy link
Member

Choose a reason for hiding this comment

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

@InAnYan Please adress, too.

Comment on lines 68 to 105
private void configureDatabaseListeners(StateManager stateManager) {
stateManager.getOpenDatabases().addListener((ListChangeListener<BibDatabaseContext>) change -> {
while (change.next()) {
if (change.wasAdded()) {
change.getAddedSubList().forEach(this::configureDatabaseListeners);
}
}
});
}

private void configureDatabaseListeners(BibDatabaseContext bibDatabaseContext) {
// GC was eating the listeners, so we have to fall back to the event bus.
bibDatabaseContext.getDatabase().registerListener(new EntriesChangedListener(bibDatabaseContext));
}

private class EntriesChangedListener {
private final BibDatabaseContext bibDatabaseContext;

public EntriesChangedListener(BibDatabaseContext bibDatabaseContext) {
this.bibDatabaseContext = bibDatabaseContext;
}

@Subscribe
public void listen(EntriesAddedEvent e) {
e.getBibEntries().forEach(entry -> {
if (aiPreferences.getAutoGenerateSummaries()) {
summarize(entry, bibDatabaseContext);
}
});
}

@Subscribe
public void listen(FieldChangedEvent e) {
if (e.getField() == StandardField.FILE && aiPreferences.getAutoGenerateSummaries()) {
summarize(e.getBibEntry(), bibDatabaseContext);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Move this code to UI.

The logic should not depend on the UI (see architecture tests).

Reason: AI should also be usable via CLI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

StateManager belongs to UI?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. See the package of the StateManger.

You can also see it, because it holds the list of opened BibTeX files

https://github.com/JabRef/jabref/blob/b44f30e461c967fa7b2dde4aedabf9a4bd74a73c/src/main/java/org/jabref/gui/StateManager.java#L59C1-L60C110

# Conflicts:
#	src/main/java/org/jabref/gui/preferences/ai/AiTab.fxml
@InAnYan InAnYan requested a review from koppor October 3, 2024 07:22
@InAnYan
Copy link
Collaborator Author

InAnYan commented Oct 3, 2024

Updated

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Two small comments, then it should be good to go!

@@ -58,12 +56,13 @@ public class ChatHistoryService implements AutoCloseable {

private static final String CHAT_HISTORY_FILE_NAME = "chat-histories.mv";

private final StateManager stateManager = Injector.instantiateModelOrService(StateManager.class);
Copy link
Member

Choose a reason for hiding this comment

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

I think, you can move the class to logic. No gui dependency as far as I can see.

Also the JavaDoc can be updated --> no more use of StateManager.

@@ -28,7 +35,8 @@
* Use this class in the logic and UI.
*/
public class IngestionService {
private final Map<LinkedFile, ProcessingInfo<LinkedFile, Void>> ingestionStatusMap = new HashMap<>();
// We use a {@link TreeMap} here for the same reasons we use it in {@link ChatHistoryService}.
private final TreeMap<LinkedFile, ProcessingInfo<LinkedFile, Void>> ingestionStatusMap = new TreeMap<>((o1, o2) -> o1 == o2 ? 0 : o1.getLink().compareTo(o2.getLink()));
Copy link
Member

Choose a reason for hiding this comment

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

@InAnYan Please adress, too.

@InAnYan
Copy link
Collaborator Author

InAnYan commented Oct 6, 2024

Updated

@InAnYan InAnYan requested a review from koppor October 6, 2024 10:28
@koppor koppor added this pull request to the merge queue Oct 6, 2024
Merged via the queue into JabRef:main with commit c9e4b23 Oct 6, 2024
23 checks passed
@koppor koppor deleted the generate-ai-for-new branch October 6, 2024 11:39
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.

3 participants