Skip to content
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

Used late initialization for context menus #3340

Merged
merged 2 commits into from
Oct 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- We changed the default paths for the OpenOffice/LibreOffice binaries to the default path for LibreOffice
- We no longer create a new entry editor when selecting a new entry to increase performance. [#3187](https://github.com/JabRef/jabref/pull/3187)
- We increased performance and decreased the memory footprint of the entry editor drastically. [#3331](https://github.com/JabRef/jabref/pull/3331)
- Late initialization of the context menus in the entry editor. This improves performance and memory footprint further [#3340](https://github.com/JabRef/jabref/pull/3340)


### Fixed
Expand Down
2 changes: 1 addition & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ task media(type: com.install4j.gradle.Install4jTask, dependsOn: "releaseJar") {
checkstyle {
// do not use other packages for checkstyle, excluding gen(erated) sources
checkstyleMain.source = "src/main/java"
toolVersion = '8.0'
toolVersion = '8.3'
}

checkstyleMain.shouldRunAfter test
Expand Down
9 changes: 6 additions & 3 deletions src/main/java/org/jabref/gui/fieldeditors/EditorTextArea.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.net.URL;
import java.util.List;
import java.util.ResourceBundle;
import java.util.function.Supplier;

import javafx.fxml.Initializable;
import javafx.scene.control.ContextMenu;
Expand Down Expand Up @@ -49,14 +50,16 @@ public EditorTextArea(String text) {
}

/**
* Adds the given list of menu items to the context menu.
* Adds the given list of menu items to the context menu. The usage of {@link Supplier} prevents that the menus need
* to be instantiated at this point. They are populated when the user needs them which prevents many unnecessary
* allocations when the main table is just scrolled with the entry editor open.
*/
public void addToContextMenu(List<MenuItem> items) {
public void addToContextMenu(Supplier<List<MenuItem>> items) {
TextAreaSkin customContextSkin = new TextAreaSkin(this) {
@Override
public void populateContextMenu(ContextMenu contextMenu) {
super.populateContextMenu(contextMenu);
contextMenu.getItems().addAll(0, items);
contextMenu.getItems().addAll(0, items.get());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand everything correctly, we could build the supplier right here and still accept a List<MenuItem> as method parameter. That would keep the changes very localized.

More discussion see below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, then I misunderstood the whole thing. Every text-field editor needs to attach its own context menu. Als long as I have wrapped it in a supplier, the code is not instantiated. So how is for instance IdentifierEditor supposed to attach a non-instantiated context menu?

textArea.addToContextMenu(EditorMenus.getDefaultMenu(textArea));

Assume we don't do the wrapping inside EditorMenus, then the menu would be instantiated exactly there. Except when I put the Supplier wrapper at this place. But then we have the suppliers scattered over even more classes. What do I miss?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, Jörg, the supplier has to be built outside of this method since otherwise the context menu items are already created. With the help of a supplier, we are able to delay the creation until the user opens the context menu (since only then the get method is called).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then I was wrong. Thanks for clarifying! In that case, please disregard my comment and keep the solution.

}
};
setSkin(customContextSkin);
Expand Down
11 changes: 3 additions & 8 deletions src/main/java/org/jabref/gui/fieldeditors/IdentifierEditor.java
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
package org.jabref.gui.fieldeditors;

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.scene.Parent;
import javafx.scene.control.Button;
import javafx.scene.control.MenuItem;
import javafx.scene.control.Tooltip;
import javafx.scene.layout.HBox;

Expand Down Expand Up @@ -43,12 +40,11 @@ public IdentifierEditor(String fieldName, TaskExecutor taskExecutor, DialogServi
lookupIdentifierButton.setTooltip(
new Tooltip(Localization.lang("Look up %0", FieldName.getDisplayName(fieldName))));

List<MenuItem> menuItems = new ArrayList<>();
if (fieldName.equalsIgnoreCase(FieldName.DOI)) {
menuItems.addAll(EditorMenus.getDOIMenu(textArea));
textArea.addToContextMenu(EditorMenus.getDOIMenu(textArea));
} else {
textArea.addToContextMenu(EditorMenus.getDefaultMenu(textArea));
}
menuItems.addAll(EditorMenus.getDefaultMenu(textArea));
textArea.addToContextMenu(menuItems);

new EditorValidator(preferences).configureValidation(viewModel.getFieldValidator().getValidationStatus(), textArea);
}
Expand Down Expand Up @@ -82,5 +78,4 @@ private void lookupIdentifier(ActionEvent event) {
private void openExternalLink(ActionEvent event) {
viewModel.openExternalLink();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.util.ArrayList;
import java.util.List;
import java.util.function.Supplier;

import javax.swing.AbstractAction;
import javax.swing.Action;
Expand All @@ -14,42 +15,73 @@
import javafx.scene.control.Tooltip;

import org.jabref.gui.actions.CopyDoiUrlAction;
import org.jabref.gui.fieldeditors.EditorTextArea;
import org.jabref.logic.formatter.bibtexfields.NormalizeNamesFormatter;
import org.jabref.logic.l10n.Localization;

/**
* Provides context menus for the text fields of the entry editor. Note that we use {@link Supplier} to prevent an early
* instantiation of the menus. Therefore, they are attached to each text field but instantiation happens on the first
* right-click of the user in that field. The late instantiation is done by {@link
* EditorTextArea#addToContextMenu(java.util.function.Supplier)}.
*/
public class EditorMenus {

public static List<MenuItem> getDefaultMenu(TextArea textArea) {
List<MenuItem> menuItems = new ArrayList<>(5);
menuItems.add(new CaseChangeMenu(textArea.textProperty()));
menuItems.add(new ConversionMenu(textArea.textProperty()));
menuItems.add(new SeparatorMenuItem());
menuItems.add(new ProtectedTermsMenu(textArea));
menuItems.add(new SeparatorMenuItem());
return menuItems;
/**
* The default menu that contains functions for changing the case of text and doing several conversions.
*
* @param textArea text-area that this menu will be connected to
* @return default context menu available for most text fields
*/
public static Supplier<List<MenuItem>> getDefaultMenu(TextArea textArea) {
return () -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what is more readable: Hiding the supplier interface here like you did or instead writing something like addToContextMenu(() -> getDefaultMenu(textArea)). @JabRef/developers any opinions on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am in favor of the second solution, i.e. putting the supplier into EditorTextArea.addToContextMenu(). Reasoning: With the current solution, the lazy initialization is dispersed over two classes. Changes are required in EditorTextArea and EditorMenus. If I understand everything correctly, we could build the supplier inside addToContextMenu() only. That would be a very localized change that hides the lazy initialization as far as possible.

My perception might be wrong, though. Please correct me if this is the case.

List<MenuItem> menuItems = new ArrayList<>(5);
menuItems.add(new CaseChangeMenu(textArea.textProperty()));
menuItems.add(new ConversionMenu(textArea.textProperty()));
menuItems.add(new SeparatorMenuItem());
menuItems.add(new ProtectedTermsMenu(textArea));
menuItems.add(new SeparatorMenuItem());
return menuItems;
};
}

public static List<MenuItem> getNameMenu(TextArea textArea) {
CustomMenuItem normalizeNames = new CustomMenuItem(new Label(Localization.lang("Normalize to BibTeX name format")));
normalizeNames.setOnAction(event -> textArea.setText(new NormalizeNamesFormatter().format(textArea.getText())));
Tooltip toolTip = new Tooltip(Localization.lang("If possible, normalize this list of names to conform to standard BibTeX name formatting"));
Tooltip.install(normalizeNames.getContent(),toolTip);
/**
* The default context menu with a specific menu for normalizing person names regarding to BibTex rules.
*
* @param textArea text-area that this menu will be connected to
* @return menu containing items of the default menu and an item for normalizing person names
*/
public static Supplier<List<MenuItem>> getNameMenu(TextArea textArea) {
return () -> {
CustomMenuItem normalizeNames = new CustomMenuItem(new Label(Localization.lang("Normalize to BibTeX name format")));
normalizeNames.setOnAction(event -> textArea.setText(new NormalizeNamesFormatter().format(textArea.getText())));
Tooltip toolTip = new Tooltip(Localization.lang("If possible, normalize this list of names to conform to standard BibTeX name formatting"));
Tooltip.install(normalizeNames.getContent(), toolTip);

List<MenuItem> menuItems = new ArrayList<>(6);
menuItems.add(normalizeNames);
menuItems.addAll(getDefaultMenu(textArea));

return menuItems;
List<MenuItem> menuItems = new ArrayList<>(6);
menuItems.add(normalizeNames);
menuItems.addAll(getDefaultMenu(textArea).get());
return menuItems;
};
}

public static List<MenuItem> getDOIMenu(TextArea textArea) {
AbstractAction copyDoiUrlAction = new CopyDoiUrlAction(textArea);
MenuItem copyDoiUrlMenuItem = new MenuItem((String) copyDoiUrlAction.getValue(Action.NAME));
copyDoiUrlMenuItem.setOnAction(event -> copyDoiUrlAction.actionPerformed(null));
/**
* The default context menu with a specific menu copying a DOI URL.
*
* @param textArea text-area that this menu will be connected to
* @return menu containing items of the default menu and an item for copying a DOI URL
*/
public static Supplier<List<MenuItem>> getDOIMenu(TextArea textArea) {
return () -> {
AbstractAction copyDoiUrlAction = new CopyDoiUrlAction(textArea);
MenuItem copyDoiUrlMenuItem = new MenuItem((String) copyDoiUrlAction.getValue(Action.NAME));
copyDoiUrlMenuItem.setOnAction(event -> copyDoiUrlAction.actionPerformed(null));

List<MenuItem> menuItems = new ArrayList<>();
menuItems.add(copyDoiUrlMenuItem);
menuItems.add(new SeparatorMenuItem());
return menuItems;
List<MenuItem> menuItems = new ArrayList<>();
menuItems.add(copyDoiUrlMenuItem);
menuItems.add(new SeparatorMenuItem());
menuItems.addAll(getDefaultMenu(textArea).get());
return menuItems;
};
}
}