-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add Markdown rendering to AI Chat #13194
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
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.
Code reads really good.
Did not try out, but demo is attached to the description, so green from me.
Thanks @Yubo-Cao!
Looks very nice. Side track: "Summary" is a very different AI function, therefore @InAnYan put another tab for "Summary". -- For instance, for large PDFs, JabRef splits into chuncks and summarizes chunks. |
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.
Only a small comment.
Are spaces correctly handled for the getTextFlowContent()
?
I assume, adding test cases is too hard here?
jabgui/src/main/java/org/jabref/gui/ai/components/aichat/chatmessage/ChatMessageComponent.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/util/SelectableTextFlow.java
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
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.
Unfortunately, I had to request one small change
jabgui/src/main/java/org/jabref/gui/ai/components/aichat/chatmessage/ChatMessageComponent.java
Outdated
Show resolved
Hide resolved
/** | ||
* A TextFlow that allows text selection and copying. | ||
*/ | ||
public class SelectableTextFlow extends TextFlow { |
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.
OMG, Yubo, is it possible to submit this code to some library? GemsFX maybe? This is a very useful piece of 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.
+1 for GemsFX. If the maintainer Dirk doesn't want it, then ControlsFX
So after actually switch to Arch Linux and run testcases over there (of course that was not needed at all to come up with this conclusion looking at how I butchered the Markdown AST), is that we currently don't handle whitespace perfectly, if at all.
I think my new attempt would be trying to keep track a bit more metadata in MarkdownTextflow and basically override |
@Yubo-Cao Please resolve conversations after you push the commits resolving them, not before |
If things get too convoluted, Note: |
Another handy resource: https://ohshitgit.com/ |
However, JabRef mainatinaers don't like force pushes - only in rare emergency cases ^^ I always direct to https://lostechies.com/joshuaflanagan/2010/09/03/use-gitk-to-understand-git/ -- then, the git commands should be more understandable. |
Before all, thank you so much for y'all's assistance in correcting me about proper usage of Resolve Conversation and Git. Apologies for any inconvenience or potential offense that may have caused. I really appreciated your understanding and patience. In this update, the following features were added
A demo can be found here: https://drive.google.com/file/d/16zosXoTq0dgksYshqLzKkAGXudKaCv3b/view?usp=drive_link |
I'm still getting that error with And BTW I'm on Arch too, and I didn't spot any major problems with Markdown rendering |
}); | ||
NumberBinding maxUsableWidth = Bindings.createDoubleBinding( | ||
() -> getScene() == null ? 600 : getScene().getWidth() - 20, sceneProperty(), | ||
getScene() == null ? widthProperty() : getScene().widthProperty() |
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 it possible to add a comment in which cases Scene is null?
Or is it "magically" null and we don't know - then maybe also comment that it is unclar, but we need to check
Reason null
is an indicaton that there is some bug in the code. In this case, it seems to be life-cycle related.
--> Could help to create an MWE and mayb file a JavaFX issue - its not always on our side that something is wrong - see e.g., JabRef#713
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.
When I proposed my fix, I used this answer of StackOverflow (warning: non-English):
I think the author gives a good, if you let me say this, "idiom" of handling this
@trag-bot didn't find any issues in the code! ✅✨ |
Closes #12234
See this: https://drive.google.com/file/d/1VOvWBLP5E9I_sLXd7RpShfXoP0ttg0JZ/view?usp=sharing for a quick demo. Ctrl+C is supported as a part of this PR as well.
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)