Improve performance of entry editor #2892

tobiasdiez · 2017-06-06T21:16:36Z

This PR improves the performance of the entry editor to a satisfactory level (switching between entries leads to a short flicker of the entry editor but no real delay). Things changed:

Tab panel is converted to JavaFX
Each tab is initialized only upon focus

Moreover, a bit of the code was refactored (e.g. the creation of the different tabs is extracted from the EntryEditor class to many smaller classes, one for each tab).

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 · 2017-06-07T07:15:43Z

src/main/java/org/jabref/gui/contentselector/ContentSelectorDialog.java

        if (changedFieldSet) {
-            // We have added or removed content selectors, update the entry editor
-            panel.rebuildAllEntryEditors();
+            // TODO: We have added or removed content selectors, update the entry editor


What about this TODO? This shouldn't be merged as long as the PR still breaks the feature.

The content selector feature is not yet reimplemented in the new FX entry editor and this PR doesn't change anything in this respect. The purpose of this particular code line was to update the content selectors in the editor if the user changes them. But since content editors are currently not enabled, I can't say which is the best way to reimplement the update functionality.

Ok, I get it. If this is just a false marking by github and the TODOs have been there before, then this can just move forward.

However, we need to be sure that we do not release 4.0 without content selectors. Should we open an issue for this? Or will you keep this in mind and open a PR at some point?

I have this in mind. I plan to implement content selectors along with auto-completion in one of the next PRs (as soon as the big problems are resolved).

lenhard

As always, it is quite hard to review this amount of code, so I cannot guarantee that I didn't miss something. Still, I tested it locally and the performance really improves noticeably. Good job :-) It is also good to see that MaterialDesign finally has codes for its Icons.

The problem that I see is that there are quite a few TODOs still in the code and as far as I tested, the content selector feature is gone again (the manage dialog is there, but I don't get them to appear in the GUI).

All of the TODOs need to be fixed before we can think of merging the PR.

lenhard · 2017-06-07T07:17:47Z

src/main/java/org/jabref/gui/entryeditor/FieldsEditorTab.java

+
+        for (String fieldName : fields) {
+
+            // TODO: Reenable/migrate this


Same here and at a few positions below. This looks like you are removing a few features including content selectors (again)?

I actually didn't change anything here in this PR (github just marks the whole file as changed since it was renamed).

lenhard · 2017-06-07T07:18:25Z

src/main/java/org/jabref/gui/entryeditor/FieldsEditorTab.java

+            
+            // Add autocompleter listener, if required for this field:
+            /*
+            AutoCompleter<String> autoCompleter = bPanel.getAutoCompleters().get(field);


Just a note: The auto completer is broken and therefore it is one of the few features that is fine to go.

lenhard · 2017-06-07T07:19:58Z

src/main/java/org/jabref/gui/entryeditor/OtherFieldsTab.java

+import org.jabref.model.entry.EntryType;
+
+public class OtherFieldsTab extends FieldsEditorTab {
+    public OtherFieldsTab(JabRefFrame frame, BasePanel basePanel, EntryType entryType, EntryEditor parent, BibEntry entry) {


Very minor, but please add a blank between the class declaration and the method.

lenhard · 2017-06-07T07:23:03Z

src/main/java/org/jabref/gui/entryeditor/SourceTab.java

+     */
+    private void setupJTextComponent(JTextComponent textComponent) {
+        /*
+        // TODO: Set up key bindings and focus listener for the code editor.


This todo is quite important

Agreed. The entry editor specific key bindings are now activated again and they work as expected. There are still issues with the key bindings, especially when it comes to more global ones - but this is already tracked as #2855.

Siedlerchr · 2017-06-07T12:51:52Z

src/main/java/org/jabref/gui/entryeditor/SourceTab.java

+            }
+
+            // First, remove fields that the user has removed.
+            for (Map.Entry<String, String> field : entry.getFieldMap().entrySet()) {


You could rewrite this to streams, using entrySet().stream()

You mean as entrySet().stream().foreach(...) ? I really like the old-style for loop more when it comes to a complicated body and find foreach only preferable when calling one method.

No, I thought about directly applying the

internalBibtexFields.isDisplayableField(fieldName) && !newEntry.hasField(fieldName)
as filter option in the stream and then applying anyMatch()
So:

entry.getFieldMap().entrySet().stream().filter(field->InternalBibtexFields.isDisplayableField(field.gett )
https://www.mkyong.com/java8/java-8-filter-a-map-examples/

But now I see it may not make sense

Ah ok, that would be possible yes. Since this is only a slight improvement, I will leave the for-loop for now.

Siedlerchr · 2017-06-07T12:52:01Z

src/main/java/org/jabref/gui/entryeditor/SourceTab.java

+            }
+
+            // Then set all fields that have been set by the user.
+            for (Map.Entry<String, String> field : newEntry.getFieldMap().entrySet()) {


Same as aboe

…ePerformanceEditor

* upstream/master: Greek Translation (#2835) Updated JabRef_tr.properties (#2894) Display deprecated fields in single-column layout Improve performance of entry editor (#2892) Improve person names checker (#2876) Fix #2871: all field editors have maximal height and width Fix #2880: NPE when importing in a database without groups Updated Menu_tr.properties (#2884) Updated JabRef_tr.properties (#2883) Updated mysql-connector-java from 5.1.40 -> 5.1.42 Updated java-string-similarity from 0.23 -> 0.24 Add small change to push build again # Conflicts: # external-libraries.txt

tobiasdiez added 6 commits May 31, 2017 23:03

Remove FXML for SimpleEditor to improve performance

2b942b6

Convert tab panel in entry editor to JavaFX

f44a1ec

Reuse icon code from external library

3b6330f

Fix order

8cf1b6d

Fix order

243fd9f

Fix order

fdf7a0e

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

lenhard reviewed Jun 7, 2017

View reviewed changes

lenhard suggested changes Jun 7, 2017

View reviewed changes

Fix style

8197c3c

Siedlerchr reviewed Jun 7, 2017

View reviewed changes

tobiasdiez added 7 commits June 7, 2017 18:28

Reimplement entry editor specific key bindings

87f03c4

Fix style

8ae1bbb

Fix tests

c2ee2cd

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

17f8036

…ePerformanceEditor

Merge

254580c

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

3446d1b

…ePerformanceEditor

Update language files

d44245c

tobiasdiez merged commit 52b887d into master Jun 7, 2017

tobiasdiez deleted the improvePerformanceEditor branch June 7, 2017 20:14

koppor added component: entry-editor dev: performance labels Feb 11, 2025

koppor mentioned this pull request Feb 11, 2025

Refactor IoC - A: Extract Entry Editor #12210

Merged

7 tasks


		for (String fieldName : fields) {

		// TODO: Reenable/migrate this

Uh oh!

Improve performance of entry editor #2892

Improve performance of entry editor #2892

Uh oh!

Conversation

tobiasdiez commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lenhard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants