Reimplement custom entry types dialog

[Siedlerchr]

Fixes #4719

Very early draft.
Field Type should be radio button to switch between optional/required

TODO:

• Add check for empty fields and types (prevent them with validator)

• Change in CHANGELOG.md described (if applicable)
• Tests created for changes (if applicable)
• Manually tested changed features in running JabRef (always required)
• Screenshots added in PR description (for bigger UI changes)
• Checked documentation: Is the information available and up to date? If not: Issue created at https://github.com/JabRef/user-documentation/issues.

[tobiasdiez]

Cool that you worked on this!

As a suggestion, I would not follow the original design very closely. Instead of having different lists for required and optional fields, I would add one big list of all fields with a checkbox toggling between required/optional.

[Siedlerchr]

Thanks for the idea, that indeed sounds more practical. Maybe a radio button column.

[Siedlerchr]

Question which just came to my mind: Do we allow changing the default entry types and their fields? e.g. Removing a field from an entry type like article?
@koppor

[tobiasdiez]

Yes, it was possible to add/remove fields from the built-in entry types.

[Siedlerchr]

@tobiasdiez What is the preferred way to add new Fields to a an existing entry type?
I saw the EntryTypesManager with the addCustomOrModifiedType but I am not sure if this is the right approach

[tobiasdiez]

Yes that methods sounds right. I vaguely remember that the complete entry type had to be "replaced" instead of only specifying the added/removed fields. Easiest way is probably to look at the original code.

[tobiasdiez]

This looks really nice and is leagues above the original dialog in terms of user friendliness. Good job!

[Siedlerchr]

Still a few things to do, especially saving and removing all the fields back to the db and the prefs, But at least adding to the gui already works (without any further effects)

[Siedlerchr]

@tobiasdiez Do you have an idea why the TableView content has these "empty" fields when I change between a list with more and fewer fields?

TableView is bound bidirecitonal to the fieldsForTypeProperty
this.fieldsForTypeProperty = new SimpleListProperty<>(existingFieldsForType);

        EasyBind.subscribe(selectedEntryTypesProperty, type -> {
            if (type != null) {
                List<FieldViewModel> typesForField = typesWithField.get(type);
                existingFieldsForType.setAll(typesForField);
            }
        });

[tobiasdiez]

The table view is programmed in such a way that always the complete visible table is filled with rows. Thus, even if there is no data
https://github.com/JabRef/jabref/pull/5799/files#diff-6f6e1e1c919b9cd2136e6a4b1d7d684aR78
creates the toggle buttons. Moreover, rows are reused (i.e. not re-created).

I think it should suffice to add the check item != null to https://github.com/JabRef/jabref/pull/5799/files#diff-acc3954e085edb6f809d80a9c419f431R26 and perhaps add an else statement with setGraphic(null).

[Siedlerchr]

Thanks! That worked!

[systemoperator]

@Siedlerchr One little remark: Hovering the dustbin icon of e.g. the field "DOI" states "Remove field from entry type DOI", which is not appropriate.

[Siedlerchr]

@systemoperator One question, does the table update properly ( I am referring to the other bug) if you add a new type, add a new field/remove a new field?

[Siedlerchr]

@burrbull All errors fixed.

[systemoperator]

@Siedlerchr Concerning updating the table, everything works perfectly. But when I add a new entry type, then I firstly get an exception:

java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at javafx.fxml/javafx.fxml.FXMLLoader$MethodHandler.invoke(FXMLLoader.java:1787)
	at javafx.fxml/javafx.fxml.FXMLLoader$ControllerMethodEventHandler.handle(FXMLLoader.java:1670)
	at javafx.base/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:86)
[...]

and afterwards I get another exception:

java.lang.NullPointerException
	at java.base/java.util.AbstractCollection.addAll(AbstractCollection.java:335)
	at javafx.base/javafx.collections.ModifiableObservableListBase.addAll(ModifiableObservableListBase.java:99)
	at javafx.base/javafx.collections.ModifiableObservableListBase.setAll(ModifiableObservableListBase.java:88)
[...]

and then the entry shows up in the table.

Subsequenty, when I delete a newly inserted entry type, then sometimes a null pointer exception occurs:

java.lang.NullPointerException
	at java.base/java.util.AbstractCollection.addAll(AbstractCollection.java:335)
	at javafx.base/javafx.collections.ModifiableObservableListBase.addAll(ModifiableObservableListBase.java:99)
	at javafx.base/javafx.collections.ModifiableObservableListBase.setAll(ModifiableObservableListBase.java:88)
	at org.jabref/org.jabref.gui.customentrytypes.CustomEntryTypeDialogViewModel.lambda$new$2(CustomEntryTypeDialogViewModel.java:91)
	at easybind@1.0.3/org.fxmisc.easybind.EasyBind.lambda$subscribe$12(EasyBind.java:263)
[...]

And when I select another entry type and simply click on the newly added entry type there is another null pointer exception.

[systemoperator]

Oh, I have not seen, that it was already mentioned previously.

[Siedlerchr]

Ah thanks. I thought I had fixed this already. Probably forgot to commit this on my other pc. Just fixed it. Will take a bit until the binaries are updated.

[systemoperator]

Testing the fixed one: Some corner cases:

• Adding several entry types without any name (or at least two types with the same name): Exceptions occur, when deleting them.
• Adding several (or sometimes just one) new fields without any name: Exception

[Siedlerchr]

That's one thing I want to prevent. Empty fields and empty entry types won't be allowed. Will add a validator that in the next days.

Just wanted to be curious about the table updating refreshing. The main difference between the other table for external file types and this one is that I created a viewModel for the table and I think in the other dialog the ExternalFileType is directly used.

[systemoperator]

Sounds good. Empty types should not be allowed as well as types with the same name.

[systemoperator]

That's one thing I want to prevent. Empty fields and empty entry types won't be allowed. Will add a validator that in the next days.

Just wanted to be curious about the table updating refreshing. The main difference between the other table for external file types and this one is that I created a viewModel for the table and I think in the other dialog the ExternalFileType is directly used.

Concerning the external file types: Maybe the problem is, that there is a type hierarchy involved and the table uses the interface ExternalFileType. Basically, all cases could be covered with just one ExternalFileType as well. (#5902)

[koppor]

There will be a follow-up PR to fix one small issue

eigentlich fehlt nur noch ein check, dass man keinen doppelten typ/field angeben muss.
Aber der wird eh nicht gespeichert bzw ist dann beim reload nicht dabei.
Ansonsten ist es soweit fertig