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

Conversation

@marisuki
Copy link
Contributor

marisuki commented Jul 22, 2018

Hi, we fixed part of the Issue#3861, change the Swing Dialog to the JavaFx Dialog: the Edit -> Replace String Dialog, we add 3 files and modified the general control to link the Replace String controller of JavaFX Dialog, and we did not delete the old dialog logic (ReplaceStringDialog.java). The outlook of Dialog hasn't change a lot, and the previous logic is inherited.

The Screenshot is pasted below, just like the design before.

githubjabrefreplacestring


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
marisuki added 3 commits Jul 22, 2018
Copy link
Contributor

Siedlerchr left a comment

Thanks again for your contribution!
Generally it goes in the right direction, but some minor improvements and it's good from my point of view.

private BasePanel basePanel;

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

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Jul 23, 2018

Contributor

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


<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">

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Jul 23, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Jul 23, 2018

Contributor

They should already be editable by default.

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

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Jul 23, 2018

Contributor

String field : fieldStrings

public int replaceItem(BibEntry be, NamedCompound ce) {
int counter = 0;
if (this.AllFieldReplace) {
for (String s : be.getFieldNames()) {

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Jul 23, 2018

Contributor

Named Compound ce -> NamedCompound compound

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Jul 23, 2018

Contributor

And String string : be.getFieldNames...

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

even better: String fieldName : ...

int ind;
int piv = 0;
int counter = 0;
int len1 = this.findString.length();

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Jul 23, 2018

Contributor

maybe better findStringLength to directly make clear what the variable represent

@marisuki

This comment has been minimized.

Copy link
Contributor Author

marisuki commented Jul 23, 2018

I am sorry for the bad naming fields, and Thank you for your advice, I will fix them later.

@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Jul 23, 2018

@marisuki you don't have to be sorry or take it personally ;) We really much appreciate your contribution
The reviews are meant to improve and maintain a clean code base, although they sometimes may seem a bit nitpicking. Variable naming is one important part, ideally variables should be self-documenting, e.g. they should be meaningful. Here are some tips. https://dzone.com/articles/best-practices-variable-and

Copy link
Member

tobiasdiez left a comment

Thanks for your contribution. In general, the code looks good but I've some (more or less) minor remarks.

Please also remove the old swing dialog (and make sure it is no longer used).

@@ -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())),

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

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

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

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

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

public void execute() {
BibDatabase database = basePanel.getDatabase();
ReplaceStringView rsc = new ReplaceStringView(database, basePanel);
rsc.showAndWait().filter(response -> rsc.isExit());

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

I think there is no need for the filter command.

public class ReplaceStringView extends BaseDialog<Void>
{

@FXML public ToggleGroup RadioGroup;

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

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.

.setAsDialogPane(this);

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

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

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 Message handler

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

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

* FXML Message handler
*/
@FXML
public void buttonReplace() {

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

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();
}

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

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

return this.exitSignal;
}

/**

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

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

public int replaceItem(BibEntry be, NamedCompound ce) {
int counter = 0;
if (this.AllFieldReplace) {
for (String s : be.getFieldNames()) {

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 23, 2018

Member

even better: String fieldName : ...

@marisuki

This comment has been minimized.

Copy link
Contributor Author

marisuki commented Jul 25, 2018

Thank you for all the advice above :), and I changed the dialog based on GridPane also the "ViewModel", and modified the miss using of field names, finally deleted the ReplaceStringDialog.java, which is about the old style logic: Swing. I hope it would be better. :)

Copy link
Member

tobiasdiez left a comment

Thanks for the quick follow-up. It clearly moves in the right direction. It would be nice if you could also change the structure to follow the usual MVVM pattern (see below for details).

stage.close();
return;
}
ReplaceStringViewModel viewModel = new ReplaceStringViewModel(panel, fieldStrings, findString, replaceString, selOnly, allFieldReplace);

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 25, 2018

Member

This works because the dialog is very simple, but goes against the usual strategy of JavaFX / MVVM. You should add properties in the ViewModel class and bind these properties to the corresponding properties of the controls. Please have a look at the other JavaFX dialogs to see how this is done.

Since this approach is a bit different from what you are used to in the Swing world, let me quickly explain the big overall picture and its advantages. You are probably used to program against the UI controls (as you do here right now): for example, if you want to know the input from the user, you use textbox.getText() to get it. Similarly, for other input controls. This works all nicely until the user reports a bug and you have to write a test for it. The problem you then face is the following: how do I automate the user interaction in my test? There are different approaches to this issue but the general answer is: there is no good solution ;-) UI automation is usually ugly and breaks as soon as you change the design. The idea behind the MVVM approach is that you completely separate the UI from the logic that performs the action. So you create a class (the ViewModel) that holds the state of the UI (i.e. you have string field that corresponds to the user input, but at this point you don't care how or where this string is manipulated). This state in the ViewModel is then bound to the actual userinterface in the View class using JavaFX bindings. In this way, you can test the ViewModel, i.e. the complete logic, without ever knowing how the user interface looks like.

}
ReplaceStringViewModel viewModel = new ReplaceStringViewModel(panel, fieldStrings, findString, replaceString, selOnly, allFieldReplace);
viewModel.replace();
stage.close();

This comment has been minimized.

Copy link
@tobiasdiez

tobiasdiez Jul 25, 2018

Member

As BaseDialog derives from Dialog you can simply use close() (without the reference to the stage).

@marisuki

This comment has been minimized.

Copy link
Contributor Author

marisuki commented Aug 7, 2018

Thank you for your comment ! :) @tobiasdiez , this time I changed the initial part of "view" and add the property bindings to make it a little more MVVM pattern alike ;-). I hope it would be better.

return selectOnlyProperty;
}

public StringProperty getFieldStringProperty() {

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Aug 8, 2018

Contributor

Properties in javafx don't have the traditional get/set prefix. So just rename them as the ones abvove,
e.g. fieldStringProperty and findStringProperty etc


final NamedCompound compound = new NamedCompound(Localization.lang("Replace string"));
int counter = 0;
if (this.panel == null)

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Aug 8, 2018

Contributor

Is this check necessary? You always pass the basePanel as constructor argument.
I would suggest you add an Objects.requireNonNull(basePanel) in the constructor to ensure that the argument is never null. If it is null, it will directly throw and exception which makes it easier to see the real reason,

This comment has been minimized.

Copy link
@marisuki

marisuki Aug 8, 2018

Author Contributor

Yes, you are right :), I forgot to check my logic after converting to a viewModel ;-). Thank You!

@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Aug 8, 2018

From my point of view this looks good. Just some minor code improvements. Let's see what @tobiasdiez thinks about it

marisuki and others added 3 commits Aug 8, 2018
Copy link
Member

tobiasdiez left a comment

Looks good to me! Thanks again for the quick follow-ups.

I removed the changelog entries since we will probably only add one big statement announcing that the ui and many dialogs where redesigned. Please resolve the conflicts and then we will merge.

@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Aug 10, 2018

I don't know why, but apparently you still have a conflict in the ReplaceStringDialog

@marisuki

This comment has been minimized.

Copy link
Contributor Author

marisuki commented Aug 10, 2018

Yeah, I tried every means to avoid the conflict ;-), but it does not work (caused by delete a file) . Does this conflict be inevitable ;-)? Or, do you have any idea to solve it? Thank you.

@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Aug 10, 2018

Okay, I tried around and found a solution:
just do git rm src/main/java/org/jabref/gui/ReplaceStringDialog.java

@Siedlerchr Siedlerchr closed this Aug 10, 2018
@Siedlerchr Siedlerchr reopened this Aug 10, 2018
@tobiasdiez

This comment has been minimized.

Copy link
Member

tobiasdiez commented Aug 10, 2018

Something like the following should work:

git pull upstream master (replace "upstream" by whatever you named the JabRef rep)
> git warns about merge conflict
git mergetool
> accept deletion of file
git commit
git push
@marisuki

This comment has been minimized.

Copy link
Contributor Author

marisuki commented Aug 10, 2018

Thank you, I will give it a try! :)

@marisuki

This comment has been minimized.

Copy link
Contributor Author

marisuki commented Aug 10, 2018

I think it works! :) this time I use "git pull git@github.com:jabref/jabref.git master" to merge and then remove and push again, there becomes no conflicts :). Thank you! But there shows a failure on build process, need I do something? (Since I did not change the related files.)

Copy link
Contributor

Siedlerchr left a comment

In general lgtm, you just need to fix the l10n and it should be fine

<RowConstraints minHeight="10.0" prefHeight="30.0" vgrow="SOMETIMES"/>
<RowConstraints minHeight="10.0" prefHeight="30.0" vgrow="SOMETIMES"/>
</rowConstraints>
<Label text="Find and Replace"/>

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Aug 10, 2018

Contributor

You need to add a % sign in front of the label text, so that it will be recognized as translatable string.
That is probably one of the reasons why the l10n test fails

@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Aug 10, 2018

The Travis CI failed, because of the localiation test. If you click on the Traivs Details and then expand the railed build jpb, you will see the reason. In this case you need to add the missing/remove the obsolete ones from the file. But first you have to add a % sign in front of every translatable label text

@marisuki

This comment has been minimized.

Copy link
Contributor Author

marisuki commented Aug 11, 2018

Finally! I updated the properties in resources and make the "text" value linked with the file: Jabref_en.properties, also, I deleted the unused key-values (may generated from the previous replace string dialog) to make no obsolete item in the file. And thank you again for your help! :) @Siedlerchr @tobiasdiez

<RowConstraints minHeight="10.0" prefHeight="30.0" vgrow="SOMETIMES"/>
<RowConstraints minHeight="10.0" prefHeight="30.0" vgrow="SOMETIMES"/>
</rowConstraints>
<Label text="Find and Replace"/>

This comment has been minimized.

Copy link
@Siedlerchr

Siedlerchr Aug 11, 2018

Contributor

I think you missed this one ;) But all others look good

This comment has been minimized.

Copy link
@marisuki

marisuki Aug 11, 2018

Author Contributor

Yeah ;-) I miss it. Thank you for mention! :)

@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Aug 11, 2018

Thank you very much for your contribution! I think you just forgot one little string, if you add this again, then we can finally merge!

Fix
@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Aug 11, 2018

As the faling test on Travis CI is not related to your changes I am going to merge it now.

@Siedlerchr Siedlerchr merged commit 5712665 into JabRef:master Aug 11, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
@marisuki

This comment has been minimized.

Copy link
Contributor Author

marisuki commented Aug 11, 2018

Thank you! :-)

@Siedlerchr Siedlerchr mentioned this pull request Aug 11, 2018
35 of 36 tasks complete
@Siedlerchr

This comment has been minimized.

Copy link
Contributor

Siedlerchr commented Aug 11, 2018

We have to thank for your contribution! 👍 That's a good start. I hope you are interested in contributing some more ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.