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

Fix Part of Issue#3861: Edit->Replace String #4227

Merged
merged 19 commits into from
Aug 11, 2018
Merged
Show file tree
Hide file tree
Changes from 3 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
27 changes: 1 addition & 26 deletions src/main/java/org/jabref/gui/BasePanel.java
Original file line number Diff line number Diff line change
Expand Up @@ -395,32 +395,7 @@ private void setupActions() {

actions.put(Actions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(this, frame.getDialogService()));

actions.put(Actions.REPLACE_ALL, () -> {
final ReplaceStringDialog rsd = new ReplaceStringDialog(frame);
rsd.setVisible(true);
if (!rsd.okPressed()) {
return;
}
int counter = 0;
final NamedCompound ce = new NamedCompound(Localization.lang("Replace string"));
if (rsd.selOnly()) {
for (BibEntry be : mainTable.getSelectedEntries()) {
counter += rsd.replace(be, ce);
}
} else {
for (BibEntry entry : bibDatabaseContext.getDatabase().getEntries()) {
counter += rsd.replace(entry, ce);
}
}

output(Localization.lang("Replaced") + ' ' + counter + ' '
+ (counter == 1 ? Localization.lang("occurrence") : Localization.lang("occurrences")) + '.');
if (counter > 0) {
ce.end();
getUndoManager().addEdit(ce);
markBaseChanged();
}
});
actions.put(Actions.REPLACE_ALL, ()-> (new ReplaceStringAction(this)).execute());

actions.put(new SpecialFieldValueViewModel(SpecialField.RELEVANCE.getValues().get(0)).getCommand(),
new SpecialFieldViewModel(SpecialField.RELEVANCE, undoManager).getSpecialFieldAction(SpecialField.RELEVANCE.getValues().get(0), frame));
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/jabref/gui/JabRefFrame.java
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,7 @@ private Node createToolbar() {
factory.createIconButton(StandardActions.CUT, new OldDatabaseCommandWrapper(Actions.CUT, this, Globals.stateManager)),
factory.createIconButton(StandardActions.COPY, new OldDatabaseCommandWrapper(Actions.COPY, this, Globals.stateManager)),
factory.createIconButton(StandardActions.PASTE, new OldDatabaseCommandWrapper(Actions.PASTE, this, Globals.stateManager)),
factory.createIconButton(StandardActions.REPLACE_ALL, new ReplaceStringAction(this.getCurrentBasePanel())),
Copy link
Member

Choose a reason for hiding this comment

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

This adds the action to the toolbar. I don't think that "replace all" is important enough to appear there. Please remove it again.


factory.createIconButton(StandardActions.CLEANUP_ENTRIES, new OldDatabaseCommandWrapper(Actions.CLEANUP, this, Globals.stateManager)),
factory.createIconButton(pushToExternal.getMenuAction(), pushToExternal),
Expand Down
26 changes: 26 additions & 0 deletions src/main/java/org/jabref/gui/ReplaceString.fxml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?xml version="1.0" encoding="UTF-8"?>

<?import javafx.scene.control.*?>
<?import javafx.scene.layout.*?>

<DialogPane maxHeight="-Infinity" maxWidth="-Infinity" minHeight="-Infinity" minWidth="-Infinity" prefHeight="339.0" prefWidth="476.0" xmlns:fx="http://javafx.com/fxml/1" xmlns="http://javafx.com/javafx/8" fx:controller="org.jabref.gui.ReplaceStringView" fx:id="pane">
<content>
<AnchorPane minHeight="0.0" minWidth="0.0" prefHeight="376.0" prefWidth="552.0">
Copy link
Member

Choose a reason for hiding this comment

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

You better should use a GridPane here for positioning the label and the text fields, that allows them to be properly aligned. Can be easily done using the Scence Builder.
You can also take a look at the FXML here from my other PR #4040 for the db login dialog:
https://github.com/JabRef/jabref/pull/4040/files#diff-200f60f39aab2550ae174ea88d604981

<children>
<Button fx:id="CancelButton" cancelButton="true" layoutX="386.0" layoutY="273.0" mnemonicParsing="false" onAction="#buttonCancel" prefHeight="28.0" prefWidth="74.0" text="Cancel" userData="CANCEL"/>
<Button fx:id="ReplaceButton" defaultButton="true" layoutX="260.0" layoutY="275.0" mnemonicParsing="false" onAction="#buttonReplace" prefHeight="25.0" prefWidth="76.0" text="Replace" textFill="#e46161" userData="OK_DONE"/>
<TextField fx:id="FindField" layoutX="107.0" layoutY="21.0" prefHeight="23.0" prefWidth="347.0" />
<TextField fx:id="ReplaceField" layoutX="107.0" layoutY="63.0" prefHeight="23.0" prefWidth="348.0" />
<Label layoutX="14.0" layoutY="21.0" prefHeight="23.0" prefWidth="75.0" text="Find:" />
<Label layoutX="11.0" layoutY="64.0" prefHeight="21.0" prefWidth="95.0" text="Replace with:" />
<CheckBox fx:id="checkLimit" layoutX="20.0" layoutY="134.0" mnemonicParsing="false" onAction="#selectOnly" onMouseReleased="#selectOnly" prefHeight="22.0" prefWidth="211.0" text="Limit to Selected Entries" />
<RadioButton layoutX="20.0" layoutY="188.0" mnemonicParsing="false" onAction="#radioAll" onMouseReleased="#radioAll" prefHeight="16.0" prefWidth="125.0" selected="true" text="All Field Replace">
<toggleGroup>
<ToggleGroup fx:id="RadioGroup" />
</toggleGroup>
</RadioButton>
<RadioButton layoutX="20.0" layoutY="217.0" mnemonicParsing="false" onAction="#radioLimit" onMouseReleased="#radioLimit" prefHeight="29.0" prefWidth="121.0" text="Limit to Fields:" toggleGroup="$RadioGroup" />
<TextField fx:id="LimitFieldInput" layoutX="157.0" layoutY="220.0" prefHeight="23.0" prefWidth="283.0" />
</children></AnchorPane>
</content>
</DialogPane>
20 changes: 20 additions & 0 deletions src/main/java/org/jabref/gui/ReplaceStringAction.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package org.jabref.gui;

import org.jabref.gui.actions.SimpleCommand;
import org.jabref.model.database.BibDatabase;

public class ReplaceStringAction extends SimpleCommand
{
private BasePanel basePanel;

public ReplaceStringAction(BasePanel bPanel) {
this.basePanel = bPanel;
Copy link
Member

Choose a reason for hiding this comment

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

Typically you name the parameter also basePanel as the field and use this to distinguish the field from the parameter.
e.g. this.baesPanel = basePanel

}

@Override
public void execute() {
BibDatabase database = basePanel.getDatabase();
ReplaceStringView rsc = new ReplaceStringView(database, basePanel);
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use abbreviations in names etc. dialog is better.

rsc.showAndWait().filter(response -> rsc.isExit());
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 there is no need for the filter command.

}
}
184 changes: 184 additions & 0 deletions src/main/java/org/jabref/gui/ReplaceStringView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
package org.jabref.gui;

import javafx.fxml.FXML;
import javafx.scene.control.Button;
import javafx.scene.control.CheckBox;
import javafx.scene.control.DialogPane;
import javafx.scene.control.TextField;
import javafx.scene.control.ToggleGroup;
import javafx.stage.Stage;

import org.jabref.gui.undo.NamedCompound;
import org.jabref.gui.undo.UndoableFieldChange;
import org.jabref.gui.util.BaseDialog;
import org.jabref.gui.util.IconValidationDecorator;
import org.jabref.logic.l10n.Localization;
import org.jabref.model.database.BibDatabase;
import org.jabref.model.entry.BibEntry;

import com.airhacks.afterburner.views.ViewLoader;
import de.saxsys.mvvmfx.utils.validation.visualization.ControlsFxVisualizer;

public class ReplaceStringView extends BaseDialog<Void>
{

@FXML public ToggleGroup RadioGroup;
Copy link
Member

Choose a reason for hiding this comment

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

we usual follow camel casing, i.e it should be radioGroup. Please also try to give these fields a more descriptive name (like you did below). Also please make these fields private.

@FXML public Button CancelButton;
@FXML public Button ReplaceButton;
@FXML private TextField LimitFieldInput;
@FXML private TextField FindField;
@FXML private TextField ReplaceField;
@FXML private CheckBox checkLimit;
@FXML private DialogPane pane;

private boolean AllFieldReplace;
private boolean selOnly;
private String findString;
private String replaceString;
private String[] fieldStrings;
private BibDatabase database;
private boolean exitSignal;
private BasePanel panel;
private Stage st;

private final ControlsFxVisualizer visualizer = new ControlsFxVisualizer();

public ReplaceStringView(BibDatabase bibDatabase, BasePanel basePanel) {
this.setTitle(Localization.lang("Replace String"));

database = bibDatabase;
AllFieldReplace = true;
exitSignal = false;
selOnly = false;
panel = basePanel;

ViewLoader.view(this)
.load()
.setAsDialogPane(this);

st = (Stage) this.pane.getScene().getWindow();
st.setOnCloseRequest(event -> st.close());
Copy link
Member

Choose a reason for hiding this comment

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

Normally there is no need for handling the close request in a special way. The problem is that you didn't defined a Cancel/Accept button in the dialog. Have a look at the keybindings dialog on how to handle this correctly:

<ButtonType fx:id="resetButton" text="%Reset Bindings" buttonData="LEFT"/>
<ButtonType fx:id="saveButton" text="%Save" buttonData="OK_DONE"/>
<ButtonType fx:constant="CANCEL"/>

ControlHelper.setAction(resetButton, getDialogPane(), event -> setDefaultBindings());
ControlHelper.setAction(saveButton, getDialogPane(), event -> saveKeyBindingsAndCloseDialog());

}

@FXML
public void initialize() {
visualizer.setDecoration(new IconValidationDecorator());
LimitFieldInput.setEditable(true);
Copy link
Member

Choose a reason for hiding this comment

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

They should already be editable by default.

FindField.setEditable(true);
ReplaceField.setEditable(true);
checkLimit.setSelected(false);
}

/**
* FXML Message handler
Copy link
Member

Choose a reason for hiding this comment

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

no need to state the obvious in a comment (the method already has a @FXML decoration).

*/
@FXML
public void buttonReplace() {
Copy link
Member

Choose a reason for hiding this comment

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

In the Model-View-ViewModel spirit, the view should never contain any logic. The complete logic should be contained in the ViewModel. The rationale behind this is, that you can test logic in the view model using ordinary (junit) tests without the need to instantiate the ui. Thus, please introduce a new ReplaceStringViewModel and bind its values against the textfields. See, for example,

@FXML
private void saveKeyBindingsAndCloseDialog() {
viewModel.saveKeyBindings();
closeDialog();
}

Copy link
Member

Choose a reason for hiding this comment

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

This applies also to all other methods below that contain logic.

findString = FindField.getText();
replaceString = ReplaceField.getText();
fieldStrings = LimitFieldInput.getText().toLowerCase().split(";");
if ("".equals(findString))
{
exitSignal = true;
st.close();
return;
}
replace();
exitSignal = true;
st.close();
}

@FXML
public void buttonCancel() {
exitSignal = true;
st.close();
}

@FXML
public void radioAll() {
AllFieldReplace = true;
}

@FXML
public void radioLimit() {
AllFieldReplace = false;
}

@FXML
public void selectOnly() {
selOnly = !selOnly;
}

public boolean isExit() {
return this.exitSignal;
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

The comments were helpful for the review. Thanks! But they should be removed in the final version.

* General replacement, same as Action:Replace_All in BasePanel.java
* Rep check: BasePanel == null
* @return : replace count
*/
public int replace() {
final NamedCompound ce = new NamedCompound(Localization.lang("Replace string"));
int counter = 0;
if (this.panel == null)
return 0;
if (this.selOnly) {
for (BibEntry bibEntry: this.panel.getSelectedEntries())
{
counter += replaceItem(bibEntry, ce);
}
}
else {
for (BibEntry bibEntry: this.database.getEntries())
{
counter += replaceItem(bibEntry, ce);
}
}
return counter;
}

/**
* Does the actual operation on a Bibtex entry based on the
* settings specified in this same dialog. Returns the number of
* occurences replaced.
* Copied and Adapted from org.jabref.gui.ReplaceStringDialog.java
*/
public int replaceItem(BibEntry be, NamedCompound ce) {
int counter = 0;
if (this.AllFieldReplace) {
for (String s : be.getFieldNames()) {
Copy link
Member

Choose a reason for hiding this comment

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

Named Compound ce -> NamedCompound compound

Copy link
Member

Choose a reason for hiding this comment

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

And String string : be.getFieldNames...

Copy link
Member

Choose a reason for hiding this comment

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

even better: String fieldName : ...

counter += replaceField(be, s, ce);
}
} else {
for (String fld : fieldStrings) {
Copy link
Member

Choose a reason for hiding this comment

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

String field : fieldStrings

counter += replaceField(be, fld, ce);
}
}
return counter;
}

private int replaceField(BibEntry be, String fieldname, NamedCompound ce) {
if (!be.hasField(fieldname)) {
return 0;
}
String txt = be.getField(fieldname).get();
StringBuilder sb = new StringBuilder();
int ind;
int piv = 0;
int counter = 0;
int len1 = this.findString.length();
Copy link
Member

Choose a reason for hiding this comment

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

maybe better findStringLength to directly make clear what the variable represent

while ((ind = txt.indexOf(this.findString, piv)) >= 0) {
counter++;
sb.append(txt, piv, ind); // Text leading up to s1
sb.append(this.replaceString); // Insert s2
piv = ind + len1;
}
sb.append(txt.substring(piv));
String newStr = sb.toString();
be.setField(fieldname, newStr);
ce.addEdit(new UndoableFieldChange(be, fieldname, txt, newStr));
return counter;
}

}