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

3189 fix group renaming #4470

Merged
merged 23 commits into from
Nov 23, 2018
Merged
Show file tree
Hide file tree
Changes from 5 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
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/groups/GroupDialog.java
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,8 @@ public GroupDialog(JabRefFrame jabrefFrame, AbstractGroup editedGroup) {
}
}

resultingGroup = new ExplicitGroup(groupName, getContext(),
keywordDelimiter);
editedGroup.nameProperty().setValue(groupName);
resultingGroup = editedGroup;
} else if (keywordsRadioButton.isSelected()) {
// regex is correct, otherwise OK would have been disabled
// therefore I don't catch anything here
Expand Down
16 changes: 12 additions & 4 deletions src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import javafx.beans.binding.BooleanBinding;
import javafx.beans.property.SimpleBooleanProperty;
import javafx.beans.property.SimpleIntegerProperty;
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;
import javafx.collections.FXCollections;
import javafx.collections.ListChangeListener;
import javafx.collections.ObservableList;
Expand Down Expand Up @@ -42,7 +44,7 @@

public class GroupNodeViewModel {

private final String displayName;
private StringProperty displayNameProperty;
private final boolean isRoot;
private final ObservableList<GroupNodeViewModel> children;
private final BibDatabaseContext databaseContext;
Expand All @@ -64,7 +66,9 @@ public GroupNodeViewModel(BibDatabaseContext databaseContext, StateManager state
this.localDragBoard = Objects.requireNonNull(localDragBoard);

LatexToUnicodeFormatter formatter = new LatexToUnicodeFormatter();
displayName = formatter.format(groupNode.getName());
displayNameProperty = new SimpleStringProperty(formatter.format(groupNode.getGroup().getName()));
Ali96kz marked this conversation as resolved.
Show resolved Hide resolved
groupNode.getGroup().nameProperty().bindBidirectional(displayNameProperty);

isRoot = groupNode.isRoot();
if (groupNode.getGroup() instanceof AutomaticGroup) {
AutomaticGroup automaticGroup = (AutomaticGroup) groupNode.getGroup();
Expand Down Expand Up @@ -134,7 +138,11 @@ public SimpleBooleanProperty hasChildrenProperty() {
}

public String getDisplayName() {
return displayName;
return displayNameProperty.getValue();
}

public StringProperty displayNameProperty() {
return displayNameProperty;
}

public boolean isRoot() {
Expand Down Expand Up @@ -166,7 +174,7 @@ public boolean equals(Object o) {
@Override
public String toString() {
return "GroupNodeViewModel{" +
"displayName='" + displayName + '\'' +
"displayNameProperty='" + displayNameProperty + '\'' +
Copy link
Member

Choose a reason for hiding this comment

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

_You should probably call here .getValue()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

", isRoot=" + isRoot +
", icon='" + getIcon() + '\'' +
", children=" + children +
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/gui/groups/GroupTreeView.java
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public void initialize() {
// Icon and group name
mainColumn.setCellValueFactory(cellData -> cellData.getValue().valueProperty());
mainColumn.setCellFactory(new ViewModelTreeTableCellFactory<GroupNodeViewModel, GroupNodeViewModel>()
.withText(GroupNodeViewModel::getDisplayName)
.withText(GroupNodeViewModel::displayNameProperty)
.withIcon(GroupNodeViewModel::getIcon)
.withTooltip(GroupNodeViewModel::getDescription));

Expand Down
16 changes: 1 addition & 15 deletions src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public void addNewSubgroup(GroupNodeViewModel parent) {
// TODO: Expand parent to make new group visible
//parent.expand();

dialogService.notify(Localization.lang("Added group \"%0\".", group.getName()));
dialogService.notify(Localization.lang("Added group \"%0\".", parent.getDisplayName()));
writeGroupChangesToMetaData();
});
}
Expand All @@ -161,20 +161,6 @@ public void editGroup(GroupNodeViewModel oldGroup) {
Optional<AbstractGroup> newGroup = dialogService
.showCustomDialogAndWait(new GroupDialog(oldGroup.getGroupNode().getGroup()));
newGroup.ifPresent(group -> {
// TODO: Keep assignments
Copy link
Member

Choose a reason for hiding this comment

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

Why did you removed this code?

boolean keepPreviousAssignments = dialogService.showConfirmationDialogAndWait(
Localization.lang("Change of Grouping Method"),
Localization.lang("Assign the original group's entries to this group?"));
// WarnAssignmentSideEffects.warnAssignmentSideEffects(newGroup, panel.frame());
boolean removePreviousAssignents = (oldGroup.getGroupNode().getGroup() instanceof ExplicitGroup)
&& (group instanceof ExplicitGroup);

oldGroup.getGroupNode().setGroup(
group,
keepPreviousAssignments,
removePreviousAssignents,
stateManager.getEntriesInCurrentDatabase());

// TODO: Add undo
// Store undo information.
// AbstractUndoableEdit undoAddPreviousEntries = null;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jabref.gui.util;

import javafx.beans.property.StringProperty;
import javafx.event.EventHandler;
import javafx.scene.Node;
import javafx.scene.control.Tooltip;
Expand All @@ -19,12 +20,12 @@
*/
public class ViewModelTreeTableCellFactory<S, T> implements Callback<TreeTableColumn<S, T>, TreeTableCell<S, T>> {

private Callback<S, String> toText;
private Callback<S, StringProperty> toText;
private Callback<S, Node> toGraphic;
private Callback<S, EventHandler<? super MouseEvent>> toOnMouseClickedEvent;
private Callback<S, String> toTooltip;

public ViewModelTreeTableCellFactory<S, T> withText(Callback<S, String> toText) {
Copy link
Member

Choose a reason for hiding this comment

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

Changing this to StringProperty shouldn't be necessary. Normally, the table automatically refreshes the cell if the underlying data is changed. Please revert this change even if then the bug is no longer fixed (see the general comment).

public ViewModelTreeTableCellFactory<S, T> withText(Callback<S, StringProperty> toText) {
this.toText = toText;
return this;
}
Expand Down Expand Up @@ -66,7 +67,7 @@ protected void updateItem(T item, boolean empty) {
} else {
S viewModel = getTreeTableRow().getItem();
if (toText != null) {
setText(toText.call(viewModel));
textProperty().bindBidirectional(toText.call(viewModel));
}
if (toGraphic != null) {
setGraphic(toGraphic.call(viewModel));
Expand Down
16 changes: 11 additions & 5 deletions src/main/java/org/jabref/model/groups/AbstractGroup.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import java.util.Objects;
import java.util.Optional;

import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;
import javafx.scene.paint.Color;

import org.jabref.model.entry.BibEntry;
Expand All @@ -18,7 +20,7 @@ public abstract class AbstractGroup implements SearchMatcher {
/**
* The group's name.
*/
protected final String name;
protected final StringProperty name = new SimpleStringProperty();
/**
* The hierarchical context of the group.
*/
Expand All @@ -29,14 +31,14 @@ public abstract class AbstractGroup implements SearchMatcher {
protected Optional<String> iconName = Optional.empty();

protected AbstractGroup(String name, GroupHierarchyType context) {
this.name = name;
this.name.setValue(name);
this.context = Objects.requireNonNull(context);
}

@Override
public String toString() {
return "AbstractGroup{" +
"name='" + name + '\'' +
"name='" + name.getValue() + '\'' +
", context=" + context +
", color=" + color +
", isExpanded=" + isExpanded +
Expand All @@ -54,13 +56,13 @@ public boolean equals(Object other) {
return false;
}
AbstractGroup that = (AbstractGroup) other;
return Objects.equals(this.name, that.name) && Objects.equals(this.description, that.description)
return Objects.equals(this.name.getValue(), that.name) && Objects.equals(this.description, that.description)
&& Objects.equals(this.context, that.context);
}

@Override
public int hashCode() {
return Objects.hash(name, description, context);
return Objects.hash(name.getValue(), description, context);
}

public Optional<Color> getColor() {
Expand Down Expand Up @@ -122,6 +124,10 @@ public GroupHierarchyType getHierarchicalContext() {
* Returns this group's name, e.g. for display in a list/tree.
*/
public final String getName() {
return name.getValue();
}

public StringProperty nameProperty() {
return name;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public String getField() {

@Override
public AbstractGroup deepCopy() {
return new AutomaticKeywordGroup(this.name, this.context, field, this.keywordDelimiter, keywordHierarchicalDelimiter);
return new AutomaticKeywordGroup(this.name.getValue(), this.context, field, this.keywordDelimiter, keywordHierarchicalDelimiter);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public int hashCode() {

@Override
public AbstractGroup deepCopy() {
return new AutomaticPersonsGroup(this.name, this.context, this.field);
return new AutomaticPersonsGroup(this.name.getValue(), this.context, this.field);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/model/groups/ExplicitGroup.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public List<String> getLegacyEntryKeys() {

@Override
public int hashCode() {
return Objects.hash(name, context, legacyEntryKeys, iconName, color, description, isExpanded);
return Objects.hash(name.getValue(), context, legacyEntryKeys, iconName, color, description, isExpanded);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/model/groups/TexGroup.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public boolean isDynamic() {
@Override
public AbstractGroup deepCopy() {
try {
return new TexGroup(name, context, filePath, auxParser, fileMonitor);
return new TexGroup(name.getValue(), context, filePath, auxParser, fileMonitor);
} catch (IOException ex) {
// This should never happen because we were able to monitor the file just fine until now
LOGGER.error("Problem creating copy of group", ex);
Expand Down