Refactoring: Lazy init of all editor tabs

[tobiasdiez]

Not to let @halirutan and @lenhard have all the fun, I'll jump onto the "improve-the-entry-editor"-train. So this PR is a natural continuation of #3331.

Changes:

• The entry editor tabs are only initialized once and then later notified about getting the focus or the change of entry.
• The different tabs no longer rely on the JabRefFrame and BasePanel (finally!)
• I removed some commented-out code in the FieldsEditorTab, that was a relic of previous (incomplete) refactorings.
• Disable tab animation.

---

• 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?

[halirutan]

Nice! I pulled your branch and I'm looking over it. One thing up front: As expected, the memory consumption went again down drastically and the same run through the 209 entry example was now possible even without the GC kicking in

I see here only 240MB of RAM that was needed and the speed is awesome.

[halirutan]

With the changes, tabs no longer fly-in but for some reason, the order is not preserved.

This is just horrible. The sorting of the observable list tabbed.getTabs() is correct all the time. The some reason for the reordering is in com.sun.javafx.scene.control.skin.TabPaneSkin. SetAll in your code works so smoothly since the skin does not really set the list. It maintains a to be removed and to be added list and unfortunately, it inserts new elements, like the FileAnnotation tab that is often not accessible at the front.

Even more unfortunate is that the TabPaneSkin is an almost completely inaccessible class. Everything important is private. I guess we will have to do the removing and insertion of tabs ourselves.

I thought maybe we can simply have all tabs in tabbed and close and open them. Therefore,
I looked into what closing a tab means but it's really just removing the tab from the list. See com.sun.javafx.scene.control.behavior.TabPaneBehavior#closeTab for details. That's the reason why you don't find a open tab there.

The best solution from the developer's point of view is disabling the tabs. We wouldn't even have to keep a tabs variable then. If this is absolutely no way to go, then we need to write a small algorithm that (a) removes unwanted tabs from the existing list and (b) inserts new tabs that were not in there at the right position.

[halirutan]

I don't have push access, but here is the patch for manually changing the tabs list without removing everything and re-adding all tabs. In IDEA you can try it out by using VCS -> Apply Patch.

[tobiasdiez]

Thanks @halirutan for the feedback and the patch. I took your ideas but implemented it somewhat different (the outcome should be the same). Everything seems to work now, so this PR is ready for review.

[Siedlerchr]

Code wise looks good to me. Will test it outl ater

[lenhard]

Great job overall. The code looks fine. I have nothing to criticize expect my usual stuff: Please add a changelog entry :-)

Here's an example of when I really try to push the entry editor to the limits with having key down pressed in the main table:

That is excellent, I am not getting JabRef above 800 MB. I also have the impression that GC is kicking in much more frequently. Here is also a diagram of some "longer" development:

You can see the initial spikes. Then during idle time, memory stays up and it takes some time for GC to start. What annoys me is that JabRef does not free more unused heap, but I would probably have to wait longer for that and in the end it is a different issue.

What I haven't tested extensively is that the entry editor actually works, i.e. that updates in the entry editor go to the right entry. I was surprised that I don't see many changes in the update structure in the code in this PR, but maybe it was simpler to achieve than I expected. I wouldn't be surprised if we find update bugs after merging this.

TL&DR: Very cool PR. It is almost ready to merge in my point of view.

[tobiasdiez]

@lenhard We already have a changelog entry for the performance improvements, so I didn't added anything there in addition.

@koppor I expect at least few beers from you for everybody at the next JabCon for bearing the constant annoying stylecheck complains 😺

[koppor]

@tobiasdiez ...if you join ... Am 23.10.2017 15:47 schrieb "Tobias Diez" <[email protected]>: @lenhard <https://github.com/lenhard> We already have a changelog entry for the performance improvements, so I didn't added anything there in addition. @koppor <https://github.com/koppor> I expect at least few beers from you for everybody at the next JabCon for bearing the constant annoying stylecheck complains 😺 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3333 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTafovGcpIBAL_WTbi64VuVCGT7FJgYks5svJjGgaJpZM4QByqk> .

[lenhard]

@tobiasdiez Alright, I am fine with that. I have removed the [WIP] part in the title, since this is no longer work in progress, I guess.

We're practically good to go here, right? @Siedlerchr wanted to do some more testing and then we can merge.

[Siedlerchr]

Tried around, works amazingly fast! Did not find any issue so far, so I would merge it now!
Thanks all contributors!