-
-
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
Use Markdown in AI Summary tab implemented #12266
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.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
Well, right now the checkbox looks quite ugly. Please place it somewhere else... |
It's not good style to mix fxmls and hard coded creation of ui elements except in some edge cases. Please place both the text area and the Web view in a StackPane. |
I tried not to mix them, but I came up with hard coded WebView, otherwise it wasn't passing unit tests |
Which unit tests? |
In |
Revert it, let us take a look. We'll help you with the tests. |
@subhramit just reverted it, you will witness that 2 unit-tests are not going to pass |
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.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
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 have requested a few changes.
For the failing localization tests, go through:
https://devdocs.jabref.org/code-howtos/faq.html#orgjabreflogicl10nlocalizationconsistencytest-findmissinglocalizationkeys-failed and the one below it.
Please also avoid force-pushing your changes.
src/main/java/org/jabref/gui/ai/components/summary/SummaryShowingComponent.fxml
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/ai/components/summary/SummaryShowingComponent.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/ai/components/summary/SummaryShowingComponent.fxml
Outdated
Show resolved
Hide resolved
I thought about it a bit. It is actually not the worst idea to lazy load the WebView for performance reasons. Please place the TextArea inside a StackPane and add the WebView on demand. That way all the managed and visible stuff can be avoided. |
And please move.the checkbox somewhere else. |
Before I forced push, the previous commit had the changed position of |
Therefore, no force push. Either try to resurrect the commit with "git reflog" or try to re-do it. |
@koppor |
… | Lazy initialization of WebView implemented
@calixtus |
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.
Great! Summary looks so much nicer now!
I was thinking: what if we could get rid of TextArea
whatsoever?
Can you implement only one WebView
that would have different HTMLs: one in plain mode, and other is rendered Markdown? This would greatly simplify the logic.
I also noticed 3 small problems:
- Text is not wrapped.
WebView
does not shrink.- Text is not styled with JabRef's main font. Can you look at how it's implemented in entry preview? The default font is Sans, but the summary in
WebView
is Serif.
src/main/java/org/jabref/gui/ai/components/summary/SummaryShowingComponent.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/ai/components/summary/SummaryShowingComponent.java
Outdated
Show resolved
Hide resolved
@InAnYan |
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.
Good, you used WebView
for both plain-text mode and rendered Markdown, looks better (however, there is a slight remain of TextArea
😄). Overall, I see these problems:
- It doesn't wrap text in plain text mode. That's strange, because in rendered Markdown mode there is some wrapping (maybe, because of the
<pre>
tag?) - In rendered Markdown mode, it wraps the text, but only if the width is bigger than some limit. What I mean is: wrapping doesn't work for all possible width changes. Maybe that's a JavaFX problem, or you need to know a lot of JavaFX internals to solve this problems. If you couldn't find the issue and fix it, it's okay to skip it. (But don't forget about wrapping in plain-text mode).
- Fonts are not handled properly.
src/main/java/org/jabref/gui/ai/components/summary/SummaryShowingComponent.fxml
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/ai/components/summary/SummaryShowingComponent.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/ai/components/summary/SummaryShowingComponent.java
Outdated
Show resolved
Hide resolved
@koppor @subhramit @calixtus |
@InAnYan |
Closes #12233
Description (UPDATED)
As it was pre-requested, I've implemented an option when user is able, using checkbox, to change the formation of the
AI Summary
. If user checks it,WebView
with formatted intohtml
markdown appears, otherwise text stays raw. It is pretty useful new feature that makesAI Summary
user-friendly for those, who loves to see formatted text, instead of rawmarkdown
that besides the text contains "weird", in user's opinion characters.How Code Changed and Why?
org/jabref/gui/ai/components/summary/SummaryShowingComponent.fxml
aCheckBox
that appears one the left below theAI Summary
org/jabref/gui/ai/components/summary/SummaryShowingComponent.java
file's methodonMarkdownToggle()
. Once user checks theCheckBox
theTextArea
disappears, andWebView
come us.TextArea
was wrapped inStackPane
to simplify managing and layering of that component.WebView
programmatically implemented (lazy initialization) to avoidfindMissingLocalizationKeys
andfindObsoleteLocalizationKeys
tests failure.org/jabref/logic/layout/format/MarkdownFormatter.java
that has been already implemented.How to Copy Formatted Content?
Press anywhere in
WebView
component pressctrl + a
/cmd + a
thenctrl + c
/cmd + c
, and paste it in the desired place ^_^Screenshots
CheckBox
checked:CheckBox
unchecked:Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)