-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Feature ai chat regenerate button #12794
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?
Changes from 3 commits
dfa3f9c
0d18592
eb3eeaa
5f84420
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
package org.jabref.gui.ai.components.aichat.chathistory; | ||
|
||
import java.util.function.Consumer; | ||
|
||
import javafx.collections.ListChangeListener; | ||
import javafx.collections.ObservableList; | ||
import javafx.fxml.FXML; | ||
|
@@ -10,11 +12,15 @@ | |
import org.jabref.gui.util.UiTaskExecutor; | ||
|
||
import com.airhacks.afterburner.views.ViewLoader; | ||
import dev.langchain4j.data.message.AiMessage; | ||
import dev.langchain4j.data.message.ChatMessage; | ||
import dev.langchain4j.data.message.UserMessage; | ||
|
||
public class ChatHistoryComponent extends ScrollPane { | ||
@FXML private VBox vBox; | ||
|
||
private Consumer<String> regenerateCallback; | ||
|
||
public ChatHistoryComponent() { | ||
ViewLoader.view(this) | ||
.root(this) | ||
|
@@ -35,14 +41,34 @@ public void setItems(ObservableList<ChatMessage> items) { | |
items.addListener((ListChangeListener<? super ChatMessage>) obs -> fill(items)); | ||
} | ||
|
||
public void setRegenerateCallback(Consumer<String> regenerateCallback) { | ||
this.regenerateCallback = regenerateCallback; | ||
} | ||
|
||
private void fill(ObservableList<ChatMessage> items) { | ||
UiTaskExecutor.runInJavaFXThread(() -> { | ||
vBox.getChildren().clear(); | ||
items.forEach(chatMessage -> | ||
vBox.getChildren().add(new ChatMessageComponent(chatMessage, chatMessageComponent -> { | ||
int index = vBox.getChildren().indexOf(chatMessageComponent); | ||
items.remove(index); | ||
}))); | ||
for (ChatMessage chatMessage : items) { | ||
ChatMessageComponent component = new ChatMessageComponent(chatMessage, comp -> { | ||
items.remove(chatMessage); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you removed this code: int index = vBox.getChildren().indexOf(chatMessageComponent);
items.remove(index); |
||
}); | ||
if (chatMessage instanceof AiMessage) { | ||
component.setOnRegenerate(comp -> { | ||
if (!items.isEmpty() && items.getLast() == chatMessage && items.size() > 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Direct access to the last element without checking if the list has sufficient elements could cause runtime exceptions. Should implement proper validation first. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reorder the conditions. I think, size check should come first and empty can be ommitted |
||
ChatMessage previous = items.get(items.size() - 2); | ||
if (previous instanceof UserMessage message) { | ||
String userText = message.singleText(); | ||
items.removeLast(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't https://github.com/JabRef/jabref/pull/12794/files#diff-53b205bc2203a6a3cd923d087c923586b689bfc6f5a48f4f5951a91e0e816bf9R96 already remove messages everywhere? |
||
items.removeLast(); | ||
if (regenerateCallback != null) { | ||
regenerateCallback.accept(userText); | ||
} | ||
} | ||
} | ||
}); | ||
} | ||
vBox.getChildren().add(component); | ||
} | ||
}); | ||
} | ||
|
||
|
@@ -51,3 +77,4 @@ public void scrollDown() { | |
this.setVvalue(this.getVmax()); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ public class ChatMessageComponent extends HBox { | |
|
||
private final ObjectProperty<ChatMessage> chatMessage = new SimpleObjectProperty<>(); | ||
private final ObjectProperty<Consumer<ChatMessageComponent>> onDelete = new SimpleObjectProperty<>(); | ||
private final ObjectProperty<Consumer<ChatMessageComponent>> onRegenerate = new SimpleObjectProperty<>(); | ||
|
||
@FXML private HBox wrapperHBox; | ||
@FXML private VBox vBox; | ||
|
@@ -63,6 +64,10 @@ public void setOnDelete(Consumer<ChatMessageComponent> onDeleteCallback) { | |
this.onDelete.set(onDeleteCallback); | ||
} | ||
|
||
public void setOnRegenerate(Consumer<ChatMessageComponent> callback) { | ||
this.onRegenerate.set(callback); | ||
} | ||
|
||
private void loadChatMessage() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also make that "Regenerate AI response" button is shown only if the message is an |
||
switch (chatMessage.get()) { | ||
case UserMessage userMessage -> { | ||
|
@@ -87,7 +92,7 @@ private void loadChatMessage() { | |
} | ||
|
||
default -> | ||
LOGGER.error("ChatMessageComponent supports only user, AI, or error messages, but other type was passed: {}", chatMessage.get().type().name()); | ||
LOGGER.error("ChatMessageComponent supports only user, AI, or error messages, but other type was passed: {}", chatMessage.get().type().name()); | ||
calixtus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
|
@@ -103,7 +108,15 @@ private void onDeleteClick() { | |
} | ||
} | ||
|
||
@FXML | ||
private void onRegenerateClick() { | ||
if (onRegenerate.get() != null) { | ||
onRegenerate.get().accept(this); | ||
} | ||
} | ||
|
||
private void setColor(String fillColor, String borderColor) { | ||
vBox.setStyle("-fx-background-color: " + fillColor + "; -fx-border-radius: 10; -fx-background-radius: 10; -fx-border-color: " + borderColor + "; -fx-border-width: 3;"); | ||
} | ||
} | ||
|
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.
Calling deleteLastMessage() twice creates code duplication. Consider creating a new method like deleteLastTwoMessages() to encapsulate this logic and improve maintainability.
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.
Or add a Java comment above
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.
False positive in this case.