[WIP] Convert entry preview panel to JavaFX #3504

tobiasdiez · 2017-12-08T23:05:04Z

I migrated the entry preview to JavaFX. My hope was to resolve the memory issues #2533, but this was sadly not successful (thus the issues really come from the citation style cache and not from the display).
Notable differences:

The default JavaFX font seems to be bigger. In my opinion, it looks better that way.
The description for two options in the right-click menu has a display bug. The reason is that I use lang.menuTitle to get the text but menu titles contain the acceleration key binding. Probably one needs to convert the main toolbar to JavaFX as well in order to fix this problem.

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

lenhard

I have tested it locally and it works except for the printing and the display bug you mentioned. These things should still be fixed or addressed in some way before this is merged.

I also have a few code comments, mainly addressing code duplication with the JFXPanel.

@halirutan has done some more analysis regarding the citation styles since you opened this PR. Maybe you can use this information to improve performance now?

lenhard · 2017-12-12T11:31:42Z

src/main/java/org/jabref/gui/BasePanel.java

+    }
+
+    private JFXPanel setupPreviewContainer(PreviewPanel preview) {
+        JFXPanel container = OS.LINUX ? new CustomJFXPanel() : new JFXPanel();


I don't like the fact this chunk of three lines is duplicated all over the place. Maybe add a new factory class that provides a method to build a JFXPanel but hides the OS aspect.

Good remark! Fixed.

lenhard · 2017-12-12T11:35:45Z

src/main/java/org/jabref/gui/PreviewPanel.java

+        copyPreview.setOnAction(event -> copyPreviewToClipBoard());
+        MenuItem printEntryPreview = new MenuItem(Localization.lang("Print entry preview"), IconTheme.JabRefIcon.PRINTED.getGraphicNode());
+        printEntryPreview.setOnAction(event -> print());
+        MenuItem previousPreviewLayout = new MenuItem(Localization.menuTitle("Previous preview layout"));


On my machine, the localization string appears prefixed with an &. Even if that's not nicely solvable here, could you at least implement a quick hack to remove the preceeding &?

Also, in the Swing version the context menu text also listed the shortcuts for the menu item. This should be the case here as well.

I removed the & and added the display of the shortcut.

lenhard · 2017-12-12T11:35:54Z

src/main/java/org/jabref/gui/PreviewPanel.java

+        printEntryPreview.setOnAction(event -> print());
+        MenuItem previousPreviewLayout = new MenuItem(Localization.menuTitle("Previous preview layout"));
+        previousPreviewLayout.setOnAction(event -> basePanel.ifPresent(BasePanel::previousPreviewStyle));
+        MenuItem nextPreviewLayout = new MenuItem(Localization.menuTitle("Next preview layout"));


Same here with the display bug.

lenhard · 2017-12-12T11:38:40Z

src/main/java/org/jabref/gui/PreviewPanel.java

-                }
-            });
-        }
+    public void print() {


This method does not work on my machine. In the swing version a dialog would appear. Here, the printer icon pops up in the taskbar, but no documents are added.

EDIT: I take it back, the method does seem to work. When I just logged into the printer I found a document waiting for me. However, this should be less of a surprise. In the old version a dialog popped up where you could configure the printer a little. We don't need the exact same thing here, but could at least have a dialog displayed for like two seconds, saying: "Printing..." or something similar.

ohh gosh. By default, I print to PDF and thus the PDF print dialog poped up and everything seemed to work just fine. But, of course, in general you want to have a confirmation dialog and a possibility to change the printer. Luckily, javafx provides a default dialog for printing, so this was easy to implement.

lenhard · 2017-12-12T11:39:08Z

src/main/java/org/jabref/gui/bibtexkeypattern/ResolveDuplicateLabelDialog.java

-            b.add(new JScrollPane(pp)).xy(3, row);
+            PreviewPanel previewPanel = new PreviewPanel(null, null);
+            previewPanel.setEntry(entry);
+            JFXPanel container = OS.LINUX ? new CustomJFXPanel() : new JFXPanel();


Again the construction of the panel should be outsourced somewhere.

lenhard · 2017-12-12T11:40:05Z

src/main/java/org/jabref/gui/importer/ImportInspectionDialog.java

@@ -230,7 +234,9 @@ public ImportInspectionDialog(JabRefFrame frame, BasePanel panel, String undoNam
        centerPan.setLayout(new BorderLayout());

        contentPane.setTopComponent(new JScrollPane(glTable));
-        contentPane.setBottomComponent(preview);
+        JFXPanel container = OS.LINUX ? new CustomJFXPanel() : new JFXPanel();


Again outsource as above.

lenhard · 2017-12-12T11:40:30Z

src/main/java/org/jabref/gui/mergeentries/MergeEntries.java

-        mainPanel.add(entryPreview, CELL_CONSTRAINTS.xyw(1, 8, 6));
+        entryPreview = new PreviewPanel(null, null);
+        entryPreview.setEntry(mergedEntry);
+        JFXPanel container = OS.LINUX ? new CustomJFXPanel() : new JFXPanel();


Again outsource as above.

lenhard · 2017-12-12T11:40:47Z

src/main/java/org/jabref/gui/openoffice/StyleSelectDialog.java

@@ -154,7 +158,9 @@ private void init() {
        builder.add(new JScrollPane(table)).xyw(1, 3, 5);
        builder.add(addButton).xy(3, 5);
        builder.add(removeButton).xy(5, 5);
-        builder.add(preview).xyw(1, 7, 5);
+        JFXPanel container = OS.LINUX ? new CustomJFXPanel() : new JFXPanel();


Again outsource as above.

lenhard · 2017-12-12T11:41:02Z

src/main/java/org/jabref/gui/preftabs/PreviewPrefsTab.java

+                PreviewPanel testPane = new PreviewPanel(null, null);
+                testPane.setFixedLayout(layout.getText());
+                testPane.setEntry(TestEntry.getTestEntry());
+                JFXPanel container = OS.LINUX ? new CustomJFXPanel() : new JFXPanel();


Again outsource as above.

lenhard · 2017-12-12T11:41:13Z

src/main/java/org/jabref/gui/search/SearchResultFrame.java

@@ -149,7 +154,10 @@ private void init(String title) {
        entryTable.addMouseListener(new TableClickListener());

        contentPane.setTopComponent(sp);
-        contentPane.setBottomComponent(preview);
+
+        JFXPanel container = OS.LINUX ? new CustomJFXPanel() : new JFXPanel();


Again outsource as above.

halirutan · 2017-12-12T18:01:21Z

@tobiasdiez @lenhard I hacked a version of the CitationStyleGenerator that manages a static instance of CSL. You find it in this gist. It vastly improves performance and memory footprint. The only sad thing is, that CSL itself needs so much memory, even for one single instance. I find over 500MB not acceptable even if many machines nowadays are equipped with at least 8GB, laptops might not.

Remarks: CSL is in general not threadsafe, but I'm not sure this is a problem with our current implementation. I tried to use ThreadLocal as well, but we have an armada of worker threads and doInBackground selects one. This means that several thread-local CSL instances are created which is not helping much.

The current behavior is that the first Preview takes as long as it took and until the style or the output format is changed, all follow-up calls are cheap, fast and don't use much memory. In numbers this means that while before one call to create the preview text needed 70.000 ms under profiling, it now takes on average about 800ms. The memory is better as well. Before we had about 85MB per preview and now 100 previews need 125MB (including the first expensive call).

Btw: Don't be confused when I say it needs over 500MB and it needs 85MB. The profiler does not count all objects and makes certain estimates. So the profiling memory will usually be smaller than what you see in your memory monitor.

lenhard · 2017-12-13T08:39:44Z

@halirutan From my point of view, we could just scrap the CSLs again and remove them from JabRef completely. But I think @koppor has a strong opinion on it.

Maybe the right way to go is to have them improve their implementation and I've seen that you've already opened an issue for that michel-kraemer/citeproc-java#40 Let's hope they react.

koppor · 2017-12-13T09:21:03Z

Yeah, CSL was wished by many users and is a strong selling point 😇 To improve speed, we added the CSL cache. Think, with JavaScript V8, it also can get much better. Maybe, we should put energy in that... Reimplementing csl-proc in Java is also possible, but much effort...

halirutan · 2017-12-15T02:17:58Z

I checked out this branch and I would have included my fix in here, but we still have the big problem, that UTF8 fonts are not displayed correctly. This might be related to this question I asked recently although now it's for JavaFX and I'm on Linux.

Anyway, in the old preview, if an entry contains e.g. textit{..}, it looks like this in the preview

In the JavaFX preview from this branch, the same entry displays like this

If you like to try this out, here is the shortened entry

@PhdThesis{deBraga2001,
  author    = {Michael deBraga},
  title     = {{The postcranial anatomy of \textit{Procolophon} (Parareptilia: Procolophonidae) and its implications for the origin of turtles}},
  school    = {Department of Zoology, University of Toronto},
  year      = {2001},
  type      = {PhD thesis},
  address   = {Canada},
  note      = {303 pp.}
}

My PR for the CSL performance should be almost unrelated to this here and can later easily be merged if we find it works acceptably.

…Preview

tobiasdiez · 2017-12-15T21:30:10Z

Thanks for the feedback. I revised the code accordingly.

@halirutan I couldn't replicate the problem with textit but I only tested it for the default preview layout (not cls). I would like to revisit this problem as soon as your PR with the simplified cls inclusion is merged so that testing gets easier. According to google, the problem occurs when custom fonts are used (see e.g. stackexchange)

koppor · 2017-12-20T09:43:22Z

~~latex2unicode uses the math alphabet instead of the normal alphabet for textit. Note hat in mathematics, mathit is the command for italics text, not textit.~~

~~See https://github.com/tomtung/latex2unicode/blob/a3610c3b8161b5d2e604e1bebc9a1a1d780e4474/src/main/scala/com/github/tomtung/latex2unicode/helper/Unary.scala#L647~~

~~and compare 𝐻 with 𝘏:~~

Everything wrong above. There is only one italics H. I totally misunderstood the comment at https://stackoverflow.com/q/46954858/873282:

--cut - emphasize is mine --
Yes. I thought those characters were just your example of supplementary characters. If your goal is simply to show bold and/or italic text, you should never use the Mathematical Alphanumeric Symbols block of Unicode to accomplish that. That Unicode block has a specific purpose, related to mathemtical formulae; any other use is abuse of the characters. You don’t even need fonts; just use "This is a test for italics and a test for bold". See also meta.stackoverflow.com/questions/342024
--end--

Siedlerchr · 2017-12-20T10:05:34Z

From what I read, the mathematic area is the only font which provides italics. And I am unsure whether we really should support that.

halirutan · 2017-12-20T10:31:59Z

Question: If there was a mystical latex2html, could this be a replacement for all usages of the latex2unicode converter?

For the Mathematica plugin, I had to transform the doc of built-in function to html. Fortunately, I could even use MathML and now the function docs contain everything from italics, simple formulas, and integrals.

Siedlerchr · 2017-12-20T11:12:11Z

@halirutan There is a latex2html converter. But not all cases should be handled by this, for example you typically want names with accents or other weird characters to be displayed as Unicode.

Siedlerchr · 2017-12-20T11:32:38Z

I propose the following which may make our life a lot easier:

Main Table is a swing component which uses HTML
The entry preview is soon a WebView Component, so it's also HTML
CSL gets also written to HTML, if I am right

So why don't we use the latex2html converter on both of them?
This would make our life a lot easier.
Maybe we should discuss this in a devcall?

halirutan · 2017-12-20T11:42:54Z

@Siedlerchr That was the reason behind my question. I ran several times into this issue on both Linux and OS X and it was the reason for the question on SO. There, it was also suggested to use HTML and I'm also confident, that it would make life easier.

In addition to "it is rendered at all" we hopefully get a better quality. On Linux the italics look so ugly that it is almost not bearable. Therefore, if easily possible, I vote for using HTML for rendering LaTeX.

lenhard · 2017-12-20T12:13:09Z

One reason for using latex2unicode was that our own custom conversion was relatively incomplete and partly broken. I am not sure on the state of the html conversion, but I wouldn't presume that it's perfect. So if we go for displaying html everywhere (which is perfectly fine in my point of view), it would also be cool if we had a robust Java library that does the LaTeX to html conversion. I am not sure if such a library exists, though, and then we will have to make do with the current conversion.

The main reason for latex2unicode was to enable a proper search of entries independent of LaTeX commands. As far as I understand, this shouldn't conflict with displaying html and can be kept as is.

tobiasdiez · 2017-12-20T13:08:36Z

So how do we want to proceed with this PR? The discussion concerning latex2unicode/html is a bit decoupled from the actual purpose of this PR (which is just to move forward with the migration to JavaFX). Maybe it also makes sense to extract our latex2html code and merge it into the latex2unicode library we use.

koppor · 2017-12-20T14:41:17Z

I remember the discussions with @oscargus who argued that our latex2html converter is the most complete one. Maybe, we should offer it as library? Refs #110

halirutan · 2017-12-20T17:18:43Z

@tobiasdiez I really would like to concentrate on the JavaFX part of this PR as well, but until the unicode issue is resolved, this breaks preview for entries that contain any LaTeX on my machine. As long as I'm not the only one where this happens, I see no way we could merge this PR without annoying a lot of users. Therefore, we need to discuss the latex2XXX issue somewhere first. Do you have a suggestion how we should proceed?

koppor · 2017-12-22T09:29:30Z

+1 for switchting to latex2html in this case (entry preview). Thus, moved to v4.2 and not for v4.1 release.

tobiasdiez · 2017-12-23T20:56:26Z

I now used latex2html for the preview and it seems to work fine. I also implemented the citation style provider as discussed with @halirutan in his recent PR.

Sadly, I made some mistakes with while checking out my code and now cannot push to this branch anymore (git claims "everything is up-to-date" - well it isn't). So continued at #3574.

Siedlerchr · 2017-12-23T21:05:40Z

@tobiasdiez in that case you have to do a force push, e.g. when amending your last commit.

halirutan · 2017-12-23T21:06:28Z

@tobiasdiez Only regarding the

Sadly, I made some mistakes with while checking out my code

When you merge updates into this branch and you use rebase, then you cannot push it again. If you are sure no one else has used your branch, you have to make a force push and everything is fine. Rebasing your (local) branch looks nicer because you don't get a cluttered history wich merge commits, but it should only be used you are working on a local branch.

tobiasdiez · 2017-12-23T21:22:35Z

Thanks for the help but the problem was not about the rebase/force pushed. I created the branch on another pc and used the github gui tool to check it out on a new one. For some reason git now thinks the remote branch associated to my local code is JabRef/javafxPreview instead of just javafxPreview. I could probably remove/readd the remote branch but it was easier to just create a new PR.

koppor · 2017-12-23T22:08:57Z

You can edit .git/config directly for changing branch config. You can also use gitk --all to check, what's going on. Finally, git push -f origin HEAD:BranchNameAtOrigin always works; regardless of your local branch name. No need to create a new PR then... Am 23.12.2017 22:22 schrieb "Tobias Diez" <notifications@github.com>:

…

Thanks for the help but the problem was not about the rebase/force pushed. I created the branch on another pc and used the github gui tool to check it out on a new one. For some reason git now thinks the remote branch associated to my local code is JabRef/javafxPreview instead of just javafxPreview. I could probably remove/readd the remote branch but it was easier to just create a new PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3504 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTafhu0cSaLVRCdGZVkQx8EhjmoDjIpks5tDW8dgaJpZM4Q7wk4> .

Convert entry preview panel to JavaFX

6d036f5

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 8, 2017

This was referenced Dec 9, 2017

Citation preview incrementally increases memory usage with each preview #2533

Closed

Cannot resolve '\textendash', etc. when displaying article title #3307

Closed

lenhard suggested changes Dec 12, 2017

View reviewed changes

Merge branch 'master' of https://github.com/JabRef/jabref into javafx…

044d01d

…Preview

Revise code according to feedback

7416b34

Siedlerchr added the status: devcall label Dec 20, 2017

LinusDietz removed the status: devcall label Dec 22, 2017

koppor added this to the v4.2 milestone Dec 22, 2017

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 22, 2017

koppor changed the title ~~Convert entry preview panel to JavaFX~~ [WIP] Convert entry preview panel to JavaFX Dec 22, 2017

tobiasdiez mentioned this pull request Dec 23, 2017

Convert entry preview panel to JavaFX #3574

Merged

6 tasks

tobiasdiez closed this Dec 23, 2017

tobiasdiez deleted the javafxPreview branch December 23, 2017 20:56

[WIP] Convert entry preview panel to JavaFX #3504

[WIP] Convert entry preview panel to JavaFX #3504

Conversation

tobiasdiez commented Dec 8, 2017

lenhard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenhard Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halirutan commented Dec 12, 2017 • edited Loading

lenhard commented Dec 13, 2017

koppor commented Dec 13, 2017 via email • edited Loading

halirutan commented Dec 15, 2017

tobiasdiez commented Dec 15, 2017 • edited Loading

koppor commented Dec 20, 2017 • edited Loading

Siedlerchr commented Dec 20, 2017

halirutan commented Dec 20, 2017

Siedlerchr commented Dec 20, 2017

Siedlerchr commented Dec 20, 2017

halirutan commented Dec 20, 2017

lenhard commented Dec 20, 2017

tobiasdiez commented Dec 20, 2017

koppor commented Dec 20, 2017

halirutan commented Dec 20, 2017

koppor commented Dec 22, 2017

tobiasdiez commented Dec 23, 2017

Siedlerchr commented Dec 23, 2017

halirutan commented Dec 23, 2017

tobiasdiez commented Dec 23, 2017

koppor commented Dec 23, 2017 via email

lenhard Dec 12, 2017 •

edited

Loading

halirutan commented Dec 12, 2017 •

edited

Loading

koppor commented Dec 13, 2017 via email •

edited

Loading

tobiasdiez commented Dec 15, 2017 •

edited

Loading

koppor commented Dec 20, 2017 •

edited

Loading