Fix for issue #2721 append to a field #3395

ethanwharris · 2017-11-03T09:50:18Z

#2721
Add the append option to the dialog.

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

…ity -> set/clear/rename fields

…rename fields

lenhard

Thanks for your PR!

I had a first look at the code and overall everything looks fine. Good work! I just have a few minor naming suggestions, but nothing big. I would still test your changes locally, but am lacking time for that right now. More feedback later.

lenhard · 2017-11-03T16:00:21Z

src/main/java/org/jabref/gui/actions/MassSetFieldAction.java

@@ -235,6 +251,10 @@ public void actionPerformed(ActionEvent e) {
                ce.addEdit(MassSetFieldAction.massRenameField(entryList, fields[0], renameTo.getText(),
                        overwrite.isSelected()));
            }
+        } else if (append.isSelected()) {
+            for (String field1 : fields) {


Nitpick: field1 is not really a good variable name here. The loop iterates over all fields and not just the first. Please rename this to field.

lenhard · 2017-11-03T16:01:54Z

src/main/java/org/jabref/gui/actions/MassSetFieldAction.java

+            text = "";
+        }
+
+        NamedCompound ce = new NamedCompound(Localization.lang("Append field"));


Since this is new code, please do proper naming (and don't get inspired by all the bad naming in JabRef). So please call this compoundEdit (or something similar) instead of ce.

lenhard · 2017-11-03T16:01:56Z

src/main/java/org/jabref/gui/actions/MassSetFieldAction.java

+
+        NamedCompound ce = new NamedCompound(Localization.lang("Append field"));
+        for (BibEntry entry : entries) {
+            Optional<String> oldVal = entry.getField(field);


Same here: Why not call it oldValue.

…tToAppend / Set etc.) for review and rename some variables from old code to match.

ethanwharris · 2017-11-05T20:48:32Z

@lenhard Thanks for your review! Totally agree, have made changes as requested and updated some of the old stuff to match.

LinusDietz

Thank you for this contribution! The functionality looks fine, I just have one remark regarding the naming of variables. Would you mind changing that? Besides that the PR can be merged to master!

LinusDietz · 2017-11-06T10:09:20Z

src/main/java/org/jabref/gui/actions/MassSetFieldAction.java

    private JRadioButton rename;
    private JComboBox<String> field;
-    private JTextField text;
+    private JTextField setText;


I know this has been there before your PR, but I would like to encourage you to change the naming of these JTextFields.

Background: In Java you usually use verbs or verbs+nouns for method names. When having some buttons in a GUI I can still understand that it is acceptable to name the variables after the button's functionality. However, when it comes to these Textfields, I would say a verb+noun combination is really misleading.

How about naming them something like textFieldSet, textFieldAppend, textFieldRename?

@LinusDietz Thank you for your review. Those names are much better! Have made the changes as requested.

LinusDietz · 2017-11-06T11:33:23Z

@ethanwharris nice! We'll merge this as soon as another maintainer approves.

lenhard

Ok, I have tested your PR now manually and everything works as it should, great job!

I have one last issue, though: Because you change the localization of the menu item, all translations except for the English one are lost. Could you please keep all translations for the menu item as they are? They won't be accurate (missing the append option), but at least they'll be partly translated. That is better then suddenly having an English menu item in a UI with an otherwise different language. And I hate to lose the effort of our translators.

…m set/clear/rename_fields

ethanwharris · 2017-11-07T14:56:40Z

@lenhard Thank you for your feedback. Have now updated the menu localization to retain original translation, have also reflected this in the JabRef localization.

lenhard · 2017-11-08T12:32:45Z

@ethanwharris Ok, perfect! I've looked at the code again and this seems to be ready for merge now (which is what I will do right away).

Thank you very much for your contribution! Feel very much invited to contribute more :-)

* upstream/master: (26 commits) Fix test for quoted lang messages (#3424) Update gradle from 4.3 to 4.3.1 Fix #3411: ordering of fields in customized entry types works again (#3422) Backport of syncLang to python2 (#3420) Remove Versioneye badge Fix some error prone warnings Fix for issue #2721 append to a field (#3395) Fix travis - hopefully Remove 3.x changelog (#3250) Try to use hint of https://github.com/TheBoegl/shadow-log4j-transformer#usage-as-library Try to enable LGTM Update guava from 23.2 -> 23.3 (#3409) Update wiremock from 2.8.0 -> 2.10.1 Move groups field from others to general (#3407) Fix checkstyle issues to repair build Fix #3046: No longer allow duplicate fields in customized entry types (#3405) Strip invalid prolog when loading CitationStyles (#3404) Integrity check "Abbreviation Detection" detects abbreviated names for journals and booktitles based on the internal list instead of only looking for "." signs. (#3362) Update gradle from 4.2.1 to 4.3 Update gradle from 4.2.1 to 4.3 ...

* upstream/master: (23 commits) Feature java version check again (#3428) Fix test for quoted lang messages (#3424) Update gradle from 4.3 to 4.3.1 Fix #3411: ordering of fields in customized entry types works again (#3422) Backport of syncLang to python2 (#3420) Remove Versioneye badge Fix some error prone warnings Fix for issue #2721 append to a field (#3395) Fix travis - hopefully Remove 3.x changelog (#3250) Try to use hint of https://github.com/TheBoegl/shadow-log4j-transformer#usage-as-library Try to enable LGTM Update guava from 23.2 -> 23.3 (#3409) Update wiremock from 2.8.0 -> 2.10.1 Move groups field from others to general (#3407) Fix checkstyle issues to repair build Fix #3046: No longer allow duplicate fields in customized entry types (#3405) Strip invalid prolog when loading CitationStyles (#3404) Integrity check "Abbreviation Detection" detects abbreviated names for journals and booktitles based on the internal list instead of only looking for "." signs. (#3362) Update gradle from 4.2.1 to 4.3 ...

* upstream/master: (30 commits) Add Hint for checking master version (#3439) Replace LinkedFiles backslashes with forward slashes (#3403) fix isbn result from chimbori (#3442) Feature java version check again (#3428) Fix test for quoted lang messages (#3424) Update gradle from 4.3 to 4.3.1 Fix #3411: ordering of fields in customized entry types works again (#3422) Backport of syncLang to python2 (#3420) Remove Versioneye badge Fix some error prone warnings Fix for issue #2721 append to a field (#3395) Fix travis - hopefully Remove 3.x changelog (#3250) Try to use hint of https://github.com/TheBoegl/shadow-log4j-transformer#usage-as-library Try to enable LGTM Update guava from 23.2 -> 23.3 (#3409) Update wiremock from 2.8.0 -> 2.10.1 Move groups field from others to general (#3407) Fix checkstyle issues to repair build Fix #3046: No longer allow duplicate fields in customized entry types (#3405) ...

ethanwharris and others added 4 commits November 1, 2017 23:08

Add append button (no functionality) to the user interface under Qual…

793b9bf

…ity -> set/clear/rename fields

Add functionality to append button under Quality -> Set/clear/append/…

4ed103e

…rename fields

Update Localization for set/clear/append/rename fields dialog

54c2cb4

Update CHANGELOG.md

4207491

LinusDietz changed the title ~~Fix for issue 2721 append to a field~~ Fix for issue #2721 append to a field Nov 3, 2017

lenhard reviewed Nov 3, 2017

View reviewed changes

Rename variables (ce -> compoundEdit, oldVal -> oldValue, text -> tex…

2e3c698

…tToAppend / Set etc.) for review and rename some variables from old code to match.

Prevent re-assignment of parameter in massAppend method

bfe2409

LinusDietz approved these changes Nov 6, 2017

View reviewed changes

Rename TextField variables for clarity

166d144

lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 6, 2017

lenhard suggested changes Nov 7, 2017

View reviewed changes

Revert localisation for set/clear/appen/rename_fields to original fro…

6cc53a1

…m set/clear/rename_fields

lenhard merged commit f2744d4 into JabRef:master Nov 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue #2721 append to a field #3395

Fix for issue #2721 append to a field #3395

ethanwharris commented Nov 3, 2017 •

edited

Loading

lenhard left a comment

lenhard Nov 3, 2017

lenhard Nov 3, 2017

lenhard Nov 3, 2017

ethanwharris commented Nov 5, 2017

LinusDietz left a comment

LinusDietz Nov 6, 2017 •

edited

Loading

ethanwharris Nov 6, 2017

LinusDietz commented Nov 6, 2017

lenhard left a comment

ethanwharris commented Nov 7, 2017

lenhard commented Nov 8, 2017

Fix for issue #2721 append to a field #3395

Fix for issue #2721 append to a field #3395

Conversation

ethanwharris commented Nov 3, 2017 • edited Loading

lenhard left a comment

Choose a reason for hiding this comment

lenhard Nov 3, 2017

Choose a reason for hiding this comment

lenhard Nov 3, 2017

Choose a reason for hiding this comment

lenhard Nov 3, 2017

Choose a reason for hiding this comment

ethanwharris commented Nov 5, 2017

LinusDietz left a comment

Choose a reason for hiding this comment

LinusDietz Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

ethanwharris Nov 6, 2017

Choose a reason for hiding this comment

LinusDietz commented Nov 6, 2017

lenhard left a comment

Choose a reason for hiding this comment

ethanwharris commented Nov 7, 2017

lenhard commented Nov 8, 2017

ethanwharris commented Nov 3, 2017 •

edited

Loading

LinusDietz Nov 6, 2017 •

edited

Loading