Fixed duplicate fields assigned to entry types #8391

dimitrisdimos00 · 2022-01-05T15:42:05Z

Changed the method addNewField of class CustomEntryTypeDialogViewModel in order to check if the field already exists. Added DialogService field to class CustomEntryTypeDialogViewModel in order to show warning notification to the user. Moreover, passed the existing dialogService field of the class CustomEntryDialogView as a parameter when calling the constructor of the CustomEntryTypeDialogViewModel inside it.
Fixes #8194

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Changed the method addNewField of class CustomEntryTypeDialogViewModel in order to check if the field already exists. Added DialogService field to class CustomEntryTypeDialogViewModel in order to show warning notification to the user. fixes JabRef#8194

Siedlerchr

Some small changes Can you also check what happens when you add a duplicate entry type (e.g. add a custom entry type article)? It could theoretically be possible, but would not make any sense...

Siedlerchr · 2022-01-05T18:19:10Z

src/main/java/org/jabref/gui/customentrytypes/CustomEntryTypeDialogViewModel.java

-import org.jabref.model.entry.field.FieldFactory;
-import org.jabref.model.entry.field.FieldPriority;
-import org.jabref.model.entry.field.OrFields;
+import org.jabref.model.entry.field.*;


There seems to be something wrong with your codestyle here, no star imports allowed
See https://github.com/JabRef/jabref/tree/main/config

Siedlerchr · 2022-01-05T18:24:47Z

src/main/java/org/jabref/gui/customentrytypes/CustomEntryTypeDialogViewModel.java

+        ObservableList<FieldViewModel> entryFields = this.selectedEntryType.getValue().fields();
+
+        // compare every entry field name with the user field name in order to find out if any of them has the same one. If so, show warning.
+        for (FieldViewModel fieldViewModel : entryFields) {


This can be simplified
entryFields.stream().anyMatch(fieldViewModel -> fieldViewModel.fieldName().getValue().equals(...)
See https://www.baeldung.com/java-8-streams-introduction#1-iterating

Siedlerchr · 2022-01-05T18:31:19Z

Thanks for your contribution (Congrats on your first 🥇 ) and the explanation of your changes! Very good start! Just some minor things.

… CustomizeEntryTypeDialogView classes fixes JabRef#8194

…vice warning inside if-else condition. fixes JabRef#8194

dimitrisdimos00 · 2022-01-05T20:41:07Z

I am glad to help! Yeah, it is indeed my first contribution to an open source project. The truth is that I found it kind of hard to navigate among the countless project files. However I managed to make it work. Moreover, I tried adding a duplicate entry type and it works. I also implemented the requested changes. Hope it's ok this time!

Siedlerchr · 2022-01-05T22:28:27Z

I am glad to help! Yeah, it is indeed my first contribution to an open source project. The truth is that I found it kind of hard to navigate among the countless project files. However I managed to make it work.

Cool :) And a solid first time contribution!
We have also a kind of high-level overview (e.g. the relationship between the different packages) and some other useful resources in our devdocs. https://devdocs.jabref.org/getting-into-the-code/high-level-documentation
May I ask how it came that you chose JabRef?

A little issue is still left:
Please have a look at the checkstyle/reviewdog (Files changed github) issues. Although these warnings are sometimes nasty, they help us to ensure a uniform code style and to prevent merge conflicts due to formatting changes.

fixes JabRef#8051

dimitrisdimos00 · 2022-01-06T11:07:17Z

Thanks! Ok I will check this out. Well you have a very good guide on seting up everything for newcomers. The most difficult thing for me with projects like this on github is actually setting them up in order to start. In other projects, the instructions are kind of vague and if you encounter an issue in the process, it is very difficult to recover and get going again. In your project, the instructions for setting up Intellij for example were very clear and they had every possible error that might show up. Finally, I like java as a language and wanted to find a project that uses it, so I ended up choosing Jabref.

Siedlerchr

codewise lgtm now. I will test your changes and wait for a second maintainer to review your code

edit// tested locally looks good

calixtus

Two style issues.
Thanks for your contribution!

calixtus · 2022-01-09T13:28:22Z

src/main/java/org/jabref/gui/customentrytypes/CustomEntryTypeDialogViewModel.java

+        // create list with all the entry's fields
+        ObservableList<FieldViewModel> entryFields = this.selectedEntryType.getValue().fields();
+
+        // compare every entry field name with the user field name in order to find out if any of them has the same one.
+        boolean fieldExists = entryFields.stream().anyMatch(fieldViewModel -> fieldViewModel.fieldName().getValue().equals(field.getDisplayName()));
+
+        // if the user field name isn't found inside the list, pass it to the entry as a new one. If that is not the case, show warning.
+        if (!fieldExists) {
+            this.selectedEntryType.getValue().addField(model);
+        } else {
+            dialogService.showWarningDialogAndWait(Localization.lang("Duplicate fields"), Localization.lang("Warning: You added field \"%0\" twice. Only one will be kept.", field.getDisplayName()));
+        }


No code-repeating comments. If the name of the variables are chosen well, then the code is self-explaining and you don't need them.

Hello. I deleted the first and last comment and removed as many as lines possible. Shall I remove the second comment too? I think it is more helpful than the other ones.

No, no comment at all, if it's not really necessary and totally misunderstandable otherwise. The best comment is always the code itself, with meaningful variable names and clean code in 98% you don't need any comment at all.
Whoever works with java nowadays knows how to read streams.

src/main/java/org/jabref/gui/customentrytypes/CustomizeEntryTypeDialogView.java

fixes JabRef#8194

calixtus · 2022-01-12T22:09:37Z

Thanks for your contribution!

dimitrisdimos00 · 2022-01-13T14:08:34Z

You're welcome!

Siedlerchr requested changes Jan 5, 2022

View reviewed changes

dimitrisdimos00 added 2 commits January 5, 2022 22:23

Reset the original import style in CustomEntryTypeDialogViewModel and…

58648ba

… CustomizeEntryTypeDialogView classes fixes JabRef#8194

Simplified for-each iteration using stream operation. Added dialogSer…

591e079

…vice warning inside if-else condition. fixes JabRef#8194

dimitrisdimos00 added 3 commits January 6, 2022 12:49

Added braces {} to if-else statement

7db0c0f

fixes JabRef#8051

Fixed minor braces issue

d860b95

fixes JabRef#8051

Fixed bare URL checkstyle issue

5c18a50

fixes JabRef#8051

Siedlerchr approved these changes Jan 6, 2022

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 6, 2022

calixtus requested changes Jan 9, 2022

View reviewed changes

dimitrisdimos00 and others added 2 commits January 12, 2022 22:24

Deleted redundant comments and line breaks

f4d2aac

fixes JabRef#8194

Merge branch 'main' into fix-for-issue-8194

fafbba1

calixtus approved these changes Jan 12, 2022

View reviewed changes

calixtus merged commit 9b8f17b into JabRef:main Jan 12, 2022

dimitrisdimos00 deleted the fix-for-issue-8194 branch January 13, 2022 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed duplicate fields assigned to entry types #8391

Fixed duplicate fields assigned to entry types #8391

dimitrisdimos00 commented Jan 5, 2022 •

edited

Loading

Siedlerchr left a comment

Siedlerchr Jan 5, 2022

Siedlerchr Jan 5, 2022

Siedlerchr commented Jan 5, 2022 •

edited

Loading

dimitrisdimos00 commented Jan 5, 2022

Siedlerchr commented Jan 5, 2022

dimitrisdimos00 commented Jan 6, 2022

Siedlerchr left a comment •

edited

Loading

calixtus left a comment

calixtus Jan 9, 2022

dimitrisdimos00 Jan 11, 2022 •

edited

Loading

calixtus Jan 12, 2022

calixtus commented Jan 12, 2022

dimitrisdimos00 commented Jan 13, 2022 •

edited

Loading

Fixed duplicate fields assigned to entry types #8391

Fixed duplicate fields assigned to entry types #8391

Conversation

dimitrisdimos00 commented Jan 5, 2022 • edited Loading

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr Jan 5, 2022

Choose a reason for hiding this comment

Siedlerchr Jan 5, 2022

Choose a reason for hiding this comment

Siedlerchr commented Jan 5, 2022 • edited Loading

dimitrisdimos00 commented Jan 5, 2022

Siedlerchr commented Jan 5, 2022

dimitrisdimos00 commented Jan 6, 2022

Siedlerchr left a comment • edited Loading

Choose a reason for hiding this comment

calixtus left a comment

Choose a reason for hiding this comment

calixtus Jan 9, 2022

Choose a reason for hiding this comment

dimitrisdimos00 Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

calixtus Jan 12, 2022

Choose a reason for hiding this comment

calixtus commented Jan 12, 2022

dimitrisdimos00 commented Jan 13, 2022 • edited Loading

dimitrisdimos00 commented Jan 5, 2022 •

edited

Loading

Siedlerchr commented Jan 5, 2022 •

edited

Loading

Siedlerchr left a comment •

edited

Loading

dimitrisdimos00 Jan 11, 2022 •

edited

Loading

dimitrisdimos00 commented Jan 13, 2022 •

edited

Loading