-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix preview performance #3533
Fix preview performance #3533
Conversation
When JabRef is not run as application, the citation styles are provided in the classpath as jar. This case is now included so that citation styles will be available during development.
The access to the CSL engine was extracted to a separate adapter class that takes care of handling the creation of the preview strings. The layout should be a bit clearer now.
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.
That you so much for fixing the annoying exception in dev mode :)
Overall the code looks fine and I just have minor remarks. But I am (sometimes) getting Exceptions when using the preview. First of all, it happens to me (even when running the fat jar from command line) that citationstyles are not found:
10:25:22.452 [AWT-EventQueue-0] ERROR org.jabref.gui.preftabs.PreviewPrefsTab - something went wrong while adding the discovered CitationStyles to the list
In that case, the configured citation style can be used, but I cannot select a new one in the preferences dialog. There, the list is empty.
Then, when trying to preview items I get exceptions. But these exceptions happen reliably for certain bib items, so it seems just like a bad combination of the randomly selected style I used for testing (ACM SIG 3+ authors) and the bib items.
For the record, here's a stack trace:
[AWT-EventQueue-0] ERROR org.jabref.gui.worker.CitationStyleWorker - Error while generating citation style
java.util.concurrent.ExecutionException: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Could not update items
at java.util.concurrent.FutureTask.report(FutureTask.java:122) ~[?:1.8.0_121]
at java.util.concurrent.FutureTask.get(FutureTask.java:192) ~[?:1.8.0_121]
at javax.swing.SwingWorker.get(SwingWorker.java:602) ~[?:1.8.0_121]
at org.jabref.gui.worker.CitationStyleWorker.done(CitationStyleWorker.java:63) [classes/:?]
at javax.swing.SwingWorker$5.run(SwingWorker.java:737) [?:1.8.0_121]
at javax.swing.SwingWorker$DoSubmitAccumulativeRunnable.run(SwingWorker.java:832) [?:1.8.0_121]
at sun.swing.AccumulativeRunnable.run(AccumulativeRunnable.java:112) [?:1.8.0_121]
at javax.swing.SwingWorker$DoSubmitAccumulativeRunnable.actionPerformed(SwingWorker.java:842) [?:1.8.0_121]
at javax.swing.Timer.fireActionPerformed(Timer.java:313) [?:1.8.0_121]
at javax.swing.Timer$DoPostEvent.run(Timer.java:245) [?:1.8.0_121]
at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:311) [?:1.8.0_121]
at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:756) [?:1.8.0_121]
at java.awt.EventQueue.access$500(EventQueue.java:97) [?:1.8.0_121]
at java.awt.EventQueue$3.run(EventQueue.java:709) [?:1.8.0_121]
at java.awt.EventQueue$3.run(EventQueue.java:703) [?:1.8.0_121]
at java.security.AccessController.doPrivileged(Native Method) [?:1.8.0_121]
at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:80) [?:1.8.0_121]
at java.awt.EventQueue.dispatchEvent(EventQueue.java:726) [?:1.8.0_121]
at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:201) [?:1.8.0_121]
at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:116) [?:1.8.0_121]
at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:105) [?:1.8.0_121]
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101) [?:1.8.0_121]
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93) [?:1.8.0_121]
at java.awt.EventDispatchThread.run(EventDispatchThread.java:82) [?:1.8.0_121]
Caused by: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalArgumentException: Could not update items
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2218) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache.get(LocalCache.java:4147) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4151) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5140) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5146) ~[guava-23.4-jre.jar:?]
at org.jabref.logic.citationstyle.CitationStyleCache.getCitationFor(CitationStyleCache.java:47) ~[classes/:?]
at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:50) ~[classes/:?]
at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:23) ~[classes/:?]
at javax.swing.SwingWorker$1.call(SwingWorker.java:295) ~[?:1.8.0_121]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
at javax.swing.SwingWorker.run(SwingWorker.java:334) ~[?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) ~[?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) ~[?:1.8.0_121]
at java.lang.Thread.run(Thread.java:745) ~[?:1.8.0_121]
Caused by: java.lang.IllegalArgumentException: Could not update items
at de.undercouch.citeproc.CSL.registerCitationItems(CSL.java:508) ~[citeproc-java-1.0.1.jar:?]
at org.jabref.logic.citationstyle.CSLAdapter.makeBibliography(CSLAdapter.java:55) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitations(CitationStyleGenerator.java:56) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitation(CitationStyleGenerator.java:47) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:37) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:34) ~[classes/:?]
at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3708) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2416) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2299) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2212) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache.get(LocalCache.java:4147) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4151) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5140) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5146) ~[guava-23.4-jre.jar:?]
at org.jabref.logic.citationstyle.CitationStyleCache.getCitationFor(CitationStyleCache.java:47) ~[classes/:?]
at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:50) ~[classes/:?]
at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:23) ~[classes/:?]
at javax.swing.SwingWorker$1.call(SwingWorker.java:295) ~[?:1.8.0_121]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
at javax.swing.SwingWorker.run(SwingWorker.java:334) ~[?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) ~[?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) ~[?:1.8.0_121]
at java.lang.Thread.run(Thread.java:745) ~[?:1.8.0_121]
Caused by: de.undercouch.citeproc.script.ScriptRunnerException: Could not call method
at de.undercouch.citeproc.script.JREScriptRunner.callMethod(JREScriptRunner.java:121) ~[citeproc-java-1.0.1.jar:?]
at de.undercouch.citeproc.CSL.registerCitationItems(CSL.java:506) ~[citeproc-java-1.0.1.jar:?]
at org.jabref.logic.citationstyle.CSLAdapter.makeBibliography(CSLAdapter.java:55) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitations(CitationStyleGenerator.java:56) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitation(CitationStyleGenerator.java:47) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:37) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:34) ~[classes/:?]
at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3708) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2416) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2299) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2212) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache.get(LocalCache.java:4147) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4151) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5140) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5146) ~[guava-23.4-jre.jar:?]
at org.jabref.logic.citationstyle.CitationStyleCache.getCitationFor(CitationStyleCache.java:47) ~[classes/:?]
at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:50) ~[classes/:?]
at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:23) ~[classes/:?]
at javax.swing.SwingWorker$1.call(SwingWorker.java:295) ~[?:1.8.0_121]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
at javax.swing.SwingWorker.run(SwingWorker.java:334) ~[?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) ~[?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) ~[?:1.8.0_121]
at java.lang.Thread.run(Thread.java:745) ~[?:1.8.0_121]
Caused by: javax.script.ScriptException: ReferenceError: "dump" is not defined in <eval> at line number 830
at jdk.nashorn.api.scripting.NashornScriptEngine.throwAsScriptException(NashornScriptEngine.java:470) ~[nashorn.jar:?]
at jdk.nashorn.api.scripting.NashornScriptEngine.invokeImpl(NashornScriptEngine.java:392) ~[nashorn.jar:?]
at jdk.nashorn.api.scripting.NashornScriptEngine.invokeMethod(NashornScriptEngine.java:199) ~[nashorn.jar:?]
at de.undercouch.citeproc.script.JREScriptRunner.callMethod(JREScriptRunner.java:117) ~[citeproc-java-1.0.1.jar:?]
at de.undercouch.citeproc.CSL.registerCitationItems(CSL.java:506) ~[citeproc-java-1.0.1.jar:?]
at org.jabref.logic.citationstyle.CSLAdapter.makeBibliography(CSLAdapter.java:55) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitations(CitationStyleGenerator.java:56) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitation(CitationStyleGenerator.java:47) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:37) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:34) ~[classes/:?]
at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3708) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2416) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2299) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2212) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache.get(LocalCache.java:4147) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4151) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5140) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5146) ~[guava-23.4-jre.jar:?]
at org.jabref.logic.citationstyle.CitationStyleCache.getCitationFor(CitationStyleCache.java:47) ~[classes/:?]
at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:50) ~[classes/:?]
at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:23) ~[classes/:?]
at javax.swing.SwingWorker$1.call(SwingWorker.java:295) ~[?:1.8.0_121]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
at javax.swing.SwingWorker.run(SwingWorker.java:334) ~[?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) ~[?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) ~[?:1.8.0_121]
at java.lang.Thread.run(Thread.java:745) ~[?:1.8.0_121]
Caused by: jdk.nashorn.internal.runtime.ECMAException: ReferenceError: "dump" is not defined
at jdk.nashorn.internal.runtime.ECMAErrors.error(ECMAErrors.java:57) ~[nashorn.jar:?]
at jdk.nashorn.internal.runtime.ECMAErrors.referenceError(ECMAErrors.java:319) ~[nashorn.jar:?]
at jdk.nashorn.internal.runtime.ECMAErrors.referenceError(ECMAErrors.java:291) ~[nashorn.jar:?]
at jdk.nashorn.internal.objects.Global.__noSuchProperty__(Global.java:1441) ~[nashorn.jar:?]
at jdk.nashorn.internal.scripts.Script$Recompilation$195$30224A$\^eval\_.error(<eval>:830) ~[?:?]
at jdk.nashorn.internal.scripts.Script$Recompilation$194$523263A$\^eval\_.dateParseArray(<eval>:12730) ~[?:?]
at jdk.nashorn.internal.scripts.Script$Recompilation$191$100473A$\^eval\_.retrieveItem(<eval>:2870) ~[?:?]
at jdk.nashorn.internal.scripts.Script$Recompilation$190$617052A$\^eval\_.doinserts(<eval>:15103) ~[?:?]
at jdk.nashorn.internal.scripts.Script$Recompilation$187$235101AAAA$\^eval\_.updateItems(<eval>:6064) ~[?:?]
at jdk.nashorn.internal.runtime.ScriptFunctionData.invoke(ScriptFunctionData.java:665) ~[nashorn.jar:?]
at jdk.nashorn.internal.runtime.ScriptFunction.invoke(ScriptFunction.java:494) ~[nashorn.jar:?]
at jdk.nashorn.internal.runtime.ScriptRuntime.apply(ScriptRuntime.java:393) ~[nashorn.jar:?]
at jdk.nashorn.api.scripting.ScriptObjectMirror.callMember(ScriptObjectMirror.java:199) ~[nashorn.jar:?]
at jdk.nashorn.api.scripting.NashornScriptEngine.invokeImpl(NashornScriptEngine.java:386) ~[nashorn.jar:?]
at jdk.nashorn.api.scripting.NashornScriptEngine.invokeMethod(NashornScriptEngine.java:199) ~[nashorn.jar:?]
at de.undercouch.citeproc.script.JREScriptRunner.callMethod(JREScriptRunner.java:117) ~[citeproc-java-1.0.1.jar:?]
at de.undercouch.citeproc.CSL.registerCitationItems(CSL.java:506) ~[citeproc-java-1.0.1.jar:?]
at org.jabref.logic.citationstyle.CSLAdapter.makeBibliography(CSLAdapter.java:55) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitations(CitationStyleGenerator.java:56) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleGenerator.generateCitation(CitationStyleGenerator.java:47) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:37) ~[classes/:?]
at org.jabref.logic.citationstyle.CitationStyleCache$1.load(CitationStyleCache.java:34) ~[classes/:?]
at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3708) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2416) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2299) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2212) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache.get(LocalCache.java:4147) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4151) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5140) ~[guava-23.4-jre.jar:?]
at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:5146) ~[guava-23.4-jre.jar:?]
at org.jabref.logic.citationstyle.CitationStyleCache.getCitationFor(CitationStyleCache.java:47) ~[classes/:?]
at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:50) ~[classes/:?]
at org.jabref.gui.worker.CitationStyleWorker.doInBackground(CitationStyleWorker.java:23) ~[classes/:?]
at javax.swing.SwingWorker$1.call(SwingWorker.java:295) ~[?:1.8.0_121]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
at javax.swing.SwingWorker.run(SwingWorker.java:334) ~[?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) ~[?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) ~[?:1.8.0_121]
at java.lang.Thread.run(Thread.java:745) ~[?:1.8.0_121]
And here's a corresponding item:
@InProceedings{harrer2012conformance,
author = {Simon Harrer and J\"org Lenhard and Guido Wirtz},
title = {{BPEL Conformance in Open Source Engines}},
booktitle = {5th IEEE International Conference on Service-Oriented Computing and Applications},
year = {2012},
pages = {237--244},
address = {Taipei, Taiwan},
month = {December 17-19},
doi = {10.1109/soca.2012.6449467},
}
* Creates the bibliography of the provided items. This method needs to run synchronized because the underlying | ||
* CSL engine is not thread-safe. | ||
*/ | ||
static synchronized List<String> makeBibliography(List<BibEntry> bibEntries, String style, CitationStyleOutputFormat outputFormat) throws IOException { |
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.
As far as I can see by looking at the code, this solution should be thread-safe. You could turn up parallelism by a tiny notch, but I cannot say if this has any tangible benefit. This could be done by not marking the whole method as synchronized, but just the part where the CSL engine is used. To do so, you would add a private lock instance variable, e.g. private static Object lock = new Object();
and then refactor the method like this:
List<CSLItemData> cslItemData = new ArrayList<>(bibEntries.size());
for (BibEntry bibEntry : bibEntries) {
cslItemData.add(bibEntryToCSLItemData(bibEntry));
}
synchronized(lock) {
initialize(style, outputFormat);
dataProvider.setData(cslItemData);
cslInstance.registerCitationItems(dataProvider.getIds());
Bibliography bibliography = cslInstance.makeBibliography();
return Arrays.asList(bibliography.getEntries());
}
You can refactor the code in this way or not, I leave this up to you. It will only have an effect if there are many concurrent calls to the method.
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.
It is an easy fix indeed. I will profile where the bottleneck is when you have many items selected and click on Copy Preview
. It might be useful to check if the loop itself should be parallelized as well. Copying the preview is too slow for my taste and I'm unsure it really comes from CSL this time. I will check.
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 profiled this and it's having no significant impact on the performance. After refactoring the creation of the cslItemData
was absorbed into our data provider and must be run synchronized as well or the data we provide to CSL will get out of synch.
String path = CitationStyle.class.getProtectionDomain().getCodeSource().getLocation().toURI().getPath(); | ||
|
||
// This is a quick fix to have the styles when running JabRef in a development environment. | ||
// The styles.jar is not extracted into the JabRef.jar and therefore, we search the classpath for it. | ||
if ((new File(path)).isDirectory()) { |
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.
Whenever we add/modify file IO code in JabRef, we try to use the new Java API for that java.nio
, not java.io
. So please replace the condition with Files.isDirectory(Paths.get(path))
.
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.
OK. No problem.
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.
Nice work! I have some ideas on how to minimally improve the code but I'm not sure if they can be implemented easily / if it they actually make sense. I'll leave this to your judgment.
private static String style; | ||
private static CitationStyleOutputFormat format; | ||
private static JabRefItemDataProvider dataProvider = new JabRefItemDataProvider(); | ||
private static CSL cslInstance; |
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 needed, that everything is static
here? I actually would prefer a usual class with non-standard methods/instance variables.
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.
Yes. This would probably be better. We could then still instantiate several CSL adapters if we need one for whatever purpose.
} | ||
|
||
@Override | ||
public CSLItemData retrieveItem(String id) { |
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'm not sure if this is really an advantage but since you are already implementing a custom data provider, I would suggest the following:
- setData accepts a list of
BibEntry
- retriveItem gets the
BibEntry
with the given key and then returns the result ofbibEntryToCSLItemData
. This solution might be a bit more memory effective since you don't have an additional list (but it only makes sense if thebibEntryToCSLItemData
method is not expensive).
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.
Ha funny. It's the exact same thing I have thought about. I wanted to wait until we are sure it works in general. I too would prefer to make a real JabRefItemDataProvider
that works on BibEntry
.
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 refactored this. retrieveItem
cannot change its signature because it is called internally by CSL (that's why it is overridden). Still, the layout is much clearer now.
@lenhard @tobiasdiez Thanks for the fast review. Here are to comments to Lenhard's first message:
Yes. I did not consider this case. I assume the fat-jar contains the styles still packed as jar. I'm sure I can fix this.
Took a while, but I could pin it down. I'm not sure I caught the exception before but now you will see "Cannot create preview" and the exception appears in the log as usual. The error comes from your BibEntry. When you look at your BiBTeX code, you see
If this is indeed valid BibTeX, then our CSL converter does not handle it correctly. You can find the Json format of what This error should have happened before as I did not touch that part. I'm not sure how this can reliably be fixed as we only extract the latex-free fields and turn them into strings. That should probably be a different PR. |
@halirutan Thanks for the investigation and I hope you did not waste too much time on that. That is not correct bibtex (well bibtex is very open and relaxed and of course you can write it in this way, but this is not how the content of the month field is expected), I just opened an old bib file and clicked around, not caring too much about the actual data. You can ignore that error in your PR here. I think a viable strategy would actually be to absorb the exception completely. Exceptions in JabRef's log should indicate problems in JabRef. This is not a problem of JabRef, but of user data and the citation styles library. Therefore, I think it would be sufficient to create a log entry stating that the attempt to parse an entry results in an error from the citation styles library. But this can be part of a different PR. |
While the CitationStyleCache uses the complete source of the style definition, this file used only the csl file name. To prevent useless re-initialization of the CSLAdapter, this was unified to use the same approach as in the preview worker.
CSLAdapter can now be instantiated several times. Each instance will hold its own CSL instance and will be fast after the first call to makeBibliography. CitationStyleGenerator has now its own static CSLAdapter instance. JabRefItemDataProvider is now refactored to work on BibEntry.
Things that need to work:
@lenhard I cannot reproduce that the styles are not found with the fat jar. Can you try to delete and rebuild it and show exactly what you try? I see all styles and I can switch them as usual. The fat jar has the same structure as the normal jar with all styles included as files. |
I am doing the following:
Furthermore, I am getting several warnings on the console:
After these warnings I updated my current installation from Java 1.8.0_144 to Java 1.8.0_152 and the warnings were gone, but the rest stays the same (no citation styles in the list). My configuration is now
|
I tested it and I found there is a problem with how you get the path. Will commit a fix. |
Use FileSystem to iterate the content of the jar files
@halirutan @lenhard I fixed the path handling and improved the code for searching inside the jar file. |
@Siedlerchr Can you point out what exactly went wrong with the path so that I don't make the mistake in the future? Otherwise nice clean up! I removed your @lenhard Can you test it again. Your approach is basically what I did too and here it worked. |
citationStyles.add(citationStyle); | ||
} | ||
|
||
Path filePath = Paths.get(CitationStyle.class.getProtectionDomain().getCodeSource().getLocation().toURI()); |
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.
@halirutan The Paths.get method has an overload for accepting an URI.
The misleading method of the URI class (getPath) does not work as itdoesn't inlcude the "scheme" part of the URI object which is used to identify the underlying FileSytem implementation.
This tutorial has a nice overview of "what is a path" https://docs.oracle.com/javase/tutorial/essential/io/path.html
If you happen to use a Classpath url or sth else, the recommended way is the same Paths.get(Class.toUrl.toUri)
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.
That wasn't my code :) When I see this right, it was coming from Christian Bartsch but it doesn't matter. Good that you spotted it.
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.
@halirutan I've tested again, and now the citation styles are again listed in the preferences dialog, as they should be. Thanks!
This unifies the access to the CSL engine. The adapter class will hold one instance of it and only recreate it when the style changes. This improves the performance of from the second call to the Preview drastically and should provide an acceptable speed to fix #2533.
In addition, I extended the search for citation styles when JabRef is run during development.
I will add changelog and friends, but for the moment my biggest problem is that the unit tests are broken on master (at least for me).
gradle localizationUpdate
?