-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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. |
jabgui/src/main/java/org/jabref/gui/util/SelectableTextFlow.java
Outdated
Show resolved
Hide resolved
Selecting text currently only works, if the mouse button still covers text, when letting go. If the mouse button is in a blank field when letting go at the end of the click, it will immediately unselect the text. |
this is a good follow-up case to cover. once this is merged you or @Yubo-Cao can create an issue (reuse this screenshot) |
I would like to briefly note that our current architecture does not support tables effectively. The best we will be able to achieve under the limitation of a TextFlow is something similar to rendering a table as a code block (with some bold/italic styling), which is part of the trade-off of not using an external markdown renderer and choosing to handroll a partial markdown support internally to save memory. |
jabgui/src/main/java/org/jabref/gui/util/SelectableTextFlow.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/util/SelectableTextFlow.java
Outdated
Show resolved
Hide resolved
Here are my findings: ImportantWhen any kind of list is used, it adds a vertical space before previous text. I find it a bit ugly. I see that there are 2 ways to overcome this:
Message I pasted was:
But then AI responded, everything was fine, but it also had error:
Non-importantWe can make a follow up issue with those moments + tables.
So, two rectangles with rounded corners.
Offtopic |
The IOB looks like it's coming from Flexmark. |
Maybe MWE using a JUnit test... |
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.
Green from me.
To summarize, TODOs:
- Submission of useful jfx snippet to GemsFX/ControlsFX
- MWE reproduction and reporting to Flexmark
- File known issues in JabRef
Current:
Address Ruslan's comment on the changelog entry.
Rest seems good to go, thank you for such wonderful work, Yubo.
OMG, maybe it is something wrong with my machine, if Yubo cannot reproduce my issues 🤣 Nice color change |
Can you do a screenshot with dark theme too please? |
Closes #12234 (partially)
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.
Before:

After:



Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)