-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Changes from 3 commits
9f5da55
44a8efd
90800c3
b601ba6
d4f00ad
91420f2
3300aa5
3c89dee
6dad44f
cf7d6b8
609a065
ff7dc5d
df86b62
d055f98
8f39723
48a4c31
6c65676
45152d5
b254487
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
<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> |
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
@Override | ||
public void execute() { | ||
BibDatabase database = basePanel.getDatabase(); | ||
ReplaceStringView rsc = new ReplaceStringView(database, basePanel); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't use abbreviations in names etc. |
||
rsc.showAndWait().filter(response -> rsc.isExit()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is no need for the |
||
} | ||
} |
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; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we usual follow camel casing, i.e it should be |
||||||||||||
@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()); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: jabref/src/main/java/org/jabref/gui/keyboard/KeyBindingsDialog.fxml Lines 24 to 26 in d5d67d7
|
||||||||||||
} | ||||||||||||
|
||||||||||||
@FXML | ||||||||||||
public void initialize() { | ||||||||||||
visualizer.setDecoration(new IconValidationDecorator()); | ||||||||||||
LimitFieldInput.setEditable(true); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||
public void buttonReplace() { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||||||||
} | ||||||||||||
|
||||||||||||
/** | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Named Compound ce -> NamedCompound compound There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And String string : be.getFieldNames... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. even better: |
||||||||||||
counter += replaceField(be, s, ce); | ||||||||||||
} | ||||||||||||
} else { | ||||||||||||
for (String fld : fieldStrings) { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||||||||
} | ||||||||||||
|
||||||||||||
} |
There was a problem hiding this comment.
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.