Convert Strings Dialog to javafx

[Siedlerchr]

Convert Strings Dialog to javafx

I decided to take a shot at the String dialog. But I have some questions about the String handling in the Database class. I know that I can define some user defined strings as kind of template and then reuse them in my bib entries.

The only thing which does not look that good is the position of the Validation error icon.
Needs perhaps some CSS tweaking, but for the moment not important.

• Store Strings back in DB
• Validate Strings and label

---

• 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?)

[tobiasdiez]

Nice that you want the dialog to JavaFX. However, I don't understand what questions you now have.

[Siedlerchr]

@tobiasdiez I was wondering why the BibtexString is stored in a Map<String, BibTexString in the databse.
The key of the Map is some generated id and is also stored in the BibTexString itself.
So I see no reason why the list of BibTexStrings is not a simple list or set.

jabref/src/main/java/org/jabref/model/database/BibDatabase.java

Lines 278 to 288 in cf8e6aa

	public synchronized void addString(BibtexString string) throws KeyCollisionException {
	if (hasStringLabel(string.getName())) {
	throw new KeyCollisionException("A string with that label already exists");
	}
	if (bibtexStrings.containsKey(string.getId())) {
	throw new KeyCollisionException("Duplicate BibTeX string id.");
	}
	bibtexStrings.put(string.getId(), string);
	}

[tobiasdiez]

This is probably for performance reasons (hashmap provides a quicker lookup than to transverse the list).

[Siedlerchr]

I now tried around and got the Validation running somehow, but it doesn't really work
I just used an EmptyValidator, but although I have entered text, it is shown as "red"

@tobiasdiez Any idea how I have to configure that? I looked at the entry editor validation, but that is a bit different as always only one single property is validated

[tobiasdiez]

t.getRowValue is easier (and please rename t to something more descriptive.

[tobiasdiez]

I strongly recommend to use the mvvmfx.validation framework instead of directly using controlsfx. The latter has the problem that UI components are mixed with validation logic. The validation should not be concerned with the text in the text field, but with the content of a StringProperty in the view model, which is bound to the text field. The mvvmfx has a good documentation at https://github.com/sialcasa/mvvmFX/wiki/Validation#validator.

[Siedlerchr]

Okay, yes, that seems pretty streightfoward, except that I have now the problem:
I have my DialogViewModel and the StringViewModel which has just the content properties.
Should I merge this two ViewModels into one or is there another solution?
I think I did it that way in my Sharelatex PR, but with the consequence that I had all stuff double on each dialog opening

[tobiasdiez]

Two view models are what you need here. One for the whole dialog (contains a list of all strings, the composite validation status, methods for load/save) and one for each string (determines how the string should be displayed and its validation status). The validation of the per-string view model should be bound to the text field similar to how you did it before.

[Siedlerchr]

Okay, I managed to get it work and the validation is triggered correctly, but I have no idea why no icon is shown. I used JabRef's own IconDecorator class. Any idea why it's not showing?
I debugged through it, but could not identify why no icon etc is shown

You have to click new -> Its triggered for the first ime
Remove all content -> Hit enter -> Validation triggers again and returns the Validation.warning/error message statusbut no icon

[Siedlerchr]

@tobiasdiez Do you have an idea why the Validation error/warning icon is not showing? the validation is triggered, but it doesn't display anything

[Siedlerchr]

As the validation still does not show up I created an issue:
sialcasa/mvvmFX#546

[Siedlerchr]

Got it working now, not perfect, but at least it works. Currently it works only after you hit enter

[Siedlerchr]

I finally solved the validation @tobiasdiez , so this dialog is now ready for review

[tobiasdiez]

Thanks for your work on this dialog. Sadly, the quality of this PR is not as high as I'm used from you. Probably because it lied dead for so long and you grew as a programmer in the meantime and I'm now used to your better PRs ;-). Anyway, I've quite a few remarks and suggestions for improvement.

[Siedlerchr]

Addressed all remaininig issues so far. Also extracted the column setup with validation to an extra Factory.
As explained earlier, decided not to move the undo stuff as all undo things are in one package now.
And moving the viewModel stuff adding the strings does not really make sense how the internal class is now architectured

[tobiasdiez]

There are still some unresolved comments.
Can you please also explain why it is not possible to refactor the save method to do something like:

• Convert list of String-ViewModel to list of BibtexStrings.
• Update all strings in the database with this new list.

Would make this method easier to understand and extract code that can be used somewhere else.

[Siedlerchr]

@tobiasdiez Moved the code to the bibdatabase for adding new strings from a collection

[tobiasdiez]

Thanks! Looks very good to me now.