Grand unified preferences dialog

[calixtus]

Long announced, long expected, long declared vaporware, but here it comes. The grand unified preferences dialog project.

Will include some impact on the preferences dialog architecture.

Totally draftish, but here for you to see some progress as it grows with every commit pushed.

• 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 UI changes)
• Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

[tobiasdiez]

So you intend to move these "other preference dialogs" to tabs in the main preference dialog? That sounds like a very good idea!
(But maybe do this for each dialog separately, otherwise review will be really hard)

[calixtus]

Yes, my plan is to make the options menu obsolete. But this is a road to walk.
I don't intent to rework all these dialogs, just to convert them to the PreferencesTab scheme. The CustomizeGeneralFieldsDialog is a good example: By usability, it sucks so much it hurts, but I did not want to stop here but to stay focused on the main goal.

I could create several PRs, one for every step, or I could distinguish the steps by commits.

[calixtus]

This is a so called PseudoAbbreviation. I have not really understood what the real benefit is of this (maybe one can doubleclick an empty space in the List and insert a new abbreviation?) It's buggy, makes the code quite complicated to understand and I see no real benefit in it. ... ... But I put it back in for now, as several tests break, because they expect this PseudoAbbreviation and it would take me now valuable life time I would rather like to spend in finishing this PR, maybe as soon as I have completed everything else...

[calixtus]

Had to change a bit more in the ProtectedTermsTabViewModel, because it was saving the changes live to the Repository. It should now store them lists only when save is pressed.

I discovered a bug in (now) ProtectedTermsTab. The checkboxes for each ProtectetTermsList are just filled with a ConstantOf the ProtectedTermsList.isEnabled. So clicking on it has no effect and one cannot enable any terms list. I wonder, why this bug was not discovered before, seems like this feature is not well known...

[calixtus]

So, I think I am done. This was a big cleanup in the preferences dialog.
Most of the changes are just rewordings, moving stuff around and cleanups. But one or two beefy commits are in this PR like 97a1146 (#7384). I had to rework the viewmodel of the dialog, because it wrote directly into the preferences if changes happened and did not wait for the user to click save.

About the tests:

• The href link test seem to be independently of my changes broken
• I think i discovered a bug in the LocalizationConsistencyTests. The somehow cannot parse the custom fx control CitationKeyPatternPanel. I changed this one a little bit to accept the fill data by the setValues method instead of the integer, so it can be loaded directly from markup. But JabRef seems not to like this. @tobiasdiez @koppor any idea how to fix this?
  Everything else should be ready to review.

The other preferences dialogs are a bit more complex and I'd rather like to work on them in separate PRs.

[tobiasdiez]

I think i discovered a bug in the LocalizationConsistencyTests. The somehow cannot parse the custom fx control CitationKeyPatternPanel

Originally, we only used fxml for dialogs. Thus, the fxml loader used in the localization tests is optimized for this. Controls are loaded in a slightly different way (e.g use the root syntax). That might be the origin of the problem. Since the load exception is thrown at CitationKeyPatternPanel.fxml:7 this points to a problem with loading the controller. However, this is strange since the controller shouldn't be loaded at all because of

jabref/src/test/java/org/jabref/logic/l10n/LocalizationParser.java

Lines 213 to 214 in 034cf8c

	// We don't want to initialize controller
	loader.setControllerFactory(Mockito::mock);

Did you tried to debug this locally?

[calixtus]

Thanks to @Siedlerchr for helping me fix the localization tests.

[tobiasdiez]

Nice that you could fix this!

What I still don't understand is why the ViewLoader is actually called. The localization tests don't use it, but instead only try to load the fxml file. Why is the controller still initialized? cc29b5f#diff-3c2319710ad0e37f5f3b8d774be9847eea6ac95bba4faaef1b5bfe47188aaf6cR220 should have prevented this.

[calixtus]

Yes, we were thinking about that. The ViewLoader is called directly from the constructor, which is called by the FXMLLoader, which parses the custom fx control-tag in the fxml file, even though the controller of the parsed fxml file is not called.
Another workaround i thought of would have been to include the panel by fx:include, but I think this is way more clean and easy.

[tobiasdiez]

ok, it's strange that the controller is initialized. But it makes sense that there are then problems, since the ViewLoader then tries to load the fxml again etc.

Thanks for investigating this. Code looks good to me!

[Landi29]

Hi @calixtus , I encountered a new problem while working on the preferences dialog. If you enter a non valid value such as a character to the font size spinner under the appearances tab, endless exception dialogs will appear, crashing not only the program, but the entire operating system. Before opening an issue on this, I wanted to hear if your changes to the preferences dialog fixes this issue, or if you can confirm that the bug persists.

[calixtus]

Thanks for the bug report. I did not work on the AppearanceTab recently, so this bug is probably still there. Would be great if you could open an issue on this. Have you also maybe some interest in fixing this?